Allow admins to archive and unarchive any vault#430
Conversation
WalkthroughRemoved the explicit 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 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 |
SailReal
left a comment
SailReal
left a comment
There was a problem hiding this comment.
Hmmmmm in the current implementation an user with the admin role can now edit each vault. I think especially because of
andhub/backend/src/main/java/org/cryptomator/hub/api/VaultResource.java
Lines 555 to 562 in 0686dcc
![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
🧹 Nitpick comments (1)
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java (1)
504-504: Make the admin bypass explicit in the annotation.Both endpoints currently rely on
@VaultRole.bypassRealmRole()defaulting toRealmRole.ADMIN. Since this is authorization-sensitive, spelling outbypassRealmRole = RealmRole.ADMINhere would make the intent obvious and avoid silently changing behavior if the annotation default is ever adjusted.Also applies to: 518-518
🤖 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 504, The `@VaultRole` usage in VaultResource is relying on the annotation default for bypassRealmRole; update the annotations that currently read `@VaultRole`(value = {VaultAccess.Role.MEMBER, VaultAccess.Role.OWNER}, onMissingVault = VaultRole.OnMissingVault.NOT_FOUND) to explicitly set bypassRealmRole = RealmRole.ADMIN so the admin bypass is explicit; apply the same change to the other `@VaultRole` instance mentioned (the second occurrence near the other method) to ensure both annotations explicitly declare bypassRealmRole = RealmRole.ADMIN.
🤖 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/common/backend.ts`:
- Around line 363-367: setArchived currently returns the raw response body as a
VaultDto (string creationTime) which violates the contract and differs from
get() that converts creationTime to a Date; update setArchived (the public async
setArchived(vaultId: string, archived: boolean) method) to parse/normalize the
returned payload's creationTime into a Date before resolving (same normalization
used by get()), and keep the existing error handling via
rethrowAndConvertIfExpected so callers always receive a VaultDto with
creationTime as a Date.
---
Nitpick comments:
In `@backend/src/main/java/org/cryptomator/hub/api/VaultResource.java`:
- Line 504: The `@VaultRole` usage in VaultResource is relying on the annotation
default for bypassRealmRole; update the annotations that currently read
`@VaultRole`(value = {VaultAccess.Role.MEMBER, VaultAccess.Role.OWNER},
onMissingVault = VaultRole.OnMissingVault.NOT_FOUND) to explicitly set
bypassRealmRole = RealmRole.ADMIN so the admin bypass is explicit; apply the
same change to the other `@VaultRole` instance mentioned (the second occurrence
near the other method) to ensure both annotations explicitly declare
bypassRealmRole = RealmRole.ADMIN.
🪄 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: 03078181-cfc9-4a35-bea6-c7a60b5827e8
📒 Files selected for processing (7)
CHANGELOG.mdbackend/src/main/java/org/cryptomator/hub/api/VaultResource.javabackend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.javafrontend/src/common/backend.tsfrontend/src/components/ArchiveVaultDialog.vuefrontend/src/components/ReactivateVaultDialog.vuefrontend/src/components/VaultDetails.vue
✅ Files skipped from review due to trivial changes (2)
- CHANGELOG.md
- backend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/VaultDetails.vue
SailReal
left a comment
SailReal
left a comment
There was a problem hiding this comment.
It looks quite good, but if the admin is also owner and has lost the account key, the admin should still be able to archive the vault:
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java (1)
965-1025: Add a non-existing vault test forPUT /{vaultId}/archived(expect404).The PR’s core auth change is “bypass only after vault lookup.” This should be locked with an explicit 404 test on a non-existing vault ID.
✅ Suggested test addition
+ `@Test` + `@Order`(5) + `@DisplayName`("PUT /vaults/7E57C0DE-0000-4000-8000-BADBADBADBAD/archived returns 404 for non-existing vault") + void testAdminArchiveNonExistingVault() { + given().contentType(ContentType.TEXT).body("true") + .when().put("/vaults/{vaultId}/archived", "7E57C0DE-0000-4000-8000-BADBADBADBAD") + .then().statusCode(404); + }🤖 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/VaultResourceIT.java` around lines 965 - 1025, Add a new test method inside the AdminArchiveVault nested class that verifies PUT /vaults/{vaultId}/archived for a non-existent UUID returns 404; name it e.g. testAdminArchiveNonExistingVault404 and use the same request style as testAdminArchiveVault (given().contentType(ContentType.TEXT).body("true").when().put("/vaults/{vaultId}/archived", "<non-existing-UUID>").then().statusCode(404)); annotate similarly (e.g. `@Test`, `@Order`, `@DisplayName`) and include `@DBRollbackAfter` if needed to match surrounding tests.
🤖 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/test/java/org/cryptomator/hub/api/VaultResourceIT.java`:
- Around line 965-1025: Add a new test method inside the AdminArchiveVault
nested class that verifies PUT /vaults/{vaultId}/archived for a non-existent
UUID returns 404; name it e.g. testAdminArchiveNonExistingVault404 and use the
same request style as testAdminArchiveVault
(given().contentType(ContentType.TEXT).body("true").when().put("/vaults/{vaultId}/archived",
"<non-existing-UUID>").then().statusCode(404)); annotate similarly (e.g. `@Test`,
`@Order`, `@DisplayName`) and include `@DBRollbackAfter` if needed to match
surrounding tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 065bf560-2c4f-4c00-85e0-5d7f72df7297
📒 Files selected for processing (3)
backend/src/main/java/org/cryptomator/hub/api/VaultResource.javabackend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.javafrontend/src/common/backend.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/common/backend.ts
- backend/src/main/java/org/cryptomator/hub/api/VaultResource.java
This was already fixed in commit Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Fixes #283.
Admins can view all vaults but previously couldn't archive or unarchive vaults they don't own. The
@VaultRoleannotation'srealmRolefield was shared between the bypass check and theonMissingVaultcheck, making it impossible to add admin bypass to thecreateOrUpdateendpoint without breaking vault creation permissions.This PR introduces a
bypassRealmRolefield on the@VaultRoleannotation to separate the two concerns. The bypass check is also moved into the vault-exists block so it only applies to existing vaults. Vault creation still requires thecreate-vaultsrole as before.Instead of adding admin bypass to the general
createOrUpdateendpoint (which would let admins modify vault name, description, and emergency key shares), a dedicatedPUT /{vaultId}/archivedendpoint handles archive toggling with its own admin bypass. ThecreateOrUpdateendpoint remains restricted to vault owners.On the frontend, archive and reactivate buttons are now visible to admin users who aren't vault owners. The buttons also appear in the vault recovery section, so admins who are vault owners but have lost their account key can still archive or reactivate. The
setArchivedresponse normalizescreationTimeto aDateobject so the vault detail view renders correctly after toggling.