Check seat count when unarchiving a vault#432
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 16 minutes and 48 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughA new private helper Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/main/java/org/cryptomator/hub/api/VaultResource.java`:
- Around line 529-537: Add the same unarchive seat-check used in the PATCH
handler into createOrUpdate(): extract the logic in VaultResource that checks if
(wasArchived && !archived) and counts occupied seats via
effectiveVaultAccessRepo.countSeatOccupyingUsers(), throwing
PaymentRequiredException when occupiedSeats > license.getEntitlements().seats(),
into a helper (e.g., ensureSeatLimitAfterUnarchive) and invoke it in
createOrUpdate() immediately after vaultRepo.persistAndFlush(vault) for
existing-vault updates so unarchiving via PUT /vaults/{vaultId} enforces the
same seat validation.
🪄 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: 9790dfd3-51f7-442d-b649-aab1238f111d
📒 Files selected for processing (6)
CHANGELOG.mdbackend/src/main/java/org/cryptomator/hub/api/VaultResource.javabackend/src/test/java/org/cryptomator/hub/api/ExceedingLicenseLimitsIT.javafrontend/src/common/backend.tsfrontend/src/components/ReactivateVaultDialog.vuefrontend/src/i18n/en-US.json
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java (1)
562-567:⚠️ Potential issue | 🟠 Major
createOrUpdatestill has an unarchive bypass without seat validation.Line 562 can unarchive an existing vault via
PUT /vaults/{vaultId}, but there is no post-flushwasArchived && !archivedcheck here. This bypasses the new 402 enforcement insetArchived.🔧 Suggested fix
public Response createOrUpdate(`@PathParam`("vaultId") UUID vaultId, `@Valid` `@NotNull` VaultDto vaultDto) { User currentUser = userRepo.findById(jwt.getSubject()); Optional<Vault> existingVault = vaultRepo.findByIdOptional(vaultId); final Vault vault; + final boolean wasArchived; if (existingVault.isPresent()) { // load existing vault: vault = existingVault.get(); + wasArchived = vault.isArchived(); } else { //if license is exceeded block vault creation, independent if the user is already sitting ensureSeatsNotExceeded(effectiveVaultAccessRepo.countSeatOccupyingUsers()); // create new vault: vault = new Vault(); vault.setId(vaultDto.id); vault.setCreationTime(Instant.now().truncatedTo(ChronoUnit.MILLIS)); + wasArchived = false; } // set regardless of whether vault is new or existing: vault.setName(vaultDto.name); vault.setDescription(vaultDto.description); vault.setArchived(existingVault.isEmpty() ? false : vaultDto.archived); var oldEmergencyKeyShares = vault.getEmergencyKeyShares().values(); vault.setRequiredEmergencyKeyShares(vaultDto.requiredEmergencyKeyShares); vault.setEmergencyKeyShares(vaultDto.emergencyKeyShares); vaultRepo.persistAndFlush(vault); // trigger PersistenceException before we continue with + if (existingVault.isPresent() && wasArchived && !vault.isArchived()) { + ensureSeatsNotExceeded(effectiveVaultAccessRepo.countSeatOccupyingUsers()); + }🤖 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/VaultResource.java` around lines 562 - 567, The createOrUpdate flow in VaultResource currently calls vault.setArchived(...) and then vaultRepo.persistAndFlush(vault) without enforcing the post-flush check that prevents unarchiving without seat validation; update the createOrUpdate method to detect an unarchive transition (capture prior wasArchived from existingVault and new archived flag) after persisting and, if wasArchived && !archived, invoke the same enforcement path used elsewhere (e.g., call the existing setArchived validation or the 402/seat-validation logic) to reject or handle the unarchive accordingly before returning; reference createOrUpdate, vault.setArchived(...), vaultRepo.persistAndFlush(vault) and the wasArchived && !archived check when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/src/main/java/org/cryptomator/hub/api/VaultResource.java`:
- Around line 562-567: The createOrUpdate flow in VaultResource currently calls
vault.setArchived(...) and then vaultRepo.persistAndFlush(vault) without
enforcing the post-flush check that prevents unarchiving without seat
validation; update the createOrUpdate method to detect an unarchive transition
(capture prior wasArchived from existingVault and new archived flag) after
persisting and, if wasArchived && !archived, invoke the same enforcement path
used elsewhere (e.g., call the existing setArchived validation or the
402/seat-validation logic) to reject or handle the unarchive accordingly before
returning; reference createOrUpdate, vault.setArchived(...),
vaultRepo.persistAndFlush(vault) and the wasArchived && !archived check when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d280d0c1-1e8c-40a0-ac20-054a61effa5c
📒 Files selected for processing (1)
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java
| // flush first so countSeatOccupyingUsers sees the unarchived vault; @Transactional rolls back on exception | ||
| if (wasArchived && !vaultDto.archived) { | ||
| ensureSeatsNotExceeded(effectiveVaultAccessRepo.countSeatOccupyingUsers()); | ||
| } |
There was a problem hiding this comment.
We have a different query that simulates seat count instead of looking at the status quo. Need to look it up when I am back on my computer. Just fyi that the "change, check, rollback" approach isn't necessary.
There was a problem hiding this comment.
Fixed in 0cf8232. Added a countSeatOccupyingUsersIfVaultUnarchived query that uses (NOT v.archived OR v.id = :vaultId) to simulate the seat count before any mutation.
There was a problem hiding this comment.
This is the check I was talking about:
hub/backend/src/main/java/org/cryptomator/hub/api/VaultResource.java
Lines 235 to 243 in 445fb27
The approach here is "get seat count in other vaults + check overlap with this vault". The difference of both sets is the number of additional seats required.
Always try reusing existing queries, only add new queries (or logic in general), if efficiency gains are significant. Otherwise, favour code reuse (or even deduplication) for improved maintainability. When in doubt, ask a human 😉
There was a problem hiding this comment.
Damn. I can't read minds after all.
There was a problem hiding this comment.
Fixed in 5708a66. Replaced countSeatOccupyingUsersIfVaultUnarchived with the usersSeatedOnOtherVaults pattern and removed the custom query. Also made createOrUpdate ignore the archived flag entirely, so unarchiving is only possible through the dedicated endpoint.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/main/java/org/cryptomator/hub/api/VaultResource.java`:
- Around line 274-276: The code currently enforces seat limits even for archived
vaults by unconditionally adding +1 in the branch using
effectiveVaultAccessRepo.isUserOccupyingSeat and ensureSeatsNotExceeded; change
the conditional to skip the ensureSeatsNotExceeded call for archived vaults by
guarding it with !vault.isArchived() so that
ensureSeatsNotExceeded(effectiveVaultAccessRepo.countSeatOccupyingUsers() + 1)
only runs when the vault is not archived (use vault.isArchived() together with
effectiveVaultAccessRepo.isUserOccupyingSeat to decide).
- Around line 547-548: The preflight license check uses
effectiveVaultAccessRepo.countSeatOccupyingUsers() but omits the creator being
added as OWNER, so a creation can exceed the license; before calling
ensureSeatsNotExceeded, account for the creator's seat by incrementing the
counted seats when the creator is not already a seat-occupying user (or call
ensureSeatsNotExceeded with an extraSeatsNeeded=1); locate the branch that adds
OWNER access for currentUser (the code around where OWNER access is added, e.g.,
the method adding OWNER for currentUser) and change the preflight to include
that extra seat when the creator would become a new seat-occupying user.
🪄 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: 7788046a-a3d5-4e0a-a2ed-1bb5d4535fa2
📒 Files selected for processing (1)
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java
Archived vaults don't contribute to seat counts (HQL queries filter NOT v.archived), so enforcing seat limits on addUser, addGroup, setDirectMembers, and grantAccess for archived vaults can produce false 402 rejections when the license is at capacity.
…gnore archived flag in createOrUpdate
There was a problem hiding this comment.
♻️ Duplicate comments (2)
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java (2)
241-241:⚠️ Potential issue | 🟠 MajorSkip seat checks while the vault is archived.
These branches still return
402on archived vaults once the license is full, which blocks preparing members or tokens before reactivation and moves enforcement away fromsetArchived().💡 Suggested direction
- ensureSeatsNotExceeded(newSeatOccupyingUsers.size()); + if (!vault.isArchived()) { + ensureSeatsNotExceeded(newSeatOccupyingUsers.size()); + } - if (!effectiveVaultAccessRepo.isUserOccupyingSeat(userId)) { + if (!vault.isArchived() && !effectiveVaultAccessRepo.isUserOccupyingSeat(userId)) { ensureSeatsNotExceeded(effectiveVaultAccessRepo.countSeatOccupyingUsers() + 1); } - ensureSeatsNotExceeded(userRepo.countEffectiveGroupUsers(groupId) - effectiveVaultAccessRepo.countSeatOccupyingUsersOfGroup(groupId) + effectiveVaultAccessRepo.countSeatOccupyingUsers()); + if (!vault.isArchived()) { + ensureSeatsNotExceeded(userRepo.countEffectiveGroupUsers(groupId) - effectiveVaultAccessRepo.countSeatOccupyingUsersOfGroup(groupId) + effectiveVaultAccessRepo.countSeatOccupyingUsers()); + } - ensureSeatsNotExceeded(occupiedSeats + usersWithoutSeat); + if (!vault.isArchived()) { + ensureSeatsNotExceeded(occupiedSeats + usersWithoutSeat); + }Also applies to: 274-276, 299-299, 475-475
🤖 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/VaultResource.java` at line 241, Skip calling ensureSeatsNotExceeded when the vault is archived: wrap the ensureSeatsNotExceeded(newSeatOccupyingUsers.size()) call in a guard that checks vault.isArchived() (e.g. if (!vault.isArchived()) ensureSeatsNotExceeded(...)), and apply the same archival guard around the other ensureSeatsNotExceeded invocations referenced (the occurrences around the 274-276, 299, and 475 spots) so seat enforcement is bypassed for archived vaults and remains enforced only for active ones.
559-559:⚠️ Potential issue | 🟠 MajorCount the creator's seat before allowing vault creation.
If the installation is exactly at the seat cap and
currentUseris not already seated, this preflight passes and the OWNER access added later pushes the system one seat over the license.💡 Suggested fix
- ensureSeatsNotExceeded(effectiveVaultAccessRepo.countSeatOccupyingUsers()); + long projectedSeats = effectiveVaultAccessRepo.countSeatOccupyingUsers() + + (effectiveVaultAccessRepo.isUserOccupyingSeat(currentUser.getId()) ? 0 : 1); + ensureSeatsNotExceeded(projectedSeats);🤖 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/VaultResource.java` at line 559, The preflight seat check calls ensureSeatsNotExceeded(effectiveVaultAccessRepo.countSeatOccupyingUsers()) but fails to account for the creator becoming an OWNER; modify the creation preflight to include the creator's seat if they are not already counted: determine whether currentUser is already a seat-occupying user (e.g., via effectiveVaultAccessRepo.isSeatOccupier(currentUser) or equivalent) and if not, add 1 to effectiveVaultAccessRepo.countSeatOccupyingUsers() before calling ensureSeatsNotExceeded so the OWNER added later cannot push the system over the license.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/src/main/java/org/cryptomator/hub/api/VaultResource.java`:
- Line 241: Skip calling ensureSeatsNotExceeded when the vault is archived: wrap
the ensureSeatsNotExceeded(newSeatOccupyingUsers.size()) call in a guard that
checks vault.isArchived() (e.g. if (!vault.isArchived())
ensureSeatsNotExceeded(...)), and apply the same archival guard around the other
ensureSeatsNotExceeded invocations referenced (the occurrences around the
274-276, 299, and 475 spots) so seat enforcement is bypassed for archived vaults
and remains enforced only for active ones.
- Line 559: The preflight seat check calls
ensureSeatsNotExceeded(effectiveVaultAccessRepo.countSeatOccupyingUsers()) but
fails to account for the creator becoming an OWNER; modify the creation
preflight to include the creator's seat if they are not already counted:
determine whether currentUser is already a seat-occupying user (e.g., via
effectiveVaultAccessRepo.isSeatOccupier(currentUser) or equivalent) and if not,
add 1 to effectiveVaultAccessRepo.countSeatOccupyingUsers() before calling
ensureSeatsNotExceeded so the OWNER added later cannot push the system over the
license.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06c34074-f439-4e57-b3e0-41957ec301ca
📒 Files selected for processing (4)
backend/src/main/java/org/cryptomator/hub/api/VaultResource.javabackend/src/test/java/org/cryptomator/hub/api/ExceedingLicenseLimitsIT.javabackend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.javafrontend/src/i18n/en-US.json
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/i18n/en-US.json
- backend/src/test/java/org/cryptomator/hub/api/ExceedingLicenseLimitsIT.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/VaultDetails.vue`:
- Around line 509-520: The post-success call to refreshLicense() in
reactivateVault can throw unhandled rejections; wrap refreshLicense() in its own
try/catch (or include it in the existing try) so any failure after successful
backend.vaults.setArchived and refreshVault is caught and handled: catch the
error, console.error it, and set onReactivateError.value appropriately (similar
to the existing catch) to surface the error to the UI; ensure references to
reactivateVault, onReactivateError, backend.vaults.setArchived, refreshVault,
and refreshLicense are used so the change is made in the correct function.
🪄 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: 6928cfa2-dfe1-4f41-ab47-97180cd2e5e2
📒 Files selected for processing (3)
frontend/src/components/ReactivateVaultDialog.vuefrontend/src/components/VaultDetails.vuefrontend/src/i18n/en-US.json
💤 Files with no reviewable changes (1)
- frontend/src/components/ReactivateVaultDialog.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/i18n/en-US.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/VaultDetails.vue`:
- Around line 225-232: The template introduces a new i18n key
t('vaultDetails.error.reactivateFailed') which may be missing in non-en locales;
either add the key to all locale files or replace the new key with an existing
translated key to avoid raw keys showing. Locate VaultDetails.vue where
t('vaultDetails.error.reactivateFailed') is used (the onReactivateError
conditional alongside PaymentRequiredError) and either (A) add the key
"vaultDetails.error.reactivateFailed" with appropriate translations to each
locale file, or (B) change the call to reuse an existing key (for example an
existing vault/error or common key used for similar messages) so all locales
already cover it.
🪄 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: 5a40b147-155c-496c-8b62-abf57567da77
📒 Files selected for processing (1)
frontend/src/components/VaultDetails.vue
3 participants
When unarchiving a vault, the seat count was not validated against the license limit. Unarchiving restores access for all vault members, which can push the effective seat count beyond the licensed limit.
This adds a seat check on the archive-to-active transition in
setArchived(PUT /vaults/{vaultId}/archived). The check reuses the existingusersSeatedOnOtherVaultspattern fromsetDirectMembersto project the seat count after unarchive. If the limit would be exceeded,PaymentRequiredException(402) is thrown before any mutation.createOrUpdate(PUT /vaults/{vaultId}) no longer handles thearchivedfield at all. Archive state is exclusively managed through the dedicated endpoint.All seat-limit checks across VaultResource now use a shared
ensureSeatsNotExceededhelper.Frontend:
ReactivateVaultDialoghas been removed. The reactivation action is now inlined directly inVaultDetails, with a 402-specific error message shown inline when the seat limit would be exceeded.Closes #431.