Repository navigation
Feature/organization roles - #48738
yaroslav02678 wants to merge 1 commit into
Conversation
aed7bbb to
1f8c841
Compare
|
@vramik Hi, I'm a programming student and my teacher has given me an assignment to try to implement this feature. I would appreciate any feedback on my solution. |
1f8c841 to
1a88383
Compare
1a88383 to
995b522
Compare
There was a problem hiding this comment.
Hi @yaroslav02678, thanks for taking the time to work on this! Organization roles are an important feature we're actively designing.
Before I dive into technical feedback, could you clarify your intent with this PR? Specifically:
- Are you looking for directional feedback on your approach (e.g., for your assignment/learning)?
- Or are you aiming to iterate this PR toward merge through the full review process?
Either way is fine — I just want to set expectations so neither of us invests time in a way that doesn't serve the goal.
Design pointers
We have discussed a design for this feature within the team, at some extend, already. Here are the key points your implementation should align with:
1. Storage: Shared Table with TYPE column
Rather than using role attributes to tag roles as "organizational," the agreed approach is to add a TYPE column to the existing KEYCLOAK_ROLE table (similar to how org groups reuse KEYCLOAK_GROUP with a type). This means:
- A schema migration adding
TYPE(e.g., 0=REALM, 1=CLIENT, 2=ORGANIZATION) and anORG_IDforeign key toKEYCLOAK_ROLE(same approach asKEYCLOAK_GROUPtable uses) - Existing queries will need to be migrated to use the
TYPE, so org roles don't leak into realm or client role listings - Composites across role types work naturally through the existing infrastructure
The current attribute-based approach ("organization.id" / "org.role.name" attributes, "org.[orgId].[roleName]" naming) avoids schema changes but introduces fragility, inefficient queries, and adds a maintenance burden in the long term.
2. Consider OrganizationModel extending RoleContainerModel
Keycloak's RealmModel and ClientModel both implement RoleContainerModel, which provides a standard interface for role CRUD. If OrganizationModel implements it too, you get role infrastructure (composites, mappings, default roles) largely for free. This is an open design question worth exploring.
3. Token mappers
Roles need to appear in tokens to be useful. The design includes an organization roles token mappers being able to add org-roles under the existing "organization" (by default) claim. Without this, roles exist in the DB but clients can't consume them.
4. IdP mappers
There are several IdP mappers that assign roles during brokered login, they support both realm and client roles, newly they should support org-roles as well.
5. Tests
New tests should go in the new test framework (tests/base/src/test/java/org/keycloak/tests/...), not the old Arquillian suite (testsuite/integration-arquillian/...).
6. Admin client dependency
The admin client module should not depend on keycloak-services — that inverts the dependency direction.
7. Eventually we will need also Admin UI for it
It's also possible to implement admin UI as follow up.
8. Documentaion
Just mentioning it for completness.
Happy to discuss any of these points further. Also please note, that I might not catch everything what needs attention, other things might occur "as we go". Once again thanks for your time on this.
|
To complement what @vramik said, our near-term goal is to deliver #47327, so that we address the authorization story for (most) CIAM/B2B use cases to enable access checks (RBAC/GBAC) on realm/platform/company clients when accessing their protected resources. We might not be able to put much effort, including reviews, because of this, and also because of other tasks we are running in parallel. |
|
Hi @vramik and the team, thank you for the detailed feedback! |
995b522 to
e9e9f93
Compare
e9e9f93 to
90af72a
Compare
|
Hi @vramik, I have completed the refactoring based on your design pointers. |
|
Hello @yaroslav02678, thanks for the update. I wanted to give you a heads up — my backlog is quite full at the moment, but I hope to take a look next week. |
|
@yaroslav02678 thanks for the updated version. There are some issues I see, which needs to fixed to proceed further: Critical Issues1. Two conflicting implementations — the PR contradicts itselfThe PR adds proper // JpaOrganizationProvider — attribute-based, the path the REST API actually uses
String internalName = "org." + organization.getId() + "." + name;
RoleModel role = realm.addRole(internalName);
role.setSingleAttribute(ORG_ROLE_ATTRIBUTE, organization.getId());
role.setSingleAttribute(ORG_ROLE_NAME_ATTRIBUTE, name);Since Fix: Remove the attribute-based implementation from 2. No authorization checks — and incomplete authorization model2a. Missing permission checks entirely
@Path("roles")
public OrganizationRolesResource roles() {
return new OrganizationRolesResource(session, organization, adminEvent);
// Missing: auth parameter
}Any authenticated admin can create/delete/assign org roles regardless of their permissions. 2b. Required permission modelExisting org member operations use a dual permission model (org-level + user-level). Org role endpoints should follow the same pattern:
The org-level 2c. Org role assignments must be blocked from the user role-mappings APISince the current attribute-based implementation stores org roles as plain realm roles, nothing prevents assigning/revoking them via With the proper 3. Inconsistent named query filtering — some queries still use
|
| Query | Filter |
|---|---|
getRealmRoles |
type = 0 |
getRealmRoleIds |
clientRole = false |
getRealmRoleByName |
type = 0 |
getRealmRoleIdByName |
clientRole = false |
Since the attribute-based approach creates org roles with clientRole = false and type = 0, the queries still filtering on clientRole will leak org roles into realm role ID lookups. All queries must be migrated consistently to use the type column.
4. setRealmId() / setClientId() side effects are order-dependent
The PR adds state-setting side effects to setters:
public void setRealmId(String realmId) {
this.realmId = realmId;
this.clientRealmConstraint = realmId;
this.type = TYPE_REALM; // NEW
this.clientRole = false; // NEW
}
public void setClientId(String clientId) {
this.clientId = clientId;
this.clientRealmConstraint = clientId;
this.type = TYPE_CLIENT; // NEW
this.clientRole = true; // NEW
}If any existing code calls these setters in an unexpected order, the entity ends up in a silently wrong state. The coupling between unrelated setters is fragile.
Significant Issues
5. Missing cascade delete for org roles
When an organization is deleted, its roles are not cleaned up. Neither OrganizationProvider.remove() nor any event listener removes org-scoped roles. This will leave orphaned rows in KEYCLOAK_ROLE.
6. Pagination ignored in JpaOrganizationProvider
getRoles() and searchRolesByName() accept first/max parameters but ignore them — they load all realm roles into memory and filter by attribute:
public Stream<RoleModel> getRoles(OrganizationModel organization, Integer first, Integer max) {
return getRealm().getRolesStream() // loads ALL realm roles
.filter(r -> organization.getId().equals(r.getFirstAttribute(ORG_ROLE_ATTRIBUTE)));
}With the TYPE column approach and proper named queries, pagination would be handled at the DB level.
7. getMembersWithRole() is O(n) per member
return session.users().getRoleMembersStream(getRealm(), realmRole)
.filter(u -> isMember(organization, u)); // individual DB lookup per userFor orgs with many role members, this is quadratic in the worst case.
8. ResourceType.REALM_ROLE used for admin events
Admin events use ResourceType.REALM_ROLE, making org role events indistinguishable from realm role events in audit logs. The comment confirms this is a known shortcut: // використовуємо існуючий тип (Ukrainian: "we use the existing type"). A proper ResourceType for org roles should be added.
9. Liquibase changelog naming doesn't follow conventions
The file is named jpa-changelog-authz-org-roles.xml instead of being included in a versioned changelog (jpa-changelog-26.X.0.xml). This will cause merge conflicts and is inconsistent with all other migrations. Also missing newline at end of file.
10. No foreign key constraint on ORGANIZATION_ID
The migration adds the column but doesn't add <addForeignKeyConstraint> referencing the ORGANIZATION table. Combined with missing cascade delete (#5), this can produce inconsistent state.
11. RoleAdapter container mismatch
When org roles are created via JpaRealmProvider.addOrganizationRole(), the returned RoleAdapter passes the realm as the container, not the organization. role.getContainer() will return the realm, breaking the OrganizationAdapter.removeRole() check role.getContainer() != this.
Minor Issues
- Unrelated CI changes: Removing the
external-linksjob and addingMAVEN_OPTStodocumentation.ymlhas nothing to do with org roles. Should be a separate PR. - Ukrainian comment in
OrganizationAdapter.removeDefaultRoles():// Логіка видалення— remove or translate. - Empty
addDefaultRole()/removeDefaultRoles()inOrganizationAdapter— no-op implementations should throwUnsupportedOperationExceptionrather than silently ignore calls. getRealmRolesnamed query has two queries merged onto one line (formatting issue).OrganizationRolesResourceconstructor:session == nullnull-guards are defensive but inconsistent — ifsessionis null, the resource methods will NPE anyway.- Test lifecycle: Uses
LifeCycle.CLASSbut deletes all orgs in@BeforeEach— should useLifeCycle.METHODor clean up in@AfterEach. - Missing token mapper: The agreed design includes an
organization_rolesclaim in tokens. Not present in this PR — could be a follow-up.
|
Closing in favor of #50606 |
[Organization] Organization-scoped Role Management Implementation
Following the design feedback from @vramik and @pedroigor, I have refactored the implementation to align with Keycloak's core architecture.
Key Architectural Changes:
TYPEcolumn (0=REALM, 1=CLIENT, 2=ORGANIZATION) and anORG_IDforeign key to theKEYCLOAK_ROLEtable.jpa-changelog-authz-org-roles.xml.OrganizationModelnow extendsRoleContainerModel. This allows organizations to natively leverage existing role infrastructure (composites, mappings, etc.).RoleProviderandJpaRealmProviderto filter roles by the newTYPE, ensuring organization roles are isolated and do not leak into realm or client role listings.tests/base/src/test/java/org/keycloak/tests/admin/organization/).keycloak-admin-clientno longer depends onkeycloak-services.Implementation Details:
AdminEventBuilder.Issue:
Closes #40585 / Related to #47326
Note to Reviewers:
The PR has been rebased on the latest main, squashed into a single commit, and properly signed off (DCO).