KEYCLOAK-3006 Fix admin event inconsistencies related to roles (point…

…s 1,3,4,15,16 from JIRA)
keycloak · May 25, 2016 · 882dbc3 · 882dbc3
1 parent 022be3a
commit 882dbc3
Show file tree

Hide file tree

Showing 17 changed files with 142 additions and 105 deletions.
diff --git a/services/src/main/java/org/keycloak/services/resources/admin/ClientRoleMappingsResource.java b/services/src/main/java/org/keycloak/services/resources/admin/ClientRoleMappingsResource.java
@@ -43,6 +43,7 @@
 import java.text.MessageFormat;
 import java.util.ArrayList;
 import java.util.HashSet;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Properties;
 import java.util.Set;
@@ -196,12 +197,15 @@ public void deleteClientRoleMapping(List<RoleRepresentation> roles) {
 
         if (roles == null) {
             Set<RoleModel> roleModels = user.getClientRoleMappings(client);
+            roles = new LinkedList<>();
+
             for (RoleModel roleModel : roleModels) {
-                if (!(roleModel.getContainer() instanceof ClientModel)) {
+                if (roleModel.getContainer() instanceof ClientModel) {
                     ClientModel client = (ClientModel) roleModel.getContainer();
                     if (!client.getId().equals(this.client.getId())) continue;
                 }
                 user.deleteRoleMapping(roleModel);
+                roles.add(ModelToRepresentation.toRepresentation(roleModel));
             }
 
         } else {
@@ -220,6 +224,7 @@ public void deleteClientRoleMapping(List<RoleRepresentation> roles) {
                 }
             }
         }
+
         adminEvent.operation(OperationType.DELETE).resourcePath(uriInfo).representation(roles).success();
     }
 }
diff --git a/services/src/main/java/org/keycloak/services/resources/admin/RoleByIdResource.java b/services/src/main/java/org/keycloak/services/resources/admin/RoleByIdResource.java
@@ -226,9 +226,7 @@ public void deleteComposites(final @PathParam("role-id") String id, List<RoleRep
         auth.requireManage();
 
         RoleModel role = getRoleModel(id);
-        deleteComposites(roles, role);
-
-        adminEvent.operation(OperationType.DELETE).resourcePath(uriInfo).representation(roles).success();
+        deleteComposites(adminEvent, uriInfo, roles, role);
     }
 
 }
diff --git a/services/src/main/java/org/keycloak/services/resources/admin/RoleContainerResource.java b/services/src/main/java/org/keycloak/services/resources/admin/RoleContainerResource.java
@@ -115,7 +115,8 @@ public Response createRole(final RoleRepresentation rep) {
             boolean scopeParamRequired = rep.isScopeParamRequired()==null ? false : rep.isScopeParamRequired();
             role.setScopeParamRequired(scopeParamRequired);
 
-            adminEvent.operation(OperationType.CREATE).resourcePath(uriInfo, role.getId()).representation(rep).success();
+            rep.setId(role.getId());
+            adminEvent.operation(OperationType.CREATE).resourcePath(uriInfo, role.getName()).representation(rep).success();
 
             return Response.created(uriInfo.getAbsolutePathBuilder().path(role.getName()).build()).build();
         } catch (ModelDuplicateException e) {
@@ -332,8 +333,7 @@ public void deleteComposites(
         if (role == null) {
             throw new NotFoundException("Could not find role");
         }
-        deleteComposites(roles, role);
-        adminEvent.operation(OperationType.DELETE).resourcePath(uriInfo).success();
+        deleteComposites(adminEvent, uriInfo, roles, role);
     }
 
 }
diff --git a/services/src/main/java/org/keycloak/services/resources/admin/RoleMapperResource.java b/services/src/main/java/org/keycloak/services/resources/admin/RoleMapperResource.java
@@ -50,6 +50,7 @@
 import java.text.MessageFormat;
 import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.Properties;
@@ -236,8 +237,9 @@ public void addRealmRoleMappings(List<RoleRepresentation> roles) {
                 throw new NotFoundException("Role not found");
             }
             roleMapper.grantRole(roleModel);
-            adminEvent.operation(OperationType.CREATE).resourcePath(uriInfo, role.getId()).representation(roles).success();
         }
