8000 Do not allow updates on the realm-management client by pedroigor · Pull Request #49624 · keycloak/keycloak · GitHub
[go: up one dir, main page]

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
43 changes: 43 additions & 0 deletions docs/documentation/upgrading/topics/changes/changes-26_6_4.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// ------------------------ 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.

=== The `view-system` admin role no longer exists

Since version 26.5.4, when introduced, the `view-system` admin role was marked as deprecated. In this release, the role
has been removed for security reasons.

Access to full server information is now restricted to users in the `master` realm holding the `manage-realm` role.

If you are still relying on the `view-system` role, you should consider accessing the full server information using a user
(or service account) from the `master` realm and granted with `manage-realm` admin roles.

Before upgrading, you should check if any of your users, service accounts, or groups are still relying on the
`view-system` role and review their access controls to ensure they have the necessary permissions to access the server
information after the upgrade. The `manage-realm` admin role is a high-privileged role and should only be granted to
highly-trusted accounts.

// ------------------------ Notable changes ------------------------ //
== Notable changes

Notable changes may include internal behavior changes that prevent common misconfigurations, bugs that are fixed, or changes to simplify running {project_name}.
It also lists significant changes to internal APIs.

=== <TODO>

// ------------------------ Deprecated features ------------------------ //
== Deprecated features

The following sections provide details on deprecated features.

=== <TODO>

// ------------------------ Removed features ------------------------ //
== Removed features

The following features have been removed from this release.

=== <TODO>

4 changes: 4 additions & 0 deletions docs/documentation/upgrading/topics/changes/changes.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@

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

=== Migrating to 26.6.4

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

=== Migrating to 26.6.3

