You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@qpid.apache.org by or...@apache.org on 2021/06/08 23:51:00 UTC

[qpid-broker-j] branch 8.0.x updated (a83f2cb -> ec80fae)

This is an automated email from the ASF dual-hosted git repository.

orudyy pushed a change to branch 8.0.x
in repository https://gitbox.apache.org/repos/asf/qpid-broker-j.git.


    from a83f2cb  QPID-8537: replace use of constructors marked deprecated-for-removal
     new 870e05b  QPID-8525:[Broker-J]Fixed child deletion issue in group providers
     new b5ed920  QPID-8525:[Broker-J] Code cleanup
     new ec80fae  NO-JIRA: Fix maven execution settings in conversion tests

The 3 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.


Summary of changes:
 .../model/adapter/FileBasedGroupProviderImpl.java  |  14 +-
 .../server/security/group/FileGroupDatabase.java   |  71 ++++--
 .../adapter/FileBasedGroupProviderImplTest.java    | 251 ++++++++++++---------
 .../FileGroupDatabaseCaseInsensitiveTest.java      |  36 ++-
 .../security/group/FileGroupDatabaseTest.java      |  14 +-
 systests/end-to-end-conversion-tests/pom.xml       |  27 ++-
 6 files changed, 255 insertions(+), 158 deletions(-)

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org


[qpid-broker-j] 03/03: NO-JIRA: Fix maven execution settings in conversion tests

Posted by or...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

orudyy pushed a commit to branch 8.0.x
in repository https://gitbox.apache.org/repos/asf/qpid-broker-j.git

commit ec80faec748c4ddc5855fdcd42139f4884bc6d98
Author: Alex Rudyy <or...@apache.org>
AuthorDate: Sat May 29 19:57:48 2021 +0100

    NO-JIRA: Fix maven execution settings in conversion tests
---
 systests/end-to-end-conversion-tests/pom.xml | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/systests/end-to-end-conversion-tests/pom.xml b/systests/end-to-end-conversion-tests/pom.xml
index bf71ee8..1133267 100644
--- a/systests/end-to-end-conversion-tests/pom.xml
+++ b/systests/end-to-end-conversion-tests/pom.xml
@@ -88,11 +88,15 @@
                         <qpid.initialConfigurationLocation>classpath:config-end-to-end-conversion-tests.json</qpid.initialConfigurationLocation>
                         <qpid.systests.end_to_end_conversion.localRepository>${settings.localRepository}</qpid.systests.end_to_end_conversion.localRepository>
                         <qpid.systests.end_to_end_conversion.remoteRepository>https://repo.maven.apache.org/maven2/</qpid.systests.end_to_end_conversion.remoteRepository>
+                        <qpid-jms-client-version>${qpid-jms-client-version}</qpid-jms-client-version>
+                        <qpid-jms-client-amqp-0-x-version>${qpid-jms-client-amqp-0-x-version}</qpid-jms-client-amqp-0-x-version>
                     </systemPropertyVariables>
+                    <reuseForks>false</reuseForks>
+                    <forkCount>6</forkCount>
                 </configuration>
                 <executions>
                     <execution>
-                        <id>0-9-1 -&gt; 1.0</id>
+                        <id>default-test</id>
                         <configuration>
                             <skipTests>${skipITs}</skipTests>
                             <systemPropertyVariables>
@@ -108,7 +112,7 @@
                         </goals>
                     </execution>
                     <execution>
-                        <id>1.0 -&gt; 0-9-1</id>
+                        <id>1.0_to_0-9-1</id>
                         <configuration>
                             <skipTests>${skipITs}</skipTests>
                             <systemPropertyVariables>
@@ -124,7 +128,7 @@
                         </goals>
                     </execution>
                     <execution>
-                        <id>0-10 -&gt; 1.0</id>
+                        <id>0-10_to_1.0</id>
                         <configuration>
                             <skipTests>${skipITs}</skipTests>
                             <systemPropertyVariables>
@@ -140,7 +144,7 @@
                         </goals>
                     </execution>
                     <execution>
-                        <id>1.0 -&gt; 0-10</id>
+                        <id>1.0_to_0-10</id>
                         <configuration>
                             <skipTests>${skipITs}</skipTests>
                             <systemPropertyVariables>
@@ -156,7 +160,7 @@
                         </goals>
                     </execution>
                     <execution>
-                        <id>0-9-1 -&gt; 0-10</id>
+                        <id>0-9-1_to_0-10</id>
                         <configuration>
                             <skipTests>${skipITs}</skipTests>
                             <systemPropertyVariables>
@@ -172,7 +176,7 @@
                         </goals>
                     </execution>
                     <execution>
-                        <id>0-10 -&gt; 0-9-1</id>
+                        <id>0-10_to_0-9-1</id>
                         <configuration>
                             <skipTests>${skipITs}</skipTests>
                             <systemPropertyVariables>
@@ -187,6 +191,17 @@
                             <goal>test</goal>
                         </goals>
                     </execution>
+                    <execution>
+                        <id>integration-test</id>
+                        <goals>
+                            <goal>test</goal>
+                        </goals>
+                        <configuration>
+                            <excludes>
+                                <exclude>**</exclude>
+                            </excludes>
+                        </configuration>
+                    </execution>
                 </executions>
             </plugin>
         </plugins>

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org


[qpid-broker-j] 02/03: QPID-8525:[Broker-J] Code cleanup

Posted by or...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

orudyy pushed a commit to branch 8.0.x
in repository https://gitbox.apache.org/repos/asf/qpid-broker-j.git