+
+        adminEvent.operation(OperationType.CREATE).resourcePath(uriInfo).representation(roles).success();
     }
 
     /**
@@ -258,10 +260,13 @@ public void deleteRealmRoleMappings(List<RoleRepresentation> roles) {
         logger.debug("deleteRealmRoleMappings");
         if (roles == null) {
             Set<RoleModel> roleModels = roleMapper.getRealmRoleMappings();
+            roles = new LinkedList<>();
+
             for (RoleModel roleModel : roleModels) {
                 roleMapper.deleteRoleMapping(roleModel);
+                roles.add(ModelToRepresentation.toRepresentation(roleModel));
             }
-            adminEvent.operation(OperationType.CREATE).resourcePath(uriInfo).representation(roles).success();
+
         } else {
             for (RoleRepresentation role : roles) {
                 RoleModel roleModel = realm.getRole(role.getName());
@@ -276,11 +281,12 @@ public void deleteRealmRoleMappings(List<RoleRepresentation> roles) {
                     throw new ErrorResponseException(me.getMessage(), MessageFormat.format(messages.getProperty(me.getMessage(), me.getMessage()), me.getParameters()),
                             Response.Status.BAD_REQUEST);
                 }
-
-                adminEvent.operation(OperationType.DELETE).resourcePath(uriInfo, role.getId()).representation(roles).success();
             }
+
         }
 
+        adminEvent.operation(OperationType.DELETE).resourcePath(uriInfo).representation(roles).success();
+
     }
 
     @Path("clients/{client}")

diff --git a/services/src/main/java/org/keycloak/services/resources/admin/RoleResource.java b/services/src/main/java/org/keycloak/services/resources/admin/RoleResource.java
@@ -65,9 +65,9 @@ protected void addComposites(AdminEventBuilder adminEvent, UriInfo uriInfo, List
                 throw new NotFoundException("Could not find composite role");
             }
             role.addCompositeRole(composite);
-
-            adminEvent.operation(OperationType.CREATE).resourcePath(uriInfo, rep.getId()).representation(roles).success();
         }
+
+        adminEvent.operation(OperationType.CREATE).resourcePath(uriInfo).representation(roles).success();
     }
 
     protected Set<RoleRepresentation> getRoleComposites(RoleModel role) {
@@ -102,13 +102,15 @@ protected Set<RoleRepresentation> getClientRoleComposites(ClientModel app, RoleM
         return composites;
     }
 
-    protected void deleteComposites(List<RoleRepresentation> roles, RoleModel role) {
+    protected void deleteComposites(AdminEventBuilder adminEvent, UriInfo uriInfo, List<RoleRepresentation> roles, RoleModel role) {
         for (RoleRepresentation rep : roles) {
             RoleModel composite = realm.getRoleById(rep.getId());
             if (composite == null) {
                 throw new NotFoundException("Could not find composite role");
             }
             role.removeCompositeRole(composite);
         }
+
+        adminEvent.operation(OperationType.DELETE).resourcePath(uriInfo).representation(roles).success();
     }
 }
diff --git a/services/src/main/java/org/keycloak/services/resources/admin/ScopeMappedClientResource.java b/services/src/main/java/org/keycloak/services/resources/admin/ScopeMappedClientResource.java
@@ -38,6 +38,7 @@
 import javax.ws.rs.core.MediaType;
 
 import java.util.ArrayList;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Set;
 
@@ -151,8 +152,9 @@ public void addClientScopeMapping(List<RoleRepresentation> roles) {
                 throw new NotFoundException("Role not found");
             }
             scopeContainer.addScopeMapping(roleModel);
-            adminEvent.operation(OperationType.CREATE).resourcePath(session.getContext().getUri(), roleModel.getId()).representation(roles).success();
         }
+
+        adminEvent.operation(OperationType.CREATE).resourcePath(session.getContext().getUri()).representation(roles).success();
     }
 
     /**
@@ -171,19 +173,23 @@ public void deleteClientScopeMapping(List<RoleRepresentation> roles) {
 
         if (roles == null) {
             Set<RoleModel> roleModels = KeycloakModelUtils.getClientScopeMappings(scopedClient, scopeContainer);//scopedClient.getClientScopeMappings(client);
+            roles = new LinkedList<>();
+
             for (RoleModel roleModel : roleModels) {
                 scopeContainer.deleteScopeMapping(roleModel);
+                roles.add(ModelToRepresentation.toRepresentation(roleModel));
             }
-            adminEvent.operation(OperationType.DELETE).resourcePath(session.getContext().getUri()).representation(roles).success();
+
         } else {
             for (RoleRepresentation role : roles) {
                 RoleModel roleModel = scopedClient.getRole(role.getName());
                 if (roleModel == null) {
                     throw new NotFoundException("Role not found");
                 }
                 scopeContainer.deleteScopeMapping(roleModel);
-                adminEvent.operation(OperationType.DELETE).resourcePath(session.getContext().getUri(), roleModel.getId()).representation(roles).success();
             }
         }
+
+        adminEvent.operation(OperationType.DELETE).resourcePath(session.getContext().getUri()).representation(roles).success();
     }
 }
diff --git a/services/src/main/java/org/keycloak/services/resources/admin/ScopeMappedResource.java b/services/src/main/java/org/keycloak/services/resources/admin/ScopeMappedResource.java
@@ -42,6 +42,7 @@
 
 import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -220,8 +221,9 @@ public void addRealmScopeMappings(List<RoleRepresentation> roles) {
                 throw new NotFoundException("Role not found");
             }
             scopeContainer.addScopeMapping(roleModel);
-            adminEvent.operation(OperationType.CREATE).resourcePath(session.getContext().getUri(), role.getId()).representation(roles).success();
         }
+
+        adminEvent.operation(OperationType.CREATE).resourcePath(session.getContext().getUri()).representation(roles).success();
     }
 
     /**
@@ -241,21 +243,25 @@ public void deleteRealmScopeMappings(List<RoleRepresentation> roles) {
 
         if (roles == null) {
             Set<RoleModel> roleModels = scopeContainer.getRealmScopeMappings();
+            roles = new LinkedList<>();
+
             for (RoleModel roleModel : roleModels) {
                 scopeContainer.deleteScopeMapping(roleModel);
+                roles.add(ModelToRepresentation.toRepresentation(roleModel));
             }
-            adminEvent.operation(OperationType.DELETE).resourcePath(session.getContext().getUri()).representation(roles).success();
+
        } else {
             for (RoleRepresentation role : roles) {
                 RoleModel roleModel = realm.getRoleById(role.getId());
                 if (roleModel == null) {
                     throw new NotFoundException("Client not found");
                 }
                 scopeContainer.deleteScopeMapping(roleModel);
-                adminEvent.operation(OperationType.DELETE).resourcePath(session.getContext().getUri(), roleModel.getId()).representation(roles).success();
             }
         }
 
+        adminEvent.operation(OperationType.DELETE).resourcePath(session.getContext().getUri()).representation(roles).success();
+
     }
 
     @Path("clients/{client}")

diff --git a/...egration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ClientTest.java b/...egration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ClientTest.java
@@ -17,7 +17,6 @@
 
 package org.keycloak.testsuite.admin;
 
-import org.hamcrest.Matchers;
 import org.junit.Test;
 import org.keycloak.OAuth2Constants;
 import org.keycloak.admin.client.resource.ClientResource;
@@ -43,10 +42,8 @@
 import java.util.Map;
 import java.util.Set;
 
-import org.keycloak.services.resources.admin.ScopeMappedResource;
 import org.keycloak.testsuite.Assert;
 import org.keycloak.testsuite.util.AdminEventPaths;
-import org.keycloak.testsuite.util.AssertAdminEvents;
 import org.keycloak.testsuite.util.ClientBuilder;
 import org.keycloak.testsuite.util.CredentialBuilder;
 import org.keycloak.testsuite.util.OAuthClient;
@@ -154,7 +151,7 @@ public void deleteDefaultRole() {
         RoleRepresentation role = new RoleRepresentation("test", "test", false);
         realm.clients().get(id).roles().create(role);
 
-        assertAdminEvents.assertEvent(realmId, OperationType.CREATE, Matchers.startsWith(AdminEventPaths.clientRolesResourcePath(id)), role);
+        assertAdminEvents.assertEvent(realmId, OperationType.CREATE, AdminEventPaths.clientRoleResourcePath(id, "test"), role);
 
         ClientRepresentation foundClientRep = realm.clients().get(id).toRepresentation();
         foundClientRep.setDefaultRoles(new String[]{"test"});
@@ -327,28 +324,24 @@ public void scopes() {
         realm.roles().create(roleRep1);
         realm.roles().create(roleRep2);
 
-        AssertAdminEvents.ExpectedAdminEvent adminEvent = assertAdminEvents.expect()
-                .realmId(realmId)
-                .operationType(OperationType.CREATE)
-                .resourcePath(Matchers.startsWith(AdminEventPaths.rolesResourcePath()));
-        adminEvent.representation(roleRep1).assertEvent();
-        adminEvent.representation(roleRep2).assertEvent();
+        assertAdminEvents.assertEvent(realmId, OperationType.CREATE, AdminEventPaths.roleResourcePath("role1"), roleRep1);
+        assertAdminEvents.assertEvent(realmId, OperationType.CREATE, AdminEventPaths.roleResourcePath("role2"), roleRep2);
 
         roleRep1 = realm.roles().get("role1").toRepresentation();
         roleRep2 = realm.roles().get("role2").toRepresentation();
 
         realm.roles().get("role1").addComposites(Collections.singletonList(roleRep2));
 
-        assertAdminEvents.assertEvent(realmId, OperationType.CREATE, Matchers.startsWith(AdminEventPaths.roleResourceCompositesPath("role1")));
+        assertAdminEvents.assertEvent(realmId, OperationType.CREATE, AdminEventPaths.roleResourceCompositesPath("role1"), Collections.singletonList(roleRep2));
 
         String accountMgmtId = realm.clients().findByClientId(Constants.ACCOUNT_MANAGEMENT_CLIENT_ID).get(0).getId();
         RoleRepresentation viewAccountRoleRep = realm.clients().get(accountMgmtId).roles().get(AccountRoles.VIEW_PROFILE).toRepresentation();
 
         scopesResource.realmLevel().add(Collections.singletonList(roleRep1));
-        assertAdminEvents.assertEvent(realmId, OperationType.CREATE, AdminEventPaths.clientScopeMappingsRealmLevelPath(id) + "/" + roleRep1.getId());
+        assertAdminEvents.assertEvent(realmId, OperationType.CREATE, AdminEventPaths.clientScopeMappingsRealmLevelPath(id), Collections.singletonList(roleRep1));
 
         scopesResource.clientLevel(accountMgmtId).add(Collections.singletonList(viewAccountRoleRep));
-        assertAdminEvents.assertEvent(realmId, OperationType.CREATE, AdminEventPaths.clientScopeMappingsClientLevelPath(id, accountMgmtId) + "/" + viewAccountRoleRep.getId());
+        assertAdminEvents.assertEvent(realmId, OperationType.CREATE, AdminEventPaths.clientScopeMappingsClientLevelPath(id, accountMgmtId), Collections.singletonList(viewAccountRoleRep));
 
         Assert.assertNames(scopesResource.realmLevel().listAll(), "role1");
         Assert.assertNames(scopesResource.realmLevel().listEffective(), "role1", "role2");
@@ -362,10 +355,10 @@ public void scopes() {
         Assert.assertNames(scopesResource.getAll().getClientMappings().get(Constants.ACCOUNT_MANAGEMENT_CLIENT_ID).getMappings(), AccountRoles.VIEW_PROFILE);
 
         scopesResource.realmLevel().remove(Collections.singletonList(roleRep1));
-        assertAdminEvents.assertEvent(realmId, OperationType.DELETE, AdminEventPaths.clientScopeMappingsRealmLevelPath(id) + "/" + roleRep1.getId());
+        assertAdminEvents.assertEvent(realmId, OperationType.DELETE, AdminEventPaths.clientScopeMappingsRealmLevelPath(id), Collections.singletonList(roleRep1));
 
         scopesResource.clientLevel(accountMgmtId).remove(Collections.singletonList(viewAccountRoleRep));
-        assertAdminEvents.assertEvent(realmId, OperationType.DELETE, AdminEventPaths.clientScopeMappingsClientLevelPath(id, accountMgmtId) + "/" + viewAccountRoleRep.getId());
+        assertAdminEvents.assertEvent(realmId, OperationType.DELETE, AdminEventPaths.clientScopeMappingsClientLevelPath(id, accountMgmtId), Collections.singletonList(viewAccountRoleRep));
 
         Assert.assertNames(scopesResource.realmLevel().listAll());
         Assert.assertNames(scopesResource.realmLevel().listEffective());

diff --git a/...rquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/RoleByIdResourceTest.java b/...rquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/RoleByIdResourceTest.java
@@ -17,7 +17,6 @@
 
 package org.keycloak.testsuite.admin;
 
-import org.hamcrest.Matchers;
 import org.junit.Before;
 import org.junit.Test;
 import org.keycloak.admin.client.resource.RoleByIdResource;
@@ -120,9 +119,7 @@ public void composites() {
         l.add(RoleBuilder.create().id(ids.get("role-c")).build());
         resource.addComposites(ids.get("role-a"), l);
 
-        // TODO adminEvents: Fix once composite roles events will be fixed...
-        assertAdminEvents.assertEvent(realmId, OperationType.CREATE, Matchers.startsWith(AdminEventPaths.roleByIdResourceCompositesPath(ids.get("role-a"))));
-        assertAdminEvents.assertEvent(realmId, OperationType.CREATE, Matchers.startsWith(AdminEventPaths.roleByIdResourceCompositesPath(ids.get("role-a"))));
+        assertAdminEvents.assertEvent(realmId, OperationType.CREATE, AdminEventPaths.roleByIdResourceCompositesPath(ids.get("role-a")), l);
 
         Set<RoleRepresentation> composites = resource.getRoleComposites(ids.get("role-a"));
 
@@ -136,7 +133,7 @@ public void composites() {
         Assert.assertNames(clientComposites, "role-c");
 
         resource.deleteComposites(ids.get("role-a"), l);
-        assertAdminEvents.assertEvent(realmId, OperationType.DELETE, AdminEventPaths.roleByIdResourceCompositesPath(ids.get("role-a")));
+        assertAdminEvents.assertEvent(realmId, OperationType.DELETE, AdminEventPaths.roleByIdResourceCompositesPath(ids.get("role-a")), l);
 
         assertFalse(resource.getRole(ids.get("role-a")).isComposite());
         assertEquals(0, resource.getRoleComposites(ids.get("role-a")).size());

diff --git a/...ntegration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java b/...ntegration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java
@@ -796,14 +796,16 @@ public void roleMappings() {
         l.add(realm.roles().get("realm-role").toRepresentation());
         l.add(realm.roles().get("realm-composite").toRepresentation());
         roles.realmLevel().add(l);
-        assertAdminEvents.assertEvent("test", OperationType.CREATE, Matchers.startsWith(AdminEventPaths.userRealmRoleMappingsPath(userId)));
-        assertAdminEvents.assertEvent("test", OperationType.CREATE, Matchers.startsWith(AdminEventPaths.userRealmRoleMappingsPath(userId)));
+        assertAdminEvents.assertEvent("test", OperationType.CREATE, AdminEventPaths.userRealmRoleMappingsPath(userId), l);
 
         // Add client roles
-        roles.clientLevel(clientUuid).add(Collections.singletonList(realm.clients().get(clientUuid).roles().get("client-role").toRepresentation()));
-        roles.clientLevel(clientUuid).add(Collections.singletonList(realm.clients().get(clientUuid).roles().get("client-composite").toRepresentation()));
-        assertAdminEvents.assertEvent("test", OperationType.CREATE, Matchers.startsWith(AdminEventPaths.userClientRoleMappingsPath(userId, clientUuid)));
-        assertAdminEvents.assertEvent("test", OperationType.CREATE, Matchers.startsWith(AdminEventPaths.userClientRoleMappingsPath(userId, clientUuid)));
+        List<RoleRepresentation> list = Collections.singletonList(realm.clients().get(clientUuid).roles().get("client-role").toRepresentation());
+        roles.clientLevel(clientUuid).add(list);
+        assertAdminEvents.assertEvent("test", OperationType.CREATE, AdminEventPaths.userClientRoleMappingsPath(userId, clientUuid), list);
+
+        list = Collections.singletonList(realm.clients().get(clientUuid).roles().get("client-composite").toRepresentation());
+        roles.clientLevel(clientUuid).add(list);
+        assertAdminEvents.assertEvent("test", OperationType.CREATE, AdminEventPaths.userClientRoleMappingsPath(userId, clientUuid), list);
 
         // List realm roles
         assertNames(roles.realmLevel().listAll(), "realm-role", "realm-composite", "user", "offline_access");
@@ -825,16 +827,14 @@ public void roleMappings() {
         // Remove realm role
         RoleRepresentation realmRoleRep = realm.roles().get("realm-role").toRepresentation();
         roles.realmLevel().remove(Collections.singletonList(realmRoleRep));
-        assertAdminEvents.assertEvent("test", OperationType.DELETE, AdminEventPaths.userRealmRoleMappingsPath(userId) + "/" + realmRoleRep.getId());
+        assertAdminEvents.assertEvent("test", OperationType.DELETE, AdminEventPaths.userRealmRoleMappingsPath(userId), Collections.singletonList(realmRoleRep));
 
         assertNames(roles.realmLevel().listAll(), "realm-composite", "user", "offline_access");
 
         // Remove client role
         RoleRepresentation clientRoleRep = realm.clients().get(clientUuid).roles().get("client-role").toRepresentation();
         roles.clientLevel(clientUuid).remove(Collections.singletonList(clientRoleRep));
-
-        // TODO: Inconsistency between event for delete realm role mapping and client role mapping (the latter doesn't have roleRep.getId() in the path)
-        assertAdminEvents.assertEvent("test", OperationType.DELETE, AdminEventPaths.userClientRoleMappingsPath(userId, clientUuid));
+        assertAdminEvents.assertEvent("test", OperationType.DELETE, AdminEventPaths.userClientRoleMappingsPath(userId, clientUuid), Collections.singletonList(clientRoleRep));
 
         assertNames(roles.clientLevel(clientUuid).listAll(), "client-composite");
     }