Support realm role attributes update #991
Conversation
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #991 +/- ##
============================================
+ Coverage 95.53% 95.70% +0.16%
- Complexity 1324 1367 +43
============================================
Files 78 80 +2
Lines 4279 4376 +97
Branches 477 494 +17
============================================
+ Hits 4088 4188 +100
- Misses 91 93 +2
+ Partials 100 95 -5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
@oriddd is there a chance you could merge current state of main into your pull request branch?
…y if fully managed
…ealm-level role will be override only if set as full
oriddd
force-pushed
the
supportRealmRoleAttributesUpdate
branch
from
0b1856e to
d8afd78
Compare
oriddd
force-pushed
the
supportRealmRoleAttributesUpdate
branch
from
d8afd78 to
bfe83ac
Compare
|
@francis-pouatcha Done. |
| | 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` | |
There was a problem hiding this comment.
Doesn't this change the defaul behavior of the application? Do we have a backward compatibility issue?
There was a problem hiding this comment.
Even is we set full as the default value, how do change the value for existing deployments?
There was a problem hiding this comment.
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.
| @@ -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); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| @@ -211,8 +212,21 @@ private void updateRoleIfNeeded( | |||
| ) { | |||
| String roleName = roleToImport.getName(); | |||
| RoleRepresentation patchedRole = CloneUtil.patch(existingRole, roleToImport, propertiesWithDependencies); | |||
| if (roleToImport.getAttributes() != null) { | |||
|
|
|||
There was a problem hiding this comment.
Looks like method does what it is supposed to. But we need tests to
- Secure the behavior
- Secure backward compatibility Role=role -> full
How do we proceed for the removal of existing attributes.
What this PR does / why we need it:
This PR fixes a bug where attributes are being overridden instead of merged.
PR Readiness Checklist:
CHANGELOG.mdrelease notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PRManaged.md was updated.