search

keyshade-xyz / keyshade

Realtime secret and configuration management tool, with the best in class security and seamless integration support
https://keyshade.xyz
Mozilla Public License 2.0
196 stars 96 forks source link

refactor(api): Change Authority to Authorities[] in checkAuthorityOverWorkspace #401

Closed anaskhan28 closed 1 month ago

anaskhan28 commented 2 months ago

User description

Description

Changed the authority check from a single Authority to an array of Authorities[] in the checkAuthorityOverWorkspace method. This allows for more flexible permission checks where a user can have multiple authorities.

Fixes #196

Dependencies

No new dependencies were added.

Future Improvements

Mentions

@[relevant-team-member] @[project-manager]

Screenshots of relevant screens

N/A - This change is backend-only and does not affect the UI.

Developer's checklist

If changes are made in the code:

Documentation Update

Note: Please review and update the documentation to reflect the change from Authority to Authorities[].

PR Type

Enhancement, Bug fix

Description

Changes walkthrough 📝

Relevant files
Enhancement
10 files
authority-checker.service.ts
Refactor authority checks to support multiple authorities
apps/api/src/common/authority-checker.service.ts
  • Changed authority parameter to authorities array in multiple methods.
  • Updated logic to check for multiple authorities.
  • Improved error messages for unauthorized access.
    		•  +77/-79 
    environment.service.ts
    Update environment service to support multiple authorities
    apps/api/src/environment/service/environment.service.ts
  • Updated calls to checkAuthorityOverProject and
    checkAuthorityOverEnvironment to use authorities array.
    		•  +5/-5     
    event.service.ts
    Update event service to support multiple authorities         
    apps/api/src/event/service/event.service.ts
  • Updated call to checkAuthorityOverWorkspace to use authorities array.
    		•  +1/-1     
    integration.service.ts
    Update integration service to support multiple authorities
    apps/api/src/integration/service/integration.service.ts
  • Updated calls to checkAuthorityOverWorkspace,
    checkAuthorityOverProject, and checkAuthorityOverEnvironment to use
    authorities array.
    		•  +10/-10 
    project.service.ts
    Update project service to support multiple authorities     
    apps/api/src/project/service/project.service.ts
  • Updated calls to checkAuthorityOverWorkspace and
    checkAuthorityOverProject to use authorities array.
    		•  +12/-12 
    secret.service.ts
    Update secret service to support multiple authorities       
    apps/api/src/secret/service/secret.service.ts
  • Updated calls to checkAuthorityOverProject,
    checkAuthorityOverEnvironment, and checkAuthorityOverSecret to use
    authorities array.
    		•  +9/-9     
    change-notifier.socket.ts
    Update change notifier to support multiple authorities     
    apps/api/src/socket/change-notifier.socket.ts
  • Updated calls to checkAuthorityOverWorkspace,
    checkAuthorityOverProject, and checkAuthorityOverEnvironment to use
    authorities array.
    		•  +3/-3     
    variable.service.ts
    Update variable service to support multiple authorities   
    apps/api/src/variable/service/variable.service.ts
  • Updated calls to checkAuthorityOverProject,
    checkAuthorityOverEnvironment, and checkAuthorityOverVariable to use
    authorities array.
    		•  +9/-9     
    workspace-role.service.ts
    Update workspace role service to support multiple authorities
    apps/api/src/workspace-role/service/workspace-role.service.ts
  • Updated calls to checkAuthorityOverWorkspace to use authorities array.

    		•  +5/-5     
    workspace.service.ts
    Update workspace service to support multiple authorities 
    apps/api/src/workspace/service/workspace.service.ts
  • Updated calls to checkAuthorityOverWorkspace to use authorities array.

    		•  +12/-12 

    💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    codiumai-pr-agent-free[bot] commented 2 months ago

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Redundant Code
    The new implementation of authority checks introduces redundancy in checking for `Authority.WORKSPACE_ADMIN` across multiple methods. Consider refactoring to a utility function to handle these repetitive checks, improving code maintainability and reducing potential errors. Error Handling
    The new error messages in exceptions are more detailed, which is good. However, ensure that exposing user IDs in error messages does not lead to potential security risks or privacy concerns.
    codiumai-pr-agent-free[bot] commented 2 months ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Add a check to ensure the authorities array is not empty ___ **Ensure that the authorities array is not empty before proceeding with checks to
    avoid false positives in authorization.** [apps/api/src/common/authority-checker.service.ts [36]](https://github.com/keyshade-xyz/keyshade/pull/401/files#diff-9dc32989afcb1043bc772670df8a1c993dae9721b8b44a8df13022471cce987eR36-R36) ```diff -const { userId, entity, authorities, prisma } = input +const { userId, entity, authorities, prisma } = input; +if (!authorities.length) { + throw new UnauthorizedException("No authorities provided for the user."); +} ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 10 Why: Adding a check to ensure the `authorities` array is not empty is crucial for security, preventing false positives in authorization checks. This is an important and necessary improvement.
    		10
    Use Object.freeze on authority arrays to prevent unintended modifications ___ **Ensure that the array of authorities is not mutable from outside the function scope
    by using Object.freeze to prevent potential security risks or bugs due to unintended
    modifications.** [apps/api/src/secret/service/secret.service.ts [57-510]](https://github.com/keyshade-xyz/keyshade/pull/401/files#diff-dee38177617754972e3d3f727d7f1536566d7a784b7ffdda74aa97d6eec4cbc5R57-R510) ```diff -authorities: [Authority.CREATE_SECRET], -authorities: [Authority.READ_ENVIRONMENT], -authorities: [Authority.UPDATE_SECRET], -authorities: [Authority.READ_ENVIRONMENT], -authorities: [Authority.UPDATE_SECRET], -authorities: [Authority.DELETE_SECRET], -authorities: [Authority.READ_SECRET], -authorities: [Authority.READ_ENVIRONMENT], -authorities: [Authority.READ_SECRET] +authorities: Object.freeze([Authority.CREATE_SECRET]), +authorities: Object.freeze([Authority.READ_ENVIRONMENT]), +authorities: Object.freeze([Authority.UPDATE_SECRET]), +authorities: Object.freeze([Authority.READ_ENVIRONMENT]), +authorities: Object.freeze([Authority.UPDATE_SECRET]), +authorities: Object.freeze([Authority.DELETE_SECRET]), +authorities: Object.freeze([Authority.READ_SECRET]), +authorities: Object.freeze([Authority.READ_ENVIRONMENT]), +authorities: Object.freeze([Authority.READ_SECRET]) ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: This suggestion enhances security by preventing unintended modifications to the authority arrays, which could lead to security vulnerabilities.
    		8
    Maintainability
    Refactor repeated authority checks into a helper function to enhance maintainability ___ **Replace the repeated code pattern for checking authorities with a helper function to
    improve code maintainability and reduce redundancy.** [apps/api/src/common/authority-checker.service.ts [70-74]](https://github.com/keyshade-xyz/keyshade/pull/401/files#diff-9dc32989afcb1043bc772670df8a1c993dae9721b8b44a8df13022471cce987eR70-R74) ```diff -const hasRequiredAuthority = authorities.some( - (auth) => - permittedAuthorities.has(auth) || - permittedAuthorities.has(Authority.WORKSPACE_ADMIN) -) +function checkRequiredAuthorities(authorities, permittedAuthorities) { + return permittedAuthorities.has(Authority.WORKSPACE_ADMIN) || authorities.some(auth => permittedAuthorities.has(auth)); +} +const hasRequiredAuthority = checkRequiredAuthorities(authorities, permittedAuthorities); ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 9 Why: Refactoring the repeated authority checks into a helper function significantly enhances code maintainability and reduces redundancy, making the codebase cleaner and easier to manage.
    		9
    Replace repeated authority arrays with constants to reduce redundancy and potential errors ___ **Consider using a constant array for authorities when the same authority is used
    multiple times across different methods to avoid redundancy and potential errors in
    future modifications.** [apps/api/src/integration/service/integration.service.ts [43-313]](https://github.com/keyshade-xyz/keyshade/pull/401/files#diff-dc7461d8d122f2cd2689e572229ca373cf9d2e7afd0104f67b513e11aa019a4eR43-R313) ```diff -authorities: [Authority.CREATE_INTEGRATION], -authorities: [Authority.READ_WORKSPACE], -authorities: [Authority.READ_PROJECT], -authorities: [Authority.READ_ENVIRONMENT], -authorities: [Authority.UPDATE_INTEGRATION], -authorities: [Authority.READ_PROJECT], -authorities: [Authority.READ_ENVIRONMENT], -authorities: [Authority.READ_INTEGRATION], -authorities: [Authority.READ_INTEGRATION], -authorities: [Authority.DELETE_INTEGRATION] +const CREATE_INTEGRATION_AUTH = [Authority.CREATE_INTEGRATION]; +const READ_WORKSPACE_AUTH = [Authority.READ_WORKSPACE]; +const READ_PROJECT_AUTH = [Authority.READ_PROJECT]; +const READ_ENVIRONMENT_AUTH = [Authority.READ_ENVIRONMENT]; +const UPDATE_INTEGRATION_AUTH = [Authority.UPDATE_INTEGRATION]; +const READ_INTEGRATION_AUTH = [Authority.READ_INTEGRATION]; +const DELETE_INTEGRATION_AUTH = [Authority.DELETE_INTEGRATION]; +// Use these constants in the respective methods ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: This suggestion improves maintainability by reducing redundancy and potential errors, but it is not crucial for functionality or security.
    		7
    Performance
    Optimize authority checks by moving constant condition outside the loop ___ **The check for Authority.WORKSPACE_ADMIN in the some method should be outside the
    loop to avoid redundancy and potential performance issues. This authority check does
    not depend on the iteration over authorities.** [apps/api/src/common/authority-checker.service.ts [70-74]](https://github.com/keyshade-xyz/keyshade/pull/401/files#diff-9dc32989afcb1043bc772670df8a1c993dae9721b8b44a8df13022471cce987eR70-R74) ```diff -const hasRequiredAuthority = authorities.some( - (auth) => - permittedAuthorities.has(auth) || - permittedAuthorities.has(Authority.WORKSPACE_ADMIN) +const hasWorkspaceAdmin = permittedAuthorities.has(Authority.WORKSPACE_ADMIN); +const hasRequiredAuthority = hasWorkspaceAdmin || authorities.some( + (auth) => permittedAuthorities.has(auth) ) ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: This suggestion optimizes the code by reducing redundant checks within the loop, which can improve performance. It is a logical and beneficial change.
    		8
    Enhancement
    Simplify the error message for unauthorized access ___ **Simplify the error message by removing redundant user and project ID information,
    which can be inferred from the context.** [apps/api/src/common/authority-checker.service.ts [143]](https://github.com/keyshade-xyz/keyshade/pull/401/files#diff-9dc32989afcb1043bc772670df8a1c993dae9721b8b44a8df13022471cce987eR143-R143) ```diff -throw new UnauthorizedException( - `User with id ${userId} does not have any of the required authorities in the project with id ${entity.id}` -) +throw new UnauthorizedException("User does not have the required authorities for this project") ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 6 Why: Simplifying the error message can improve readability, but it removes potentially useful debugging information. The change is minor and more subjective.
    		6
    Simplify the condition check by using the authorities parameter directly ___ **Refactor the method to directly use the authorities parameter in the condition check
    instead of reassigning it, which simplifies the code and avoids unnecessary
    complexity.** [apps/api/src/workspace-role/service/workspace-role.service.ts [53-333]](https://github.com/keyshade-xyz/keyshade/pull/401/files#diff-fa0a0e5d18d758148d81c6236ae88c9e4e117214826e1a03cc9cc0441d111320R53-R333) ```diff -authorities: [Authority.CREATE_WORKSPACE_ROLE], -authorities: [Authority.READ_WORKSPACE_ROLE], -authorities: [Authority.READ_WORKSPACE_ROLE], -authorities: Authority +// Directly use the authorities parameter +if (!permittedAuthorities.has(authorities) && !permittedAuthorities.has(Authority.WORKSPACE_ADMIN)) { + throw new UnauthorizedException('You do not have the required authority.'); +} ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 5 Why: Simplifying the condition check improves code readability and reduces complexity, but it is a minor enhancement rather than a significant improvement.
    		5
    Best practice
    Consolidate multiple authority checks into a single method to improve clarity and reduce potential errors ___ **To improve code clarity and reduce potential errors, consider consolidating checks
    into a single method if multiple checks are always performed together.** [apps/api/src/variable/service/variable.service.ts [58-503]](https://github.com/keyshade-xyz/keyshade/pull/401/files#diff-db1895137a9c036373abefb8c924150d4f2022fd6d4f5271cc6c95c443bf01cfR58-R503) ```diff -authorities: [Authority.CREATE_VARIABLE], -authorities: [Authority.READ_ENVIRONMENT], -authorities: [Authority.UPDATE_VARIABLE], -authorities: [Authority.READ_ENVIRONMENT], -authorities: [Authority.UPDATE_VARIABLE], -authorities: [Authority.DELETE_VARIABLE], -authorities: [Authority.READ_VARIABLE], -authorities: [Authority.READ_ENVIRONMENT], -authorities: [Authority.READ_VARIABLE] +// Example method that consolidates checks +checkMultipleAuthorities(userId, entityId, authoritiesArray) { + authoritiesArray.forEach(authority => { + this.authorityCheckerService.checkAuthority({ + userId: userId, + entity: { id: entityId }, + authorities: [authority], + prisma: this.prisma + }); + }); +} +// Use this method in the respective places ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 6 Why: Consolidating checks into a single method can improve code clarity and reduce potential errors, but it is more of a best practice than a critical improvement.
    		6
    rajdip-b commented 1 month ago

    Hey bro, I was fixing your PR, but then something went wrong i guess and then git didnt pick up your changes and marked the PR invalid. Although, the contribution was valid and I have merged the code in our develop branch. You can refer to this screenshot: image