commit b5ed920367e9c50af171b70036889b5221986895
Author: Alex Rudyy <or...@apache.org>
AuthorDate: Wed Jun 9 00:10:32 2021 +0100

    QPID-8525:[Broker-J] Code cleanup
    
    (cherry picked from commit a36fea373e043a0fd8e4b9d16382056c4c5ddb8e)
---
 .../server/security/group/FileGroupDatabase.java   |  18 +-
 .../adapter/FileBasedGroupProviderImplTest.java    | 237 +++++++--------------
 .../FileGroupDatabaseCaseInsensitiveTest.java      |  17 +-
 .../security/group/FileGroupDatabaseTest.java      |   1 -
 4 files changed, 99 insertions(+), 174 deletions(-)

diff --git a/broker-core/src/main/java/org/apache/qpid/server/security/group/FileGroupDatabase.java b/broker-core/src/main/java/org/apache/qpid/server/security/group/FileGroupDatabase.java
index 8d908e6..f8fce88 100644
--- a/broker-core/src/main/java/org/apache/qpid/server/security/group/FileGroupDatabase.java
+++ b/broker-core/src/main/java/org/apache/qpid/server/security/group/FileGroupDatabase.java
@@ -106,15 +106,13 @@ public class FileGroupDatabase implements GroupDatabase
     public synchronized void addUserToGroup(String user, String group)
     {
         final String groupKey = keySearch(_groupToUserMap.keySet(), group);
-        Set<String> groupUsers = _groupToUserMap.get(groupKey);
+        final 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");
+            throw new IllegalArgumentException(String.format("Group %s does not exist so could not add %s to it",
+                                                             group,
+                                                             user));
         }
         else if (groupUsers.contains(userKey))
         {
@@ -136,7 +134,7 @@ public class FileGroupDatabase implements GroupDatabase
         Set<String> groups = _userToGroupMap.get(userKey);
         if (groups == null)
         {
-            groups = new ConcurrentSkipListSet<String>();
+            groups = new ConcurrentSkipListSet<>();
             _userToGroupMap.put(user, groups);
         }
         groups.add(groupKey);
@@ -193,7 +191,7 @@ public class FileGroupDatabase implements GroupDatabase
     {
         if (!exists(group, _groupToUserMap.keySet()))
         {
-            Set<String> users = new ConcurrentSkipListSet<String>();
+            Set<String> users = new ConcurrentSkipListSet<>();
             _groupToUserMap.put(group, users);
             update();
         }
@@ -274,7 +272,7 @@ public class FileGroupDatabase implements GroupDatabase
 
                 if (groupsForThisUser == null)
                 {
-                    groupsForThisUser = new ConcurrentSkipListSet<String>();
+                    groupsForThisUser = new ConcurrentSkipListSet<>();
                     _userToGroupMap.put(userName, groupsForThisUser);
                 }
 
@@ -322,7 +320,7 @@ public class FileGroupDatabase implements GroupDatabase
     private ConcurrentSkipListSet<String> buildUserSetFromCommaSeparateValue(String userString)
     {
         String[] users = userString.split(",");
-        final ConcurrentSkipListSet<String> userSet = new ConcurrentSkipListSet<String>();
+        final ConcurrentSkipListSet<String> userSet = new ConcurrentSkipListSet<>();
         for (String user : users)
         {
             final String trimmed = user.trim();
diff --git a/broker-core/src/test/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImplTest.java b/broker-core/src/test/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImplTest.java
index 410c913..e7d2057 100644
--- a/broker-core/src/test/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImplTest.java
+++ b/broker-core/src/test/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImplTest.java
@@ -23,12 +23,12 @@ package org.apache.qpid.server.model.adapter;
 import static org.apache.qpid.server.model.adapter.FileBasedGroupProviderImpl.GROUP_FILE_PROVIDER_TYPE;
 import static org.hamcrest.CoreMatchers.instanceOf;
 import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
-import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.fail;
 import static org.mockito.Mockito.when;
@@ -46,7 +46,6 @@ import java.util.stream.Collectors;
 
 import com.google.common.collect.Sets;
 import org.junit.After;
-import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -57,7 +56,6 @@ import org.apache.qpid.server.model.ConfiguredObjectFactory;
 import org.apache.qpid.server.model.Group;
 import org.apache.qpid.server.model.GroupMember;
 import org.apache.qpid.server.model.GroupProvider;
-import org.apache.qpid.server.security.group.FileGroupDatabase;
 import org.apache.qpid.test.utils.TestFileUtils;
 import org.apache.qpid.test.utils.UnitTestBase;
 
@@ -78,15 +76,9 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
     @After
     public void tearDown() throws Exception
     {
-        try
-        {
-            if (_groupFile.exists())
-            {
-                _groupFile.delete();
-            }
-        }
-        finally
+        if (_groupFile.exists())
         {
+            _groupFile.delete();
         }
     }
 
@@ -119,8 +111,8 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
     public void testValidationOnCreateWithInvalidGroups()
     {
         _groupFile = TestFileUtils.createTempFile(this, "groups", "=blah");
-        Map<String, Object> attributes = new HashMap<>();
-        String groupsFile = _groupFile.getAbsolutePath();
+        final Map<String, Object> attributes = new HashMap<>();
+        final String groupsFile = _groupFile.getAbsolutePath();
         attributes.put(FileBasedGroupProvider.TYPE, GROUP_FILE_PROVIDER_TYPE);
         attributes.put(FileBasedGroupProvider.PATH, groupsFile);
         attributes.put(FileBasedGroupProvider.NAME, getTestName());
@@ -144,30 +136,21 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
         Map<String, Set<String>> input = new HashMap<>();
         input.put("super", Sets.newHashSet("root"));
 
-        _groupFile = createTemporaryGroupFile(input);
-
-        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());
+        final GroupProvider<?> provider = createGroupProvider(input);
 
-        @SuppressWarnings("unchecked")
-        GroupProvider<?> provider = _objectFactory.create(GroupProvider.class, providerAttrs, _broker);
-
-        Set<Principal> adminGroups = provider.getGroupPrincipalsForUser(() -> "root");
+        final Set<Principal> adminGroups = provider.getGroupPrincipalsForUser(() -> "root");
         assertThat("root has unexpected group membership",
                    adminGroups.stream().map(Principal::getName).collect(Collectors.toSet()),
                    containsInAnyOrder("super"));
 
-        Collection<Group> groups = provider.getChildren(Group.class);
+        final Collection<Group> groups = provider.getChildren(Group.class);
         assertThat(groups.size(), is(equalTo(1)));
-        Group<?> superGroup = groups.iterator().next();
+        final Group<?> superGroup = groups.iterator().next();
         assertThat(superGroup.getName(), is(equalTo("super")));
 
-        Collection<GroupMember> members = superGroup.getChildren(GroupMember.class);
+        final Collection<GroupMember> members = superGroup.getChildren(GroupMember.class);
         assertThat(members.size(), is(equalTo(1)));
-        GroupMember rootMember = members.iterator().next();
+        final GroupMember<?> rootMember = members.iterator().next();
         assertThat(rootMember.getName(), is(equalTo("root")));
     }
 
@@ -175,29 +158,20 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
     @Test
     public void testGetGroupPrincipalsForUserCaseAware() throws Exception
     {
-        Map<String, Set<String>> input = new HashMap<>();
+        final Map<String, Set<String>> input = new HashMap<>();
         input.put("super", Sets.newHashSet("root"));
 
-        _groupFile = createTemporaryGroupFile(input);
-
-        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);
+        final GroupProvider<?> provider = createGroupProvider(input);
         assertThat(provider, is(instanceOf(FileBasedGroupProvider.class)));
-        assertThat(((FileBasedGroupProvider) provider).isCaseSensitive(), is(true));
+        assertThat(((FileBasedGroupProvider<?>) provider).isCaseSensitive(), is(true));
 
-        Set<Principal> adminGroups = provider.getGroupPrincipalsForUser(() -> "Root");
+        final Set<Principal> adminGroups = provider.getGroupPrincipalsForUser(() -> "Root");
         assertThat("No group should be found when caseSensitive=true",
                    adminGroups.stream().map(Principal::getName).collect(Collectors.toSet()),
                    is(empty()));
 
         provider.setAttributes(Collections.singletonMap("caseSensitive", false));
-        assertThat(((FileBasedGroupProvider) provider).isCaseSensitive(), is(false));
+        assertThat(((FileBasedGroupProvider<?>) provider).isCaseSensitive(), is(false));
         Set<Principal> adminGroups2 = provider.getGroupPrincipalsForUser(() -> "Root");
         assertThat("root has unexpected group membership",
                    adminGroups2.stream().map(Principal::getName).collect(Collectors.toSet()),
@@ -207,54 +181,36 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
     @Test
     public void testAddGroupAndMember() 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);
+        final GroupProvider<?> provider = createGroupProvider(Collections.emptyMap());
 
         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 String groupName = "supers";
+        final Group<?> superGroup = provider.createChild(Group.class, Collections.singletonMap(Group.NAME, groupName));
+        assertThat(superGroup.getName(), is(equalTo(groupName)));
 
         final Map<String, Object> memberAttrs = Collections.singletonMap(GroupMember.NAME, "root");
-        GroupMember rootMember = (GroupMember) superGroup.createChild(GroupMember.class, memberAttrs);
+        final GroupMember<?> rootMember = (GroupMember<?>) superGroup.createChild(GroupMember.class, memberAttrs);
         assertThat(rootMember.getName(), is(equalTo("root")));
     }
 
     @Test
     public void testRemoveGroupAndMember() throws Exception
     {
-        Map<String, Set<String>> input = new HashMap<>();
+        final Map<String, Set<String>> input = new HashMap<>();
         input.put("supers", Sets.newHashSet("root"));
         input.put("operators", Sets.newHashSet("operator", "root"));
 
-        _groupFile = createTemporaryGroupFile(input);
-
-        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);
+        final GroupProvider<?> provider = createGroupProvider(input);
 
         assertThat(provider.getChildren(Group.class).size(), is(equalTo(2)));
 
-        Group operators = provider.getChildByName(Group.class, "operators");
-        GroupMember rootMember = (GroupMember) operators.getChildByName(GroupMember.class, "root");
+        final Group<?> operators = provider.getChildByName(Group.class, "operators");
+        final GroupMember<?> rootMember = (GroupMember<?>) operators.getChildByName(GroupMember.class, "root");
         rootMember.delete();
 
         assertThat(operators.getChildren(GroupMember.class).size(), is(equalTo(1)));
-        Group supers = provider.getChildByName(Group.class, "supers");
+        final Group<?> supers = provider.getChildByName(Group.class, "supers");
         assertThat(supers.getChildren(GroupMember.class).size(), is(equalTo(1)));
 
         operators.delete();
@@ -266,8 +222,8 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
     {
         _groupFile = createTemporaryGroupFile(Collections.emptyMap());
 
-        Map<String, Object> providerAttrs = new HashMap<>();
-        String groupsFile = _groupFile.getAbsolutePath();
+        final Map<String, Object> providerAttrs = new HashMap<>();
+        final String groupsFile = _groupFile.getAbsolutePath();
         providerAttrs.put(FileBasedGroupProvider.TYPE, GROUP_FILE_PROVIDER_TYPE);
         providerAttrs.put(FileBasedGroupProvider.PATH, groupsFile);
         providerAttrs.put(FileBasedGroupProvider.NAME, getTestName());
@@ -278,7 +234,7 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
             assertThat(provider.getChildren(Group.class).size(), is(equalTo(0)));
 
             final Map<String, Object> groupAttrs = Collections.singletonMap(Group.NAME, "group");
-            final Group group = provider.createChild(Group.class, groupAttrs);
+            final Group<?> group = provider.createChild(Group.class, groupAttrs);
 
             final Map<String, Object> memberAttrs = Collections.singletonMap(GroupMember.NAME, "user");
             group.createChild(GroupMember.class, memberAttrs);
@@ -291,9 +247,9 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
                     _objectFactory.create(GroupProvider.class, providerAttrs, _broker);
             assertThat(provider.getChildren(Group.class).size(), is(equalTo(1)));
 
-            final Group group = provider.getChildByName(Group.class, "group");
+            final Group<?> group = provider.getChildByName(Group.class, "group");
             assertThat(group.getChildren(GroupMember.class).size(), is(equalTo(1)));
-            final GroupMember member = (GroupMember) group.getChildByName(GroupMember.class, "user");
+            final GroupMember<?> member = (GroupMember<?>) group.getChildByName(GroupMember.class, "user");
 
             member.delete();
             provider.close();
@@ -302,7 +258,7 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
         {
             @SuppressWarnings("unchecked") final GroupProvider<?> provider =
                     _objectFactory.create(GroupProvider.class, providerAttrs, _broker);
-            final Group group = provider.getChildByName(Group.class, "group");
+            final Group<?> group = provider.getChildByName(Group.class, "group");
             assertThat(group.getChildren(GroupMember.class).size(), is(equalTo(0)));
 
             group.delete();
@@ -320,16 +276,7 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
     @Test
     public void testProvideDelete() 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") final GroupProvider<?> provider =
-                _objectFactory.create(GroupProvider.class, providerAttrs, _broker);
+        final GroupProvider<?> provider = createGroupProvider(Collections.emptyMap());
 
         provider.delete();
         assertThat(_groupFile.exists(), is(equalTo(false)));
@@ -370,106 +317,75 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
     @Test
     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);
+        final GroupProvider<?> provider = createGroupProvider(Collections.emptyMap());
 
         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 String groupName = "supers";
+        final Group<?> superGroup = provider.createChild(Group.class, Collections.singletonMap(Group.NAME, groupName));
+        assertThat(superGroup.getName(), is(equalTo(groupName)));
 
-        assertThrows("Group member with name root1 already exists", IllegalConfigurationException.class, ()->provider.createChild(Group.class, groupAttrs));
+        assertThrows("Group member with name root1 already exists",
+                     IllegalConfigurationException.class,
+                     () -> provider.createChild(Group.class, Collections.singletonMap(Group.NAME, groupName)));
     }
 
     @Test
     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);
+        final GroupProvider<?> provider = createGroupProvider(Collections.emptyMap());
 
         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 String groupName = "supers";
+        final Group<?> superGroup = provider.createChild(Group.class, Collections.singletonMap(Group.NAME, groupName));
+        assertThat(superGroup.getName(), is(equalTo(groupName)));
 
-        final Map<String, Object> memberAttrs1 = Collections.singletonMap(GroupMember.NAME, "root1");
-        GroupMember rootMember = (GroupMember) superGroup.createChild(GroupMember.class, memberAttrs1);
-        assertThat(rootMember.getName(), is(equalTo("root1")));
+        final String memberName = "root1";
+        final Map<String, Object> memberAttributes = Collections.singletonMap(GroupMember.NAME, memberName);
+        final GroupMember<?> rootMember = (GroupMember<?>) superGroup.createChild(GroupMember.class, memberAttributes);
+        assertThat(rootMember.getName(), is(equalTo(memberName)));
 
-        assertThrows("Group member with name root1 already exists", IllegalConfigurationException.class, ()->superGroup.createChild(GroupMember.class, memberAttrs1));
+        assertThrows("Group member with name root1 already exists",
+                     IllegalConfigurationException.class,
+                     () -> superGroup.createChild(GroupMember.class, memberAttributes));
         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);
