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

fix(api): Enable global project access #580

Merged

Conversation

muntaxir4
Copy link
Contributor

@muntaxir4 muntaxir4 commented Dec 6, 2024

User description

Description

  • Created a condition for authorityCheck over environment when projects accesslevel is global. Previously the permittedAuthorites and authorites were compared, but a non member would not have those roles.
  • Add environmentCount, variableCount, secretCount properties in a test.

Fixes #575

Screenshots of relevant screens

Previously:

Permitted Authorities for Workspace:

Authorities: READ_ENVIRONMENT,READ_SECRET,READ_VARIABLE

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

Bug fix, Tests


Description

  • Enabled and enhanced a test to verify that any user can access a global project, including checks for environmentCount, secretCount, and variableCount.
  • Modified the authority check logic in the ProjectService to allow access to global projects without requiring specific authorities.

Changes walkthrough 📝

Relevant files
Tests
project.e2e.spec.ts
Enable and enhance test for global project access               

apps/api/src/project/project.e2e.spec.ts

  • Uncommented and enabled a test for global project access.
  • Added checks for environmentCount, secretCount, and variableCount in
    the test.
  • +20/-17 
    Bug fix
    project.service.ts
    Adjust authority checks for global project access               

    apps/api/src/project/service/project.service.ts

  • Modified authority check for global projects.
  • Removed authority requirements for global project access.
  • +8/-5     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    575 - Fully compliant

    Compliant requirements:

    • Fixed failing test for global project access
    • Made GLOBAL projects accessible by anyone by removing authority requirements
    • Uncommented and fixed test in project.e2e.spec.ts
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Smell
    The comment states this code block will become invalid after RBAC implementation. Consider if this is the right time to refactor this code instead of adding more complexity to it.

    Test Coverage
    The test only verifies read access. Consider adding tests for other operations (create, update, delete) with global projects to ensure complete access level validation.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add null safety check to prevent potential runtime errors when accessing object properties

    Add a null check for project.accessLevel to prevent potential runtime errors if
    project is undefined or accessLevel is null.

    apps/api/src/project/service/project.service.ts [1225-1231]

    -project.accessLevel == ProjectAccessLevel.GLOBAL
    +project?.accessLevel === ProjectAccessLevel.GLOBAL
               ? []
               : [
                   Authority.READ_ENVIRONMENT,
                   Authority.READ_SECRET,
                   Authority.READ_VARIABLE
                 ],
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding null safety check with the optional chaining operator is a good defensive programming practice that prevents runtime errors if project is undefined, which could happen in real scenarios.

    7
    General
    Use strict equality comparison to ensure type-safe comparisons

    Use strict equality operator (===) instead of loose equality (==) for type-safe
    comparisons in TypeScript.

    apps/api/src/project/service/project.service.ts [1225]

    -project.accessLevel == ProjectAccessLevel.GLOBAL
    +project.accessLevel === ProjectAccessLevel.GLOBAL
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Using === instead of == is a TypeScript best practice that prevents unexpected type coercion, though in this case the risk is low since we're comparing against an enum.

    5

    💡 Need additional feedback ? start a PR chat

    Copy link
    Member

    @rajdip-b rajdip-b left a comment

    Choose a reason for hiding this comment

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

    LGTM

    @rajdip-b rajdip-b merged commit b3a0309 into keyshade-xyz:develop Dec 7, 2024
    6 checks passed
    @muntaxir4 muntaxir4 deleted the fix/global-project-access-level branch December 7, 2024 16:45
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    BUG: Test failure while non-member tries to access a global project
    2 participants