You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2021/05/24 15:20:29 UTC
[GitHub] [qpid-broker-j] deepyaraj opened a new pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
deepyaraj opened a new pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88
Any duplicate creation of a group and a group member resulted in deletion of such child objects and takes effect on subsequent restart.
Fixed the issue and added appropriate exception
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r644690778
##########
File path: broker-core/src/main/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImpl.java
##########
@@ -205,17 +205,22 @@ public String getPath()
{
throw new IllegalConfigurationException(String.format("Group provider '%s' is not activated. Cannot create a group.", getName()));
}
+ Set<String> availableGroups = _groupDatabase.getAllGroups();
+ if(!availableGroups.contains(groupName))
+ {
+ _groupDatabase.createGroup(groupName);
- _groupDatabase.createGroup(groupName);
-
- Map<String,Object> attrMap = new HashMap<String, Object>();
- UUID id = UUID.randomUUID();
- attrMap.put(ConfiguredObject.ID, id);
- attrMap.put(ConfiguredObject.NAME, groupName);
- GroupAdapter groupAdapter = new GroupAdapter(attrMap);
- groupAdapter.create();
- return Futures.immediateFuture((C) groupAdapter);
-
+ Map<String, Object> attrMap = new HashMap<String, Object>();
+ UUID id = UUID.randomUUID();
+ attrMap.put(ConfiguredObject.ID, id);
+ attrMap.put(ConfiguredObject.NAME, groupName);
+ GroupAdapter groupAdapter = new GroupAdapter(attrMap);
+ groupAdapter.create();
+ return Futures.immediateFuture((C) groupAdapter);
+ }
+ else{
Review comment:
Done!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r645469850
##########
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:
Done!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r644778648
##########
File path: broker-core/src/main/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImpl.java
##########
@@ -359,16 +364,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());
+ if(!users.contains(memberName))
Review comment:
Done!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r645487711
##########
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:
Done!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r646254344
##########
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);
Review comment:
Done!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r645583709
##########
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:
Done!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r645470751
##########
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:
Done!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r645469101
##########
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:
Done!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r644690778
##########
File path: broker-core/src/main/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImpl.java
##########
@@ -205,17 +205,22 @@ public String getPath()
{
throw new IllegalConfigurationException(String.format("Group provider '%s' is not activated. Cannot create a group.", getName()));
}
+ Set<String> availableGroups = _groupDatabase.getAllGroups();
+ if(!availableGroups.contains(groupName))
+ {
+ _groupDatabase.createGroup(groupName);
- _groupDatabase.createGroup(groupName);
-
- Map<String,Object> attrMap = new HashMap<String, Object>();
- UUID id = UUID.randomUUID();
- attrMap.put(ConfiguredObject.ID, id);
- attrMap.put(ConfiguredObject.NAME, groupName);
- GroupAdapter groupAdapter = new GroupAdapter(attrMap);
- groupAdapter.create();
- return Futures.immediateFuture((C) groupAdapter);
-
+ Map<String, Object> attrMap = new HashMap<String, Object>();
+ UUID id = UUID.randomUUID();
+ attrMap.put(ConfiguredObject.ID, id);
+ attrMap.put(ConfiguredObject.NAME, groupName);
+ GroupAdapter groupAdapter = new GroupAdapter(attrMap);
+ groupAdapter.create();
+ return Futures.immediateFuture((C) groupAdapter);
+ }
+ else{
Review comment:
Done!
##########
File path: broker-core/src/main/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImpl.java
##########
@@ -205,17 +205,22 @@ public String getPath()
{
throw new IllegalConfigurationException(String.format("Group provider '%s' is not activated. Cannot create a group.", getName()));
}
+ Set<String> availableGroups = _groupDatabase.getAllGroups();
+ if(!availableGroups.contains(groupName))
Review comment:
Done!
##########
File path: broker-core/src/main/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImpl.java
##########
@@ -359,16 +364,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());
+ if(!users.contains(memberName))
Review comment:
Done!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r645486034
##########
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:
Removed the dead code, but replacing the group to mygroup1 wont result in the exception
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] alex-rufous commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] alex-rufous commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#issuecomment-856452068
> The changes look good to me. Though, some further improvements could be made to tests, for example, refactoring of duplicate blocks, superfluous exception annotations, etc but nothing major...
>
> Please merge it into a single commit
Merged to a single commit!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r645489174
##########
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:
Done!
##########
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:
Done!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r646308117
##########
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:
Done!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r645473474
##########
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:
This call to keysearch is made explicitly not to add duplicate for a case sensitive provider. So replacing this will lead to undesriable results
##########
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:
Done!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r646281312
##########
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);
Review comment:
Done!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r645482117
##########
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:
Done!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] asfgit closed pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] alex-rufous commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
alex-rufous commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r639285379
##########
File path: broker-core/src/main/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImpl.java
##########
@@ -359,16 +364,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());
+ if(!users.contains(memberName))
Review comment:
If a group provider is configured with caseSensitive==false, the user lookup should be case insensitive. It seems we can perform the user existence check directly in `Database#addUserToGroup(String, String)`. Thus, let's move the check there, andm throw `IllegalConfigurationException` from implementation of `Database#addUserToGroup(String, String)`.
##########
File path: broker-core/src/main/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImpl.java
##########
@@ -205,17 +205,22 @@ public String getPath()
{
throw new IllegalConfigurationException(String.format("Group provider '%s' is not activated. Cannot create a group.", getName()));
}
+ Set<String> availableGroups = _groupDatabase.getAllGroups();
+ if(!availableGroups.contains(groupName))
+ {
+ _groupDatabase.createGroup(groupName);
- _groupDatabase.createGroup(groupName);
-
- Map<String,Object> attrMap = new HashMap<String, Object>();
- UUID id = UUID.randomUUID();
- attrMap.put(ConfiguredObject.ID, id);
- attrMap.put(ConfiguredObject.NAME, groupName);
- GroupAdapter groupAdapter = new GroupAdapter(attrMap);
- groupAdapter.create();
- return Futures.immediateFuture((C) groupAdapter);
-
+ Map<String, Object> attrMap = new HashMap<String, Object>();
+ UUID id = UUID.randomUUID();
+ attrMap.put(ConfiguredObject.ID, id);
+ attrMap.put(ConfiguredObject.NAME, groupName);
+ GroupAdapter groupAdapter = new GroupAdapter(attrMap);
+ groupAdapter.create();
+ return Futures.immediateFuture((C) groupAdapter);
+ }
+ else{
Review comment:
The formatting does not follow Qpid code style
##########
File path: broker-core/src/main/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImpl.java
##########
@@ -205,17 +205,22 @@ public String getPath()
{
throw new IllegalConfigurationException(String.format("Group provider '%s' is not activated. Cannot create a group.", getName()));
}
+ Set<String> availableGroups = _groupDatabase.getAllGroups();
+ if(!availableGroups.contains(groupName))
Review comment:
If a group provider is configured with `caseSensitive==false`, the group lookup should be case insensitive. I would like to suggest to move group existence check into `Database#createGroup(String)` as database implementation already has a code to perform case insensitive group lookup. If group exists, an implementation of `Database#createGroup(String)` can throw `IllegalConfigurationException`
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r645487404
##########
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:
Done!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r645380751
##########
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:
Done!
##########
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:
Removed it!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] alex-rufous commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
alex-rufous commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r645590391
##########
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);
Review comment:
assertThrows does not invoke any code
##########
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);
Review comment:
assertThrows does not invoke any code
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r645487948
##########
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:
Added!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r645582735
##########
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:
Done!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r645469201
##########
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:
Done!
##########
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:
Removed it!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] deepyaraj commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
deepyaraj commented on a change in pull request #88:
URL: https://github.com/apache/qpid-broker-j/pull/88#discussion_r644691027
##########
File path: broker-core/src/main/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImpl.java
##########
@@ -205,17 +205,22 @@ public String getPath()
{
throw new IllegalConfigurationException(String.format("Group provider '%s' is not activated. Cannot create a group.", getName()));
}
+ Set<String> availableGroups = _groupDatabase.getAllGroups();
+ if(!availableGroups.contains(groupName))
Review comment:
Done!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-broker-j] alex-rufous commented on a change in pull request #88: QPID:8525:[Broker-J]Fixed child deletion issue in group providers
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org