include::changes-26_6_3.adoc[leveloffset=2]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,6 @@ public class AdminRoles {
public static final String VIEW_IDENTITY_PROVIDERS = "view-identity-providers";
public static final String VIEW_AUTHORIZATION = "view-authorization";
public static final String VIEW_ORGANIZATIONS = "view-organizations";
@Deprecated(since = "26.4", forRemoval = true)
public static final String VIEW_SYSTEM = "view-system";

public static final String MANAGE_REALM = "manage-realm";
public static final String MANAGE_USERS = "manage-users";
Expand Down Expand Up @@ -72,6 +70,5 @@ public class AdminRoles {
ALL_ROLES.add(ADMIN);
ALL_ROLES.add(CREATE_REALM);
ALL_ROLES.add(REALM_ADMIN);
ALL_ROLES.add(VIEW_SYSTEM);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ public void deleteRole(final @Parameter(description = "role's name (not id!)") @
throw ErrorResponse.error(roleName + " is default role of the realm and cannot be removed.",
Response.Status.BAD_REQUEST);
}
auth.roles().requireManage(role);
RoleRepresentation roleRepresentation = new RoleRepresentation();
roleRepresentation.setId(role.getId());
roleRepresentation.setName(role.getName());
Expand Down Expand Up @@ -332,6 +333,7 @@ public Response updateRole(final @Parameter(description = "role's name (not id!)
if (role == null) {
throw new NotFoundException("Could not find role");
}
auth.roles().requireManage(role);
try {
updateRole(rep, role, realm, session);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

import jakarta.ws.rs.ForbiddenException;

import org.keycloak.Config;
import org.keycloak.authorization.AuthorizationProvider;
import org.keycloak.authorization.common.ClientModelIdentity;
import org.keycloak.authorization.common.DefaultEvaluationContext;
Expand All @@ -42,6 +43,7 @@
import org.keycloak.models.AdminRoles;
import org.keycloak.models.ClientModel;
import org.keycloak.models.ClientScopeModel;
import org.keycloak.models.Constants;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.RealmModel;
import org.keycloak.representations.AccessToken;
Expand Down Expand Up @@ -383,6 +385,9 @@ public Map<String, Collection<String>> getBaseAttributes() {

@Override
public boolean canManage(ClientModel client) {
if (isInternal(client)) {
return false;
}
if (canManageClientsDefault()) return true;
if (!root.isAdminSameRealm()) {
return false;
Expand Down Expand Up @@ -411,6 +416,9 @@ public boolean canManage(ClientModel client) {

@Override
public boolean canConfigure(ClientModel client) {
if (isInternal(client)) {
return false;
}
if (canManage(client)) return true;
if (!root.isAdminSameRealm()) {
return false;
Expand Down Expand Up @@ -708,4 +716,15 @@ private boolean hasPermission(Resource resource, String scope) {
return false;
}

protected boolean isInternal(ClientModel client) {
if (client == null) {
return false;
}

if (realm.getName().equals(Config.getAdminRealm())) {
return client.getClientId().endsWith(AdminRoles.APP_SUFFIX);
Comment thread
pedroigor marked this conversation as resolved.
Resolved
}

return Constants.REALM_MANAGEMENT_CLIENT_ID.equals(client.getClientId());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ public boolean canConfigure(ClientModel client) {

@Override
public boolean canManage(ClientModel client) {
if (isInternal(client)) {
return false;
}
if (root.hasOneAdminRole(AdminRoles.MANAGE_CLIENTS)) {
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.Collections;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;

Expand Down Expand Up @@ -484,7 +485,7 @@ public void requireMapClientScope(RoleModel role) {
@Override
public boolean canManage(RoleModel role) {
if (role.getContainer() instanceof RealmModel) {
return root.realm().canManageRealm();
return root.realm().canManageRealm() && !isRealmAdminRole(role);
} else if (role.getContainer() instanceof ClientModel) {
ClientModel client = (ClientModel)role.getContainer();
return root.clients().canConfigure(client);
Expand Down Expand Up @@ -682,4 +683,7 @@ private ResourceServer getResourceServer(RoleModel role) {
}
return resourceServer;
}
private boolean isRealmAdminRole(RoleModel role) {
return role.getContainer() instanceof RealmModel && List.of(AdminRoles.ADMIN, AdminRoles.CREATE_REALM).contains(role.getName());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,6 @@ class RolePermissionsV2 extends RolePermissions {
@Override
public boolean canMapRole(RoleModel role) {
if (isRealmAdminRole(role)) {
if (AdminRoles.VIEW_SYSTEM.equals(role.getName()) && !root.isAdmin(root.getMasterRealm())) {
return false;
}
if (realm.isAdminPermissionsEnabled()) {
// only server or realm admins can map roles if FGAP is enabled
return root.isRealmAdmin();
Expand All @@ -76,9 +73,6 @@ public boolean canMapRole(RoleModel role) {
@Override
public boolean canMapComposite(RoleModel role) {
if (isRealmAdminRole(role)) {
if (AdminRoles.VIEW_SYSTEM.equals(role.getName()) && !root.isAdmin(root.getMasterRealm())) {
return false;
}
if (realm.isAdminPermissionsEnabled()) {
// only server or realm admins can map roles if FGAP is enabled
return root.isRealmAdmin();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
import org.keycloak.events.EventType;
import org.keycloak.events.admin.OperationType;
import org.keycloak.events.admin.ResourceType;
import org.keycloak.models.AdminRoles;
import org.keycloak.models.GroupModel;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.RealmModel;
Expand Down Expand Up @@ -127,17 +126,20 @@ public ServerInfoRepresentation getInfo() {
ServerInfoRepresentation info = new ServerInfoRepresentation();
RealmModel userRealm = session.getContext().getRealm();
AdminPermissionEvaluator adminEvaluator = AdminPermissions.evaluator(session, userRealm, auth);
if (RealmManager.isAdministrationRealm(userRealm) || adminEvaluator.hasOneAdminRole(AdminRoles.VIEW_SYSTEM)) {
// system information is only for admins in the administration realm or fallback view-system role
info.setSystemInfo(SystemInfoRepresentation.create(session.getKeycloakSessionFactory().getServerStartupTimestamp(), Version.VERSION));
info.setCpuInfo(CpuInfoRepresentation.create());
info.setMemoryInfo(MemoryInfoRepresentation.create());
} else if (adminEvaluator.realm().canManageRealm()) {
// If the user can manage his own realm just add the version information
SystemInfoRepresentation systemInfo = new SystemInfoRepresentation();
systemInfo.setVersion(Version.VERSION);
info.setSystemInfo(systemInfo);

if (adminEvaluator.realm().canManageRealm()) {
if (RealmManager.isAdministrationRealm(userRealm)) {
info.setSystemInfo(SystemInfoRepresentation.create(session.getKeycloakSessionFactory().getServerStartupTimestamp(), Version.VERSION));
info.setCpuInfo(CpuInfoRepresentation.create());
info.setMemoryInfo(MemoryInfoRepresentation.create());
} else {
// If the user can manage his own realm just add the version information
SystemInfoRepresentation systemInfo = new SystemInfoRepresentation();
systemInfo.setVersion(Version.VERSION);
info.setSystemInfo(systemInfo);
}
}

info.setProfileInfo(createProfileInfo());
info.setFeatures(createFeatureRepresentations());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,19 @@ public void beforeEach() { // todo rewrite
managedMasterRealm.admin().users().get(roleUserUuid).roles().clientLevel(clientUuid).add(Collections.singletonList(roleRep));
}

for (String role : AdminRoles.ALL_REALM_ROLES) {
response = managedMasterRealm.admin().users().create(UserBuilder.create()
.username("master-user-" + role)
.password("password")
.build());
String roleUserUuid = ApiUtil.getCreatedId(response);
managedMasterRealm.cleanup().add(r -> r.users().delete(roleUserUuid).close());

String clientUuid = managedMasterRealm.admin().clients().findByClientId("master-realm").get(0).getId();
RoleRepresentation roleRep = managedMasterRealm.admin().clients().get(clientUuid).roles().get(role).toRepresentation();
managedMasterRealm.admin().users().get(roleUserUuid).roles().clientLevel(clientUuid).add(Collections.singletonList(roleRep));
}

clients.put(AdminRoles.REALM_ADMIN,
adminClientFactory.create().realm(REALM_NAME).username(AdminRoles.REALM_ADMIN).password("password").clientId("test-client").clientSecret("secret").build());

Expand All @@ -98,6 +111,10 @@ public void beforeEach() { // todo rewrite
adminClientFactory.create().realm("master").username("permissions-test-master-" + role).password("password").clientId(Constants.ADMIN_CLI_CLIENT_ID).build());
}

for (String role : AdminRoles.ALL_ROLES) {
clients.put("master-admin-" + role,
adminClientFactory.create().realm("master").username("master-user-" + role).password("password").clientId(Constants.ADMIN_CLI_CLIENT_ID).build());
}
}

protected void invoke(final Invocation invocation, AdminAuth.Resource resource, boolean manage) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,11 @@
import java.util.Objects;
import java.util.Set;
import java.util.function.Consumer;
import java.util.function.Predicate;
import java.util.stream.Collectors;

import jakarta.ws.rs.BadRequestException;
import jakarta.ws.rs.ForbiddenException;
import jakarta.ws.rs.NotFoundException;
import jakarta.ws.rs.core.Response;

Expand Down Expand Up @@ -95,6 +97,7 @@

import static java.util.Arrays.asList;

import static org.keycloak.models.Constants.REALM_MANAGEMENT_CLIENT_ID;
import static org.keycloak.models.Constants.defaultClients;

import static org.hamcrest.CoreMatchers.is;
Expand Down Expand Up @@ -450,13 +453,7 @@ public void removeClientWithDependentCompositeRoles() {

@Test
public void removeInternalClientExpectingBadRequestException() {
final String testRealmClientId = AdminApiUtil.findClientByClientId(managedMasterRealm.admin(), managedRealm.getName() + "-realm")
.toRepresentation().getId();

assertThrows(BadRequestException.class,
() -> managedMasterRealm.admin().clients().get(testRealmClientId).remove());

defaultClients.forEach(defaultClient -> {
defaultClients.stream().filter(Predicate.not(REALM_MANAGEMENT_CLIENT_ID::equals)).forEach(defaultClient -> {
final String defaultClientId = AdminApiUtil.findClientByClientId(managedRealm.admin(), defaultClient)
.toRepresentation().getId();

Expand All @@ -465,6 +462,23 @@ public void removeInternalClientExpectingBadRequestException() {
});
}

@Test
public void removeRealmManagementClientForbiddenException() {
assertThrows(ForbiddenException.class,
() -> {
String testRealmClientId = AdminApiUtil.findClientByClientId(managedMasterRealm.admin(), managedRealm.getName() + "-realm")
.toRepresentation().getId();
managedMasterRealm.admin().clients().get(testRealmClientId).remove();
});

assertThrows(ForbiddenException.class,
() -> {
String testRealmClientId = AdminApiUtil.findClientByClientId(managedRealm.admin(), REALM_MANAGEMENT_CLIENT_ID)
.toRepresentation().getId();
managedRealm.admin().clients().get(testRealmClientId).remove();
});
}

@Test
public void getClientRepresentation() {
String id = createClient().getId();
Expand Down
Loading
Loading
0