+        final GroupProvider<?> provider = createGroupProvider(Collections.emptyMap());
 
         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 String groupName = "supers";
+        final Group<?> superGroup = provider.createChild(Group.class, Collections.singletonMap(Group.NAME, groupName));
+        assertThat(superGroup.getName(), is(equalTo(groupName)));
 
-        final Map<String, Object> groupAttrs2 = Collections.singletonMap(Group.NAME, "Supers");
-        Group superGroup2 = provider.createChild(Group.class, groupAttrs2);
-        assertThat(superGroup2.getName(), is(equalTo("Supers")));
+        final String groupName2 = "Supers";
+        final Group<?> superGroup2 = provider.createChild(Group.class, Collections.singletonMap(Group.NAME, groupName2));
+        assertThat(superGroup2.getName(), is(equalTo(groupName2)));
 
         assertThat(provider.getChildren(Group.class).size(), is(equalTo(2)));
     }
 
-    @Test(expected = IllegalConfigurationException.class)
+    @Test
     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);
+        final GroupProvider<?> provider = createGroupProvider(Collections.emptyMap());
 
         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 String groupName = "supers";
+        final Group<?> superGroup = provider.createChild(Group.class, Collections.singletonMap(Group.NAME, groupName));
+        assertThat(superGroup.getName(), is(equalTo(groupName)));
 
