# HG changeset patch
# User Severin Gehwolf <sgehwolf@redhat.com>
# Date 1370250186 -7200
# Node ID bb4bde056ead8a8c9a2aa215d8dbd25651a5ad40
# Parent  aa48ed4752c164c22eea285174926afd12a0bb53
Fix bug in expansion of recursive roles.
Reviewed-by: neugens
Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2013-June/006919.html

diff -r aa48ed4752c1 -r bb4bde056ead web/server/src/main/java/com/redhat/thermostat/web/server/auth/spi/RolesAmender.java
--- a/web/server/src/main/java/com/redhat/thermostat/web/server/auth/spi/RolesAmender.java	Wed Jun 05 11:14:27 2013 +0200
+++ b/web/server/src/main/java/com/redhat/thermostat/web/server/auth/spi/RolesAmender.java	Mon Jun 03 11:03:06 2013 +0200
@@ -197,10 +197,10 @@
      * Add roles to users which are member of a recursive role
      */
     private void expandRoles(RolesInfo recRole) {
-        @SuppressWarnings("unchecked") // we've added them so safe
-        Enumeration<BasicRole> members = (Enumeration<BasicRole>)recRole.getRole().members();
         Iterator<String> memberUsersIterator = recRole.getMemberUsers().iterator();
         while (memberUsersIterator.hasNext()) {
+            @SuppressWarnings("unchecked") // we've added them so safe
+            Enumeration<BasicRole> members = (Enumeration<BasicRole>)recRole.getRole().members();
             String username = memberUsersIterator.next();
             Set<BasicRole> userRoles = rolesMap.get(username);
             while (members.hasMoreElements()) {
diff -r aa48ed4752c1 -r bb4bde056ead web/server/src/test/java/com/redhat/thermostat/web/server/auth/spi/PropertiesUsernameRolesLoginModuleTest.java
--- a/web/server/src/test/java/com/redhat/thermostat/web/server/auth/spi/PropertiesUsernameRolesLoginModuleTest.java	Wed Jun 05 11:14:27 2013 +0200
+++ b/web/server/src/test/java/com/redhat/thermostat/web/server/auth/spi/PropertiesUsernameRolesLoginModuleTest.java	Mon Jun 03 11:03:06 2013 +0200
@@ -211,6 +211,56 @@
     }
     
     @Test
+    public void testRecursiveRolesMultiple() throws LoginException {
+        LoginModule loginModule = new PropertiesUsernameRolesLoginModule();
+        mockCallBack = new SimpleCallBackHandler("user3", "password".toCharArray());
+        URL userFile = this.getClass().getResource("/properties_module_test_users.properties");
+        URL rolesFile = this.getClass().getResource("/properties_module_test_roles.properties");
+        mockOptions.put("users.properties", userFile.getFile());
+        mockOptions.put("roles.properties", rolesFile.getFile());
+        loginModule.initialize(subject, mockCallBack, mockSharedState, mockOptions);
+        assertTrue(loginModule.login());
+        try {
+            boolean retval = loginModule.commit();
+            // pass
+            assertTrue("Commit should have returned true", retval);
+        } catch (LoginException e) {
+            fail("'user3' should have been able to commit");
+        }
+        // assert principals are added to subject
+        Set<Principal> principals = subject.getPrincipals();
+        // other-role, role1, Roles, user3
+        assertEquals(4, principals.size());
+        assertTrue(principals.contains(new RolePrincipal("role1")));
+        assertTrue(principals.contains(new RolePrincipal("Roles")));
+        // via recursive role 'role1'
+        assertTrue(principals.contains(new RolePrincipal("other-role")));
+        assertTrue(principals.contains(new UserPrincipal("user3")));
+        loginModule = new PropertiesUsernameRolesLoginModule();
+        mockCallBack = new SimpleCallBackHandler("user2", "password".toCharArray());
+        subject = new Subject();
+        loginModule.initialize(subject, mockCallBack, mockSharedState, mockOptions);
+        assertTrue(loginModule.login());
+        try {
+            boolean retval = loginModule.commit();
+            // pass
+            assertTrue("Commit should have returned true", retval);
+        } catch (LoginException e) {
+            fail("'user2' should have been able to commit");
+        }
+        // assert principals are added to subject
+        principals = subject.getPrincipals();
+        // new-role, other-role, role1, Roles, user2
+        assertEquals(5, principals.size());
+        assertTrue(principals.contains(new RolePrincipal("role1")));
+        assertTrue(principals.contains(new RolePrincipal("Roles")));
+        // via recursive role 'role1'
+        assertTrue(principals.contains(new RolePrincipal("other-role")));
+        assertTrue(principals.contains(new UserPrincipal("user2")));
+        assertTrue(principals.contains(new RolePrincipal("new-role")));
+    }
+    
+    @Test
     public void cannotCommitWithoutLoggingIn() throws LoginException {
         LoginModule loginModule = new PropertiesUsernameRolesLoginModule();
         assertFalse(loginModule.commit());
diff -r aa48ed4752c1 -r bb4bde056ead web/server/src/test/java/com/redhat/thermostat/web/server/auth/spi/RolesAmenderTest.java
--- a/web/server/src/test/java/com/redhat/thermostat/web/server/auth/spi/RolesAmenderTest.java	Wed Jun 05 11:14:27 2013 +0200
+++ b/web/server/src/test/java/com/redhat/thermostat/web/server/auth/spi/RolesAmenderTest.java	Mon Jun 03 11:03:06 2013 +0200
@@ -73,6 +73,8 @@
         Set<Object> users = new HashSet<>();
         users.add("user1");
         users.add("user2");
+        users.add("someuser");
+        users.add("someotheruser");
         validUsers = Collections.unmodifiableSet(users);
     }
     
@@ -129,10 +131,60 @@
     }
     
     @Test
+    public void canParseRecursiveRolesWithMultipleMemberships() {
+        rolesAmender = new RolesAmender(validFile, validUsers);
+        Set<BasicRole> roles = rolesAmender.getRoles("user2");
+        // new-role, role1, other-role
+        assertEquals(3, roles.size());
+        Iterator<BasicRole> it = roles.iterator();
+        while (it.hasNext()) {
+            BasicRole role = it.next();
+            assertTrue(role instanceof RolePrincipal);
+            if (role.getName().equals("role1")) {
+                // nested role
+                int count = 0;
+                @SuppressWarnings("rawtypes")
+                Enumeration members = role.members();
+                while (members.hasMoreElements()) {
+                    count++;
+                    assertEquals("other-role", ((Principal)members.nextElement()).getName());
+                }
+                assertEquals(1, count);
+            }
+            if (role.getName().equals("new-role")) {
+                // not nested
+                assertFalse(role.members().hasMoreElements());
+            }
+        }
+        assertTrue(roles.contains(new RolePrincipal("other-role")));
+        // role1 is not a user
+        roles = rolesAmender.getRoles("role1");
+        // expect empty
+        assertEquals(0, roles.size());
+        // test another user which is member of the recursive role
+        roles = rolesAmender.getRoles("someuser");
+        assertEquals(
+                "Expected someuser to be a member of 'testing', 'role1' and 'other-role'",
+                3, roles.size());
+        assertTrue(roles.contains(new RolePrincipal("other-role")));
+        assertTrue(roles.contains(new RolePrincipal("testing")));
+        assertTrue(roles.contains(new RolePrincipal("role1")));
+        // and yet another user which is part of a recursive role
+        roles = rolesAmender.getRoles("someotheruser");
+        assertEquals(
+                "Expected someotheruser to be a member of 'role1' and 'other-role'",
+                2, roles.size());
+        assertTrue(roles.contains(new RolePrincipal("other-role")));
+        assertTrue(roles.contains(new RolePrincipal("role1")));
+    }
+    
+    @Test
     public void canParseRolesWithMoreUsersThanUsedInRoles() {
         Set<Object> users = new HashSet<>();
         users.add("user1");
         users.add("user2");
+        users.add("someuser");
+        users.add("someotheruser");
         users.add("user-with-no-roles");
         validUsers = Collections.unmodifiableSet(users);
         try {
diff -r aa48ed4752c1 -r bb4bde056ead web/server/src/test/resources/properties_module_test_roles.properties
--- a/web/server/src/test/resources/properties_module_test_roles.properties	Wed Jun 05 11:14:27 2013 +0200
+++ b/web/server/src/test/resources/properties_module_test_roles.properties	Mon Jun 03 11:03:06 2013 +0200
@@ -1,3 +1,4 @@
 user1 = my-role, my-role2
 user2 = new-role, role1
+user3 = role1
 role1 = other-role
\ No newline at end of file
diff -r aa48ed4752c1 -r bb4bde056ead web/server/src/test/resources/properties_module_test_users.properties
--- a/web/server/src/test/resources/properties_module_test_users.properties	Wed Jun 05 11:14:27 2013 +0200
+++ b/web/server/src/test/resources/properties_module_test_users.properties	Mon Jun 03 11:03:06 2013 +0200
@@ -1,2 +1,3 @@
 user1 = somepassword
-user2 = password
\ No newline at end of file
+user2 = password
+user3 = password
\ No newline at end of file
diff -r aa48ed4752c1 -r bb4bde056ead web/server/src/test/resources/test_roles.properties
--- a/web/server/src/test/resources/test_roles.properties	Wed Jun 05 11:14:27 2013 +0200
+++ b/web/server/src/test/resources/test_roles.properties	Mon Jun 03 11:03:06 2013 +0200
@@ -1,4 +1,6 @@
+someuser = testing, role1
 user1 = my-role, my-role2
 # empty should get skipped (', ,')
 user2 = new-role, role1, ,
-role1 = other-role
\ No newline at end of file
+role1 = other-role
+someotheruser = role1