Merge KeycloakAdminService into KeycloakAuthorityPuller#463
Open
tobihagemann wants to merge 2 commits into
Open
Merge KeycloakAdminService into KeycloakAuthorityPuller#463tobihagemann wants to merge 2 commits into
tobihagemann wants to merge 2 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR consolidates Keycloak administrative operations from a dedicated Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
backend/src/test/java/org/cryptomator/hub/api/UsersResourceIT.java (1)
411-441: ⚡ Quick winAdd coverage for non-empty
realmRolesduring user creation.This suite only tests
realmRoles: [], so the new branch that callskeycloakAuthorityPuller.updateUserRoles(...)on create is currently untested. Add one success case with roles and verify the call to prevent silent regressions in role propagation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/test/java/org/cryptomator/hub/api/UsersResourceIT.java` around lines 411 - 441, Add a new integration test in UsersResourceIT (alongside testCreateUserSuccess) that posts a user payload with a non-empty "realmRoles" array, mock keycloakAuthorityPuller.createUser to return a UserRepresentation (as in testCreateUserSuccess), and then verify that keycloakAuthorityPuller.updateUserRoles(...) is invoked with the created user's id and the expected role list; the test should assert the endpoint still returns 201 and include verification of the updateUserRoles call to cover the newly added branch.backend/src/test/java/org/cryptomator/hub/api/GroupsResourceIT.java (1)
241-255: ⚡ Quick winAdd explicit delegation verification in update-group success test.
testUpdateGroupSuccessstubskeycloakAuthorityPuller.updateGroup(...)but never verifies it was called. On this route, a 200 can still be returned from DB state even if delegation regresses, so this test can false-pass.Suggested patch
given().contentType(ContentType.JSON).body(body) .when().put("/groups/group1") .then().statusCode(200); + + Mockito.verify(keycloakAuthorityPuller).updateGroup("group1", "Updated Group", null); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/test/java/org/cryptomator/hub/api/GroupsResourceIT.java` around lines 241 - 255, The test testUpdateGroupSuccess currently stubs keycloakAuthorityPuller.updateGroup(...) but never verifies it was invoked, allowing a false positive; update the test in GroupsResourceIT to assert delegation by adding a Mockito.verify call after the PUT request that verifies keycloakAuthorityPuller.updateGroup was called once with the expected arguments (e.g., Mockito.verify(keycloakAuthorityPuller, Mockito.times(1)).updateGroup(Mockito.eq("group1"), Mockito.eq("Updated Group"), Mockito.isNull())). Ensure the verify uses the same argument matchers as the stub so the verification reliably catches missing delegation.backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java (2)
233-234: 💤 Low valueAdd defensive null check for Location header.
If Keycloak fails to return a
Locationheader (unexpected but possible),location.substring()will throw NPE. Consider adding a null check with a clear error message.case 201 -> { var location = response.getHeaderString("Location"); + if (location == null) { + throw new InternalServerErrorException("Keycloak did not return Location header for created user"); + } yield location.substring(location.lastIndexOf('/') + 1); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java` around lines 233 - 234, In KeycloakAuthorityPuller where you call response.getHeaderString("Location") and then do location.substring(...), add a defensive null check for the Location header (variable location) before calling substring; if location is null, throw a clear IllegalStateException or log and rethrow with a descriptive message that the Location header was missing from the Keycloak response so callers can diagnose the failure. Ensure the check is placed in the same method in KeycloakAuthorityPuller that currently yields the substring from location.
506-508: 💤 Low valueAdd defensive null check for Location header (same issue as createUser).
case 201 -> { var location = response.getHeaderString("Location"); + if (location == null) { + throw new InternalServerErrorException("Keycloak did not return Location header for created group"); + } yield location.substring(location.lastIndexOf('/') + 1); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java` around lines 506 - 508, The case handling the 201 response in KeycloakAuthorityPuller reads the Location header without null checks; update the response.getHeaderString("Location") usage to first verify the header is non-null (as done in createUser), and handle the null case by logging/throwing a clear exception or returning an appropriate error instead of calling substring on a null; ensure the code references the same extraction logic (variable name location) and returns or yields the new id only after the null check passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java`:
- Around line 530-532: The code risks a NullPointerException because name may be
null before calling name.isBlank(); update the condition in
KeycloakAuthorityPuller so it first checks for null (e.g., name != null &&
!name.isBlank()) before calling isBlank(), or use Objects.nonNull(name) &&
!name.isBlank(), and only call group.setName(name) when that combined check
passes to avoid NPEs when setting the group name.
- Around line 282-284: The call to effectiveGroupMembershipRepo.updateGroups in
KeycloakAuthorityPuller uses groupIds unguarded and can receive null; modify the
code so updateGroups is only invoked with a non-null/non-empty set—either
replace groupIds with the non-null joinedGroupIds (which is guaranteed non-null)
or add a null/empty guard such as if (groupIds != null && !groupIds.isEmpty())
before calling effectiveGroupMembershipRepo.updateGroups(groupIds) to avoid NPE
while preserving the intended test behavior.
---
Nitpick comments:
In
`@backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java`:
- Around line 233-234: In KeycloakAuthorityPuller where you call
response.getHeaderString("Location") and then do location.substring(...), add a
defensive null check for the Location header (variable location) before calling
substring; if location is null, throw a clear IllegalStateException or log and
rethrow with a descriptive message that the Location header was missing from the
Keycloak response so callers can diagnose the failure. Ensure the check is
placed in the same method in KeycloakAuthorityPuller that currently yields the
substring from location.
- Around line 506-508: The case handling the 201 response in
KeycloakAuthorityPuller reads the Location header without null checks; update
the response.getHeaderString("Location") usage to first verify the header is
non-null (as done in createUser), and handle the null case by logging/throwing a
clear exception or returning an appropriate error instead of calling substring
on a null; ensure the code references the same extraction logic (variable name
location) and returns or yields the new id only after the null check passes.
In `@backend/src/test/java/org/cryptomator/hub/api/GroupsResourceIT.java`:
- Around line 241-255: The test testUpdateGroupSuccess currently stubs
keycloakAuthorityPuller.updateGroup(...) but never verifies it was invoked,
allowing a false positive; update the test in GroupsResourceIT to assert
delegation by adding a Mockito.verify call after the PUT request that verifies
keycloakAuthorityPuller.updateGroup was called once with the expected arguments
(e.g., Mockito.verify(keycloakAuthorityPuller,
Mockito.times(1)).updateGroup(Mockito.eq("group1"), Mockito.eq("Updated Group"),
Mockito.isNull())). Ensure the verify uses the same argument matchers as the
stub so the verification reliably catches missing delegation.
In `@backend/src/test/java/org/cryptomator/hub/api/UsersResourceIT.java`:
- Around line 411-441: Add a new integration test in UsersResourceIT (alongside
testCreateUserSuccess) that posts a user payload with a non-empty "realmRoles"
array, mock keycloakAuthorityPuller.createUser to return a UserRepresentation
(as in testCreateUserSuccess), and then verify that
keycloakAuthorityPuller.updateUserRoles(...) is invoked with the created user's
id and the expected role list; the test should assert the endpoint still returns
201 and include verification of the updateUserRoles call to cover the newly
added branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47ca199f-b4c9-4b26-bd8c-21d1a58b2051
📒 Files selected for processing (8)
backend/src/main/java/org/cryptomator/hub/api/GroupsResource.javabackend/src/main/java/org/cryptomator/hub/api/UsersResource.javabackend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAdminService.javabackend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityProvider.javabackend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.javabackend/src/test/java/org/cryptomator/hub/api/GroupsResourceIT.javabackend/src/test/java/org/cryptomator/hub/api/UsersResourceIT.javabackend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.java
💤 Files with no reviewable changes (1)
- backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAdminService.java
SailReal
requested changes
Jun 10, 2026
Member
There was a problem hiding this comment.
Will do a closer look into the changes but here is an early feedback. I really like that we dropped the AdminService. But we should also address the following: Every .toRepresentation() is a call against keycloak. Sometimes we call .toRepresentation() way to often, e.g. before syncUser(), in syncUser() and after syncUser(), why those multiple calls against keycloak for the same resource?
(I know this was not added in the PR but would be awesome if we could address it in this refactoring).
SailReal
requested changes
Jun 10, 2026
SailReal
left a comment
SailReal
left a comment
Member
There was a problem hiding this comment.
It is better now, but we need also show the error to the user, see e.g.
2026-06-10 14:54:39,354 WARN [org.cry.hub.key.KeycloakAuthorityPuller] (executor-thread-9) Failed to check federated identity for user bob.: org.jboss.resteasy.reactive.ClientWebApplicationException: Received: 'HTTP 403 Forbidden' when invoking REST Client method: 'org.keycloak.admin.client.resource.UserResource#getFederatedIdentity'
at org.jboss.resteasy.reactive.client.impl.RestClientRequestContext.unwrapException(RestClientRequestContext.java:220)
at org.jboss.resteasy.reactive.common.core.AbstractResteasyReactiveContext.handleException(AbstractResteasyReactiveContext.java:331)
at org.jboss.resteasy.reactive.common.core.AbstractResteasyReactiveContext.run(AbstractResteasyReactiveContext.java:175)
at io.smallrye.context.impl.wrappers.SlowContextualRunnable.run(SlowContextualRunnable.java:19)
at org.jboss.resteasy.reactive.client.handlers.ClientSwitchToRequestContextRestHandler$1$1.handle(ClientSwitchToRequestContextRestHandler.java:38)
at org.jboss.resteasy.reactive.client.handlers.ClientSwitchToRequestContextRestHandler$1$1.handle(ClientSwitchToRequestContextRestHandler.java:35)
at io.vertx.core.impl.ContextInternal.dispatch(ContextInternal.java:270)
at io.vertx.core.impl.ContextInternal.dispatch(ContextInternal.java:252)
at io.vertx.core.impl.ContextInternal.lambda$runOnContext$0(ContextInternal.java:50)
at io.netty.util.concurrent.AbstractEventExecutor.runTask(AbstractEventExecutor.java:173)
at io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:166)
at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:472)
at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:569)
at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:998)
at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
at java.base/java.lang.Thread.run(Thread.java:1516)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Folds the write-side CRUD bean (
KeycloakAdminService) into the scheduledKeycloakAuthorityPuller, so a single@ApplicationScopedbean owns all Keycloak interaction (same shape asLicenseHolder).UsersResourceandGroupsResourcenow inject the puller directly.The duplicated single-entity sync (the old
syncUser/syncGroup// TODO deduplicateblocks) is unified with the batch helpers via sharedapplyUser/applyGroup. Realm-role handling is preserved: single-entitysyncUserleaves stored roles untouched, only the batch path andupdateUserRoleswrite them.Adds failure-only logging (JBoss
Logger, matching the rest of the codebase) across the merged write methods, a top-level catch around the scheduledsync()so a failed cycle is logged and the scheduler keeps running, and the provider's paginated reads.Two pre-existing bugs in the moved code are fixed along the way:
createUsernow writes the DBgroup_membershiprows for successfully-joined groups. Previously it only joined the user in Keycloak, so a new user's Hub-side group and vault access lagged until the next scheduled sync.syncGroupnow paginates its member read instead of relying on a single capped page, so large groups no longer truncate stored membership.Unit coverage added for the moved write methods and both fixes; the resource ITs re-point their
@InjectMockto the puller.