-        final Map<String, Object> memberAttrs1 = Collections.singletonMap(GroupMember.NAME, "root1");
-        GroupMember rootMember = (GroupMember) superGroup.createChild(GroupMember.class, memberAttrs1);
-        assertThat(rootMember.getName(), is(equalTo("root1")));
+        final String memberName = "root1";
+        final Map<String, Object> memberAttrs1 = Collections.singletonMap(GroupMember.NAME, memberName);
+        final GroupMember<?> rootMember = superGroup.createChild(GroupMember.class, memberAttrs1);
+        assertThat(rootMember.getName(), is(equalTo(memberName)));
 
-        superGroup.createChild(GroupMember.class, memberAttrs1);
         assertThrows("Group member with name root1 already exists",
                      IllegalConfigurationException.class,
                      () -> superGroup.createChild(GroupMember.class, memberAttrs1));
@@ -483,12 +399,10 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
 
         Properties props = new Properties();
 
-        Map<String, String> m = groups.entrySet()
+        final Map<String, String> m = groups.entrySet()
                                       .stream()
                                       .collect(Collectors.toMap(e -> e.getKey() + ".users",
-                                                                e -> e.getValue()
-                                                                      .stream()
-                                                                      .collect(Collectors.joining(","))));
+                                                                e -> String.join(",", e.getValue())));
         props.putAll(m);
         try (final FileOutputStream out = new FileOutputStream(groupFile))
         {
@@ -496,4 +410,17 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
         }
         return groupFile;
     }
