alex-rufous commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r645165431
##########
File path:
broker-core/src/main/java/org/apache/qpid/server/security/group/FileGroupDatabase.java
##########
@@ -104,17 +105,21 @@ public synchronized void setGroupFile(String groupFile)
throws IOException
@Override
public synchronized void addUserToGroup(String user, String group)
{
- Set<String> users =
_groupToUserMap.get(keySearch(_groupToUserMap.keySet(), group));
- if (users == null)
+ Set<String> groupUsers =
_groupToUserMap.get(keySearch(_groupToUserMap.keySet(), group));
+ if (groupUsers == null)
{
throw new IllegalArgumentException("Group "
+ group
+ " does not exist so could not
add "
+ user
+ " to it");
}
+ else if (groupUsers.contains(keySearch(_userToGroupMap.keySet(),
user)))
Review comment:
`keySearch(_userToGroupMap.keySet(), user))` is invoked multiple times.
Please introduce a variable to keep the result of call
`keySearch(_userToGroupMap.keySet(), user))` in order to reuse it later
##########
File path:
broker-core/src/test/java/org/apache/qpid/server/security/group/FileGroupDatabaseCaseInsensitiveTest.java
##########
@@ -360,6 +357,34 @@ public void
testCreateGroupPersistedToFileCaseInsensitive() throws Exception
assertTrue(newGroups.contains(MY_GROUP));
}
+ @Test(expected = IllegalConfigurationException.class)
+ public void testDuplicateCreateGroupPersistedToFileCaseInsensitive()
throws Exception
+ {
+ _util.writeAndSetGroupFile();
+
+ Set<String> groups = _fileGroupDatabase.getAllGroups();
+ assertTrue(groups.isEmpty());
+
+ _fileGroupDatabase.createGroup(MY_GROUP);
+
+ groups = _fileGroupDatabase.getAllGroups();
+ assertEquals(1, groups.size());
+ assertTrue(groups.contains(MY_GROUP));
+
+ _fileGroupDatabase.createGroup(MY_GROUP);
Review comment:
the test will throw exception at this line
the rest of test code is "dead". please remove code after this line and
please replace `MY_GROUP` with "MyGroup1"
##########
File path:
broker-core/src/test/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImplTest.java
##########
@@ -364,6 +366,116 @@ public void testSharingUnderlyingFileDisallowed() throws
Exception
}
}
+ @Test(expected = IllegalConfigurationException.class)
+ public void testDuplicateAddGroup() throws Exception
+ {
+ _groupFile = createTemporaryGroupFile(Collections.emptyMap());
+
+ Map<String, Object> providerAttrs = new HashMap<>();
+ String groupsFile = _groupFile.getAbsolutePath();
+ providerAttrs.put(FileBasedGroupProvider.TYPE,
GROUP_FILE_PROVIDER_TYPE);
+ providerAttrs.put(FileBasedGroupProvider.PATH, groupsFile);
+ providerAttrs.put(FileBasedGroupProvider.NAME, getTestName());
+
+ @SuppressWarnings("unchecked")
+ GroupProvider<?> provider = _objectFactory.create(GroupProvider.class,
providerAttrs, _broker);
+
+ assertThat(provider.getChildren(Group.class).size(), is(equalTo(0)));
+
+ final Map<String, Object> groupAttrs =
Collections.singletonMap(Group.NAME, "supers");
+ Group superGroup = provider.createChild(Group.class, groupAttrs);
+ assertThat(superGroup.getName(), is(equalTo("supers")));
+
+ provider.createChild(Group.class, groupAttrs);
+ assertThrows("Group with name supers already exists",
IllegalConfigurationException.class, null);
+ assertThat(superGroup.getChildren(Group.class).size(), is(equalTo(1)));
+
+ }
+ @Test(expected = IllegalConfigurationException.class)
Review comment:
new line is missed
##########
File path:
broker-core/src/test/java/org/apache/qpid/server/security/group/FileGroupDatabaseTest.java
##########
@@ -215,21 +217,14 @@ public void
testGetGroupPrincipalsForUserWhenUserRemovedFromGroup() throws Excep
assertTrue(_fileGroupDatabase.getGroupsForUser(USER1.toUpperCase()).isEmpty());
}
- @Test
+ @Test(expected = IllegalConfigurationException.class)
public void
testGetGroupPrincipalsForUserWhenUserAddedToGroupTheyAreAlreadyIn() throws
Exception
{
_util.writeAndSetGroupFile("myGroup.users", USER1);
_fileGroupDatabase.addUserToGroup(USER1, MY_GROUP);
Review comment:
the test throws exception at this line
the rest of the test is a dead code... please remove code after this line
##########
File path:
broker-core/src/test/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImplTest.java
##########
@@ -364,6 +366,116 @@ public void testSharingUnderlyingFileDisallowed() throws
Exception
}
}
+ @Test(expected = IllegalConfigurationException.class)
+ public void testDuplicateAddGroup() throws Exception
+ {
+ _groupFile = createTemporaryGroupFile(Collections.emptyMap());
+
+ Map<String, Object> providerAttrs = new HashMap<>();
+ String groupsFile = _groupFile.getAbsolutePath();
+ providerAttrs.put(FileBasedGroupProvider.TYPE,
GROUP_FILE_PROVIDER_TYPE);
+ providerAttrs.put(FileBasedGroupProvider.PATH, groupsFile);
+ providerAttrs.put(FileBasedGroupProvider.NAME, getTestName());
+
+ @SuppressWarnings("unchecked")
+ GroupProvider<?> provider = _objectFactory.create(GroupProvider.class,
providerAttrs, _broker);
+
+ assertThat(provider.getChildren(Group.class).size(), is(equalTo(0)));
+
+ final Map<String, Object> groupAttrs =
Collections.singletonMap(Group.NAME, "supers");
+ Group superGroup = provider.createChild(Group.class, groupAttrs);
+ assertThat(superGroup.getName(), is(equalTo("supers")));
+
+ provider.createChild(Group.class, groupAttrs);
+ assertThrows("Group with name supers already exists",
IllegalConfigurationException.class, null);
+ assertThat(superGroup.getChildren(Group.class).size(), is(equalTo(1)));
+
+ }
+ @Test(expected = IllegalConfigurationException.class)
+ public void testDuplicateAddMember() throws Exception
+ {
+ _groupFile = createTemporaryGroupFile(Collections.emptyMap());
+
+ Map<String, Object> providerAttrs = new HashMap<>();
+ String groupsFile = _groupFile.getAbsolutePath();
+ providerAttrs.put(FileBasedGroupProvider.TYPE,
GROUP_FILE_PROVIDER_TYPE);
+ providerAttrs.put(FileBasedGroupProvider.PATH, groupsFile);
+ providerAttrs.put(FileBasedGroupProvider.NAME, getTestName());
+
+ @SuppressWarnings("unchecked")
+ GroupProvider<?> provider = _objectFactory.create(GroupProvider.class,
providerAttrs, _broker);
+
+ assertThat(provider.getChildren(Group.class).size(), is(equalTo(0)));
+
+ final Map<String, Object> groupAttrs =
Collections.singletonMap(Group.NAME, "supers");
+ Group superGroup = provider.createChild(Group.class, groupAttrs);
+ assertThat(superGroup.getName(), is(equalTo("supers")));
+
+ final Map<String, Object> memberAttrs1 =
Collections.singletonMap(GroupMember.NAME, "root1");
+ GroupMember rootMember = (GroupMember)
superGroup.createChild(GroupMember.class, memberAttrs1);
+ assertThat(rootMember.getName(), is(equalTo("root1")));
+
+ superGroup.createChild(GroupMember.class, memberAttrs1);
+ assertThrows("Group member with name root1 already exists",
IllegalConfigurationException.class, null);
+ assertThat(superGroup.getChildren(GroupMember.class).size(),
is(equalTo(1)));
+ }
+
+ @Test
+ public void testDuplicateAddGroupCaseSensitive() throws Exception
Review comment:
please rename this test into testCreateGroups
##########
File path:
broker-core/src/main/java/org/apache/qpid/server/security/group/FileGroupDatabase.java
##########
@@ -104,17 +105,21 @@ public synchronized void setGroupFile(String groupFile)
throws IOException
@Override
public synchronized void addUserToGroup(String user, String group)
{
- Set<String> users =
_groupToUserMap.get(keySearch(_groupToUserMap.keySet(), group));
- if (users == null)
+ Set<String> groupUsers =
_groupToUserMap.get(keySearch(_groupToUserMap.keySet(), group));
+ if (groupUsers == null)
{
throw new IllegalArgumentException("Group "
+ group
+ " does not exist so could not
add "
+ user
+ " to it");
}
+ else if (groupUsers.contains(keySearch(_userToGroupMap.keySet(),
user)))
+ {
+ throw new IllegalConfigurationException(String.format("Group
member with name'%s' already exists", user));
+ }
- users.add(keySearch(users, user));
+ groupUsers.add(keySearch(groupUsers, user));
Review comment:
lt seems it would be safe to swap a call `keySearch(groupUsers, user)`
with a local variable introduced for `keySearch(_userToGroupMap.keySet(),
user)`. Though if these to operations return different results, that would
indicate an inconsistency and an exception would need to be thrown
##########
File path:
broker-core/src/main/java/org/apache/qpid/server/security/group/FileGroupDatabase.java
##########
@@ -104,17 +105,21 @@ public synchronized void setGroupFile(String groupFile)
throws IOException
@Override
public synchronized void addUserToGroup(String user, String group)
{
- Set<String> users =
_groupToUserMap.get(keySearch(_groupToUserMap.keySet(), group));
- if (users == null)
+ Set<String> groupUsers =
_groupToUserMap.get(keySearch(_groupToUserMap.keySet(), group));
+ if (groupUsers == null)
{
throw new IllegalArgumentException("Group "
+ group
+ " does not exist so could not
add "
+ user
+ " to it");
}
+ else if (groupUsers.contains(keySearch(_userToGroupMap.keySet(),
user)))
+ {
+ throw new IllegalConfigurationException(String.format("Group
member with name'%s' already exists", user));
+ }
- users.add(keySearch(users, user));
+ groupUsers.add(keySearch(groupUsers, user));
Set<String> groups =
_userToGroupMap.get(keySearch(_userToGroupMap.keySet(), user));
Review comment:
let's swap a call to `keySearch(_userToGroupMap.keySet(), user)` with a
local variable (as per comments above)
##########
File path:
broker-core/src/main/java/org/apache/qpid/server/security/group/FileGroupDatabase.java
##########
@@ -174,10 +179,27 @@ public synchronized void removeUserFromGroup(String user,
String group)
@Override
public synchronized void createGroup(String group)
{
- Set<String> users = new ConcurrentSkipListSet<String>();
- _groupToUserMap.put(group, users);
+ if (!isAvailable(group, _groupToUserMap))
+ {
+ Set<String> users = new ConcurrentSkipListSet<String>();
+ _groupToUserMap.put(group, users);
+ update();
+ }
+ else
+ {
+ throw new IllegalConfigurationException(String.format("Group with
name '%s' already exists", group));
+ }
+ }
- update();
+ private boolean isAvailable(String searchString,
+ final Map<String, Set<String>> collection)
+ {
+ String groupAvailable = keySearch(collection.keySet(), searchString);
+ if (collection.get(groupAvailable) == null && groupAvailable ==
searchString)
Review comment:
Please replace this method implementation with below
private boolean exists(final String searchString, final Set<String> set)
{
if (!_groupProvider.isCaseSensitive())
{
for (String key : set)
{
if (key.equalsIgnoreCase(searchString))
{
return true;
}
}
return false
}
return set.contains(searchString);
}
##########
File path:
broker-core/src/main/java/org/apache/qpid/server/security/group/FileGroupDatabase.java
##########
@@ -104,17 +105,21 @@ public synchronized void setGroupFile(String groupFile)
throws IOException
@Override
public synchronized void addUserToGroup(String user, String group)
{
- Set<String> users =
_groupToUserMap.get(keySearch(_groupToUserMap.keySet(), group));
- if (users == null)
+ Set<String> groupUsers =
_groupToUserMap.get(keySearch(_groupToUserMap.keySet(), group));
Review comment:
`keySearch(_groupToUserMap.keySet(), group)` is invoked multiple times
in method.
Please, introduce a local variable to keep result of
`keySearch(_groupToUserMap.keySet(), group)`
##########
File path:
broker-core/src/main/java/org/apache/qpid/server/model/adapter/Test.java
##########
@@ -0,0 +1,5 @@
+package org.apache.qpid.server.model.adapter;
Review comment:
unexpected code
please remove
##########
File path:
broker-core/src/test/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImplTest.java
##########
@@ -364,6 +366,116 @@ public void testSharingUnderlyingFileDisallowed() throws
Exception
}
}
+ @Test(expected = IllegalConfigurationException.class)
+ public void testDuplicateAddGroup() throws Exception
Review comment:
please rename test into testCreateDuplicateGroup
##########
File path:
broker-core/src/test/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImplTest.java
##########
@@ -364,6 +366,116 @@ public void testSharingUnderlyingFileDisallowed() throws
Exception
}
}
+ @Test(expected = IllegalConfigurationException.class)
+ public void testDuplicateAddGroup() throws Exception
+ {
+ _groupFile = createTemporaryGroupFile(Collections.emptyMap());
+
+ Map<String, Object> providerAttrs = new HashMap<>();
+ String groupsFile = _groupFile.getAbsolutePath();
+ providerAttrs.put(FileBasedGroupProvider.TYPE,
GROUP_FILE_PROVIDER_TYPE);
+ providerAttrs.put(FileBasedGroupProvider.PATH, groupsFile);
+ providerAttrs.put(FileBasedGroupProvider.NAME, getTestName());
+
+ @SuppressWarnings("unchecked")
+ GroupProvider<?> provider = _objectFactory.create(GroupProvider.class,
providerAttrs, _broker);
+
+ assertThat(provider.getChildren(Group.class).size(), is(equalTo(0)));
+
+ final Map<String, Object> groupAttrs =
Collections.singletonMap(Group.NAME, "supers");
+ Group superGroup = provider.createChild(Group.class, groupAttrs);
+ assertThat(superGroup.getName(), is(equalTo("supers")));
+
+ provider.createChild(Group.class, groupAttrs);
+ assertThrows("Group with name supers already exists",
IllegalConfigurationException.class, null);
+ assertThat(superGroup.getChildren(Group.class).size(), is(equalTo(1)));
+
+ }
+ @Test(expected = IllegalConfigurationException.class)
+ public void testDuplicateAddMember() throws Exception
Review comment:
please rename this test into testCreateDuplicateMember
##########
File path:
broker-core/src/test/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImplTest.java
##########
@@ -364,6 +366,116 @@ public void testSharingUnderlyingFileDisallowed() throws
Exception
}
}
+ @Test(expected = IllegalConfigurationException.class)
+ public void testDuplicateAddGroup() throws Exception
+ {
+ _groupFile = createTemporaryGroupFile(Collections.emptyMap());
+
+ Map<String, Object> providerAttrs = new HashMap<>();
+ String groupsFile = _groupFile.getAbsolutePath();
+ providerAttrs.put(FileBasedGroupProvider.TYPE,
GROUP_FILE_PROVIDER_TYPE);
+ providerAttrs.put(FileBasedGroupProvider.PATH, groupsFile);
+ providerAttrs.put(FileBasedGroupProvider.NAME, getTestName());
+
+ @SuppressWarnings("unchecked")
+ GroupProvider<?> provider = _objectFactory.create(GroupProvider.class,
providerAttrs, _broker);
+
+ assertThat(provider.getChildren(Group.class).size(), is(equalTo(0)));
+
+ final Map<String, Object> groupAttrs =
Collections.singletonMap(Group.NAME, "supers");
+ Group superGroup = provider.createChild(Group.class, groupAttrs);
+ assertThat(superGroup.getName(), is(equalTo("supers")));
+
+ provider.createChild(Group.class, groupAttrs);
+ assertThrows("Group with name supers already exists",
IllegalConfigurationException.class, null);
+ assertThat(superGroup.getChildren(Group.class).size(), is(equalTo(1)));
+
+ }
+ @Test(expected = IllegalConfigurationException.class)
+ public void testDuplicateAddMember() throws Exception
+ {
+ _groupFile = createTemporaryGroupFile(Collections.emptyMap());
+
+ Map<String, Object> providerAttrs = new HashMap<>();
+ String groupsFile = _groupFile.getAbsolutePath();
+ providerAttrs.put(FileBasedGroupProvider.TYPE,
GROUP_FILE_PROVIDER_TYPE);
+ providerAttrs.put(FileBasedGroupProvider.PATH, groupsFile);
+ providerAttrs.put(FileBasedGroupProvider.NAME, getTestName());
+
+ @SuppressWarnings("unchecked")
+ GroupProvider<?> provider = _objectFactory.create(GroupProvider.class,
providerAttrs, _broker);
+
+ assertThat(provider.getChildren(Group.class).size(), is(equalTo(0)));
+
+ final Map<String, Object> groupAttrs =
Collections.singletonMap(Group.NAME, "supers");
+ Group superGroup = provider.createChild(Group.class, groupAttrs);
+ assertThat(superGroup.getName(), is(equalTo("supers")));
+
+ final Map<String, Object> memberAttrs1 =
Collections.singletonMap(GroupMember.NAME, "root1");
+ GroupMember rootMember = (GroupMember)
superGroup.createChild(GroupMember.class, memberAttrs1);
+ assertThat(rootMember.getName(), is(equalTo("root1")));
+
+ superGroup.createChild(GroupMember.class, memberAttrs1);
+ assertThrows("Group member with name root1 already exists",
IllegalConfigurationException.class, null);
+ assertThat(superGroup.getChildren(GroupMember.class).size(),
is(equalTo(1)));
+ }
+
+ @Test
+ public void testDuplicateAddGroupCaseSensitive() throws Exception
+ {
+ _groupFile = createTemporaryGroupFile(Collections.emptyMap());
+
+ Map<String, Object> providerAttrs = new HashMap<>();
+ String groupsFile = _groupFile.getAbsolutePath();
+ providerAttrs.put(FileBasedGroupProvider.TYPE,
GROUP_FILE_PROVIDER_TYPE);
+ providerAttrs.put(FileBasedGroupProvider.PATH, groupsFile);
+ providerAttrs.put(FileBasedGroupProvider.NAME, getTestName());
+
+ @SuppressWarnings("unchecked")
+ GroupProvider<?> provider = _objectFactory.create(GroupProvider.class,
providerAttrs, _broker);
+
+ assertThat(provider.getChildren(Group.class).size(), is(equalTo(0)));
+
+ final Map<String, Object> groupAttrs =
Collections.singletonMap(Group.NAME, "supers");
+ Group superGroup = provider.createChild(Group.class, groupAttrs);
+ assertThat(superGroup.getName(), is(equalTo("supers")));
+
+ final Map<String, Object> groupAttrs2 =
Collections.singletonMap(Group.NAME, "Supers");
+ Group superGroup2 = provider.createChild(Group.class, groupAttrs2);
+ assertThat(superGroup2.getName(), is(equalTo("Supers")));
+
+ assertThat(provider.getChildren(Group.class).size(), is(equalTo(2)));
+
+ }
+ @Test(expected = IllegalConfigurationException.class)
+ public void testDuplicateAddMemberCaseSensitive() throws Exception
Review comment:
please rename this test into testCreateMembers
##########
File path:
broker-core/src/main/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImpl.java
##########
@@ -359,16 +359,21 @@ protected void onOpen()
if (childClass == GroupMember.class)
{
String memberName = (String) attributes.get(GroupMember.NAME);
-
- _groupDatabase.addUserToGroup(memberName, getName());
- UUID id = UUID.randomUUID();
- Map<String,Object> attrMap = new HashMap<String, Object>();
- attrMap.put(GroupMember.ID,id);
- attrMap.put(GroupMember.NAME, memberName);
- GroupMemberAdapter groupMemberAdapter = new
GroupMemberAdapter(attrMap);
- groupMemberAdapter.create();
- return Futures.immediateFuture((C) groupMemberAdapter);
-
+// Set<String> users =
_groupDatabase.getUsersInGroup(getName());
Review comment:
commented code....
the changes in this hunk needs to be reverted
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
