Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add create-vaults role #296

Merged
merged 10 commits into from
Oct 18, 2024
Merged

Add create-vaults role #296

merged 10 commits into from
Oct 18, 2024

Conversation

overheadhunter
Copy link
Member

This PR fixes #206 by introducing the create-vaults role.

Note that the dev-realm adds this role only for users admin, alice and bob, while the users carol, dave and erin are not allowed to create vaults.

* `PUT /api/vaults/{vaultId}`
* `POST /api/vaults/{vaultId}/claim-ownership`
* `/app/vaults/create`
* `/app/vaults/recover`
@overheadhunter overheadhunter added this to the 1.4.0 milestone Oct 11, 2024
Copy link

coderabbitai bot commented Oct 11, 2024

Walkthrough

The changes in this pull request introduce a new role, create-vaults, in Keycloak to manage permissions for vault creation. The VaultResource class has been updated to restrict access to certain methods based on this new role. Additionally, various components in the frontend have been modified to reflect this role-based access control, including the introduction of a Forbidden component for unauthorized access. Other updates include dependency upgrades and improvements in error handling and role-checking logic throughout the application.

Changes

File Change Summary
CHANGELOG.md Updated with new entries for added role, dependency upgrades, and various fixes.
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java Changed role requirements for vault operations and updated method logic for creation and ownership claims.
backend/src/main/java/org/cryptomator/hub/filters/VaultRole.java Added new enum value REQUIRE_REALM_ROLE and realmRole method for role specification.
backend/src/main/java/org/cryptomator/hub/filters/VaultRoleFilter.java Enhanced authorization logic to check realm roles for non-existing vaults.
backend/src/test/java/org/cryptomator/hub/filters/VaultRoleFilterTest.java Updated tests to validate new realm role checks in the filter.
frontend/src/common/auth.ts Removed specific role-checking methods and added a generic hasRole method.
frontend/src/components/Forbidden.vue Introduced new component for displaying "403 Forbidden" messages.
frontend/src/components/NavigationBar.vue Updated role-checking logic for dropdown menu.
frontend/src/components/VaultDetails.vue Modified role-checking logic and refined error handling.
frontend/src/components/VaultList.vue Added canCreateVaults property and updated logic for vault creation permissions.
frontend/src/i18n/de-DE.json Added new localization entries for vault creation errors.
frontend/src/i18n/en-US.json Added new localization entries for vault creation errors.

Assessment against linked issues