+
+    private GroupProvider<?> createGroupProvider(final Map<String, Set<String>> objectObjectMap) throws Exception
+    {
+        _groupFile = createTemporaryGroupFile(objectObjectMap);
+
+        final Map<String, Object> providerAttrs = new HashMap<>();
+        final String groupsFile = _groupFile.getAbsolutePath();
+        providerAttrs.put(FileBasedGroupProvider.TYPE, GROUP_FILE_PROVIDER_TYPE);
+        providerAttrs.put(FileBasedGroupProvider.PATH, groupsFile);
+        providerAttrs.put(FileBasedGroupProvider.NAME, getTestName());
+
+        return _objectFactory.create(GroupProvider.class, providerAttrs, _broker);
+    }
 }
diff --git a/broker-core/src/test/java/org/apache/qpid/server/security/group/FileGroupDatabaseCaseInsensitiveTest.java b/broker-core/src/test/java/org/apache/qpid/server/security/group/FileGroupDatabaseCaseInsensitiveTest.java
index 2288977..deb6479 100644
--- a/broker-core/src/test/java/org/apache/qpid/server/security/group/FileGroupDatabaseCaseInsensitiveTest.java
+++ b/broker-core/src/test/java/org/apache/qpid/server/security/group/FileGroupDatabaseCaseInsensitiveTest.java
@@ -36,7 +36,6 @@ import org.junit.Before;
 import org.junit.Test;
 
 import org.apache.qpid.server.configuration.IllegalConfigurationException;
-import org.apache.qpid.server.model.Group;
 import org.apache.qpid.server.model.adapter.FileBasedGroupProvider;
 import org.apache.qpid.test.utils.UnitTestBase;
 
@@ -54,7 +53,7 @@ public class FileGroupDatabaseCaseInsensitiveTest extends UnitTestBase
     private FileGroupDatabase _fileGroupDatabase;
     private GroupProviderUtil _util;
     private String _groupFile;
-    private FileBasedGroupProvider _groupProvider;
+    private FileBasedGroupProvider<?> _groupProvider;
 
     @Before
     public void setUp() throws IOException
@@ -171,15 +170,15 @@ public class FileGroupDatabaseCaseInsensitiveTest extends UnitTestBase
         assertTrue(groups.contains(MY_GROUP1));
     }
 
-    @Test(expected = IllegalConfigurationException.class)
+    @Test
     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, ()->_fileGroupDatabase.addUserToGroup(USER1, MY_GROUP));
 
+        _fileGroupDatabase.getGroupsForUser(USER1.toUpperCase());
+        assertThrows("Group with name supers already exists",
+                     IllegalConfigurationException.class,
+                     () -> _fileGroupDatabase.addUserToGroup(USER1.toUpperCase(), MY_GROUP));
     }
 
     @Test
@@ -372,7 +371,9 @@ public class FileGroupDatabaseCaseInsensitiveTest extends UnitTestBase
         assertEquals(1, groups.size());
         assertTrue(groups.contains(MY_GROUP));
 
-        assertThrows("Group with name myGroup already exists", IllegalConfigurationException.class, ()->_fileGroupDatabase.createGroup(MY_GROUP));
+        assertThrows("Group with name myGroup already exists",
+                     IllegalConfigurationException.class,
+                     () -> _fileGroupDatabase.createGroup(MY_GROUP.toUpperCase()));
     }
 
     @Test
