alex-rufous commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r645542563
##########
File path:
broker-core/src/main/java/org/apache/qpid/server/security/group/FileGroupDatabase.java
##########
@@ -104,25 +105,31 @@ 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)
+ final String groupKey = keySearch(_groupToUserMap.keySet(), group);
+ Set<String> groupUsers = _groupToUserMap.get(groupKey);
+ final String userKey = keySearch(_userToGroupMap.keySet(), user);
+ if (groupUsers == null)
{
throw new IllegalArgumentException("Group "
+ group
+ " does not exist so could not
add "
+ user
+ " to it");
}
+ else if (groupUsers.contains(userKey))
+ {
+ throw new IllegalConfigurationException(String.format("Group
member with name'%s' already exists", user));
Review comment:
a space is missing between name and '%s'
##########
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:
if group provider is case insensitive, the group "MyGroup1" would be a
duplicate of "myGroup1". Thus, an exception would need to be thrown. Why it is
not thrown?
##########
File path:
broker-core/src/test/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImplTest.java
##########
@@ -364,6 +366,117 @@ public void testSharingUnderlyingFileDisallowed() throws
Exception
}
}
+ @Test(expected = IllegalConfigurationException.class)
+ public void testCreateDuplicateGroup() 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 testCreateDuplicateMember() 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 testCreateGroups() 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 testCreateMembers() 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);
Review comment:
assertThrows does not call any application code
I think the intention here was to do
assertThrows("Group member with name root1 already exists",
IllegalConfigurationException.class,
()->superGroup.createChild(GroupMember.class, memberAttrs1));
Please fix the assert
##########
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:
Could you please add an explicit check here that group user is the same
as a user key
final String groupKey = keySearch(groupUsers, user);
if (!userKey.equals(groupKey))
{
throw new IllegalConfigurationException(String.format("Inconsistent
data: user key '%s' is not equal to a group key '%s'", userKey, groupKey));
}
##########
File path:
broker-core/src/test/java/org/apache/qpid/server/security/group/FileGroupDatabaseCaseInsensitiveTest.java
##########
@@ -168,20 +170,15 @@ public void
testGetGroupPrincipalsForUserWhenUserRemovedFromGroupCaseInsensitive
assertTrue(groups.contains(MY_GROUP1));
}
- @Test
+ @Test(expected = IllegalConfigurationException.class)
public void
testGetGroupPrincipalsForUserWhenUserAddedToGroupTheyAreAlreadyInCaseInsensitive()
throws Exception
{
_util.writeAndSetGroupFile("myGroup.users", USER1);
_fileGroupDatabase.addUserToGroup(USER1, MY_GROUP);
Set<String> groups =
_fileGroupDatabase.getGroupsForUser(USER1.toUpperCase());
+ assertThrows("Group with name supers already exists",
IllegalConfigurationException.class, null);
Review comment:
an application code is expected to call here
assertThrows("Group with name supers already exists",
IllegalConfigurationException.class,
()->_fileGroupDatabase.addUserToGroup(USER1, MY_GROUP));
Please refactor the test
--
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
