Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support realm role attributes update #991

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Support realm role attributes update #991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
66af3df
refactor: change the code to override realm-level role attributes onl…
Feb 21, 2024
6a94282
docs: update managed.md documentation to specify that attributes of r…
Feb 21, 2024
bfe83ac
fix: checkstyle and typo
Feb 21, 2024
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
36 changes: 18 additions & 18 deletions docs/MANAGED.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,24 +20,24 @@ groups will be deleted. If you define `groups` but set an empty array, keycloak

## Supported full managed resources

| Type | Additional Information | Resource Name |
|---------------------------------|----------------------------------------------------------------------------------|----------------------------------|
| Groups | - | `group` |
| Required Actions | You have to copy the default one to you import json. | `required-action` |
| Client Scopes | - | `client-scope` |
| Scope Mappings | - | `scope-mapping` |
| Client Scope Mappings | - | `client-scope-mapping` |
| Roles | - | `role` |
| Components | You have to copy the default components to you import json. | `component` |
| Sub Components | You have to copy the default components to you import json. | `sub-component` |
| Authentication Flows | You have to copy the default components to you import json, expect builtin flows | `authentication-flow` |
| Identity Providers | - | `identity-provider` |
| Identity Provider Mappers | - | `identity-provider-mapper` |
| Clients | - | `client` |
| Clients Authorization Resources | The 'Default Resource' is always included. | `client-authorization-resources` |
| Clients Authorization Policies | - | `client-authorization-policies` |
| Clients Authorization Scopes | - | `client-authorization-scopes` |
| Message Bundles | Only message bundles imported with config-cli will be managed/deleted. | `message-bundles` |
| Type | Additional Information | Resource Name |
|---------------------------------|-------------------------------------------------------------------------------------------------|----------------------------------|
| Groups | - | `group` |
| Required Actions | You have to copy the default one to you import json. | `required-action` |
| Client Scopes | - | `client-scope` |
| Scope Mappings | - | `scope-mapping` |
| Client Scope Mappings | - | `client-scope-mapping` |
| Roles | If not set as 'full', the attributes of realm-level role will be updated instead of override. | `role` |
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this change the defaul behavior of the application? Do we have a backward compatibility issue?

Copy link
Member

Choose a reason for hiding this comment

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

Even is we set full as the default value, how do change the value for existing deployments?

Copy link
Author

Choose a reason for hiding this comment

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

As far as I see it this change fixes a bug.
If I set the roles to be fully managed, then yes, I expect the data will be overridden.
I expect nothing will be deleted if I set the Roles with no-delete value.
Without this fix, there is data loss once the roles have no-delete managed configuration.

| Components | You have to copy the default components to you import json. | `component` |
| Sub Components | You have to copy the default components to you import json. | `sub-component` |
| Authentication Flows | You have to copy the default components to you import json, expect builtin flows | `authentication-flow` |
| Identity Providers | - | `identity-provider` |
| Identity Provider Mappers | - | `identity-provider-mapper` |
| Clients | - | `client` |
| Clients Authorization Resources | The 'Default Resource' is always included. | `client-authorization-resources` |
| Clients Authorization Policies | - | `client-authorization-policies` |
| Clients Authorization Scopes | - | `client-authorization-scopes` |
| Message Bundles | Only message bundles imported with config-cli will be managed/deleted. | `message-bundles` |

## Disable deletion of managed entities

Expand Down
2 changes: 1 addition & 1 deletion src/main/java/de/adorsys/keycloak/config/repository/RoleRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public RoleRepresentation getRealmRole(String realmName, String roleName) {

public List<RoleRepresentation> getRealmRoles(String realmName) {
return realmRepository.getResource(realmName)
.roles().list();
.roles().list(false);
Copy link
Member

Choose a reason for hiding this comment

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

Any code that relies on the getRealmRoles method to retrieve a complete list of all roles (including composite ones) will now be affected. Do we know all consumers of this method?

Copy link
Author

Choose a reason for hiding this comment

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

getRealmRoles is used by only one method (RealmImportService :: doImport).
Anyway, the false flag is for the "briefRepresentation" - which is true by default to save memory I guess.
In that case, we need to get the full role and not only summarization cause we want to update it.

}

public List<RoleRepresentation> getRealmRolesByName(String realmName, Collection<String> roles) {
Expand Down
16 changes: 15 additions & 1 deletion src/main/java/de/adorsys/keycloak/config/service/RoleImportService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Service;

import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
Expand Down Expand Up @@ -211,8 +212,21 @@ private void updateRoleIfNeeded(
) {
String roleName = roleToImport.getName();
RoleRepresentation patchedRole = CloneUtil.patch(existingRole, roleToImport, propertiesWithDependencies);
if (roleToImport.getAttributes() != null) {

Copy link
Member

Choose a reason for hiding this comment

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

Looks like method does what it is supposed to. But we need tests to

  1. Secure the behavior
  2. Secure backward compatibility Role=role -> full

How do we proceed for the removal of existing attributes.

if (importConfigProperties.getManaged().getRole() == ImportConfigProperties.ImportManagedProperties.ImportManagedPropertiesValues.FULL
&& roleToImport.getAttributes() != null) {
logger.debug("Setting the attributes of the patched realm-level role as roleToImport attributes");
patchedRole.setAttributes(roleToImport.getAttributes());
} else {
logger.debug("Setting the attributes of the patched realm-level role as a merge of roleToImport and existingRole");
Map<String, List<String>> attributes = new HashMap<>();
if (existingRole.getAttributes() != null) {
attributes.putAll(existingRole.getAttributes());
}
if (roleToImport.getAttributes() != null) {
attributes.putAll(roleToImport.getAttributes());
}
patchedRole.setAttributes(attributes);
}

if (!CloneUtil.deepEquals(existingRole, patchedRole)) {
Expand Down
Loading