From 94928323ee3a2ec76916030c83b9ad67d3a3b5b9 Mon Sep 17 00:00:00 2001 From: pedroigor Date: Fri, 15 Dec 2017 12:55:07 -0200 Subject: [PATCH] [KEYCLOAK-5877] - Allow saving permissions without policies --- .../permission/ResourcePermissionForm.java | 4 +- .../permission/ScopePermissionForm.java | 4 +- .../ResourcePermissionManagementTest.java | 44 +++++++++-- .../ScopePermissionManagementTest.java | 78 +++++++++++++------ .../resources/js/authz/authz-controller.js | 6 +- ...esource-server-policy-resource-detail.html | 4 +- .../resource-server-policy-scope-detail.html | 4 +- 7 files changed, 106 insertions(+), 38 deletions(-) diff --git a/testsuite/integration-arquillian/tests/other/console/src/main/java/org/keycloak/testsuite/console/page/clients/authorization/permission/ResourcePermissionForm.java b/testsuite/integration-arquillian/tests/other/console/src/main/java/org/keycloak/testsuite/console/page/clients/authorization/permission/ResourcePermissionForm.java index e31ac2a43f..d53f8205fa 100644 --- a/testsuite/integration-arquillian/tests/other/console/src/main/java/org/keycloak/testsuite/console/page/clients/authorization/permission/ResourcePermissionForm.java +++ b/testsuite/integration-arquillian/tests/other/console/src/main/java/org/keycloak/testsuite/console/page/clients/authorization/permission/ResourcePermissionForm.java @@ -72,7 +72,9 @@ public class ResourcePermissionForm extends Form { resourceSelect.update(expected.getResources()); } - policySelect.update(expected.getPolicies()); + if (expected.getPolicies() != null) { + policySelect.update(expected.getPolicies()); + } save(); } diff --git a/testsuite/integration-arquillian/tests/other/console/src/main/java/org/keycloak/testsuite/console/page/clients/authorization/permission/ScopePermissionForm.java b/testsuite/integration-arquillian/tests/other/console/src/main/java/org/keycloak/testsuite/console/page/clients/authorization/permission/ScopePermissionForm.java index deb7f0663b..696ff2250d 100644 --- a/testsuite/integration-arquillian/tests/other/console/src/main/java/org/keycloak/testsuite/console/page/clients/authorization/permission/ScopePermissionForm.java +++ b/testsuite/integration-arquillian/tests/other/console/src/main/java/org/keycloak/testsuite/console/page/clients/authorization/permission/ScopePermissionForm.java @@ -75,7 +75,9 @@ public class ScopePermissionForm extends Form { scopeSelect.update(expected.getScopes()); } - policySelect.update(expected.getPolicies()); + if (expected.getPolicies() != null) { + policySelect.update(expected.getPolicies()); + } save(); } diff --git a/testsuite/integration-arquillian/tests/other/console/src/test/java/org/keycloak/testsuite/console/authorization/ResourcePermissionManagementTest.java b/testsuite/integration-arquillian/tests/other/console/src/test/java/org/keycloak/testsuite/console/authorization/ResourcePermissionManagementTest.java index f6a967e320..8ac8b3baad 100644 --- a/testsuite/integration-arquillian/tests/other/console/src/test/java/org/keycloak/testsuite/console/authorization/ResourcePermissionManagementTest.java +++ b/testsuite/integration-arquillian/tests/other/console/src/test/java/org/keycloak/testsuite/console/authorization/ResourcePermissionManagementTest.java @@ -18,6 +18,7 @@ package org.keycloak.testsuite.console.authorization; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; import java.util.stream.Collectors; @@ -30,7 +31,6 @@ import org.keycloak.admin.client.resource.RolePoliciesResource; import org.keycloak.admin.client.resource.RolesResource; import org.keycloak.representations.idm.RoleRepresentation; import org.keycloak.representations.idm.authorization.DecisionStrategy; -import org.keycloak.representations.idm.authorization.Logic; import org.keycloak.representations.idm.authorization.ResourcePermissionRepresentation; import org.keycloak.representations.idm.authorization.ResourceRepresentation; import org.keycloak.representations.idm.authorization.RolePolicyRepresentation; @@ -80,12 +80,28 @@ public class ResourcePermissionManagementTest extends AbstractAuthorizationSetti resources.create(new ResourceRepresentation("Resource B")); } + @Test + public void testCreateWithoutPolicies() throws InterruptedException { + authorizationPage.navigateTo(); + ResourcePermissionRepresentation expected = new ResourcePermissionRepresentation(); + + expected.setName("testCreateWithoutPolicies Permission"); + expected.setDescription("description"); + expected.addResource("Resource A"); + + expected = createPermission(expected); + + authorizationPage.navigateTo(); + ResourcePermission actual = authorizationPage.authorizationTabs().permissions().name(expected.getName()); + assertPolicy(expected, actual); + } + @Test public void testUpdateResource() throws InterruptedException { authorizationPage.navigateTo(); ResourcePermissionRepresentation expected = new ResourcePermissionRepresentation(); - expected.setName("Test Resource A Permission"); + expected.setName("testUpdateResource Permission"); expected.setDescription("description"); expected.addResource("Resource A"); expected.addPolicy("Policy A"); @@ -96,7 +112,7 @@ public class ResourcePermissionManagementTest extends AbstractAuthorizationSetti String previousName = expected.getName(); - expected.setName("Changed Test Resource A Permission"); + expected.setName(expected.getName() + " Changed"); expected.setDescription("Changed description"); expected.setDecisionStrategy(DecisionStrategy.CONSENSUS); expected.getResources().clear(); @@ -110,7 +126,16 @@ public class ResourcePermissionManagementTest extends AbstractAuthorizationSetti authorizationPage.navigateTo(); ResourcePermission actual = authorizationPage.authorizationTabs().permissions().name(expected.getName()); + assertPolicy(expected, actual); + expected.getPolicies().clear(); + + authorizationPage.navigateTo(); + authorizationPage.authorizationTabs().permissions().update(expected.getName(), expected); + assertAlertSuccess(); + + authorizationPage.navigateTo(); + actual = authorizationPage.authorizationTabs().permissions().name(expected.getName()); assertPolicy(expected, actual); } @@ -119,7 +144,7 @@ public class ResourcePermissionManagementTest extends AbstractAuthorizationSetti authorizationPage.navigateTo(); ResourcePermissionRepresentation expected = new ResourcePermissionRepresentation(); - expected.setName("Test Resource B Type Permission"); + expected.setName("testUpdateResourceType Permission"); expected.setDescription("description"); expected.setResourceType("test-resource-type"); expected.addPolicy("Policy A"); @@ -130,7 +155,7 @@ public class ResourcePermissionManagementTest extends AbstractAuthorizationSetti String previousName = expected.getName(); - expected.setName("Changed Test Resource B Type Permission"); + expected.setName(expected.getName() + " Changed"); expected.setDescription("Changed description"); expected.setDecisionStrategy(DecisionStrategy.CONSENSUS); @@ -152,7 +177,7 @@ public class ResourcePermissionManagementTest extends AbstractAuthorizationSetti authorizationPage.navigateTo(); ResourcePermissionRepresentation expected = new ResourcePermissionRepresentation(); - expected.setName("Test Delete Resource Permission"); + expected.setName("testDelete Permission"); expected.setDescription("description"); expected.addResource("Resource B"); expected.addPolicy("Policy C"); @@ -170,7 +195,7 @@ public class ResourcePermissionManagementTest extends AbstractAuthorizationSetti authorizationPage.navigateTo(); ResourcePermissionRepresentation expected = new ResourcePermissionRepresentation(); - expected.setName("Test Delete Resource Permission"); + expected.setName("testDeleteFromList Permission"); expected.setDescription("description"); expected.addResource("Resource B"); expected.addPolicy("Policy C"); @@ -195,6 +220,11 @@ public class ResourcePermissionManagementTest extends AbstractAuthorizationSetti assertEquals(expected.getDescription(), actual.getDescription()); assertEquals(expected.getDecisionStrategy(), actual.getDecisionStrategy()); assertEquals(expected.getResourceType(), actual.getResourceType()); + if (expected.getPolicies() == null) { + assertTrue(actual.getPolicies() == null || actual.getPolicies().isEmpty()); + } else { + assertEquals(expected.getPolicies().size(), actual.getPolicies().size()); + } assertEquals(0, actual.getPolicies().stream().filter(actualPolicy -> !expected.getPolicies().stream() .filter(expectedPolicy -> actualPolicy.equals(expectedPolicy)) .findFirst().isPresent()) diff --git a/testsuite/integration-arquillian/tests/other/console/src/test/java/org/keycloak/testsuite/console/authorization/ScopePermissionManagementTest.java b/testsuite/integration-arquillian/tests/other/console/src/test/java/org/keycloak/testsuite/console/authorization/ScopePermissionManagementTest.java index e755335071..7599537158 100644 --- a/testsuite/integration-arquillian/tests/other/console/src/test/java/org/keycloak/testsuite/console/authorization/ScopePermissionManagementTest.java +++ b/testsuite/integration-arquillian/tests/other/console/src/test/java/org/keycloak/testsuite/console/authorization/ScopePermissionManagementTest.java @@ -18,6 +18,7 @@ package org.keycloak.testsuite.console.authorization; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; import org.junit.Before; import org.junit.Test; @@ -33,7 +34,6 @@ import org.keycloak.representations.idm.authorization.RolePolicyRepresentation; import org.keycloak.representations.idm.authorization.ScopePermissionRepresentation; import org.keycloak.representations.idm.authorization.ScopeRepresentation; import org.keycloak.representations.idm.authorization.UserPolicyRepresentation; -import org.keycloak.testsuite.console.page.clients.authorization.permission.ResourcePermission; import org.keycloak.testsuite.console.page.clients.authorization.permission.ScopePermission; /** @@ -84,31 +84,17 @@ public class ScopePermissionManagementTest extends AbstractAuthorizationSettings } @Test - public void testUpdateScopeOnly() throws InterruptedException { + public void testCreateWithoutPolicies() throws InterruptedException { authorizationPage.navigateTo(); ScopePermissionRepresentation expected = new ScopePermissionRepresentation(); - expected.setName("Test Scope Only Permission"); + expected.setName("testCreateWithoutPolicies Permission"); expected.setDescription("description"); - expected.addScope("Scope C", "Scope A", "Scope B"); - expected.addPolicy("Policy C", "Policy A", "Policy B"); + expected.addResource("Resource A"); + expected.addScope("Scope A"); expected = createPermission(expected); - String previousName = expected.getName(); - - expected.setName(previousName + "Changed"); - expected.setDescription("changed"); - expected.setDecisionStrategy(DecisionStrategy.CONSENSUS); - expected.getScopes().clear(); - expected.addScope("Scope B"); - expected.getPolicies().clear(); - expected.addPolicy("Policy C"); - - authorizationPage.navigateTo(); - authorizationPage.authorizationTabs().permissions().update(previousName, expected); - assertAlertSuccess(); - authorizationPage.navigateTo(); ScopePermission actual = authorizationPage.authorizationTabs().permissions().name(expected.getName()); assertPolicy(expected, actual); @@ -119,7 +105,7 @@ public class ScopePermissionManagementTest extends AbstractAuthorizationSettings authorizationPage.navigateTo(); ScopePermissionRepresentation expected = new ScopePermissionRepresentation(); - expected.setName("Test Resource Scope Permission"); + expected.setName("testUpdateResourceScope Permission"); expected.setDescription("description"); expected.addResource("Resource A"); expected.addScope("Scope A"); @@ -146,6 +132,47 @@ public class ScopePermissionManagementTest extends AbstractAuthorizationSettings authorizationPage.navigateTo(); ScopePermission actual = authorizationPage.authorizationTabs().permissions().name(expected.getName()); assertPolicy(expected, actual); + + expected.getPolicies().clear(); + + authorizationPage.navigateTo(); + authorizationPage.authorizationTabs().permissions().update(expected.getName(), expected); + assertAlertSuccess(); + + authorizationPage.navigateTo(); + actual = authorizationPage.authorizationTabs().permissions().name(expected.getName()); + assertPolicy(expected, actual); + } + + @Test + public void testUpdateScopeOnly() throws InterruptedException { + authorizationPage.navigateTo(); + ScopePermissionRepresentation expected = new ScopePermissionRepresentation(); + + expected.setName("testUpdateScopeOnly Permission"); + expected.setDescription("description"); + expected.addScope("Scope C", "Scope A", "Scope B"); + expected.addPolicy("Policy C", "Policy A", "Policy B"); + + expected = createPermission(expected); + + String previousName = expected.getName(); + + expected.setName(previousName + "Changed"); + expected.setDescription("changed"); + expected.setDecisionStrategy(DecisionStrategy.CONSENSUS); + expected.getScopes().clear(); + expected.addScope("Scope B"); + expected.getPolicies().clear(); + expected.addPolicy("Policy C"); + + authorizationPage.navigateTo(); + authorizationPage.authorizationTabs().permissions().update(previousName, expected); + assertAlertSuccess(); + + authorizationPage.navigateTo(); + ScopePermission actual = authorizationPage.authorizationTabs().permissions().name(expected.getName()); + assertPolicy(expected, actual); } @Test @@ -153,7 +180,7 @@ public class ScopePermissionManagementTest extends AbstractAuthorizationSettings authorizationPage.navigateTo(); ScopePermissionRepresentation expected = new ScopePermissionRepresentation(); - expected.setName("Test Delete Scope Permission"); + expected.setName("testDelete Permission"); expected.setDescription("description"); expected.addScope("Scope C"); expected.addPolicy("Policy C"); @@ -171,7 +198,7 @@ public class ScopePermissionManagementTest extends AbstractAuthorizationSettings authorizationPage.navigateTo(); ScopePermissionRepresentation expected = new ScopePermissionRepresentation(); - expected.setName("Test Delete Scope Permission"); + expected.setName("testDeleteFromList Permission"); expected.setDescription("description"); expected.addScope("Scope C"); expected.addPolicy("Policy C"); @@ -196,7 +223,12 @@ public class ScopePermissionManagementTest extends AbstractAuthorizationSettings assertEquals(expected.getDescription(), actual.getDescription()); assertEquals(expected.getDecisionStrategy(), actual.getDecisionStrategy()); - assertEquals(expected.getPolicies().size(), actual.getPolicies().size()); + if (expected.getPolicies() == null) { + assertTrue(actual.getPolicies() == null || actual.getPolicies().isEmpty()); + } else { + assertEquals(expected.getPolicies().size(), actual.getPolicies().size()); + } + assertEquals(0, actual.getPolicies().stream().filter(actualPolicy -> !expected.getPolicies().stream() .filter(expectedPolicy -> actualPolicy.equals(expectedPolicy)) .findFirst().isPresent()) diff --git a/themes/src/main/resources/theme/base/admin/resources/js/authz/authz-controller.js b/themes/src/main/resources/theme/base/admin/resources/js/authz/authz-controller.js index e164f59a03..116c12458e 100644 --- a/themes/src/main/resources/theme/base/admin/resources/js/authz/authz-controller.js +++ b/themes/src/main/resources/theme/base/admin/resources/js/authz/authz-controller.js @@ -1050,8 +1050,10 @@ module.controller('ResourceServerPolicyResourceDetailCtrl', function($scope, $ro var policies = []; - for (i = 0; i < $scope.selectedPolicies.length; i++) { - policies.push($scope.selectedPolicies[i].id); + if ($scope.selectedPolicies) { + for (i = 0; i < $scope.selectedPolicies.length; i++) { + policies.push($scope.selectedPolicies[i].id); + } } $scope.policy.policies = policies; diff --git a/themes/src/main/resources/theme/base/admin/resources/partials/authz/permission/provider/resource-server-policy-resource-detail.html b/themes/src/main/resources/theme/base/admin/resources/partials/authz/permission/provider/resource-server-policy-resource-detail.html index 6a02a0ad45..89f8b36083 100644 --- a/themes/src/main/resources/theme/base/admin/resources/partials/authz/permission/provider/resource-server-policy-resource-detail.html +++ b/themes/src/main/resources/theme/base/admin/resources/partials/authz/permission/provider/resource-server-policy-resource-detail.html @@ -56,7 +56,7 @@
- +
{{:: 'authz-policy-apply-policy.tooltip' | translate}} @@ -81,7 +81,7 @@
- +
diff --git a/themes/src/main/resources/theme/base/admin/resources/partials/authz/permission/provider/resource-server-policy-scope-detail.html b/themes/src/main/resources/theme/base/admin/resources/partials/authz/permission/provider/resource-server-policy-scope-detail.html index df4377fd77..49e533fe6c 100644 --- a/themes/src/main/resources/theme/base/admin/resources/partials/authz/permission/provider/resource-server-policy-scope-detail.html +++ b/themes/src/main/resources/theme/base/admin/resources/partials/authz/permission/provider/resource-server-policy-scope-detail.html @@ -60,7 +60,7 @@
- +
{{:: 'authz-policy-apply-policy.tooltip' | translate}} @@ -84,7 +84,7 @@
- +