Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ protected boolean isRetry() {
* @return a {@link ProtectionResource}
*/
public ProtectionResource protection(String userName, String password) {
patSupplier = null;
return new ProtectionResource(this.http, this.serverConfiguration, configuration, createPatSupplier(userName, password));
}

Expand Down
11 changes: 11 additions & 0 deletions docs/documentation/upgrading/topics/changes/changes-26_5_6.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// ------------------------ Breaking changes ------------------------ //
== Breaking changes

Breaking changes are identified as those that might require changes for existing users to their configurations or applications.
In minor or patch releases, {project_name} will only introduce breaking changes to fix bugs.

=== Stricter access control for managing permission tickets

In order to improve security, the access control for managing permission tickets has been made stricter where
only users (or service accounts) granted with the `uma-protection` role can manage permission tickets for a given
resource server. The only exception is the resource server itself, which can manage its permission tickets without the `uma-protection` role.
8 changes: 8 additions & 0 deletions docs/documentation/upgrading/topics/changes/changes.adoc
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
[[migration-changes]]
== Migration Changes

=== Migrating to 26.5.6

include::changes-26_5_6.adoc[leveloffset=2]

=== Migrating to 26.5.5
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The notes for 26.5.5 were not here. Please let me know if it was a mistake and if this change makes sense.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pedroigor - this change makes sense, cc: @mabartos


include::changes-26_5_5.adoc[leveloffset=2]

=== Migrating to 26.5.4

include::changes-26_5_4.adoc[leveloffset=2]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@
import org.keycloak.authorization.store.PermissionTicketStore;
import org.keycloak.authorization.store.ResourceStore;
import org.keycloak.common.util.Time;
import org.keycloak.models.ClientModel;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.UserModel;
import org.keycloak.models.utils.KeycloakModelUtils;

import static org.keycloak.models.jpa.PaginationUtils.paginateQuery;
Expand Down Expand Up @@ -82,9 +85,27 @@ private List<Predicate> getPredicates(CriteriaBuilder builder,
ResourceServer resourceServer,
Map<PermissionTicket.FilterOption, String> attributes) {
List<Predicate> predicates = new ArrayList<>();
KeycloakSession session = provider.getKeycloakSession();

if (resourceServer != null) {
if (resourceServer != null && !Boolean.parseBoolean(attributes.get(PermissionTicket.FilterOption.IS_ADMIN))) {
predicates.add(builder.equal(root.get("resourceServer").get("id"), resourceServer.getId()));
ClientModel resourceServerClient = session.clients().getClientById(session.getContext().getRealm(), resourceServer.getClientId());
UserModel currentUser = session.getContext().getUser();

if (resourceServerClient.isServiceAccountsEnabled()) {
UserModel serviceAccount = session.users().getServiceAccount(resourceServerClient);

if (serviceAccount != null && serviceAccount.equals(currentUser)) {
currentUser = null;
}
}

if (currentUser != null) {
predicates.add(builder.or(
builder.equal(root.get("owner"), currentUser.getId()),
builder.equal(root.get("requester"), currentUser.getId())
));
}
}

attributes.forEach((filterOption, value) -> {
Expand Down Expand Up @@ -121,6 +142,8 @@ private List<Predicate> getPredicates(CriteriaBuilder builder,
case POLICY_IS_NOT_NULL:
predicates.add(builder.isNotNull(root.get("policy")));
break;
case IS_ADMIN:
break;
default:
throw new IllegalArgumentException("Unsupported filter [" + filterOption + "]");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.util.Map;

import org.keycloak.authorization.model.PermissionTicket;
import org.keycloak.authorization.model.PermissionTicket.FilterOption;
import org.keycloak.authorization.model.Policy;
import org.keycloak.authorization.model.ResourceServer;
import org.keycloak.authorization.model.Scope;
Expand Down Expand Up @@ -81,6 +82,7 @@ public static void removePolicy(PermissionTicket ticket, StoreFactory storeFacto
filter.put(PermissionTicket.FilterOption.REQUESTER, ticket.getRequester());
filter.put(PermissionTicket.FilterOption.RESOURCE_ID, ticket.getResource().getId());
filter.put(PermissionTicket.FilterOption.GRANTED, Boolean.TRUE.toString());
filter.put(FilterOption.IS_ADMIN, Boolean.TRUE.toString());

List<PermissionTicket> tickets = storeFactory.getPermissionTicketStore().find(ticket.getResourceServer(), filter, null, null);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,11 @@ public static enum FilterOption {
REQUESTER("requester"),
REQUESTER_IS_NULL("requester_is_null"),
POLICY_IS_NOT_NULL("policy_is_not_null"),
POLICY_ID("policy.id")
;
POLICY_ID("policy.id"),
/** a special filter option to ignore owner and requester checks in order to return any ticket, including those
* that the user is not the owner or requester
*/
IS_ADMIN("is_admin");

private final String name;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.keycloak.authorization.common.KeycloakIdentity;
import org.keycloak.authorization.fgap.AdminPermissionsSchema;
import org.keycloak.authorization.model.PermissionTicket;
import org.keycloak.authorization.model.PermissionTicket.FilterOption;
import org.keycloak.authorization.model.Policy;
import org.keycloak.authorization.model.ResourceServer;
import org.keycloak.authorization.model.Scope;
Expand Down Expand Up @@ -194,6 +195,7 @@ private static PolicyEvaluationResponse.PolicyResultRepresentation toRepresentat
Map<PermissionTicket.FilterOption, String> filters = new EnumMap<>(PermissionTicket.FilterOption.class);

filters.put(PermissionTicket.FilterOption.POLICY_ID, policy.getId());
filters.put(FilterOption.IS_ADMIN, Boolean.TRUE.toString());

List<PermissionTicket> tickets = authorization.getStoreFactory().getPermissionTicketStore().find(resourceServer, filters, -1, 1);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ public Response authorize(KeycloakAuthorizationRequest request) {

if (identity != null) {
event.user(identity.getId());
request.getKeycloakSession().getContext().setBearerToken(identity.getAccessToken());
}

ResourceServer resourceServer = getResourceServer(ticket, request);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public Object permission() {

@Path("/permission/ticket")
public Object ticket() {
KeycloakIdentity identity = createIdentity(false);
KeycloakIdentity identity = createIdentity(true);

return new PermissionTicketService(identity, getResourceServer(identity), this.authorization);
}
Expand All @@ -100,6 +100,11 @@ private KeycloakIdentity createIdentity(boolean checkProtectionScope) {
ClientModel client = realm.getClientById(resourceServer.getClientId());

if (checkProtectionScope) {
if (identity.isResourceServer()) {
// if the identity is the resource server itself, then we don't need to check for uma_protection scope
return identity;
}

if (!identity.hasClientRole(client.getClientId(), "uma_protection")) {
throw new ErrorResponseException(OAuthErrorException.INVALID_SCOPE, "Requires uma_protection scope.", Status.FORBIDDEN);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ public Response create(PermissionTicketRepresentation representation) {
Resource resource = rstore.findById(resourceServer, representation.getResource());
if (resource == null ) throw new ErrorResponseException("invalid_resource_id", "Resource set with id [" + representation.getResource() + "] does not exists in this server.", Response.Status.BAD_REQUEST);

if (!resource.getOwner().equals(this.identity.getId()))
throw new ErrorResponseException("not_authorised", "permissions for [" + representation.getResource() + "] can be only created by the owner", Response.Status.FORBIDDEN);
if (!identity.isResourceServer() && !resource.getOwner().equals(this.identity.getId()))
throw new ErrorResponseException("not_authorised", "permissions for [" + representation.getResource() + "] can be only created by the owner or by the resource server itself", Response.Status.FORBIDDEN);
if (!resource.isOwnerManagedAccess())
throw new ErrorResponseException("invalid_permission", "permission can only be created for resources with user-managed access enabled", Response.Status.BAD_REQUEST);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
public abstract class AbstractRestServiceTest extends AbstractTestRealmKeycloakTest {

@Rule
public TokenUtil tokenUtil = new TokenUtil();
public TokenUtil tokenUtil = new TokenUtil("test-user@localhost", "password");

@Rule
public AssertEvents events = new AssertEvents(this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@

import com.fasterxml.jackson.core.type.TypeReference;
import org.junit.BeforeClass;
import org.junit.Rule;
import org.junit.Test;

import static org.keycloak.common.util.Encode.encodePathAsIs;
Expand All @@ -78,6 +79,9 @@
*/
public class ResourcesRestServiceTest extends AbstractRestServiceTest {

@Rule
public TokenUtil tokenUtil = new TokenUtil("test-authz-user@localhost", "password");

private AuthzClient authzClient;
private List<String> userNames = new ArrayList<>(Arrays.asList("alice", "jdoe", "bob"));

Expand All @@ -89,13 +93,17 @@ public static void enabled() {
@Override
public void configureTestRealm(RealmRepresentation testRealm) {
super.configureTestRealm(testRealm);
RealmRepresentation realmRepresentation = testRealm;

realmRepresentation.setUserManagedAccessAllowed(true);
testRealm.setUserManagedAccessAllowed(true);

testRealm.getUsers().add(createUser("alice", "password"));
testRealm.getUsers().add(createUser("jdoe", "password"));
testRealm.getUsers().add(createUser("bob", "password"));
testRealm.getUsers().add(UserBuilder.create().username("test-authz-user@localhost").password("password")
.addRoles("uma_authorization", "uma_protection")
.role("my-resource-server", "uma_protection")
.role("account", AccountRoles.MANAGE_ACCOUNT)
.build());

ClientRepresentation client = ClientBuilder.create()
.clientId("my-resource-server")
Expand Down Expand Up @@ -149,7 +157,7 @@ public void before() {
ticket.setResource(resource.getId());
ticket.setScopeName(scope);

authzClient.protection("test-user@localhost", "password").permission().create(ticket);
authzClient.protection("test-authz-user@localhost", "password").permission().create(ticket);
}
}
}
Expand Down Expand Up @@ -524,12 +532,12 @@ public void testGetPermissionRequests() {
PermissionTicketRepresentation ticket = new PermissionTicketRepresentation();

ticket.setGranted(false);
ticket.setOwner("test-user@localhost");
ticket.setOwner("test-authz-user@localhost");
ticket.setRequesterName(userName);
ticket.setResource(resource.getId());
ticket.setScopeName(scope);

authzClient.protection("test-user@localhost", "password").permission().create(ticket);
authzClient.protection("test-authz-user@localhost", "password").permission().create(ticket);
}
}

Expand Down Expand Up @@ -588,12 +596,12 @@ public void testApprovePermissionRequest() throws IOException {
PermissionTicketRepresentation ticket = new PermissionTicketRepresentation();

ticket.setGranted(false);
ticket.setOwner("test-user@localhost");
ticket.setOwner("test-authz-user@localhost");
ticket.setRequesterName(userName);
ticket.setResource(resource.getId());
ticket.setScopeName(scope);

authzClient.protection("test-user@localhost", "password").permission().create(ticket);
authzClient.protection("test-authz-user@localhost", "password").permission().create(ticket);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,10 @@ public void addTestRealms(List<RealmRepresentation> testRealms) {
.user(UserBuilder.create().username("alice").password("password")
.addRoles("uma_authorization", "uma_protection")
.role("resource-server-test", "uma_protection"))
.user(UserBuilder.create().username("kolo").password("password"))
.user(UserBuilder.create().username("kolo").password("password")
.addRoles("uma_authorization", "uma_protection")
.role("resource-server-test", "uma_protection")
)
.client(ClientBuilder.create().clientId("resource-server-test")
.secret("secret")
.authorizationServicesEnabled(true)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,51 @@ public void testSameTicketForSamePermissionRequest() throws Exception {
assertNotNull(response.getTicket());
}

@Test
public void testSearchTicket() throws Exception {
ResourceRepresentation resource = addResource("Resource A", "kolo", true);
AuthzClient authzClient = getAuthzClient();
PermissionResponse response = authzClient.protection("marta", "password").permission().create(new PermissionRequest(resource.getId()));
AuthorizationRequest request = new AuthorizationRequest();
request.setTicket(response.getTicket());
request.setClaimToken(authzClient.obtainAccessToken("marta", "password").getToken());
try {
authzClient.authorization().authorize(request);
} catch (Exception e) {

}
List<PermissionTicketRepresentation> tickets = authzClient.protection("kolo", "password").permission().find(resource.getId(), null, null, null, null, true, -1, -1);
assertEquals(1, tickets.size());
tickets = authzClient.protection("marta", "password").permission().find(resource.getId(), null, null, null, null, true, -1, -1);
assertEquals(1, tickets.size());

response = authzClient.protection("alice", "password").permission().create(new PermissionRequest(resource.getId()));
request = new AuthorizationRequest();
request.setTicket(response.getTicket());
request.setClaimToken(authzClient.obtainAccessToken("alice", "password").getToken());
try {
authzClient.authorization().authorize(request);
} catch (Exception e) {
}
// two tickets open for kolo, one for alice and one for marta
tickets = authzClient.protection("kolo", "password").permission().find(resource.getId(), null, null, null, null, true, -1, -1);
assertEquals(2, tickets.size());
assertEquals(2L, (long) authzClient.protection("kolo", "password").permission().count(resource.getId(), null, null, null, null, true));
// one ticket for alice
tickets = authzClient.protection("alice", "password").permission().find(resource.getId(), null, null, null, null, true, -1, -1);
assertEquals(1, tickets.size());
assertEquals(1L, (long) authzClient.protection("alice", "password").permission().count(resource.getId(), null, null, null, null, true));
PermissionTicketRepresentation aliceTicket = tickets.get(0);
// one ticket for marta
tickets = authzClient.protection("marta", "password").permission().find(resource.getId(), null, null, null, null, true, -1, -1);
assertEquals(1, tickets.size());
assertEquals(1L, (long) authzClient.protection("marta", "password").permission().count(resource.getId(), null, null, null, null, true));
// the ticket for alice is different from the ticket for marta
assertFalse(aliceTicket.getId().equals(tickets.get(0).getId()));


}

private void assertPersistence(PermissionResponse response, ResourceRepresentation resource, String... scopeNames) throws Exception {
String ticket = response.getTicket();
assertNotNull(ticket);
Expand Down
Loading