From fad194fc8f56548e6a61184cdf26ae7e7ba1cc94 Mon Sep 17 00:00:00 2001 From: Stian Thorgersen Date: Mon, 25 Nov 2013 13:41:24 +0000 Subject: [PATCH] Improved error handling in admin console. Delete roles through admin console. --- .../META-INF/resources/admin/index.html | 8 +---- .../META-INF/resources/admin/js/app.js | 11 ++++--- .../admin/js/controllers/applications.js | 2 +- .../resources/admin/js/controllers/realm.js | 4 +-- .../partials/application-role-detail.html | 2 +- .../idm/ErrorRepresentation.java | 19 +++++++++++ .../keycloak/models/RoleContainerModel.java | 2 ++ .../models/jpa/ApplicationAdapter.java | 27 ++++++++++++--- .../org/keycloak/models/jpa/RealmAdapter.java | 33 ++++++++++++------- .../models/picketlink/ApplicationAdapter.java | 11 +++++++ .../models/picketlink/RealmAdapter.java | 11 +++++++ .../admin/RoleContainerResource.java | 22 +++++++------ .../resources/admin/UsersResource.java | 9 ++--- .../services/resources/flows/ErrorFlows.java | 19 +++++++++++ .../services/resources/flows/Flows.java | 4 +++ .../java/org/keycloak/test/AdapterTest.java | 27 +++++++++++++++ 16 files changed, 163 insertions(+), 48 deletions(-) create mode 100644 core/src/main/java/org/keycloak/representations/idm/ErrorRepresentation.java create mode 100644 services/src/main/java/org/keycloak/services/resources/flows/ErrorFlows.java diff --git a/admin-ui/src/main/resources/META-INF/resources/admin/index.html b/admin-ui/src/main/resources/META-INF/resources/admin/index.html index f0c8996c92..5a1e6fc760 100755 --- a/admin-ui/src/main/resources/META-INF/resources/admin/index.html +++ b/admin-ui/src/main/resources/META-INF/resources/admin/index.html @@ -64,13 +64,7 @@
-
- -
- -
+
diff --git a/admin-ui/src/main/resources/META-INF/resources/admin/js/app.js b/admin-ui/src/main/resources/META-INF/resources/admin/js/app.js index d97aed788c..f010695c37 100755 --- a/admin-ui/src/main/resources/META-INF/resources/admin/js/app.js +++ b/admin-ui/src/main/resources/META-INF/resources/admin/js/app.js @@ -320,18 +320,21 @@ module.config(function($httpProvider) { }); -module.factory('errorInterceptor', function($q, $window, $rootScope, $location, Auth) { +module.factory('errorInterceptor', function($q, $window, $rootScope, $location, Auth, Notifications) { return function(promise) { return promise.then(function(response) { - $rootScope.httpProviderError = null; return response; }, function(response) { if (response.status == 401) { console.log('session timeout?'); Auth.loggedIn = false; window.location = '/auth-server/rest/saas/login?path=' + $location.path(); - } else { - $rootScope.httpProviderError = response.status; + } else if (response.status) { + if (response.data && response.data.errorMessage) { + Notifications.error(response.data.errorMessage); + } else { + Notifications.error("An unexpected server error has occurred"); + } } return $q.reject(response); }); diff --git a/admin-ui/src/main/resources/META-INF/resources/admin/js/controllers/applications.js b/admin-ui/src/main/resources/META-INF/resources/admin/js/controllers/applications.js index 9d93d6f62e..8846801296 100755 --- a/admin-ui/src/main/resources/META-INF/resources/admin/js/controllers/applications.js +++ b/admin-ui/src/main/resources/META-INF/resources/admin/js/controllers/applications.js @@ -159,7 +159,7 @@ module.controller('ApplicationRoleDetailCtrl', function($scope, realm, applicati $scope.role.$remove({ realm : realm.id, application : application.id, - role : $scope.role.name + roleId : $scope.role.id }, function() { $location.url("/realms/" + realm.id + "/applications/" + application.id + "/roles"); Notifications.success("The role has been deleted."); diff --git a/admin-ui/src/main/resources/META-INF/resources/admin/js/controllers/realm.js b/admin-ui/src/main/resources/META-INF/resources/admin/js/controllers/realm.js index 9271bfb9e5..ad9bca4a09 100755 --- a/admin-ui/src/main/resources/META-INF/resources/admin/js/controllers/realm.js +++ b/admin-ui/src/main/resources/META-INF/resources/admin/js/controllers/realm.js @@ -23,7 +23,7 @@ module.controller('GlobalCtrl', function($scope, $http, Auth, Current, $location Current.realms = data; if (data.length > 0) { Current.realm = data[0]; - $location.url("/realms/" + Current.realm.id); + //$location.url("/realms/" + Current.realm.id); } }); }); @@ -611,7 +611,7 @@ module.controller('RoleDetailCtrl', function($scope, realm, role, Role, $locatio Dialog.confirmDelete($scope.role.name, 'role', function() { $scope.role.$remove({ realm : realm.id, - role : $scope.role.name + roleId : $scope.role.id }, function() { $location.url("/realms/" + realm.id + "/roles"); Notifications.success("The role has been deleted."); diff --git a/admin-ui/src/main/resources/META-INF/resources/admin/partials/application-role-detail.html b/admin-ui/src/main/resources/META-INF/resources/admin/partials/application-role-detail.html index a147697400..063ab8480d 100755 --- a/admin-ui/src/main/resources/META-INF/resources/admin/partials/application-role-detail.html +++ b/admin-ui/src/main/resources/META-INF/resources/admin/partials/application-role-detail.html @@ -46,7 +46,7 @@
- + diff --git a/core/src/main/java/org/keycloak/representations/idm/ErrorRepresentation.java b/core/src/main/java/org/keycloak/representations/idm/ErrorRepresentation.java new file mode 100644 index 0000000000..3b0aac1b17 --- /dev/null +++ b/core/src/main/java/org/keycloak/representations/idm/ErrorRepresentation.java @@ -0,0 +1,19 @@ +package org.keycloak.representations.idm; + +/** + * @author Stian Thorgersen + */ +public class ErrorRepresentation { + public String errorMessage; + + public ErrorRepresentation() { + } + + public String getErrorMessage() { + return errorMessage; + } + + public void setErrorMessage(String errorMessage) { + this.errorMessage = errorMessage; + } +} diff --git a/model/api/src/main/java/org/keycloak/models/RoleContainerModel.java b/model/api/src/main/java/org/keycloak/models/RoleContainerModel.java index 4442359573..cf5748b54c 100755 --- a/model/api/src/main/java/org/keycloak/models/RoleContainerModel.java +++ b/model/api/src/main/java/org/keycloak/models/RoleContainerModel.java @@ -11,6 +11,8 @@ public interface RoleContainerModel { RoleModel addRole(String name); + boolean removeRole(String id); + List getRoles(); RoleModel getRoleById(String id); diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/ApplicationAdapter.java b/model/jpa/src/main/java/org/keycloak/models/jpa/ApplicationAdapter.java index 00ed540041..1ca34b6320 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/ApplicationAdapter.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/ApplicationAdapter.java @@ -3,10 +3,7 @@ package org.keycloak.models.jpa; import org.keycloak.models.ApplicationModel; import org.keycloak.models.RoleModel; import org.keycloak.models.UserModel; -import org.keycloak.models.jpa.entities.ApplicationEntity; -import org.keycloak.models.jpa.entities.ApplicationScopeMappingEntity; -import org.keycloak.models.jpa.entities.ApplicationUserRoleMappingEntity; -import org.keycloak.models.jpa.entities.RoleEntity; +import org.keycloak.models.jpa.entities.*; import javax.persistence.EntityManager; import javax.persistence.TypedQuery; @@ -103,7 +100,7 @@ public class ApplicationAdapter implements ApplicationModel { return new RoleAdapter(role); } } - return null; //To change body of implemented methods use File | Settings | File Templates. + return null; } @Override @@ -118,6 +115,26 @@ public class ApplicationAdapter implements ApplicationModel { return new RoleAdapter(entity); } + @Override + public boolean removeRole(String id) { + RoleEntity role = em.find(RoleEntity.class, id); + if (role == null) { + return false; + } + + application.getRoles().remove(role); + application.getDefaultRoles().remove(role); + + em.createQuery("delete from " + ApplicationScopeMappingEntity.class.getSimpleName() + " where role = :role").setParameter("role", role).executeUpdate(); + em.createQuery("delete from " + ApplicationUserRoleMappingEntity.class.getSimpleName() + " where role = :role").setParameter("role", role).executeUpdate(); + em.createQuery("delete from " + RealmScopeMappingEntity.class.getSimpleName() + " where role = :role").setParameter("role", role).executeUpdate(); + em.createQuery("delete from " + RealmUserRoleMappingEntity.class.getSimpleName() + " where role = :role").setParameter("role", role).executeUpdate(); + + em.remove(role); + + return true; + } + @Override public List getRoles() { ArrayList list = new ArrayList(); diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/RealmAdapter.java b/model/jpa/src/main/java/org/keycloak/models/jpa/RealmAdapter.java index f3463c78ff..7f98fd0fdb 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/RealmAdapter.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/RealmAdapter.java @@ -10,18 +10,7 @@ import org.keycloak.models.RoleModel; import org.keycloak.models.SocialLinkModel; import org.keycloak.models.UserCredentialModel; import org.keycloak.models.UserModel; -import org.keycloak.models.jpa.entities.ApplicationEntity; -import org.keycloak.models.jpa.entities.ApplicationScopeMappingEntity; -import org.keycloak.models.jpa.entities.ApplicationUserRoleMappingEntity; -import org.keycloak.models.jpa.entities.CredentialEntity; -import org.keycloak.models.jpa.entities.OAuthClientEntity; -import org.keycloak.models.jpa.entities.RealmEntity; -import org.keycloak.models.jpa.entities.RealmScopeMappingEntity; -import org.keycloak.models.jpa.entities.RealmUserRoleMappingEntity; -import org.keycloak.models.jpa.entities.RequiredCredentialEntity; -import org.keycloak.models.jpa.entities.RoleEntity; -import org.keycloak.models.jpa.entities.SocialLinkEntity; -import org.keycloak.models.jpa.entities.UserEntity; +import org.keycloak.models.jpa.entities.*; import org.keycloak.models.utils.SHAPasswordEncoder; import org.keycloak.models.utils.TimeBasedOTP; @@ -799,6 +788,26 @@ public class RealmAdapter implements RealmModel { return new RoleAdapter(entity); } + @Override + public boolean removeRole(String id) { + RoleEntity role = em.find(RoleEntity.class, id); + if (role == null) { + return false; + } + + realm.getRoles().remove(role); + realm.getDefaultRoles().remove(role); + + em.createQuery("delete from " + ApplicationScopeMappingEntity.class.getSimpleName() + " where role = :role").setParameter("role", role).executeUpdate(); + em.createQuery("delete from " + ApplicationUserRoleMappingEntity.class.getSimpleName() + " where role = :role").setParameter("role", role).executeUpdate(); + em.createQuery("delete from " + RealmScopeMappingEntity.class.getSimpleName() + " where role = :role").setParameter("role", role).executeUpdate(); + em.createQuery("delete from " + RealmUserRoleMappingEntity.class.getSimpleName() + " where role = :role").setParameter("role", role).executeUpdate(); + + em.remove(role); + + return true; + } + @Override public List getRoles() { ArrayList list = new ArrayList(); diff --git a/model/picketlink/src/main/java/org/keycloak/models/picketlink/ApplicationAdapter.java b/model/picketlink/src/main/java/org/keycloak/models/picketlink/ApplicationAdapter.java index 436b23ce8e..98a510a638 100755 --- a/model/picketlink/src/main/java/org/keycloak/models/picketlink/ApplicationAdapter.java +++ b/model/picketlink/src/main/java/org/keycloak/models/picketlink/ApplicationAdapter.java @@ -5,6 +5,7 @@ import org.keycloak.models.RoleModel; import org.keycloak.models.UserModel; import org.keycloak.models.picketlink.mappings.ApplicationData; import org.keycloak.models.picketlink.relationships.ScopeRelationship; +import org.picketlink.idm.IdentityManagementException; import org.picketlink.idm.IdentityManager; import org.picketlink.idm.PartitionManager; import org.picketlink.idm.RelationshipManager; @@ -154,6 +155,16 @@ public class ApplicationAdapter implements ApplicationModel { return new RoleAdapter(role, getIdm()); } + @Override + public boolean removeRole(String id) { + try { + getIdm().remove(getIdm().lookupIdentityById(Role.class, id)); + return true; + } catch (IdentityManagementException e) { + return false; + } + } + @Override public List getRoles() { IdentityQuery query = getIdm().createIdentityQuery(Role.class); diff --git a/model/picketlink/src/main/java/org/keycloak/models/picketlink/RealmAdapter.java b/model/picketlink/src/main/java/org/keycloak/models/picketlink/RealmAdapter.java index 3767e656c6..3f6631c93f 100755 --- a/model/picketlink/src/main/java/org/keycloak/models/picketlink/RealmAdapter.java +++ b/model/picketlink/src/main/java/org/keycloak/models/picketlink/RealmAdapter.java @@ -21,6 +21,7 @@ import org.keycloak.models.picketlink.relationships.RequiredApplicationCredentia import org.keycloak.models.picketlink.relationships.RequiredCredentialRelationship; import org.keycloak.models.picketlink.relationships.ScopeRelationship; import org.keycloak.models.picketlink.relationships.SocialLinkRelationship; +import org.picketlink.idm.IdentityManagementException; import org.picketlink.idm.IdentityManager; import org.picketlink.idm.PartitionManager; import org.picketlink.idm.RelationshipManager; @@ -552,6 +553,16 @@ public class RealmAdapter implements RealmModel { return new RoleAdapter(role, getIdm()); } + @Override + public boolean removeRole(String id) { + try { + getIdm().remove(getIdm().lookupIdentityById(Role.class, id)); + return true; + } catch (IdentityManagementException e) { + return false; + } + } + @Override public List getRoles() { IdentityManager idm = getIdm(); 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 index 1ab393f6a1..1e70e6ab21 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/RoleContainerResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/RoleContainerResource.java @@ -5,16 +5,9 @@ import org.keycloak.models.Constants; import org.keycloak.models.RoleContainerModel; import org.keycloak.models.RoleModel; import org.keycloak.representations.idm.RoleRepresentation; +import org.keycloak.services.resources.flows.Flows; -import javax.ws.rs.Consumes; -import javax.ws.rs.GET; -import javax.ws.rs.InternalServerErrorException; -import javax.ws.rs.NotFoundException; -import javax.ws.rs.POST; -import javax.ws.rs.PUT; -import javax.ws.rs.Path; -import javax.ws.rs.PathParam; -import javax.ws.rs.Produces; +import javax.ws.rs.*; import javax.ws.rs.core.Context; import javax.ws.rs.core.Response; import javax.ws.rs.core.UriInfo; @@ -63,6 +56,15 @@ public class RoleContainerResource { return rep; } + @Path("roles/{id}") + @DELETE + @NoCache + public void deleteRole(final @PathParam("id") String id) { + if (!roleContainer.removeRole(id)) { + throw new NotFoundException(); + } + } + @Path("roles/{id}") @PUT @Consumes("application/json") @@ -80,7 +82,7 @@ public class RoleContainerResource { @Consumes("application/json") public Response createRole(final @Context UriInfo uriInfo, final RoleRepresentation rep) { if (roleContainer.getRole(rep.getName()) != null || rep.getName().startsWith(Constants.INTERNAL_ROLE)) { - throw new InternalServerErrorException(); // todo appropriate status here. + return Flows.errors().exists("Role with name " + rep.getName() + " already exists"); } RoleModel role = roleContainer.addRole(rep.getName()); if (role == null) { diff --git a/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java b/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java index 2901c9c286..61e70bf682 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java @@ -9,12 +9,9 @@ import org.keycloak.models.RealmModel; import org.keycloak.models.RoleModel; import org.keycloak.models.UserCredentialModel; import org.keycloak.models.UserModel; -import org.keycloak.representations.idm.ApplicationMappingsRepresentation; -import org.keycloak.representations.idm.CredentialRepresentation; -import org.keycloak.representations.idm.MappingsRepresentation; -import org.keycloak.representations.idm.RoleRepresentation; -import org.keycloak.representations.idm.UserRepresentation; +import org.keycloak.representations.idm.*; import org.keycloak.services.managers.RealmManager; +import org.keycloak.services.resources.flows.Flows; import javax.ws.rs.Consumes; import javax.ws.rs.DELETE; @@ -71,7 +68,7 @@ public class UsersResource { @Consumes("application/json") public Response createUser(final @Context UriInfo uriInfo, final UserRepresentation rep) { if (realm.getUser(rep.getUsername()) != null) { - throw new InternalServerErrorException(); // todo appropriate status here. + return Flows.errors().exists("User with username " + rep.getUsername() + " already exists"); } UserModel user = realm.addUser(rep.getUsername()); if (user == null) { diff --git a/services/src/main/java/org/keycloak/services/resources/flows/ErrorFlows.java b/services/src/main/java/org/keycloak/services/resources/flows/ErrorFlows.java new file mode 100644 index 0000000000..5a965662f4 --- /dev/null +++ b/services/src/main/java/org/keycloak/services/resources/flows/ErrorFlows.java @@ -0,0 +1,19 @@ +package org.keycloak.services.resources.flows; + +import org.keycloak.representations.idm.ErrorRepresentation; + +import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.Response; + +/** + * @author Stian Thorgersen + */ +public class ErrorFlows { + + public Response exists(String message) { + ErrorRepresentation error = new ErrorRepresentation(); + error.setErrorMessage(message); + return Response.status(Response.Status.CONFLICT).entity(error).type(MediaType.APPLICATION_JSON).build(); + } + +} diff --git a/services/src/main/java/org/keycloak/services/resources/flows/Flows.java b/services/src/main/java/org/keycloak/services/resources/flows/Flows.java index 41aa04f3bc..e46632a9e5 100755 --- a/services/src/main/java/org/keycloak/services/resources/flows/Flows.java +++ b/services/src/main/java/org/keycloak/services/resources/flows/Flows.java @@ -45,4 +45,8 @@ public class Flows { return new OAuthFlows(realm, request, uriInfo, authManager, tokenManager); } + public static ErrorFlows errors() { + return new ErrorFlows(); + } + } diff --git a/services/src/test/java/org/keycloak/test/AdapterTest.java b/services/src/test/java/org/keycloak/test/AdapterTest.java index 7c2d405dec..e504c39504 100755 --- a/services/src/test/java/org/keycloak/test/AdapterTest.java +++ b/services/src/test/java/org/keycloak/test/AdapterTest.java @@ -227,6 +227,33 @@ public class AdapterTest extends AbstractKeycloakTest { Assert.assertNull(realmModel.getApplicationById(app.getId())); } + + @Test + public void testRemoveRole() throws Exception { + test1CreateRealm(); + + UserModel user = realmModel.addUser("bburke"); + + OAuthClientModel client = realmModel.addOAuthClient("client"); + + ApplicationModel app = realmModel.addApplication("test-app"); + + RoleModel appRole = app.addRole("test"); + app.grantRole(user, appRole); + app.addScopeMapping(client.getOAuthAgent(), appRole); + + RoleModel realmRole = realmModel.addRole("test"); + realmModel.addScopeMapping(app.getApplicationUser(), realmRole); + + Assert.assertTrue(realmModel.removeRole(realmRole.getId())); + Assert.assertFalse(realmModel.removeRole(realmRole.getId())); + Assert.assertNull(realmModel.getRole(realmRole.getName())); + + Assert.assertTrue(app.removeRole(appRole.getId())); + Assert.assertFalse(app.removeRole(appRole.getId())); + Assert.assertNull(app.getRole(appRole.getName())); + } + @Test public void testUserSearch() throws Exception { test1CreateRealm();