diff --git a/broker-core/src/test/java/org/apache/qpid/server/security/group/FileGroupDatabaseTest.java b/broker-core/src/test/java/org/apache/qpid/server/security/group/FileGroupDatabaseTest.java
index 4a6d269..58f4f20 100644
--- a/broker-core/src/test/java/org/apache/qpid/server/security/group/FileGroupDatabaseTest.java
+++ b/broker-core/src/test/java/org/apache/qpid/server/security/group/FileGroupDatabaseTest.java
@@ -222,7 +222,6 @@ public class FileGroupDatabaseTest extends UnitTestBase
     {
         _util.writeAndSetGroupFile("myGroup.users", USER1);
         _fileGroupDatabase.addUserToGroup(USER1, MY_GROUP);
-
     }
 
     @Test

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org


[qpid-broker-j] 01/03: QPID-8525:[Broker-J]Fixed child deletion issue in group providers

Posted by or...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

orudyy pushed a commit to branch 8.0.x
in repository https://gitbox.apache.org/repos/asf/qpid-broker-j.git

commit 870e05b04b1db3347e439ac8c0c677c01de497be
Author: Dedeepya T <de...@yahoo.co.in>
AuthorDate: Tue Jun 8 10:53:18 2021 +0530

    QPID-8525:[Broker-J]Fixed child deletion issue in group providers
    
    This closes #88
    
    (cherry picked from commit db639ff523b2cf28f8a64b2d06e54b5534182111)
---
 .../model/adapter/FileBasedGroupProviderImpl.java  |  14 +-
 .../server/security/group/FileGroupDatabase.java   |  57 ++++++--
 .../adapter/FileBasedGroupProviderImplTest.java    | 156 ++++++++++++++++++---
 .../FileGroupDatabaseCaseInsensitiveTest.java      |  29 +++-
 .../security/group/FileGroupDatabaseTest.java      |  13 +-
 5 files changed, 213 insertions(+), 56 deletions(-)

diff --git a/broker-core/src/main/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImpl.java b/broker-core/src/main/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImpl.java
index 7760f7b..8e108ec 100644
--- a/broker-core/src/main/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImpl.java
+++ b/broker-core/src/main/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImpl.java
@@ -203,19 +203,19 @@ public class FileBasedGroupProviderImpl
 
             if (getState() != State.ACTIVE)
             {
-                throw new IllegalConfigurationException(String.format("Group provider '%s' is not activated. Cannot create a group.", getName()));
+                throw new IllegalConfigurationException(String.format(
+                        "Group provider '%s' is not activated. Cannot create a group.",
+                        getName()));
             }
-
             _groupDatabase.createGroup(groupName);
 
-            Map<String,Object> attrMap = new HashMap<String, Object>();
+            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
         {
@@ -359,16 +359,14 @@ public class FileBasedGroupProviderImpl
             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);
+                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);
-
             }
             else
             {
diff --git a/broker-core/src/main/java/org/apache/qpid/server/security/group/FileGroupDatabase.java b/broker-core/src/main/java/org/apache/qpid/server/security/group/FileGroupDatabase.java
index 8f08ccb..8d908e6 100644
--- a/broker-core/src/main/java/org/apache/qpid/server/security/group/FileGroupDatabase.java
+++ b/broker-core/src/main/java/org/apache/qpid/server/security/group/FileGroupDatabase.java
@@ -36,6 +36,7 @@ import com.google.common.base.Joiner;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
 import org.apache.qpid.server.model.adapter.FileBasedGroupProvider;
 import org.apache.qpid.server.util.BaseAction;
 import org.apache.qpid.server.util.FileHelper;
@@ -104,8 +105,10 @@ public class FileGroupDatabase implements GroupDatabase
     @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
@@ -113,16 +116,30 @@ public class FileGroupDatabase implements GroupDatabase
                                                + user
                                                + " to it");
         }
+        else if (groupUsers.contains(userKey))
+        {
+            throw new IllegalConfigurationException(String.format("Group member with name '%s' already exists", user));
+        }
 
-        users.add(keySearch(users, user));
-
-        Set<String> groups = _userToGroupMap.get(keySearch(_userToGroupMap.keySet(), user));
+        final String groupUserKey = keySearch(groupUsers, user);
+        if (!userKey.equals(groupUserKey))
+        {
+            throw new IllegalConfigurationException(String.format(
+                    "Inconsistent data: user  key '%s' is not equal to a group key '%s'",
+                    userKey,
+                    groupKey));
+        }
+        else
+        {
+            groupUsers.add(groupUserKey);
+        }
+        Set<String> groups = _userToGroupMap.get(userKey);
         if (groups == null)
         {
             groups = new ConcurrentSkipListSet<String>();
             _userToGroupMap.put(user, groups);
         }
-        groups.add(keySearch(_groupToUserMap.keySet(), group));
+        groups.add(groupKey);
 
         update();
     }
@@ -174,10 +191,32 @@ public class FileGroupDatabase implements GroupDatabase
     @Override
     public synchronized void createGroup(String group)
     {
-        Set<String> users = new ConcurrentSkipListSet<String>();
-        _groupToUserMap.put(group, users);
+        if (!exists(group, _groupToUserMap.keySet()))
+        {
+            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 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);
     }
 
     @Override
diff --git a/broker-core/src/test/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImplTest.java b/broker-core/src/test/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImplTest.java
index 8a5c32e..410c913 100644
--- a/broker-core/src/test/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImplTest.java
+++ b/broker-core/src/test/java/org/apache/qpid/server/model/adapter/FileBasedGroupProviderImplTest.java
@@ -29,6 +29,7 @@ import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.fail;
 import static org.mockito.Mockito.when;
 
