[KEYCLOAK-12438] - Scope-based policies falsely give a permit with an empty scope list

This commit is contained in:
Pedro Igor 2019-12-12 18:23:54 -03:00 committed by Stian Thorgersen
parent d46620569a
commit c821dcf820
2 changed files with 87 additions and 8 deletions

View file

@ -91,7 +91,7 @@ public class DecisionPermissionCollector extends AbstractDecisionCollector {
userManagedPermissions.add(policyResult);
}
if (!resourceGranted) {
resourceGranted = containsResource;
resourceGranted = isGrantingAccessToResource(resource, policy) && containsResource;
}
} else {
if (isResourcePermission(policy)) {
@ -150,6 +150,25 @@ public class DecisionPermissionCollector extends AbstractDecisionCollector {
}
}
/**
* Checks if the given {@code policy} is eligible to grant access to a resource. Resources are only granted if policy is
* not a scope-permission or, if so, the resource is a user-owned resource so that permissions can be overridden when
* inheriting policies from a typed/parent resource.
*
* @param resource the resource
* @param policy the policy that grants access to the resources
* @return {@code true} if the resource should be granted
*/
private boolean isGrantingAccessToResource(Resource resource, Policy policy) {
boolean scopePermission = isScopePermission(policy);
if (!scopePermission) {
return true;
}
return resource != null && !resource.getOwner().equals(resourceServer.getId());
}
public Collection<Permission> results() {
return permissions;
}

View file

@ -16,6 +16,7 @@
*/
package org.keycloak.testsuite.authz;
import static org.hamcrest.CoreMatchers.hasItem;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
@ -45,7 +46,9 @@ import org.jboss.arquillian.test.api.ArquillianResource;
import org.jetbrains.annotations.NotNull;
import org.junit.After;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
import org.keycloak.OAuth2Constants;
import org.keycloak.admin.client.resource.AuthorizationResource;
import org.keycloak.admin.client.resource.ClientResource;
@ -58,7 +61,6 @@ import org.keycloak.authorization.client.AuthzClient;
import org.keycloak.authorization.client.Configuration;
import org.keycloak.authorization.client.representation.TokenIntrospectionResponse;
import org.keycloak.authorization.client.util.HttpResponseException;
import org.keycloak.authorization.permission.ResourcePermission;
import org.keycloak.common.util.Base64Url;
import org.keycloak.models.utils.KeycloakModelUtils;
import org.keycloak.protocol.oidc.OIDCLoginProtocol;
@ -115,6 +117,9 @@ public class EntitlementAPITest extends AbstractAuthzTest {
@ArquillianResource
protected ContainerController controller;
@Rule
public ExpectedException expectedException = ExpectedException.none();
@Override
public void addTestRealms(List<RealmRepresentation> testRealms) {
testRealms.add(RealmBuilder.create().name("authz-test")
@ -233,13 +238,12 @@ public class EntitlementAPITest extends AbstractAuthzTest {
obj.put("claim-a", "claim-a");
request.setClaimToken(Base64Url.encode(JsonSerialization.writeValueAsBytes(obj)));
this.expectedException.expect(AuthorizationDeniedException.class);
this.expectedException.expectCause(Matchers.allOf(Matchers.instanceOf(HttpResponseException.class), Matchers.hasProperty("statusCode", Matchers.is(403))));
this.expectedException.expectMessage("Public clients are not allowed to send claims");
this.expectedException.reportMissingExceptionWithMessage("Should fail, public clients not allowed");
try {
getAuthzClient(AUTHZ_CLIENT_CONFIG).authorization(response.getAccessToken()).authorize(request);
} catch (AuthorizationDeniedException expected) {
assertEquals(403, HttpResponseException.class.cast(expected.getCause()).getStatusCode());
assertTrue(HttpResponseException.class.cast(expected.getCause()).toString().contains("Public clients are not allowed to send claims"));
}
getAuthzClient(AUTHZ_CLIENT_CONFIG).authorization(response.getAccessToken()).authorize(request);
}
@Test
@ -1925,6 +1929,62 @@ public class EntitlementAPITest extends AbstractAuthzTest {
}
}
@Test
public void testDenyScopeNotManagedByScopePolicy() throws Exception {
ClientResource client = getClient(getRealm(), RESOURCE_SERVER_TEST);
AuthorizationResource authorization = client.authorization();
JSPolicyRepresentation policy = new JSPolicyRepresentation();
policy.setName(KeycloakModelUtils.generateId());
policy.setCode("$evaluation.grant();");
authorization.policies().js().create(policy).close();
ResourceRepresentation resource = new ResourceRepresentation();
resource.setName(KeycloakModelUtils.generateId());
resource.addScope("sensors:view", "sensors:update", "sensors:delete");
try (Response response = authorization.resources().create(resource)) {
resource = response.readEntity(ResourceRepresentation.class);
}
ScopePermissionRepresentation permission = new ScopePermissionRepresentation();
permission.setName(KeycloakModelUtils.generateId());
permission.addResource(resource.getId());
permission.addScope("sensors:view");
permission.addPolicy(policy.getName());
authorization.permissions().scope().create(permission).close();
String accessToken = new OAuthClient().realm("authz-test").clientId(RESOURCE_SERVER_TEST).doGrantAccessTokenRequest("secret", "kolo", "password").getAccessToken();
AuthzClient authzClient = getAuthzClient(AUTHZ_CLIENT_CONFIG);
AuthorizationRequest request = new AuthorizationRequest();
request.addPermission(resource.getId(), "sensors:view");
AuthorizationResponse response = authzClient.authorization(accessToken).authorize(request);
assertNotNull(response.getToken());
Collection<Permission> permissions = toAccessToken(response.getToken()).getAuthorization().getPermissions();
assertEquals(1, permissions.size());
for (Permission grantedPermission : permissions) {
assertEquals(resource.getId(), grantedPermission.getResourceId());
assertEquals(1, grantedPermission.getScopes().size());
assertThat(grantedPermission.getScopes(), hasItem("sensors:view"));
}
request = new AuthorizationRequest();
request.addPermission(resource.getId(), "sensors:update");
this.expectedException.expect(AuthorizationDeniedException.class);
this.expectedException.expectCause(Matchers.allOf(Matchers.instanceOf(HttpResponseException.class), Matchers.hasProperty("statusCode", Matchers.is(403))));
this.expectedException.reportMissingExceptionWithMessage("should fail, session invalidated");
authzClient.authorization().authorize(request);
}
private void testRptRequestWithResourceName(String configFile) {
Metadata metadata = new Metadata();