From 99f8e5f80885ac050b625026fd16e4cd3a3d2fdb Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Wed, 20 Feb 2019 21:26:09 -0300 Subject: [PATCH] [KEYCLOAK-9489] - Fixing fine-grained permission functionality --- .../AuthenticationManagementResource.java | 4 +-- .../admin/permissions/ClientPermissions.java | 3 ++- .../permissions/RealmPermissionEvaluator.java | 4 +++ .../admin/permissions/RealmPermissions.java | 12 +++++++++ .../admin/permissions/RolePermissions.java | 2 +- .../testsuite/admin/PermissionsTest.java | 25 ++++++++++++++----- 6 files changed, 40 insertions(+), 10 deletions(-) diff --git a/services/src/main/java/org/keycloak/services/resources/admin/AuthenticationManagementResource.java b/services/src/main/java/org/keycloak/services/resources/admin/AuthenticationManagementResource.java index 6582ad134a..087aeb1246 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/AuthenticationManagementResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/AuthenticationManagementResource.java @@ -182,7 +182,7 @@ public class AuthenticationManagementResource { @NoCache @Produces(MediaType.APPLICATION_JSON) public List getFlows() { - auth.realm().requireViewRealm(); + auth.realm().requireViewAuthenticationFlows(); List flows = new LinkedList<>(); for (AuthenticationFlowModel flow : realm.getAuthenticationFlows()) { @@ -900,7 +900,7 @@ public class AuthenticationManagementResource { @Produces(MediaType.APPLICATION_JSON) @NoCache public List getRequiredActions() { - auth.realm().requireViewRealm(); + auth.realm().requireViewRequiredActions(); List list = new LinkedList<>(); for (RequiredActionProviderModel model : realm.getRequiredActionProviders()) { diff --git a/services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissions.java b/services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissions.java index ae8ac33de6..6d45ea8077 100644 --- a/services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissions.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissions.java @@ -224,7 +224,8 @@ class ClientPermissions implements ClientPermissionEvaluator, ClientPermissionM @Override public boolean canList() { - return canView() || root.hasOneAdminRole(AdminRoles.QUERY_CLIENTS); + // when the user is assigned with query-users role, administrators can restrict which clients the user can see when using fine-grained admin permissions + return canView() || root.hasOneAdminRole(AdminRoles.QUERY_CLIENTS, AdminRoles.QUERY_USERS); } public boolean canList(ClientModel clientModel) { diff --git a/services/src/main/java/org/keycloak/services/resources/admin/permissions/RealmPermissionEvaluator.java b/services/src/main/java/org/keycloak/services/resources/admin/permissions/RealmPermissionEvaluator.java index 7020667fff..05499c80bd 100644 --- a/services/src/main/java/org/keycloak/services/resources/admin/permissions/RealmPermissionEvaluator.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/permissions/RealmPermissionEvaluator.java @@ -58,4 +58,8 @@ public interface RealmPermissionEvaluator { boolean canViewEvents(); void requireViewEvents(); + + void requireViewRequiredActions(); + + void requireViewAuthenticationFlows(); } diff --git a/services/src/main/java/org/keycloak/services/resources/admin/permissions/RealmPermissions.java b/services/src/main/java/org/keycloak/services/resources/admin/permissions/RealmPermissions.java index c24ac3be5d..891d4fb4b9 100644 --- a/services/src/main/java/org/keycloak/services/resources/admin/permissions/RealmPermissions.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/permissions/RealmPermissions.java @@ -183,7 +183,19 @@ class RealmPermissions implements RealmPermissionEvaluator { } } + @Override + public void requireViewRequiredActions() { + if (!(canViewRealm() || root.hasOneAdminRole(AdminRoles.QUERY_USERS))) { + throw new ForbiddenException(); + } + } + @Override + public void requireViewAuthenticationFlows() { + if (!(canViewRealm() || root.hasOneAdminRole(AdminRoles.QUERY_CLIENTS))) { + throw new ForbiddenException(); + } + } } diff --git a/services/src/main/java/org/keycloak/services/resources/admin/permissions/RolePermissions.java b/services/src/main/java/org/keycloak/services/resources/admin/permissions/RolePermissions.java index 3280d27642..8c13dc0139 100644 --- a/services/src/main/java/org/keycloak/services/resources/admin/permissions/RolePermissions.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/permissions/RolePermissions.java @@ -453,7 +453,7 @@ class RolePermissions implements RolePermissionEvaluator, RolePermissionManageme return root.realm().canManageRealm(); } else if (role.getContainer() instanceof ClientModel) { ClientModel client = (ClientModel)role.getContainer(); - return root.clients().canManage(client); + return root.clients().canConfigure(client); } return false; } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/PermissionsTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/PermissionsTest.java index d7c2bf375d..23dd134900 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/PermissionsTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/PermissionsTest.java @@ -491,12 +491,6 @@ public class PermissionsTest extends AbstractKeycloakTest { l = clients.get(AdminRoles.VIEW_CLIENTS).realm(REALM_NAME).clients().findAll(); Assert.assertThat(l, Matchers.not(Matchers.empty())); - // this should throw forbidden as "query-users" role isn't enough - invoke(new Invocation() { - public void invoke(RealmResource realm) { - clients.get(AdminRoles.QUERY_USERS).realm(REALM_NAME).clients().findAll(); - } - }, clients.get(AdminRoles.QUERY_USERS), false); ClientRepresentation client = l.get(0); invoke(new InvocationWithResponse() { @Override @@ -764,6 +758,12 @@ public class PermissionsTest extends AbstractKeycloakTest { realm.clients().get(foo.getId()).roles().get("nosuch").getClientRoleComposites("nosuch"); } }, Resource.CLIENT, false); + // users with query-client role should be able to query flows so the client detail page can be rendered successfully when fine-grained permissions are enabled. + invoke(new Invocation() { + public void invoke(RealmResource realm) { + realm.flows().getFlows(); + } + }, clients.get(AdminRoles.QUERY_CLIENTS), true); } @Test @@ -1603,6 +1603,19 @@ public class PermissionsTest extends AbstractKeycloakTest { realm.users().get(user.getId()).update(user); } }, clients.get(AdminRoles.QUERY_CLIENTS), false); + // users with query-user role should be able to query required actions so the user detail page can be rendered successfully when fine-grained permissions are enabled. + invoke(new Invocation() { + public void invoke(RealmResource realm) { + realm.flows().getRequiredActions(); + } + }, clients.get(AdminRoles.QUERY_USERS), true); + // users with query-user role should be able to query clients so the user detail page can be rendered successfully when fine-grained permissions are enabled. + // if the admin wants to restrict the clients that an user can see he can define permissions for these clients + invoke(new Invocation() { + public void invoke(RealmResource realm) { + clients.get(AdminRoles.QUERY_USERS).realm(REALM_NAME).clients().findAll(); + } + }, clients.get(AdminRoles.QUERY_USERS), true); } @Test