@@ -45,6 +46,7 @@ import java.util.stream.Collectors;
 
 import com.google.common.collect.Sets;
 import org.junit.After;
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -55,6 +57,7 @@ import org.apache.qpid.server.model.ConfiguredObjectFactory;
 import org.apache.qpid.server.model.Group;
 import org.apache.qpid.server.model.GroupMember;
 import org.apache.qpid.server.model.GroupProvider;
+import org.apache.qpid.server.security.group.FileGroupDatabase;
 import org.apache.qpid.test.utils.TestFileUtils;
 import org.apache.qpid.test.utils.UnitTestBase;
 
@@ -90,7 +93,7 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
     @Test
     public void testValidationOnCreateWithInvalidPath()
     {
-        Map<String,Object> attributes = new HashMap<>();
+        Map<String, Object> attributes = new HashMap<>();
         _groupFile = TestFileUtils.createTempFile(this, "groups");
 
         String groupsFile = _groupFile.getAbsolutePath() + File.separator + "groups";
@@ -103,12 +106,12 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
         {
             _objectFactory.create(GroupProvider.class, attributes, _broker);
             fail("Exception is expected on validation of groups provider with invalid path");
-        } catch (IllegalConfigurationException e)
+        }
+        catch (IllegalConfigurationException e)
         {
             assertEquals("Unexpected exception message:" + e.getMessage(),
-                                String.format("Cannot create groups file at '%s'", groupsFile),
-                                e.getMessage());
-
+                         String.format("Cannot create groups file at '%s'", groupsFile),
+                         e.getMessage());
         }
     }
 
@@ -130,8 +133,8 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
         catch (IllegalConfigurationException e)
         {
             assertEquals("Unexpected exception message:" + e.getMessage(),
-                                String.format("Cannot load groups from '%s'", groupsFile),
-                                e.getMessage());
+                         String.format("Cannot load groups from '%s'", groupsFile),
+                         e.getMessage());
         }
     }
 
@@ -186,15 +189,15 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
         @SuppressWarnings("unchecked")
         GroupProvider<?> provider = _objectFactory.create(GroupProvider.class, providerAttrs, _broker);
         assertThat(provider, is(instanceOf(FileBasedGroupProvider.class)));
-        assertThat(((FileBasedGroupProvider)provider).isCaseSensitive(), is(true));
+        assertThat(((FileBasedGroupProvider) provider).isCaseSensitive(), is(true));
 
         Set<Principal> adminGroups = provider.getGroupPrincipalsForUser(() -> "Root");
         assertThat("No group should be found when caseSensitive=true",
                    adminGroups.stream().map(Principal::getName).collect(Collectors.toSet()),
-                  is(empty()));
+                   is(empty()));
 
         provider.setAttributes(Collections.singletonMap("caseSensitive", false));
-        assertThat(((FileBasedGroupProvider)provider).isCaseSensitive(), is(false));
+        assertThat(((FileBasedGroupProvider) provider).isCaseSensitive(), is(false));
         Set<Principal> adminGroups2 = provider.getGroupPrincipalsForUser(() -> "Root");
         assertThat("root has unexpected group membership",
                    adminGroups2.stream().map(Principal::getName).collect(Collectors.toSet()),
@@ -230,7 +233,7 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
     public void testRemoveGroupAndMember() throws Exception
     {
         Map<String, Set<String>> input = new HashMap<>();
-        input.put("supers", Sets.newHashSet( "root"));
+        input.put("supers", Sets.newHashSet("root"));
         input.put("operators", Sets.newHashSet("operator", "root"));
 
         _groupFile = createTemporaryGroupFile(input);
@@ -270,8 +273,8 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
         providerAttrs.put(FileBasedGroupProvider.NAME, getTestName());
 
         {
-            @SuppressWarnings("unchecked")
-            final GroupProvider<?> provider = _objectFactory.create(GroupProvider.class, providerAttrs, _broker);
+            @SuppressWarnings("unchecked") final 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, "group");
@@ -284,8 +287,8 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
         }
 
         {
-            @SuppressWarnings("unchecked")
-            final GroupProvider<?> provider = _objectFactory.create(GroupProvider.class, providerAttrs, _broker);
+            @SuppressWarnings("unchecked") final GroupProvider<?> provider =
+                    _objectFactory.create(GroupProvider.class, providerAttrs, _broker);
             assertThat(provider.getChildren(Group.class).size(), is(equalTo(1)));
 
             final Group group = provider.getChildByName(Group.class, "group");
@@ -297,8 +300,8 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
         }
 
         {
-            @SuppressWarnings("unchecked")
-            final GroupProvider<?> provider = _objectFactory.create(GroupProvider.class, providerAttrs, _broker);
+            @SuppressWarnings("unchecked") final GroupProvider<?> provider =
+                    _objectFactory.create(GroupProvider.class, providerAttrs, _broker);
             final Group group = provider.getChildByName(Group.class, "group");
             assertThat(group.getChildren(GroupMember.class).size(), is(equalTo(0)));
 
@@ -307,8 +310,8 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
         }
 
         {
-            @SuppressWarnings("unchecked")
-            final GroupProvider<?> provider = _objectFactory.create(GroupProvider.class, providerAttrs, _broker);
+            @SuppressWarnings("unchecked") final GroupProvider<?> provider =
+                    _objectFactory.create(GroupProvider.class, providerAttrs, _broker);
             assertThat(provider.getChildren(Group.class).size(), is(equalTo(0)));
             provider.close();
         }
@@ -325,8 +328,8 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
         providerAttrs.put(FileBasedGroupProvider.PATH, groupsFile);
         providerAttrs.put(FileBasedGroupProvider.NAME, getTestName());
 
-        @SuppressWarnings("unchecked")
-        final GroupProvider<?> provider = _objectFactory.create(GroupProvider.class, providerAttrs, _broker);
+        @SuppressWarnings("unchecked") final GroupProvider<?> provider =
+                _objectFactory.create(GroupProvider.class, providerAttrs, _broker);
 
         provider.delete();
         assertThat(_groupFile.exists(), is(equalTo(false)));
@@ -364,6 +367,115 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
         }
     }
 
