Repository navigation
Introduce a capability allowing to change current membershipType for … - #52370
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The provider return contract conflicts with the implemented and tested idempotent behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Admin API support for changing an existing organization member between managed and unmanaged lifecycle modes.
Changes:
- Adds the REST endpoint and admin-client method.
- Implements persistence and cache invalidation.
- Adds lifecycle tests and documentation.
File summaries
| File | Description |
|---|---|
OrganizationMemberTest.java |
Tests updates, events, errors, and lifecycle behavior. |
OrganizationMemberResource.java (services) |
Implements the secured REST endpoint. |
OrganizationProvider.java |
Adds the provider API contract. |
JpaOrganizationProvider.java |
Persists membership-type changes. |
InfinispanOrganizationProvider.java |
Invalidates membership caches. |
OrganizationMemberResource.java (admin client) |
Exposes the endpoint to clients. |
managing-members.adoc |
Documents usage and lifecycle implications. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort l 8000 evel: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
899c6e3 to
c9474f1
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
| return Response.noContent().build(); | ||
| } | ||
|
|
||
| throw ErrorResponse.error("Not a member of the organization", Status.BAD_REQUEST); |
|
|
||
| UserEntity userEntity = em.find(UserEntity.class, member.getId()); | ||
| if (userEntity == null) { | ||
| return false; |
| logger.warnf("Changed membership type to MANAGED for user '%s' in organization '%s'. " | ||
| + "Removing this member or deleting the organization will also remove the user from the realm.", | ||
| member.getUsername(), organization.getName()); | ||
| } else { | ||
| logger.warnf("Changed membership type to UNMANAGED for user '%s' in organization '%s'. " | ||
| + "The user may lack local credentials if originally federated via an identity provider.", | ||
| member.getUsername(), organization.getName()); | ||
| } |
| if (StringUtil.isBlank(memberId)) { | ||
| throw ErrorResponse.error("id cannot be null", Status.BAD_REQUEST); | ||
| } |
| adminEvents.clear(); | ||
| try (Response response = organization.members().member(member.getId()).updateMembershipType(MembershipType.MANAGED)) { | ||
| assertEquals(Status.NO_CONTENT.getStatusCode(), response.getStatus()); | ||
| } | ||
|
|
||
| MemberRepresentation updated = organization.members().member(member.getId()).toRepresentation(); | ||
| assertEquals(MembershipType.MANAGED, updated.getMembershipType()); | ||
|
|
||
| // Verify admin event | ||
| AdminEventRepresentation event = adminEvents.poll(); | ||
| AdminEventAssertion.assertSuccess(event) | ||
| .operationType(OperationType.UPDATE) | ||
| .resourceType(ResourceType.ORGANIZATION_MEMBERSHIP); | ||
| assertEquals(MembershipType.MANAGED.name(), event.getDetails().get(MembershipType.NAME)); |
c9474f1 to
0982c79
Compare
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#salesPostEncTestKeycloak CI - Adapter IT Strict Cookies |
…existing organization members Closes keycloak#48913 Signed-off-by: Martin Kanis <mkanis@ibm.com>
0982c79 to
f970109
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The JPA implementation mishandles federated members and does not atomically enforce the single-managed-organization invariant.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
| if (MembershipType.MANAGED.equals(membershipType)) { | ||
| throwIfManagedByAnotherOrg(organization, member); |
| UserEntity userEntity = em.find(UserEntity.class, member.getId()); | ||
| if (userEntity == null) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
@martin-kanis Nice work on this one! The SPI contract is clean — the Javadoc on updateMembershipType clearly documents the idempotent return semantics. Cache invalidation via registerMemberInvalidation is correctly placed before delegating in the Infinispan layer, which is easy to get wrong. The test coverage is very solid too — especially the lifecycle test that verifies UNMANAGED→MANAGED followed by member removal actually deletes the user from the realm. And the documentation warning about lifecycle implications is a good addition.
One concern:
(As you also point out in the docs) Being a MANAGED member means the organization owns the user's lifecycle — removing the member or deleting the org also deletes the user from the realm. Having a user MANAGED in multiple orgs is nonsensical: which org "owns" the lifecycle? And it's dangerous: removing the user from org-A calls userProvider.removeUser(), which deletes them from the realm entirely, silently removing them from org-B as well.
This state might be already reachable today via the IdP broker flow (IdpAddOrganizationMemberAuthenticator unconditionally calls addManagedMember), but this PR adds another path to it via updateMembershipType.
I think updateMembershipType should reject changing to MANAGED if the user is already a MANAGED member of another organization (409 Conflict ??).
Ideally the same guard should also be added to JpaOrganizationProvider.addMember(org, user, metadata) — the private method that both addManagedMember and RegistrationUserCreation.addOrganizationMember use — so all runtime paths are covered in one place. That could be handled in a separate PR, if you'd prefer so.
|
+1 to |
…existing organization members
Closes #48913