Objective Addressed Explanation
Implement create-vaults role in Keycloak (#206)
Ensure users can be configured to create vaults (#206)

🐇 In the meadow, hopping bright,
New vaults are born, a joyful sight.
With roles defined, and paths made clear,
A "Forbidden" sign for those who veer.
Permissions granted, let’s create with glee,
In our little hub, come vault with me! 🐰✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 508c651 and 7ec3ad0.

📒 Files selected for processing (1)
  • backend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java (2 hunks)
🧰 Additional context used
🔇 Additional comments (4)
backend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java (4)

244-255: LGTM: New test case for creating vault without required role

The new test method testCreateVaultWithMissingRole is well-implemented. It correctly verifies that a user without the "create-vaults" role receives a 403 Forbidden status when attempting to create a vault. This test enhances the coverage of role-based access control for vault creation.


260-260: LGTM: Updated test security annotation

The @testsecurity annotation has been correctly updated to include the "create-vaults" role. This change ensures that the tests in the CreateVaults class are executed with the appropriate permissions, reflecting the new role-based access control for vault creation.


662-662: LGTM: Updated test security annotation for ExceedingLicenseLimits

The @testsecurity annotation in the ExceedingLicenseLimits class has been correctly updated to include the "create-vaults" role. This change maintains consistency with the new role-based access control and ensures that the tests in this class have the necessary permissions to create vaults when testing license limit scenarios.


Line range hint 1-1266: Overall assessment: Comprehensive implementation of "create-vaults" role

The changes in this file effectively implement and test the new "create-vaults" role for vault creation permissions. Key points:

  1. A new test case has been added to verify that users without the "create-vaults" role cannot create vaults.
  2. Existing test classes that involve vault creation (CreateVaults and ExceedingLicenseLimits) have been updated to include the new role in their @testsecurity annotations.
  3. The changes are consistent throughout the file and do not introduce any regressions in existing tests.

These modifications enhance the test coverage for role-based access control in vault creation, ensuring that the new permission system is working as expected.


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (11)
frontend/src/components/Forbidden.vue (1)

2-16: Styling and responsiveness look good, with a minor suggestion.

The use of Tailwind CSS classes for styling and responsiveness is well-implemented. The component adapts to different screen sizes, and the color scheme aligns with the application's primary color.

Consider adding a transition class to the button for a smoother hover effect:

- <router-link to="/app/vaults" class="inline-flex items-center px-4 py-2 border border-transparent text-sm font-medium rounded-md shadow-sm text-white bg-primary focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-primary">
+ <router-link to="/app/vaults" class="inline-flex items-center px-4 py-2 border border-transparent text-sm font-medium rounded-md shadow-sm text-white bg-primary focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-primary transition hover:bg-primary-dark">
CHANGELOG.md (1)

15-15: LGTM! Consider adding a brief impact statement.

The added changelog entry accurately reflects the new feature introduced by this PR, aligning well with the objectives stated in issue #206. The wording is clear and concise, and the placement is appropriate.

To enhance clarity for users reading the changelog, consider adding a brief statement about the impact of this change. For example:

- Permission to create new vaults can now be controlled via the `create-vaults` role in Keycloak (#206)
+ Permission to create new vaults can now be controlled via the `create-vaults` role in Keycloak, allowing for finer-grained access control (#206)

This addition would provide users with a clearer understanding of the feature's significance without significantly increasing the entry's length.

frontend/src/common/auth.ts (1)

Line range hint 1-50: Overall assessment of changes in auth.ts

The changes in this file effectively support the PR objective of introducing a new create-vaults role by replacing specific role-checking methods with a more flexible hasRole method. This modification allows for easier management of various roles, including the new create-vaults role.

Key points:

  1. The new hasRole method is more versatile than the previous isAdmin and isUser methods.
  2. The implementation uses modern JavaScript features for safer property access.
  3. The overall structure of the Auth class remains intact, maintaining consistency with the rest of the codebase.

While the changes are generally good, consider implementing the suggested improvements to the hasRole method for increased robustness and type safety.

As the application evolves to include more roles and permissions:

  1. Consider implementing a more comprehensive role management system, possibly using a separate service or utility for role-based access control (RBAC).
  2. Document the available roles and their meanings in a central location for easier maintenance and onboarding of new developers.
  3. Implement unit tests for the Auth class, especially the new hasRole method, to ensure correct behavior across different scenarios.
frontend/src/components/NavigationBar.vue (1)

Line range hint 1-116: Consider broader implications of role-based access control implementation.

The change to use hasRole('admin') instead of isAdmin() is a good step towards implementing a more flexible role-based access control system. However, to ensure consistency and proper implementation across the entire application, consider the following:

  1. Review other components and services that may be using isAdmin() and update them to use the new hasRole method.
  2. Ensure that the 'admin' role encompasses all the permissions that were previously granted by the isAdmin check.
  3. Update documentation to reflect this change in the authentication and authorization system.
  4. Consider adding unit tests to verify the correct behavior of the hasRole method with different user roles.

Would you like assistance in identifying other areas of the codebase that might need similar updates?

backend/src/main/resources/dev-realm.json (1)

Line range hint 1-265: Consider adding "create-vaults" to scopeMappings

The changes implement the required role-based access control for vault creation and correctly assign permissions to Admin, Alice, and Bob. However, I noticed that the "create-vaults" role is not included in the "scopeMappings" section for the "cryptomatorhub" client. This might affect the frontend's ability to determine if a user has vault creation permissions.

Consider adding "create-vaults" to the "scopeMappings" section to ensure proper role mapping in the frontend. Here's a suggested change:

  "scopeMappings": [
    {
      "client": "cryptomatorhub",
      "roles": [
        "user",
        "admin",
+       "create-vaults"
      ]
    }
  ],

This addition would ensure that the "create-vaults" role is properly mapped and accessible in the frontend application.

backend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java (1)

255-266: LGTM: New test case for role-based access control

This new test case effectively validates the enforcement of the "create-vaults" role, ensuring that users without this role cannot create vaults. It's a valuable addition to the test suite and aligns well with the PR objectives.

However, there's a minor typo in the method name that should be corrected:

-    public void testCreteVaultWithMissingRole() {
+    public void testCreateVaultWithMissingRole() {
frontend/src/router/index.ts (1)

1-1: Remove unnecessary import and update to NavigationGuard

Since the checkRole function returns NavigationGuard, you should import NavigationGuard instead of NavigationGuardWithThis. This cleans up the import statement and ensures type consistency.

Apply this diff to update the import:

-import { createRouter, createWebHistory, NavigationGuardWithThis, RouteLocationRaw, RouteRecordRaw } from 'vue-router';
+import { createRouter, createWebHistory, NavigationGuard, RouteLocationRaw, RouteRecordRaw } from 'vue-router';
frontend/src/components/VaultDetails.vue (4)

Line range hint 153-162: Remove redundant type check for AuthorityDto in addAuthority function

Since the authority parameter is already typed as AuthorityDto, the type check if (!isAuthorityDto(authority)) is redundant and can be removed to simplify the code.


Line range hint 368-377: Add default case to switch statement in updateMemberRole

In the updateMemberRole function, consider adding a default case to the switch statement to handle unexpected member.type values. This enhances robustness and ensures that any unforeseen types are appropriately handled.


Line range hint 290-299: Add default case to switch statement in addAuthorityBackend

In the addAuthorityBackend function, consider including a default case in the switch statement to handle unexpected authority.type values, which can prevent potential errors due to unhandled types.


Line range hint 215-218: Add error handling in loadVaultKeys function

Consider wrapping the code in loadVaultKeys within a try-catch block to handle potential errors from decryptUserKeysWithBrowser() and decryptWithUserKey(). This will improve error resilience and provide a better user experience if decryption fails.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c30dfe4 and 8aee735.

📒 Files selected for processing (10)
  • CHANGELOG.md (1 hunks)
  • backend/src/main/java/org/cryptomator/hub/api/VaultResource.java (2 hunks)
  • backend/src/main/resources/dev-realm.json (3 hunks)
  • backend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java (4 hunks)
  • frontend/src/common/auth.ts (1 hunks)
  • frontend/src/components/Forbidden.vue (1 hunks)
  • frontend/src/components/NavigationBar.vue (1 hunks)
  • frontend/src/components/VaultDetails.vue (1 hunks)
  • frontend/src/components/VaultList.vue (3 hunks)
  • frontend/src/router/index.ts (5 hunks)
🧰 Additional context used
🔇 Additional comments (21)
frontend/src/components/Forbidden.vue (2)

1-17: LGTM: Well-structured and informative error page.

The template provides a clear and informative "403 Forbidden" error page. The structure is well-organized, displaying the error code, a heading, and a descriptive message. The layout appears to be responsive, adapting to different screen sizes through the use of Tailwind CSS classes.


5-13: Enhance accessibility and verify router link.

The error message is clearly conveyed through text, which is good for screen readers. The router link provides a clear navigation option.

Consider these accessibility enhancements:

  1. Add an aria-label to the error code for better screen reader context:
    - <p class="text-4xl font-extrabold text-primary sm:text-5xl">403</p>
    + <p class="text-4xl font-extrabold text-primary sm:text-5xl" aria-label="Error code 403">403</p>
  2. Use a more semantic <h2> tag for the "Forbidden" text:
    - <h1 class="text-4xl font-extrabold text-gray-900 tracking-tight sm:text-5xl">Forbidden</h1>
    + <h2 class="text-4xl font-extrabold text-gray-900 tracking-tight sm:text-5xl">Forbidden</h2>

Verify the router link functionality with this script:

CHANGELOG.md (1)

Line range hint 1-41: Overall changelog structure and content look great!

The changelog is well-structured and comprehensive, following the Keep a Changelog format. It effectively captures all the major changes, including the new create-vaults role, dependency updates, and security fixes. The categorization of changes into Added, Changed, Fixed, and Security sections provides clear organization and makes it easy for users to understand the nature of each update.

frontend/src/common/auth.ts (1)

48-50: Approve changes with suggestions for improvements

The new hasRole method is a good addition that provides more flexibility in role checking. It aligns well with the PR objective of introducing a new create-vaults role. However, there are a couple of suggestions for improvement:

  1. Consider adding a type check for the role parameter to ensure it's a non-empty string.
  2. The method could be more robust by also checking for roles directly on tokenParsed if realm_access is undefined.

Here's a suggested implementation that addresses these points:

public hasRole(role: string): boolean {
  if (typeof role !== 'string' || role.length === 0) {
    throw new Error('Role must be a non-empty string');
  }
  return (
    this.keycloak.tokenParsed?.realm_access?.roles?.includes(role) ??
    this.keycloak.tokenParsed?.roles?.includes(role) ??
    false
  );
}

This implementation:

  1. Adds a type check for the role parameter.
  2. Checks for roles in both realm_access.roles and directly in roles on tokenParsed.
  3. Maintains the use of optional chaining and nullish coalescing for safe access.

To ensure that this change doesn't break existing functionality, please run the following script to check for any remaining usage of the removed isAdmin and isUser methods:

If any occurrences are found, they should be updated to use the new hasRole method.

✅ Verification successful

Verified: No remaining usages of isAdmin() or isUser() methods found in the ./frontend directory.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining usage of removed methods

# Test: Search for isAdmin() and isUser() method calls
echo "Searching for isAdmin() method calls:"
rg --type typescript "\.isAdmin\(\)" ./frontend

echo "Searching for isUser() method calls:"
rg --type typescript "\.isUser\(\)" ./frontend

Length of output: 336


Script:

#!/bin/bash
# Description: Check for any remaining usage of removed methods with correct file types

# Test: Search for isAdmin() and isUser() method calls in TypeScript files
echo "Searching for isAdmin() method calls:"
rg --type ts "\.isAdmin\(\)" ./frontend

echo "Searching for isUser() method calls:"
rg --type ts "\.isUser\(\)" ./frontend

# Additionally, search in .tsx files if present
echo "Searching for isAdmin() method calls in .tsx files:"
rg --type tsx "\.isAdmin\(\)" ./frontend

echo "Searching for isUser() method calls in .tsx files:"
rg --type tsx "\.isUser\(\)" ./frontend

Length of output: 615


Script:

#!/bin/bash
# Description: Check for any remaining usage of removed methods using file extension patterns

# Test: Search for isAdmin() and isUser() method calls in .ts and .tsx files
echo "Searching for isAdmin() method calls in .ts and .tsx files:"
rg "\.isAdmin\(\)" ./frontend -g "*.ts" -g "*.tsx"

echo "Searching for isUser() method calls in .ts and .tsx files:"
rg "\.isUser\(\)" ./frontend -g "*.ts" -g "*.tsx"

Length of output: 360

frontend/src/components/NavigationBar.vue (1)

110-110: LGTM! Verify 'admin' role setup and hasRole implementation.

The change from isAdmin() to hasRole('admin') aligns well with the PR objectives of implementing a role-based permission system. This approach is more flexible and consistent with RBAC best practices.

To ensure this change works as intended, please verify:

  1. The 'admin' role is correctly set up in the backend.
  2. The hasRole method in the auth object is implemented correctly.

Run the following script to check the implementation of the hasRole method:

backend/src/main/resources/dev-realm.json (4)

19-23: LGTM: New role "create-vaults" correctly defined

The new "create-vaults" role is well-defined with an appropriate name, description, and composite flag. This addition aligns with the PR objectives to introduce a role for managing vault creation permissions.


30-31: LGTM: "admin" role correctly updated with "create-vaults" composite

The "admin" role has been appropriately updated to include the "create-vaults" role in its composites. This change ensures that administrators have the necessary permissions to create vaults, which is consistent with the PR objectives.


82-82: LGTM: Alice's realm roles correctly updated

Alice's realm roles have been appropriately updated to include both "user" and "create-vaults". This change aligns with the PR objectives, granting Alice the permission to create vaults while maintaining her basic user permissions.


91-91: LGTM: Bob's realm roles correctly updated

Bob's realm roles have been appropriately updated to include both "user" and "create-vaults". This change aligns with the PR objectives, granting Bob the permission to create vaults while maintaining his basic user permissions.

backend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java (5)

248-248: LGTM: Updated role in @testsecurity annotation

The change from roles = {"user"} to roles = {"create-vaults"} aligns with the PR objective of introducing a new "create-vaults" role. This ensures that the tests in this class are run with the appropriate permissions, accurately reflecting the new role-based access control for vault creation.


663-663: LGTM: Updated roles in @testsecurity annotation

The change to include both "user" and "create-vaults" roles in the @TestSecurity annotation is appropriate. This ensures that the tests in this class are run with both standard user permissions and the new vault creation permissions, accurately reflecting the intended role-based access control. This change is consistent with the PR objective of introducing the new "create-vaults" role while maintaining existing functionality.


1010-1025: LGTM: New test case for role-based access control on claim ownership

This new test case effectively validates the enforcement of the "create-vaults" role for the claim ownership functionality. It ensures that users without this role cannot claim ownership of vaults, which is consistent with the PR objectives. The test is well-structured and provides good coverage for the new role-based access control implementation.


Line range hint 1-1170: Summary: Comprehensive implementation and testing of the new "create-vaults" role

The changes in this file consistently implement and test the new "create-vaults" role across different functionalities of the VaultResource. Key points:

  1. Test security annotations have been updated to include the new role where appropriate.
  2. New test cases have been added to validate the role-based access control for vault creation and ownership claiming.
  3. Existing test classes have had their roles updated to include or focus on the "create-vaults" role.

These changes align well with the PR objectives of introducing a new role for vault creation. The test coverage for the new role seems comprehensive, and there's a good balance between testing the new functionality and ensuring existing functionality is not broken.

Overall, these changes provide a solid foundation for the new role-based access control system for vault creation and management.


848-848: LGTM with a question: Updated role in @testsecurity annotation

The change to include only the "create-vaults" role in the @TestSecurity annotation aligns with the PR objective of introducing the new role for vault creation. However, I have a question:

Does removing the "user" role impact any tests in this class that might rely on standard user permissions? If not, this change is appropriate. If there are tests that require standard user permissions, we might need to keep both roles or split the test class.

To verify if any tests in this class require standard user permissions, we can run the following script:

✅ Verification successful

To further assess the impact of removing the "user" role from the @TestSecurity annotation in VaultResourceIT.java, let's identify all instances where the "user" role is used.


Verified: Removal of "user" role does not impact other tests

The removal of the "user" role from the @TestSecurity annotation in VaultResourceIT.java (lines 848-848) is appropriate. Other tests within the class continue to utilize the "user" role where necessary, ensuring that standard user permissions remain intact.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for method calls or assertions that might rely on standard user permissions
rg -i 'user|permission|access' backend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java

Length of output: 16557


Script:

#!/bin/bash
# List all @TestSecurity annotations with the "user" role in VaultResourceIT.java
rg '@TestSecurity.*\{"user"\}' backend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java

Length of output: 654

frontend/src/router/index.ts (4)

67-68: Usage of checkRole('create-vaults') is appropriate

The role check for 'create-vaults' is correctly applied to the vault creation route. This ensures that only authorized users can access the vault creation page.


73-74: Role check for vault recovery is correctly implemented

The beforeEnter guard correctly checks for the 'create-vaults' role on the vault recovery route. This maintains consistent access control for vault-related actions.


87-87: Admin route role check is appropriately updated

The beforeEnter guard now uses checkRole('admin'), streamlining the role-checking logic for admin routes. This enhances code maintainability and readability.


128-132: Forbidden route is correctly defined

The /app/forbidden route is added appropriately to handle unauthorized access. Setting meta: { skipAuth: true, skipSetup: true } ensures that users are redirected here without additional authentication or setup checks.

backend/src/main/java/org/cryptomator/hub/api/VaultResource.java (2)

391-391: Verify role assignment for createOrUpdate method

The @RolesAllowed("create-vaults") annotation restricts access to users with the create-vaults role for the createOrUpdate method. Please ensure that all users who need to create or update vaults are assigned this role in Keycloak, and that this aligns with the intended permission model.


442-442: Confirm role restriction for claimOwnership method

The @RolesAllowed("create-vaults") annotation now restricts the claimOwnership method to users with the create-vaults role. Verify that this change correctly reflects the desired access control and that users who need to claim ownership have the appropriate role.

frontend/src/components/VaultDetails.vue (1)

286-286: Change to use hasRole('admin') for admin status check is appropriate

The update from isAdmin to hasRole('admin') correctly determines admin privileges based on roles. This change aligns with the introduction of the create-vaults role and enhances role-based access control within the component.

frontend/src/router/index.ts Outdated Show resolved Hide resolved
frontend/src/components/VaultList.vue Outdated Show resolved Hide resolved
frontend/src/components/VaultList.vue Show resolved Hide resolved
frontend/src/components/VaultList.vue Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
frontend/src/router/index.ts (1)

129-133: LGTM with a minor suggestion: /app/forbidden route

The addition of the /app/forbidden route is a necessary component of the role-based access control system. The use of the Forbidden component and the meta settings are appropriate.

Consider adding a name to this route for easier reference in navigation guards:

{
  path: '/app/forbidden',
  name: 'forbidden',
  component: Forbidden,
  meta: { skipAuth: true, skipSetup: true }
},

This named route can then be used in navigation guards like this: return { name: 'forbidden', replace: true };

Would you like me to provide a code snippet for this change?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 8aee735 and 460b3fa.

📒 Files selected for processing (2)
  • frontend/src/components/VaultList.vue (3 hunks)
  • frontend/src/router/index.ts (5 hunks)
🧰 Additional context used
🔇 Additional comments (7)
frontend/src/router/index.ts (4)

17-17: LGTM: Import of Forbidden component

The addition of the Forbidden component import is consistent with the implementation of role-based access control, as outlined in the PR objectives.


68-69: LGTM: Role-based access control for vault creation

The changes to the vaults/create route correctly implement the role-based access control for vault creation, as specified in the PR objectives. The use of a props function to set recover: false is a good practice for passing data to components.


74-75: LGTM: Consistent role-based access control for vault recovery

The changes to the vaults/recover route are consistent with those made to the vaults/create route, correctly implementing role-based access control for vault recovery. This aligns with the PR objectives and maintains consistency across related routes.


Line range hint 1-233: Summary: Successful implementation of role-based access control

The changes in this file successfully implement the role-based access control for vault creation and recovery, as specified in the PR objectives. The new checkRole function provides a centralized mechanism for role verification, and the route updates consistently apply this check where needed.

Key points:

  1. The checkRole function implementation is solid, with minor suggestions for improvement in type safety and logging.
  2. Route guards for vault creation and recovery have been updated consistently.
  3. A new '/app/forbidden' route has been added to handle unauthorized access attempts.

Overall, the changes are well-implemented and align closely with the PR objectives. The suggested improvements are minor and do not impact the core functionality.

frontend/src/components/VaultList.vue (3)

43-43: Changes look good for disabling the MenuButton

The MenuButton now correctly disables the ability to create vaults when the license is violated or the user lacks the create-vaults role.


150-151: Proper initialization of reactive properties

Initializing isAdmin and canCreateVaults to false prevents undefined behavior before the asynchronous assignment completes in fetchData().


181-182: Optimize role checks by minimizing redundant await calls

As previously suggested, you can await auth once and then perform all role checks to improve performance.

frontend/src/router/index.ts Show resolved Hide resolved
Copy link
Member

@SailReal SailReal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do some testing now, but as initial feedback: Not having the new create-vault role is the new default, and specifically because of this, we need to change the display text in the vault list for users without the create-vault role to say that they cannot create vaults because they do not have that role set...to not confuse them too much.

image

Maybe we should also add this note in a tooltip to the Add button when disabled, so that even users who are members of another vault know why they cannot create vaults, or should we remove the button altogether?

@overheadhunter
Copy link
Member Author

overheadhunter commented Oct 15, 2024

Maybe we should also add this note in a tooltip to the Add button when disabled, so that even users who are members of another vault know why they cannot create vaults, or should we remove the button altogether?

I decided against removing the button altogether as seeing the button sparks the question why the user can't add a vault (as opposed to where she can do so, if it were completely missing).

I believe that when something is not working as expected, a good UI should at least make you ask the right questions.

An even better UI even provides the answer and I agree that a tooltip (among others) is a good means.

Edit: Added in f9eda5c

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
frontend/src/components/VaultList.vue (2)

43-46: LGTM! Consider adding aria-label for improved accessibility.

The changes effectively implement role-based access control for vault creation and provide user feedback through the title attribute, aligning with the PR objectives. Great job on addressing the suggestion to add a tooltip!

To further enhance accessibility, consider adding an aria-label attribute that matches the title content. This ensures screen readers can convey the button's state and any associated messages.

-        <MenuButton :disabled="isLicenseViolated || !canCreateVaults" class="inline-flex items-center px-4 py-2 border border-transparent rounded-md shadow-sm text-sm font-medium text-white bg-primary hover:bg-primary-d1 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-primary disabled:cursor-not-allowed disabled:opacity-50" :title="isLicenseViolated ? t('vaultList.addVault.disabled.licenseViolation') : canCreateVaults ? undefined : t('vaultList.addVault.disabled.missingPermission')">
+        <MenuButton :disabled="isLicenseViolated || !canCreateVaults" class="inline-flex items-center px-4 py-2 border border-transparent rounded-md shadow-sm text-sm font-medium text-white bg-primary hover:bg-primary-d1 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-primary disabled:cursor-not-allowed disabled:opacity-50" :title="isLicenseViolated ? t('vaultList.addVault.disabled.licenseViolation') : canCreateVaults ? undefined : t('vaultList.addVault.disabled.missingPermission')" :aria-label="isLicenseViolated ? t('vaultList.addVault.disabled.licenseViolation') : canCreateVaults ? t('vaultList.addVault') : t('vaultList.addVault.disabled.missingPermission')">

181-182: LGTM! Consider optimizing role checks.

The implementation of role-based access control for vault creation aligns well with the PR objectives. Good job on using auth.hasRole() to check for both admin and create-vaults roles.

To optimize performance, consider awaiting auth once and then performing both role checks. This addresses a suggestion from a past review comment:

-    isAdmin.value = (await auth).hasRole('admin');
-    canCreateVaults.value = (await auth).hasRole('create-vaults');
+    const userAuth = await auth;
+    isAdmin.value = userAuth.hasRole('admin');
+    canCreateVaults.value = userAuth.hasRole('create-vaults');

This change reduces the number of async operations and slightly improves the function's performance.

frontend/src/i18n/en-US.json (1)

248-248: LGTM! Consider adding more detail to the message.

The new entry for the license violation scenario is well-placed and follows the existing naming conventions. It provides clear feedback to the user about why they can't add a vault.

Consider expanding the message slightly to provide more context or guidance, such as:

-  "vaultList.addVault.disabled.licenseViolation": "License limit exceeded.",
+  "vaultList.addVault.disabled.licenseViolation": "License limit exceeded. Please contact your administrator.",

This additional information could help users understand what action they can take next.

frontend/src/i18n/de-DE.json (1)

248-248: Approve with minor grammatical correction

The new entry "vaultList.addVault.disabled.missingPermission" is appropriate and aligns with the PR objectives. However, there's a small grammatical error in the German text.

Please correct the grammatical error in the German text:

- "vaultList.addVault.disabled.missingPermission": "Die fehlt die Berechtigung zur Erstellung von Tresoren.",
+ "vaultList.addVault.disabled.missingPermission": "Dir fehlt die Berechtigung zur Erstellung von Tresoren.",

This change replaces "Die fehlt" with "Dir fehlt", which is the correct German grammar for "You lack" or "You're missing".

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 460b3fa and f9eda5c.

📒 Files selected for processing (3)
  • frontend/src/components/VaultList.vue (4 hunks)
  • frontend/src/i18n/de-DE.json (1 hunks)
  • frontend/src/i18n/en-US.json (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
frontend/src/components/VaultList.vue (2)

101-101: LGTM! Good user experience consideration.

The conditional rendering of the empty vault list description based on canCreateVaults is a thoughtful addition. It ensures that users without the permission to create vaults don't see potentially confusing or irrelevant information, aligning well with the PR objectives.


150-150: LGTM! Addressed past review comment.

Initializing isAdmin with false is a good practice. It prevents potential undefined behavior in the template and addresses the concern raised in the past review comments.

frontend/src/i18n/en-US.json (2)

249-249: Excellent addition! Aligns perfectly with PR objectives.

This new entry for the missing permission scenario is well-implemented. It follows the established naming conventions and provides clear feedback to the user. The message content directly addresses the PR objectives by informing users about their permission status for vault creation.

This implementation also aligns with the suggestion made in the PR comments about adding a tooltip to the disabled "Add" button. It provides the necessary text for such a tooltip, enhancing the user experience by explaining why the action is unavailable.


248-249: Summary: Effective implementation of user feedback for vault creation restrictions

The two new entries added to this localization file effectively address the PR objectives by providing clear messages for scenarios where vault creation is restricted. These additions enhance the user experience by offering immediate feedback about license limitations and permission issues.

The changes are minimal yet impactful, seamlessly integrating with the existing localization structure. They lay the groundwork for implementing the tooltip suggestion mentioned in the PR comments, which will further improve the application's usability.

frontend/src/i18n/de-DE.json (2)

247-247: LGTM: Clear message for license violation

The new entry "vaultList.addVault.disabled.licenseViolation" with the value "Lizenz überschritten." is a clear and concise message in German, indicating that the license limit has been exceeded. This aligns well with the PR objectives of implementing role-based access for vault creation.


247-248: Summary: New translations align with PR objectives

The two new entries added to the vaultList section of the German localization file effectively support the implementation of role-based access for vault creation, as outlined in the PR objectives. These additions will provide clear feedback to users when they are unable to create vaults due to license violations or missing permissions.

The changes are consistent with the existing structure and naming conventions in the file, enhancing the user experience by providing appropriate localized messages.

The fallback is (currently) only used in case of `onMissingVault=REQUIRE_REALM_ROLE`, when no vault role exists yet
* allowing any user to modify a owned vault
* allowing only users with `create-vaults` role to create vault
* allowing any user to claim ownership (if they can prove worthy)
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (4)
backend/src/main/java/org/cryptomator/hub/filters/VaultRole.java (1)

36-42: LGTM: New realmRole() method enhances role-based access control

The addition of the realmRole() method to the VaultRole annotation is well-implemented and documented. It provides a flexible way to specify additional realm roles required for access, which aligns with the PR's objectives.

Consider adding a brief example in the Javadoc to illustrate how this method can be used in conjunction with onMissingVault(). This could help developers quickly understand its application. For instance:

/**
 * ...
 * @return realm role required to access the annotated resource.
 *
 * Example usage:
 * @VaultRole(onMissingVault = OnMissingVault.REQUIRE_REALM_ROLE, realmRole = "create-vaults")
 */
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java (3)

391-392: LGTM! Role-based access control for vault creation implemented.

The new @VaultRole annotation with the realmRole = "create-vaults" parameter effectively implements the requested "create-vaults" role from issue #206. This change ensures that only users with the appropriate permissions can create new vaults.

Consider adding a comment explaining the role requirements for clarity:

+ // Requires "user" role for authentication and either "OWNER" role for existing vaults or "create-vaults" realm role for new vaults
 @RolesAllowed("user") // general authentication. VaultRole filter will check for specific access rights
 @VaultRole(value = VaultAccess.Role.OWNER, onMissingVault = VaultRole.OnMissingVault.REQUIRE_REALM_ROLE, realmRole = "create-vaults")

Line range hint 463-464: LGTM! Improved security for vault ownership claims.

The new check for the existence of vault.authenticationPublicKey prevents multiple users from claiming ownership of the same vault. This is a good security improvement.

Consider adding a comment to explain the significance of this check:

+ // If authenticationPublicKey is null, the vault has already been claimed by another user
 var authPubKey = vault.getAuthenticationPublicKeyOptional().orElseThrow(() -> new ClientErrorException(Response.Status.CONFLICT));

Line range hint 1-563: Overall, the changes look good and address the PR objectives.

The implementation of the "create-vaults" role and the improvements to the vault ownership claim process are well-executed. These changes enhance the security and control over vault management, aligning with the requirements outlined in issue #206.

Consider documenting these new role-based access controls and the vault ownership claim process in the project's technical documentation or API documentation to ensure that other developers and potential API consumers are aware of these security measures.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between f9eda5c and 508c651.

📒 Files selected for processing (4)
  • backend/src/main/java/org/cryptomator/hub/api/VaultResource.java (1 hunks)
  • backend/src/main/java/org/cryptomator/hub/filters/VaultRole.java (1 hunks)
  • backend/src/main/java/org/cryptomator/hub/filters/VaultRoleFilter.java (2 hunks)
  • backend/src/test/java/org/cryptomator/hub/filters/VaultRoleFilterTest.java (5 hunks)
🧰 Additional context used
🔇 Additional comments (3)
backend/src/main/java/org/cryptomator/hub/filters/VaultRole.java (2)

34-34: LGTM: New enum value adds flexibility for role-based access control

The addition of REQUIRE_REALM_ROLE to the OnMissingVault enum enhances the annotation's capability to handle cases where a vault doesn't exist. This change aligns well with the PR's objective of introducing a new role for vault creation, allowing for more granular control over access permissions.


34-42: Verify proper usage of new role-based access control features

The additions to the VaultRole annotation significantly enhance the system's capability for fine-grained access control, particularly for vault creation. This aligns well with the PR objectives and provides a more flexible and potentially more secure way to manage permissions.

To ensure these changes are leveraged effectively:

  1. Verify that all relevant endpoints are updated to use the new REQUIRE_REALM_ROLE option where appropriate.
  2. Ensure that the create-vaults role is properly assigned in Keycloak as mentioned in the PR description.
  3. Consider adding integration tests to verify the behavior of endpoints using this new access control mechanism.

To help verify the proper usage of these new features, you can run the following script:

This script will help identify where the new features are being used and potentially misused, allowing for a more thorough review of their implementation across the codebase.

✅ Verification successful

Verified proper usage of new role-based access control features

All usages of REQUIRE_REALM_ROLE and realmRole have been properly implemented without any detected misuses. The annotation updates align with the intended fine-grained access control enhancements.

  • VaultResource.java uses REQUIRE_REALM_ROLE with realmRole = "create-vaults"
  • VaultRoleFilterTest.java uses REQUIRE_REALM_ROLE with realmRole = "foobar"
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for usage of new REQUIRE_REALM_ROLE and realmRole features

# Test 1: Check for usage of REQUIRE_REALM_ROLE
echo "Checking for usage of REQUIRE_REALM_ROLE:"
rg --type java "onMissingVault\s*=\s*OnMissingVault\.REQUIRE_REALM_ROLE"

# Test 2: Check for usage of realmRole
echo "Checking for usage of realmRole:"
rg --type java "realmRole\s*=\s*\"[^\"]+\""

# Test 3: Check for potential misuse (realmRole without REQUIRE_REALM_ROLE)
echo "Checking for potential misuse of realmRole:"
rg --type java "realmRole\s*=\s*\"[^\"]+\"" -C 5 | rg -v "onMissingVault\s*=\s*OnMissingVault\.REQUIRE_REALM_ROLE"

Length of output: 3623

backend/src/test/java/org/cryptomator/hub/filters/VaultRoleFilterTest.java (1)

179-205: Excellent addition of tests for realm role verification

The new RequireRealmRole nested class effectively tests scenarios where a user lacks or possesses the required realm role when accessing a non-existing vault. This enhances test coverage and ensures that the OnMissingVault.REQUIRE_REALM_ROLE behavior is correctly implemented.

Copy link
Member

@SailReal SailReal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice implementation 👍

I think we need to document this in as many places as possible:
If you want to have Cryptomator Hub in the same functionality as before this, just set the create-vaults role as the default role for the realm:

image

The new default is create-vaults is only possible for users with the admin role or when explicitly applied to the user/group/identity provider or set as default for the realm.

@overheadhunter overheadhunter merged commit 0c23ef0 into develop Oct 18, 2024
5 checks passed
@overheadhunter overheadhunter deleted the feature/create-vaults-role branch November 3, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create-Vault Role
2 participants