+    @Test
+    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")));
+
+        assertThrows("Group member with name root1 already exists", IllegalConfigurationException.class, ()->provider.createChild(Group.class, groupAttrs));
+    }
+
+    @Test
+    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")));
+
+        assertThrows("Group member with name root1 already exists", IllegalConfigurationException.class, ()->superGroup.createChild(GroupMember.class, memberAttrs1));
+        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,
+                     () -> superGroup.createChild(GroupMember.class, memberAttrs1));
+        assertThat(superGroup.getChildren(GroupMember.class).size(), is(equalTo(1)));
+    }
+
     private File createTemporaryGroupFile(Map<String, Set<String>> groups) throws Exception
     {
         File groupFile = File.createTempFile("group", "grp");
@@ -378,7 +490,7 @@ public class FileBasedGroupProviderImplTest extends UnitTestBase
                                                                       .stream()
                                                                       .collect(Collectors.joining(","))));
         props.putAll(m);
-        try(final FileOutputStream out = new FileOutputStream(groupFile))
+        try (final FileOutputStream out = new FileOutputStream(groupFile))
         {
             props.store(out, "test group file");
         }
diff --git a/broker-core/src/test/java/org/apache/qpid/server/security/group/FileGroupDatabaseCaseInsensitiveTest.java b/broker-core/src/test/java/org/apache/qpid/server/security/group/FileGroupDatabaseCaseInsensitiveTest.java
index 2f0a923..2288977 100644
--- a/broker-core/src/test/java/org/apache/qpid/server/security/group/FileGroupDatabaseCaseInsensitiveTest.java
+++ b/broker-core/src/test/java/org/apache/qpid/server/security/group/FileGroupDatabaseCaseInsensitiveTest.java
@@ -21,6 +21,7 @@ package org.apache.qpid.server.security.group;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.Mockito.mock;
@@ -34,6 +35,8 @@ import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
+import org.apache.qpid.server.model.Group;
 import org.apache.qpid.server.model.adapter.FileBasedGroupProvider;
 import org.apache.qpid.test.utils.UnitTestBase;
 
@@ -168,20 +171,15 @@ public class FileGroupDatabaseCaseInsensitiveTest extends UnitTestBase
         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, ()->_fileGroupDatabase.addUserToGroup(USER1, MY_GROUP));
 
-        assertEquals(1, groups.size());
-        assertTrue(groups.contains(MY_GROUP));
-
-        Set<String> users = _fileGroupDatabase.getUsersInGroup(MY_GROUP.toUpperCase());
-        assertEquals(1, users.size());
-        assertTrue(users.contains(USER1));
     }
 
     @Test
@@ -361,6 +359,23 @@ public class FileGroupDatabaseCaseInsensitiveTest extends UnitTestBase
     }
 
     @Test
+    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));
+
+        assertThrows("Group with name myGroup already exists", IllegalConfigurationException.class, ()->_fileGroupDatabase.createGroup(MY_GROUP));
+    }
+
+    @Test
     public void testRemoveGroupPersistedToFileCaseInsensitive() throws Exception
     {
         _util.writeAndSetGroupFile("myGroup1.users", "user1,user2", "myGroup2.users", "user1,user2");
diff --git a/broker-core/src/test/java/org/apache/qpid/server/security/group/FileGroupDatabaseTest.java b/broker-core/src/test/java/org/apache/qpid/server/security/group/FileGroupDatabaseTest.java
index 0ccbe3c..4a6d269 100644
--- a/broker-core/src/test/java/org/apache/qpid/server/security/group/FileGroupDatabaseTest.java
+++ b/broker-core/src/test/java/org/apache/qpid/server/security/group/FileGroupDatabaseTest.java
@@ -21,6 +21,7 @@ package org.apache.qpid.server.security.group;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.Mockito.mock;
@@ -34,6 +35,7 @@ import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
 import org.apache.qpid.server.model.adapter.FileBasedGroupProvider;
 import org.apache.qpid.test.utils.UnitTestBase;
 
@@ -215,21 +217,12 @@ public class FileGroupDatabaseTest extends UnitTestBase
         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);
 
-        Set<String> groups = _fileGroupDatabase.getGroupsForUser(USER1);
-
-        assertEquals(1, groups.size());
-        assertTrue(groups.contains(MY_GROUP));
-
-        Set<String> users = _fileGroupDatabase.getUsersInGroup(MY_GROUP);
-        assertEquals(1, users.size());
-        assertTrue(users.contains(USER1));
-        assertTrue(_fileGroupDatabase.getGroupsForUser(MY_GROUP.toUpperCase()).isEmpty());
     }
 
     @Test

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org