Disabled users should not require a license seat#428
Merged
tobihagemann merged 7 commits intodevelopfrom Mar 26, 2026
Merged
Conversation
SailReal
reviewed
Mar 24, 2026
backend/src/main/java/org/cryptomator/hub/keycloak/KeycloakAuthorityProvider.java
Outdated
Show resolved
Hide resolved
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a persisted boolean Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (6)
backend/src/test/java/org/cryptomator/hub/api/UsersResourceIT.java (1)
606-621: Consider adding the symmetric enable-path test as well.Line 607 currently validates only
"enabled": false; a matching"enabled": truecase would better protect request-to-service mapping regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/java/org/cryptomator/hub/api/UsersResourceIT.java` around lines 606 - 621, Add a symmetric test for the enable-path to mirror the existing disable test: create a new test (e.g., testSetUserEnabledEnableSuccess) in UsersResourceIT that stubs keycloakAdminService.setUserEnabled("user1", true) with Mockito.doNothing(), sends a PUT to "/users/user1/enabled" with a JSON body {"enabled": true}, asserts a 204 response, and verifies keycloakAdminService.setUserEnabled("user1", true) was invoked; this ensures the request-to-service mapping is exercised for the true case as well.frontend/src/common/backend.ts (1)
529-531: Consider adding error handling for expected failure cases.The
setUserEnabledmethod doesn't handle expected error responses (e.g., 404 if user not found, 403 if not authorized). Other methods inUserServiceuserethrowAndConvertIfExpectedto convert HTTP errors to typedBackendErrorsubclasses.Proposed fix
public async setUserEnabled(userId: string, enabled: boolean): Promise<void> { - await axiosAuth.put(`/users/${userId}/enabled`, { enabled }); + await axiosAuth.put(`/users/${userId}/enabled`, { enabled }) + .catch((error) => rethrowAndConvertIfExpected(error, 403, 404)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/common/backend.ts` around lines 529 - 531, The setUserEnabled method currently calls axiosAuth.put(`/users/${userId}/enabled`, { enabled }) without converting HTTP errors into typed BackendError instances; wrap the request in the same error-handling pattern used elsewhere by calling rethrowAndConvertIfExpected around the axiosAuth.put call (preserve the signature of public async setUserEnabled(userId: string, enabled: boolean): Promise<void>) so that 404/403 and other expected responses are converted to the appropriate BackendError subclasses instead of leaking raw HTTP errors.backend/src/main/java/org/cryptomator/hub/api/AuthorityResource.java (1)
34-36: Filterenabledstatus at the query level instead of in memory.The search method filters disabled users in memory after querying, which is inefficient for large result sets. Like other User-related queries in the codebase (e.g.,
User.requiringAccessGrant,User.getEffectiveGroupUsers), the filtering should be applied at the query level inAuthority.byName:SELECT DISTINCT a FROM Authority a WHERE LOWER(a.name) LIKE :name AND (NOT (a.type = 'USER') OR a.enabled = true)Alternatively, create a new named query dedicated to searching authorities with disabled users excluded.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/cryptomator/hub/api/AuthorityResource.java` around lines 34 - 36, The search method currently filters disabled User instances in memory; update the query layer instead by changing authorityRepo.byName (or adding a new repository method, e.g., byNameExcludingDisabledUsers) so the JPQL/named query excludes disabled users (logic: WHERE LOWER(a.name) LIKE :name AND (a.type <> 'USER' OR a.enabled = true)); then have AuthorityResource.search call the updated repository method (AuthorityResource.search -> authorityRepo.byName... or authorityRepo.byNameExcludingDisabledUsers) and keep mapping to AuthorityDto.fromEntity(authority, withMemberSize) unchanged.backend/src/main/java/org/cryptomator/hub/api/UsersResource.java (1)
474-477: Consider usingBooleanwrapper type for explicit null handling.The
booleanprimitive inSetUserEnabledDtowill default tofalseif theenabledfield is omitted from the JSON payload. This could lead to accidentally disabling users if clients send malformed requests. Using@NotNull Boolean enabledwould reject requests missing the field.♻️ Proposed fix
public record SetUserEnabledDto( - `@JsonProperty`("enabled") boolean enabled + `@JsonProperty`("enabled") `@NotNull` Boolean enabled ) { }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/cryptomator/hub/api/UsersResource.java` around lines 474 - 477, The DTO SetUserEnabledDto uses a primitive boolean which defaults to false when the JSON key is missing; change the field to the wrapper type and enforce not-null by replacing "boolean enabled" with "Boolean enabled" and annotate it with `@NotNull` (and keep `@JsonProperty`("enabled")) so missing fields are rejected by validation; update imports for javax/ jakarta.validation.@NotNull as appropriate and ensure any controller method handling this DTO has `@Valid` on the parameter so validation is triggered.frontend/src/components/authority/UserDetail.vue (1)
125-132: Consider surfacing errors to the user.The
enableUserfunction catches errors and logs them to the console, but the user receives no feedback when enabling fails. Consider showing an error notification or inline message similar to the disable dialog's error display.💡 Proposed improvement
+const enableUserError = ref<Error | null>(null); + const enableUser = async () => { + enableUserError.value = null; try { await backend.users.setUserEnabled(props.id, true); user.value = await backend.users.getUser(props.id); } catch (error) { console.error('Enabling user failed.', error); + enableUserError.value = error instanceof Error ? error : new Error('Unknown Error'); + // Display error via toast or inline message } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/authority/UserDetail.vue` around lines 125 - 132, The enableUser function swallows errors to console only; update it to surface failures to the user by catching the error thrown by backend.users.setUserEnabled(props.id, true) and then invoking the same UI error path used by the disable flow (e.g., call the notification/toast helper or set the component's error state used by the disable dialog) so the user sees an inline message or toast; keep the existing user.value refresh logic (user.value = await backend.users.getUser(props.id)) in the success path and ensure the error branch uses the shared error display (or emits an error event) with the caught error for context.frontend/src/components/authority/UserDisableDialog.vue (1)
27-33: Consider a fallback for missingpictureUrl.The
pictureUrlis defined as optional in theUserinterface, but it's used directly in theimgtag. IfpictureUrlis undefined, this could result in a broken image. Based on learnings,AuthorityService.fillInMissingPicture()should populate this at the service layer, but a defensive fallback would ensure robustness.💡 Suggested fallback
- <img :src="user.pictureUrl" class="w-8 h-8 rounded-full border" /> + <img v-if="user.pictureUrl" :src="user.pictureUrl" class="w-8 h-8 rounded-full border" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/authority/UserDisableDialog.vue` around lines 27 - 33, The img uses user.pictureUrl directly which is optional on User; update the template in UserDisableDialog.vue to defensively use a fallback when user.pictureUrl is missing (e.g., render a default avatar URL or a scoped placeholder component) or conditionally render a styled initials/avatar element; ensure this complements AuthorityService.fillInMissingPicture by referencing the same user.pictureUrl property and keep the img src binding to a computed/derived value (e.g., a local computed like displayPictureUrl) so the fallback logic is centralized and easy to test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/src/main/java/org/cryptomator/hub/api/AuthorityResource.java`:
- Around line 34-36: The search method currently filters disabled User instances
in memory; update the query layer instead by changing authorityRepo.byName (or
adding a new repository method, e.g., byNameExcludingDisabledUsers) so the
JPQL/named query excludes disabled users (logic: WHERE LOWER(a.name) LIKE :name
AND (a.type <> 'USER' OR a.enabled = true)); then have AuthorityResource.search
call the updated repository method (AuthorityResource.search ->
authorityRepo.byName... or authorityRepo.byNameExcludingDisabledUsers) and keep
mapping to AuthorityDto.fromEntity(authority, withMemberSize) unchanged.
In `@backend/src/main/java/org/cryptomator/hub/api/UsersResource.java`:
- Around line 474-477: The DTO SetUserEnabledDto uses a primitive boolean which
defaults to false when the JSON key is missing; change the field to the wrapper
type and enforce not-null by replacing "boolean enabled" with "Boolean enabled"
and annotate it with `@NotNull` (and keep `@JsonProperty`("enabled")) so missing
fields are rejected by validation; update imports for javax/
jakarta.validation.@NotNull as appropriate and ensure any controller method
handling this DTO has `@Valid` on the parameter so validation is triggered.
In `@backend/src/test/java/org/cryptomator/hub/api/UsersResourceIT.java`:
- Around line 606-621: Add a symmetric test for the enable-path to mirror the
existing disable test: create a new test (e.g., testSetUserEnabledEnableSuccess)
in UsersResourceIT that stubs keycloakAdminService.setUserEnabled("user1", true)
with Mockito.doNothing(), sends a PUT to "/users/user1/enabled" with a JSON body
{"enabled": true}, asserts a 204 response, and verifies
keycloakAdminService.setUserEnabled("user1", true) was invoked; this ensures the
request-to-service mapping is exercised for the true case as well.
In `@frontend/src/common/backend.ts`:
- Around line 529-531: The setUserEnabled method currently calls
axiosAuth.put(`/users/${userId}/enabled`, { enabled }) without converting HTTP
errors into typed BackendError instances; wrap the request in the same
error-handling pattern used elsewhere by calling rethrowAndConvertIfExpected
around the axiosAuth.put call (preserve the signature of public async
setUserEnabled(userId: string, enabled: boolean): Promise<void>) so that 404/403
and other expected responses are converted to the appropriate BackendError
subclasses instead of leaking raw HTTP errors.
In `@frontend/src/components/authority/UserDetail.vue`:
- Around line 125-132: The enableUser function swallows errors to console only;
update it to surface failures to the user by catching the error thrown by
backend.users.setUserEnabled(props.id, true) and then invoking the same UI error
path used by the disable flow (e.g., call the notification/toast helper or set
the component's error state used by the disable dialog) so the user sees an
inline message or toast; keep the existing user.value refresh logic (user.value
= await backend.users.getUser(props.id)) in the success path and ensure the
error branch uses the shared error display (or emits an error event) with the
caught error for context.
In `@frontend/src/components/authority/UserDisableDialog.vue`:
- Around line 27-33: The img uses user.pictureUrl directly which is optional on
User; update the template in UserDisableDialog.vue to defensively use a fallback
when user.pictureUrl is missing (e.g., render a default avatar URL or a scoped
placeholder component) or conditionally render a styled initials/avatar element;
ensure this complements AuthorityService.fillInMissingPicture by referencing the
same user.pictureUrl property and keep the img src binding to a computed/derived
value (e.g., a local computed like displayPictureUrl) so the fallback logic is
centralized and easy to test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4cf40320-9ed2-45a1-ba73-9483a00a8446
📒 Files selected for processing (21)
backend/src/main/java/org/cryptomator/hub/api/AuthorityResource.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/entities/EffectiveVaultAccess.javabackend/src/main/java/org/cryptomator/hub/entities/User.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/KeycloakUserDto.javabackend/src/main/resources/org/cryptomator/hub/flyway/V25__User_Enabled.sqlbackend/src/test/java/org/cryptomator/hub/api/UsersResourceIT.javabackend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityProviderTest.javabackend/src/test/java/org/cryptomator/hub/keycloak/KeycloakAuthorityPullerTest.javafrontend/src/common/backend.tsfrontend/src/components/authority/UserDetail.vuefrontend/src/components/authority/UserDisableDialog.vuefrontend/src/components/authority/UserInfo.vuefrontend/src/components/authority/UserList.vuefrontend/src/i18n/en-US.jsonfrontend/test/common/emergencyaccess.spec.ts
SailReal
requested changes
Mar 25, 2026
Member
SailReal
left a comment
SailReal
left a comment
There was a problem hiding this comment.
Beside the requested changes, it looks good to me 👍
backend/src/main/java/org/cryptomator/hub/api/UsersResource.java
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/common/backend.ts (1)
529-531: Align error handling with the rest ofUserService.This new admin mutation should convert expected backend statuses (at least
403/404) to typedBackendErrors for consistent caller behavior.♻️ Suggested adjustment
public async setUserEnabled(userId: string, enabled: boolean): Promise<void> { - await axiosAuth.put(`/users/${userId}/enabled`, enabled, { headers: { 'Content-Type': 'text/plain' } }); + await axiosAuth.put(`/users/${userId}/enabled`, enabled, { headers: { 'Content-Type': 'text/plain' } }) + .catch((error) => rethrowAndConvertIfExpected(error, 403, 404)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/common/backend.ts` around lines 529 - 531, The setUserEnabled method should convert expected backend statuses into typed BackendError like other UserService methods: wrap the axiosAuth.put call in try/catch inside setUserEnabled, detect axios error.response.status for 403 and 404 and throw a BackendError instance with the appropriate status/message (matching how other methods create BackendError), otherwise rethrow the original error; reference setUserEnabled and BackendError to locate where to add the catch and mapping logic so caller behavior is consistent with other UserService methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/common/backend.ts`:
- Around line 529-531: The setUserEnabled method should convert expected backend
statuses into typed BackendError like other UserService methods: wrap the
axiosAuth.put call in try/catch inside setUserEnabled, detect axios
error.response.status for 403 and 404 and throw a BackendError instance with the
appropriate status/message (matching how other methods create BackendError),
otherwise rethrow the original error; reference setUserEnabled and BackendError
to locate where to add the catch and mapping logic so caller behavior is
consistent with other UserService methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e645a09c-dc55-4674-948f-2987cba58d84
📒 Files selected for processing (3)
backend/src/main/java/org/cryptomator/hub/api/UsersResource.javabackend/src/test/java/org/cryptomator/hub/api/UsersResourceIT.javafrontend/src/common/backend.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/test/java/org/cryptomator/hub/api/UsersResourceIT.java
- backend/src/main/java/org/cryptomator/hub/api/UsersResource.java
SailReal
approved these changes
Mar 26, 2026
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.
Fixes #427
Syncs the
enabledproperty from Keycloak'sUserRepresentationinto Hub's user entity. Disabled users are excluded from license seat calculations and hidden from non-admin views (authority search, vault member lists, access grant lists, group member queries). Admins can still see disabled users in the user management page, where they're marked with a "Disabled" badge. Admins can also enable or disable users directly from the user detail page.Changes:
enabledcolumn touser_detailsenabledflag on every sync cycleEffectiveVaultAccessfilter onu.enabledUserDtoexposes theenabledfield to the frontendPUT /users/{id}/enabledadmin endpoint to toggle a user's enabled state in Keycloak