Use Constructor Injection + add JSpecify#470
Conversation
# Conflicts: # backend/src/main/java/org/cryptomator/hub/api/GroupsResource.java # backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAdminService.java
# Conflicts: # backend/src/main/java/org/cryptomator/hub/api/GroupsResource.java # backend/src/main/java/org/cryptomator/hub/api/UsersResource.java # backend/src/main/java/org/cryptomator/hub/api/VaultResource.java # backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAdminService.java # backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java # backend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAdminServiceTest.java
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughThe pull request upgrades the backend Java toolchain from 21 to 25 across CI workflows, Maven build configuration, Dockerfiles, and IntelliJ IDE settings, and updates the Quarkus platform BOM from Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/api/UserDto.java`:
- Line 11: The justPublicInfo() factory method in UserDto passes user.getEmail()
directly to the UserDto constructor, but User.getEmail() returns a nullable
value while the UserDto constructor parameter and email field expect non-null
values. Fix this contract violation by modifying the justPublicInfo() method to
handle the null case from user.getEmail() - either provide a sensible non-null
default value (such as an empty string or a placeholder) when the email is null,
or alternatively, mark the email field in UserDto as `@Nullable` and update the
constructor parameter and getter at UserDto to use `@Nullable` annotation, then
update all call sites including AuthorityDto.fromEntity() and the methods in
UsersResource to handle the nullable email appropriately.
🪄 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: 86a5bf27-c90e-4e38-970c-4cac92225373
📒 Files selected for processing (91)
.github/workflows/build.yml.github/workflows/dependabot-frontend-build.ymlCHANGELOG.mdbackend/.idea/misc.xmlbackend/pom.xmlbackend/src/main/docker/Dockerfile.jvmbackend/src/main/docker/Dockerfile.nativebackend/src/main/java/org/cryptomator/hub/Main.javabackend/src/main/java/org/cryptomator/hub/api/AuditLogResource.javabackend/src/main/java/org/cryptomator/hub/api/AuthorityDto.javabackend/src/main/java/org/cryptomator/hub/api/AuthorityResource.javabackend/src/main/java/org/cryptomator/hub/api/BillingResource.javabackend/src/main/java/org/cryptomator/hub/api/ConfigResource.javabackend/src/main/java/org/cryptomator/hub/api/DeviceResource.javabackend/src/main/java/org/cryptomator/hub/api/EmergencyAccessResource.javabackend/src/main/java/org/cryptomator/hub/api/GroupDto.javabackend/src/main/java/org/cryptomator/hub/api/GroupsResource.javabackend/src/main/java/org/cryptomator/hub/api/LicenseResource.javabackend/src/main/java/org/cryptomator/hub/api/MemberDto.javabackend/src/main/java/org/cryptomator/hub/api/SettingsResource.javabackend/src/main/java/org/cryptomator/hub/api/UserDto.javabackend/src/main/java/org/cryptomator/hub/api/UsersResource.javabackend/src/main/java/org/cryptomator/hub/api/VaultResource.javabackend/src/main/java/org/cryptomator/hub/api/VersionResource.javabackend/src/main/java/org/cryptomator/hub/api/package-info.javabackend/src/main/java/org/cryptomator/hub/entities/AccessToken.javabackend/src/main/java/org/cryptomator/hub/entities/Authority.javabackend/src/main/java/org/cryptomator/hub/entities/Batch.javabackend/src/main/java/org/cryptomator/hub/entities/Device.javabackend/src/main/java/org/cryptomator/hub/entities/EffectiveGroupMembership.javabackend/src/main/java/org/cryptomator/hub/entities/EffectiveVaultAccess.javabackend/src/main/java/org/cryptomator/hub/entities/EffectiveWot.javabackend/src/main/java/org/cryptomator/hub/entities/EmergencyRecoveryProcess.javabackend/src/main/java/org/cryptomator/hub/entities/Group.javabackend/src/main/java/org/cryptomator/hub/entities/LegacyDevice.javabackend/src/main/java/org/cryptomator/hub/entities/RecoveredEmergencyKeyShares.javabackend/src/main/java/org/cryptomator/hub/entities/Settings.javabackend/src/main/java/org/cryptomator/hub/entities/User.javabackend/src/main/java/org/cryptomator/hub/entities/Vault.javabackend/src/main/java/org/cryptomator/hub/entities/VaultAccess.javabackend/src/main/java/org/cryptomator/hub/entities/events/AuditEvent.javabackend/src/main/java/org/cryptomator/hub/entities/events/DeviceRegisteredEvent.javabackend/src/main/java/org/cryptomator/hub/entities/events/DeviceRemovedEvent.javabackend/src/main/java/org/cryptomator/hub/entities/events/EmergencyAccessRecoveryAbortedEvent.javabackend/src/main/java/org/cryptomator/hub/entities/events/EmergencyAccessRecoveryCompletedEvent.javabackend/src/main/java/org/cryptomator/hub/entities/events/EventLogger.javabackend/src/main/java/org/cryptomator/hub/entities/events/SettingWotUpdateEvent.javabackend/src/main/java/org/cryptomator/hub/entities/events/SignedWotIdEvent.javabackend/src/main/java/org/cryptomator/hub/entities/events/UserAccountResetEvent.javabackend/src/main/java/org/cryptomator/hub/entities/events/UserKeysChangeEvent.javabackend/src/main/java/org/cryptomator/hub/entities/events/UserSetupCodeChangeEvent.javabackend/src/main/java/org/cryptomator/hub/entities/events/VaultAccessGrantedEvent.javabackend/src/main/java/org/cryptomator/hub/entities/events/VaultCreatedEvent.javabackend/src/main/java/org/cryptomator/hub/entities/events/VaultKeyRetrievedEvent.javabackend/src/main/java/org/cryptomator/hub/entities/events/VaultMemberAddedEvent.javabackend/src/main/java/org/cryptomator/hub/entities/events/VaultMemberRemovedEvent.javabackend/src/main/java/org/cryptomator/hub/entities/events/VaultMemberUpdatedEvent.javabackend/src/main/java/org/cryptomator/hub/entities/events/VaultOwnershipClaimedEvent.javabackend/src/main/java/org/cryptomator/hub/entities/events/VaultUpdatedEvent.javabackend/src/main/java/org/cryptomator/hub/entities/events/package-info.javabackend/src/main/java/org/cryptomator/hub/entities/package-info.javabackend/src/main/java/org/cryptomator/hub/filters/ActiveLicenseFilter.javabackend/src/main/java/org/cryptomator/hub/filters/FrontendRootPathFilter.javabackend/src/main/java/org/cryptomator/hub/filters/VaultRoleFilter.javabackend/src/main/java/org/cryptomator/hub/filters/package-info.javabackend/src/main/java/org/cryptomator/hub/flyway/package-info.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/main/java/org/cryptomator/hub/keycloak/KeycloakGroupDto.javabackend/src/main/java/org/cryptomator/hub/keycloak/KeycloakRealmRoles.javabackend/src/main/java/org/cryptomator/hub/keycloak/KeycloakUserDto.javabackend/src/main/java/org/cryptomator/hub/keycloak/package-info.javabackend/src/main/java/org/cryptomator/hub/license/HubLicenseEntitlements.javabackend/src/main/java/org/cryptomator/hub/license/LicenseHolder.javabackend/src/main/java/org/cryptomator/hub/license/LicenseValidator.javabackend/src/main/java/org/cryptomator/hub/license/LicenseVerifierProducer.javabackend/src/main/java/org/cryptomator/hub/license/package-info.javabackend/src/main/java/org/cryptomator/hub/metrics/package-info.javabackend/src/main/java/org/cryptomator/hub/package-info.javabackend/src/main/java/org/cryptomator/hub/util/package-info.javabackend/src/main/java/org/cryptomator/hub/validation/package-info.javabackend/src/test/java/org/cryptomator/hub/api/ConfigResourceTest.javabackend/src/test/java/org/cryptomator/hub/filters/ActiveLicenseFilterTest.javabackend/src/test/java/org/cryptomator/hub/filters/VaultRoleFilterTest.javabackend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAdminServiceTest.javabackend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityProviderTest.javabackend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.javabackend/src/test/java/org/cryptomator/hub/license/LicenseHolderTest.javabackend/src/test/java/org/cryptomator/hub/license/LicenseValidatorTest.javabackend/src/test/resources/org/cryptomator/hub/flyway/V9999__Test_Data.sql
| import org.cryptomator.hub.entities.User; | ||
| import org.cryptomator.hub.validation.OnlyBase64Chars; | ||
| import org.cryptomator.hub.validation.ValidJWE; | ||
| import org.jspecify.annotations.Nullable; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any null checks or validations before UserDto construction
rg -nC3 'UserDto\.justPublicInfo' --type javaRepository: cryptomator/hub
Length of output: 3573
🏁 Script executed:
#!/bin/bash
# Check User.java for getEmail() nullability and email field declaration
rg -n '`@Nullable`|private.*email|public.*getEmail' backend/src/main/java/org/cryptomator/hub/entities/User.java -A2 -B2Repository: cryptomator/hub
Length of output: 2406
🏁 Script executed:
#!/bin/bash
# Check UserDto.justPublicInfo() implementation
rg -n 'justPublicInfo|email' backend/src/main/java/org/cryptomator/hub/api/UserDto.java -A5 -B2Repository: cryptomator/hub
Length of output: 2025
🏁 Script executed:
#!/bin/bash
# Check the exact line of User.email field
sed -n '50,70p' backend/src/main/java/org/cryptomator/hub/entities/User.javaRepository: cryptomator/hub
Length of output: 565
🏁 Script executed:
#!/bin/bash
# Get the complete justPublicInfo() method implementation
sed -n '151,175p' backend/src/main/java/org/cryptomator/hub/api/UserDto.javaRepository: cryptomator/hub
Length of output: 589
Fix null-to-nonnull contract violation for email in UserDto.justPublicInfo().
User.getEmail() returns @Nullable String, but UserDto requires non-null email: the field is declared without @Nullable (line 19), the constructor parameter is marked @NotNull (line 35), and the getter returns non-nullable String (line 80). The factory method justPublicInfo() at line 156 passes user.getEmail() directly without null handling, violating the contract.
If Keycloak syncs a user without an email, calling justPublicInfo(user) will pass null to a @NotNull parameter, causing a contract violation. This affects all call sites: AuthorityDto.fromEntity(), and multiple methods in UsersResource.
Either mark UserDto.email as @Nullable and update related code, or provide a non-null default in justPublicInfo().
🤖 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/api/UserDto.java` at line 11, The
justPublicInfo() factory method in UserDto passes user.getEmail() directly to
the UserDto constructor, but User.getEmail() returns a nullable value while the
UserDto constructor parameter and email field expect non-null values. Fix this
contract violation by modifying the justPublicInfo() method to handle the null
case from user.getEmail() - either provide a sensible non-null default value
(such as an empty string or a placeholder) when the email is null, or
alternatively, mark the email field in UserDto as `@Nullable` and update the
constructor parameter and getter at UserDto to use `@Nullable` annotation, then
update all call sites including AuthorityDto.fromEntity() and the methods in
UsersResource to handle the nullable email appropriately.
[ci skip]
# Conflicts: # backend/.idea/misc.xml
[build iamge]
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/src/main/java/org/cryptomator/hub/api/GroupsResource.java (1)
112-114: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd context to the
IllegalStateExceptionfor DB/Keycloak sync drift.Throwing a bare
IllegalStateExceptionhere makes production diagnosis harder when cross-system sync fails. Include the created group id (and optionally name) in the message.Proposed patch
- if (group == null) { // group was created in Keycloak but not found in database after sync - throw new IllegalStateException(); + if (group == null) { // group was created in Keycloak but not found in database after sync + throw new IllegalStateException("Group created in Keycloak but missing in DB after sync. groupId=" + groupRepresentation.getId()); }🤖 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/api/GroupsResource.java` around lines 112 - 114, In the GroupsResource.java file where the IllegalStateException is thrown due to a group not being found in the database after Keycloak creation, replace the bare exception with one that includes a descriptive message containing the group id (and optionally the group name) to provide better context for debugging sync failures between Keycloak and the database. Use String.format or string concatenation to construct an informative error message that includes these identifiers before throwing the exception.backend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.java (1)
571-619: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a
ProcessingExceptioncreateUser join-failure test.Current cases cover
WebApplicationException-style failures, but not client processing failures. A dedicated test would lock the intended tolerance behavior and prevent regressions.🤖 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/keycloak/KeycloakAuthorityPullerTest.java` around lines 571 - 619, Add a new test method in the KeycloakAuthorityPullerTest class to verify that the createUser method tolerates ProcessingException failures when joining groups. The test should follow the same pattern as testCreateUserToleratesGroupJoinFailure and testCreateUserPartialGroupJoinFailure but instead mock userResource.joinGroup to throw a ProcessingException (rather than InternalServerErrorException). The test should verify that the user is still created successfully and the expected repositories are called appropriately when ProcessingException occurs during group join operations.
🤖 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 310-329: The deleteUser method currently deletes from the local
database first and then attempts to delete from Keycloak, which can leave
systems out of sync if the Keycloak deletion fails. Since Keycloak is the source
of truth, reorder the operations in the deleteUser method to delete from
Keycloak first (the realm.users().delete(userId) call), and only delete from the
database (userRepo.deleteById(userId)) after the Keycloak deletion succeeds.
This ensures that if the Keycloak operation fails, the local database remains
intact and the systems stay consistent.
- Around line 260-263: Replace the `groupIds` parameter in the
`effectiveGroupMembershipRepo.updateGroups()` call with `joinedGroupIds` to
ensure only groups that were actually successfully joined are used for
membership invalidation. This avoids passing a potentially null value and
ensures that membership updates only reflect groups that were truly written to
the system, preventing inconsistencies if the Keycloak user was already created
before this update step fails.
- Around line 241-247: In the createUser method's group join exception handling
block where userResource.joinGroup(groupId) is called, expand the catch clause
to catch both WebApplicationException and ProcessingException (not just
WebApplicationException). This ensures that if ProcessingException is thrown
during the group join attempt, it will be caught and logged instead of
propagating up and preventing the subsequent syncUser() and database group
membership sync operations from executing.
---
Nitpick comments:
In `@backend/src/main/java/org/cryptomator/hub/api/GroupsResource.java`:
- Around line 112-114: In the GroupsResource.java file where the
IllegalStateException is thrown due to a group not being found in the database
after Keycloak creation, replace the bare exception with one that includes a
descriptive message containing the group id (and optionally the group name) to
provide better context for debugging sync failures between Keycloak and the
database. Use String.format or string concatenation to construct an informative
error message that includes these identifiers before throwing the exception.
In
`@backend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.java`:
- Around line 571-619: Add a new test method in the KeycloakAuthorityPullerTest
class to verify that the createUser method tolerates ProcessingException
failures when joining groups. The test should follow the same pattern as
testCreateUserToleratesGroupJoinFailure and
testCreateUserPartialGroupJoinFailure but instead mock userResource.joinGroup to
throw a ProcessingException (rather than InternalServerErrorException). The test
should verify that the user is still created successfully and the expected
repositories are called appropriately when ProcessingException occurs during
group join operations.
🪄 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: 3540d366-1fe7-4697-8336-4a2c35222dba
📒 Files selected for processing (10)
backend/src/main/java/org/cryptomator/hub/api/DeviceResource.javabackend/src/main/java/org/cryptomator/hub/api/GroupsResource.javabackend/src/main/java/org/cryptomator/hub/api/UsersResource.javabackend/src/main/java/org/cryptomator/hub/api/VaultResource.javabackend/src/main/java/org/cryptomator/hub/entities/Device.javabackend/src/main/java/org/cryptomator/hub/entities/events/EventLogger.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/keycloak/KeycloakAuthorityPullerTest.javabackend/src/test/resources/org/cryptomator/hub/flyway/V9999__Test_Data.sql
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityProvider.java
- backend/src/main/java/org/cryptomator/hub/entities/events/EventLogger.java
- backend/src/main/java/org/cryptomator/hub/api/DeviceResource.java
- backend/src/main/java/org/cryptomator/hub/entities/Device.java
- backend/src/main/java/org/cryptomator/hub/api/VaultResource.java
- backend/src/main/java/org/cryptomator/hub/api/UsersResource.java
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/src/main/java/org/cryptomator/hub/api/GroupsResource.java (1)
112-114: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd context to the
IllegalStateExceptionfor DB/Keycloak sync drift.Throwing a bare
IllegalStateExceptionhere makes production diagnosis harder when cross-system sync fails. Include the created group id (and optionally name) in the message.Proposed patch
- if (group == null) { // group was created in Keycloak but not found in database after sync - throw new IllegalStateException(); + if (group == null) { // group was created in Keycloak but not found in database after sync + throw new IllegalStateException("Group created in Keycloak but missing in DB after sync. groupId=" + groupRepresentation.getId()); }🤖 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/api/GroupsResource.java` around lines 112 - 114, In the GroupsResource.java file where the IllegalStateException is thrown due to a group not being found in the database after Keycloak creation, replace the bare exception with one that includes a descriptive message containing the group id (and optionally the group name) to provide better context for debugging sync failures between Keycloak and the database. Use String.format or string concatenation to construct an informative error message that includes these identifiers before throwing the exception.backend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.java (1)
571-619: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a
ProcessingExceptioncreateUser join-failure test.Current cases cover
WebApplicationException-style failures, but not client processing failures. A dedicated test would lock the intended tolerance behavior and prevent regressions.🤖 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/keycloak/KeycloakAuthorityPullerTest.java` around lines 571 - 619, Add a new test method in the KeycloakAuthorityPullerTest class to verify that the createUser method tolerates ProcessingException failures when joining groups. The test should follow the same pattern as testCreateUserToleratesGroupJoinFailure and testCreateUserPartialGroupJoinFailure but instead mock userResource.joinGroup to throw a ProcessingException (rather than InternalServerErrorException). The test should verify that the user is still created successfully and the expected repositories are called appropriately when ProcessingException occurs during group join operations.
🤖 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 310-329: The deleteUser method currently deletes from the local
database first and then attempts to delete from Keycloak, which can leave
systems out of sync if the Keycloak deletion fails. Since Keycloak is the source
of truth, reorder the operations in the deleteUser method to delete from
Keycloak first (the realm.users().delete(userId) call), and only delete from the
database (userRepo.deleteById(userId)) after the Keycloak deletion succeeds.
This ensures that if the Keycloak operation fails, the local database remains
intact and the systems stay consistent.
- Around line 260-263: Replace the `groupIds` parameter in the
`effectiveGroupMembershipRepo.updateGroups()` call with `joinedGroupIds` to
ensure only groups that were actually successfully joined are used for
membership invalidation. This avoids passing a potentially null value and
ensures that membership updates only reflect groups that were truly written to
the system, preventing inconsistencies if the Keycloak user was already created
before this update step fails.
- Around line 241-247: In the createUser method's group join exception handling
block where userResource.joinGroup(groupId) is called, expand the catch clause
to catch both WebApplicationException and ProcessingException (not just
WebApplicationException). This ensures that if ProcessingException is thrown
during the group join attempt, it will be caught and logged instead of
propagating up and preventing the subsequent syncUser() and database group
membership sync operations from executing.
---
Nitpick comments:
In `@backend/src/main/java/org/cryptomator/hub/api/GroupsResource.java`:
- Around line 112-114: In the GroupsResource.java file where the
IllegalStateException is thrown due to a group not being found in the database
after Keycloak creation, replace the bare exception with one that includes a
descriptive message containing the group id (and optionally the group name) to
provide better context for debugging sync failures between Keycloak and the
database. Use String.format or string concatenation to construct an informative
error message that includes these identifiers before throwing the exception.
In
`@backend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.java`:
- Around line 571-619: Add a new test method in the KeycloakAuthorityPullerTest
class to verify that the createUser method tolerates ProcessingException
failures when joining groups. The test should follow the same pattern as
testCreateUserToleratesGroupJoinFailure and
testCreateUserPartialGroupJoinFailure but instead mock userResource.joinGroup to
throw a ProcessingException (rather than InternalServerErrorException). The test
should verify that the user is still created successfully and the expected
repositories are called appropriately when ProcessingException occurs during
group join operations.
🪄 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: 3540d366-1fe7-4697-8336-4a2c35222dba
📒 Files selected for processing (10)
backend/src/main/java/org/cryptomator/hub/api/DeviceResource.javabackend/src/main/java/org/cryptomator/hub/api/GroupsResource.javabackend/src/main/java/org/cryptomator/hub/api/UsersResource.javabackend/src/main/java/org/cryptomator/hub/api/VaultResource.javabackend/src/main/java/org/cryptomator/hub/entities/Device.javabackend/src/main/java/org/cryptomator/hub/entities/events/EventLogger.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/keycloak/KeycloakAuthorityPullerTest.javabackend/src/test/resources/org/cryptomator/hub/flyway/V9999__Test_Data.sql
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityProvider.java
- backend/src/main/java/org/cryptomator/hub/entities/events/EventLogger.java
- backend/src/main/java/org/cryptomator/hub/api/DeviceResource.java
- backend/src/main/java/org/cryptomator/hub/entities/Device.java
- backend/src/main/java/org/cryptomator/hub/api/VaultResource.java
- backend/src/main/java/org/cryptomator/hub/api/UsersResource.java
🛑 Comments failed to post (3)
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java (3)
241-247:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
find . -type f -name "KeycloakAuthorityPuller.java" | head -5Repository: cryptomator/hub
Length of output: 141
🏁 Script executed:
cat -n ./backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java | sed -n '200,280p'Repository: cryptomator/hub
Length of output: 3490
🏁 Script executed:
head -50 ./backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.javaRepository: cryptomator/hub
Length of output: 1991
🌐 Web query:
Keycloak admin client UserResource joinGroup ProcessingException💡 Result:
When using the Keycloak Admin Client to manage group memberships, encountering issues typically involves either incorrect parameter usage or client-side RESTEASY configuration errors rather than a native library ProcessingException specific to the joinGroup method itself [1][2][3]. If you are calling userResource.joinGroup(groupId), ensure the following: 1. Correct Parameter: The joinGroup method requires the group ID (a UUID string), not the group name [1]. 2. Resource Handling: The joinGroup method is a PUT request to /admin/realms/{realm}/users/{id}/groups/{groupId} [4][2][5]. This endpoint often expects an empty JSON body ({}) if your client or proxy environment strictly enforces request body requirements [2]. 3. Understanding ProcessingException: If you encounter a javax.ws.rs.ProcessingException (such as RESTEASY003215), this is a common JAX-RS/RESTEasy issue where the client cannot locate necessary message body writers [3]. This frequently occurs when the Keycloak Admin Client is initialized or executed within threads (e.g., custom thread pools, ForkJoinPool) where the JAX-RS provider discovery mechanism fails to register standard writers [3]. The common workaround is to ensure the admin client is initialized or called within a context where the thread classloader can access the necessary RESTEasy providers [3]. If you receive a 404 error, verify that both the User ID and Group ID are correct and exist within the target realm [1]. If you encounter silent failures or unexpected behavior, note that Keycloak may ignore the request if the user is already a member of the group, either directly or indirectly (e.g., through a parent group) [6].
Citations:
- 1: https://forum.keycloak.org/t/get-error-404-when-adding-a-user-to-group-or-role-to-user-with-admin-client/20809
- 2: keycloak/keycloak#29844
- 3: https://stackoverflow.com/questions/53091984/keycloak-admin-client-javax-ws-rs-processingexception-resteasy003215
- 4: https://www.keycloak.org/docs-api/latest/javadocs/org/keycloak/admin/client/resource/UserResource.html
- 5: https://github.com/keycloak/keycloak/blob/main/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java
- 6: keycloak/keycloak#9345
🏁 Script executed:
rg "ProcessingException" ./backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPuller.java -A 2 -B 2Repository: cryptomator/hub
Length of output: 3425
Also catch
ProcessingExceptionfor group joins increateUser.The group join loop at line 242 only catches
WebApplicationException, but other methods in this same file catch bothWebApplicationExceptionandProcessingExceptionfrom the samejoinGroup()operation. IfProcessingExceptionescapes this catch block, it prevents the subsequentsyncUser()call (line 253) and the DB group membership sync (lines 256-258), leaving the user in Keycloak but out of sync with the application database.Suggested fix
- } catch (WebApplicationException e) { + } catch (WebApplicationException | ProcessingException e) { LOG.warnv(e, "Failed to add user {0} to group {1}.", userId, groupId); // TODO: shall we fail the whole user creation here? undo previous steps? }🤖 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 241 - 247, In the createUser method's group join exception handling block where userResource.joinGroup(groupId) is called, expand the catch clause to catch both WebApplicationException and ProcessingException (not just WebApplicationException). This ensures that if ProcessingException is thrown during the group join attempt, it will be caught and logged instead of propagating up and preventing the subsequent syncUser() and database group membership sync operations from executing.
260-263:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the successful join set (or empty set) for membership invalidation.
groupIdsis treated as nullable earlier, but this call passes it directly and can fail after the Keycloak user is already created. UsejoinedGroupIdshere to avoid null-risk and to invalidate only memberships that were actually written.Suggested fix
- effectiveGroupMembershipRepo.updateGroups(groupIds); + effectiveGroupMembershipRepo.updateGroups(joinedGroupIds);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// update effective group membership now that the DB contains the membership data // (we can assume that the groups already exist, otherwise the caller wouldn't have been able to provide their IDs): effectiveGroupMembershipRepo.updateGroups(joinedGroupIds);🤖 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 260 - 263, Replace the `groupIds` parameter in the `effectiveGroupMembershipRepo.updateGroups()` call with `joinedGroupIds` to ensure only groups that were actually successfully joined are used for membership invalidation. This avoids passing a potentially null value and ensures that membership updates only reflect groups that were truly written to the system, preventing inconsistencies if the Keycloak user was already created before this update step fails.
310-329:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid DB-first mutation ordering for Keycloak writes.
These methods update local DB first and then call Keycloak, but external REST calls are outside DB transactions. This ordering can still produce cross-system divergence on commit/partial-failure boundaries; prefer Keycloak-first mutation followed by local sync/reconciliation.
Based on learnings: in Keycloak services, Keycloak should be the source of truth and local DB updates should not rely on database rollback semantics for Keycloak REST operations.
Also applies to: 422-457, 563-577
🤖 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 310 - 329, The deleteUser method currently deletes from the local database first and then attempts to delete from Keycloak, which can leave systems out of sync if the Keycloak deletion fails. Since Keycloak is the source of truth, reorder the operations in the deleteUser method to delete from Keycloak first (the realm.users().delete(userId) call), and only delete from the database (userRepo.deleteById(userId)) after the Keycloak deletion succeeds. This ensures that if the Keycloak operation fails, the local database remains intact and the systems stay consistent.Source: Learnings
1 participant
This is a follow-up to #469, updating the codebase by refactoring injected fields and adding nullability annotations.