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

refactor(api): Update logic for forking projects #398

Merged
merged 2 commits into from
Jul 29, 2024

Conversation

Dark-Kernel
Copy link
Contributor

@Dark-Kernel Dark-Kernel commented Jul 28, 2024

User description

Description

  1. Updated the logic to fetch from Prisma.
  2. Developed if condition block to check for accessLevel is set to Global or not.
  3. Added another if condition to check if the project exists.

Fixes #364

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:

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

Enhancement, Bug fix


Description

  • Updated the logic for forking projects to fetch project details directly from Prisma.
  • Added validation to check if the project exists and if the user has the required access level.
  • Introduced NotFoundException for missing projects and UnauthorizedException for insufficient access levels.

Changes walkthrough 📝

Relevant files
Enhancement
project.service.ts
Update project forking logic with enhanced validation       

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

  • Added NotFoundException and UnauthorizedException imports.
  • Replaced authority check with a direct Prisma query to fetch project
    details.
  • Added checks for project existence and access level.
  • Threw appropriate exceptions for missing project or insufficient
    access level.
  • +24/-8   

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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Sensitive Data Exposure:
    The code includes potential exposure of sensitive data (privateKey) which might not be securely handled or necessary in the given context. This could lead to security vulnerabilities if the data is mishandled or unnecessarily exposed.

    ⚡ Key issues to review

    Missing User Context
    The variable user.id is used in line 358 but there is no indication that the user object is available in the scope of the function. This could lead to a runtime error if user is not defined.

    Sensitive Data Exposure
    The privateKey field is selected from the database in line 348 but it's unclear if it's securely handled or if it's necessary to expose this sensitive data in this context.

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Jul 28, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Improve robustness by adding error handling for database operations

    Add error handling for the asynchronous findUnique method call to catch and manage
    potential exceptions or rejections from the database operation.

    apps/api/src/project/service/project.service.ts [340-349]

    -const project = await this.prisma.project.findUnique({
    -  where: { id: projectId },
    -  select: {
    -    id: true,
    -    name: true,
    -    description: true,
    -    storePrivateKey: true,
    -    accessLevel: true,
    -    privateKey: true
    -  }
    -})
    +let project;
    +try {
    +  project = await this.prisma.project.findUnique({
    +    where: { id: projectId },
    +    select: {
    +      id: true,
    +      name: true,
    +      description: true,
    +      storePrivateKey: true,
    +      accessLevel: true,
    +      privateKey: true
    +    }
    +  });
    +} catch (error) {
    +  throw new BadRequestException(`Failed to retrieve project: ${error.message}`);
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: Adding error handling for the asynchronous findUnique method call is essential for robustness. It ensures that potential exceptions or rejections from the database operation are managed properly, preventing unexpected crashes and providing meaningful error messages.

    10
    Security
    Improve security by preventing potential exposure of sensitive keys

    Consider adding a check for storePrivateKey and privateKey fields to ensure they are
    not inadvertently exposed or logged, which could lead to security vulnerabilities.

    apps/api/src/project/service/project.service.ts [346-348]

    -storePrivateKey: true,
    -privateKey: true
    +storePrivateKey: false,
    +privateKey: false
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion is crucial for security as it prevents the inadvertent exposure of sensitive information such as storePrivateKey and privateKey. Ensuring these fields are not exposed or logged is a significant improvement.

    9
    Possible bug
    Ensure the user context is securely handled by replacing direct access to user.id

    Replace the direct access of user.id with a parameter passed to the function or
    obtained through a secure method to ensure that the user context is correctly
    handled and to avoid potential runtime errors if the user object is not defined in
    this scope.

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

    -`User with id ${user.id} does not have the authority in the project with id ${project.id}`
    +`User with id ${currentUser.id} does not have the authority in the project with id ${project.id}`
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion addresses a potential runtime error and improves security by ensuring that the user context is correctly handled. However, it assumes that currentUser is defined and accessible in the scope, which may require additional context or changes.

    8
    Maintainability
    Enhance code readability and maintainability by refactoring condition checks into separate methods

    Refactor the condition checking the accessLevel to a separate method to enhance
    readability and maintainability of the code.

    apps/api/src/project/service/project.service.ts [356-359]

    -if (project.accessLevel !== 'GLOBAL') {
    +if (!this.isGlobalAccess(project)) {
       throw new UnauthorizedException(
         `User with id ${user.id} does not have the authority in the project with id ${project.id}`
       )
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Refactoring the condition check into a separate method improves readability and maintainability. However, it is a minor improvement and does not address any critical issues.

    7

    @rajdip-b rajdip-b changed the title chore(api): Update logic for forking projects refactor(api): Update logic for forking projects Jul 28, 2024
    @rajdip-b rajdip-b merged commit 8eba65a into keyshade-xyz:develop Jul 29, 2024
    4 checks passed
    rajdip-b pushed a commit that referenced this pull request Sep 5, 2024
    ## [2.4.0](v2.3.0...v2.4.0) (2024-09-05)
    
    ### 🚀 Features
    
    * **api-client:** Create controller for Event module ([#399](#399)) ([122df35](122df35))
    * **api-client:** Create controller for Integration module ([#397](#397)) ([697d38b](697d38b))
    * **api-client:** Create controller for Project module ([#370](#370)) ([fa25866](fa25866))
    * **api-client:** Create controller for Secret module ([#396](#396)) ([7e929c0](7e929c0))
    * **api-client:** Create controller for Variable module ([#395](#395)) ([3e114d9](3e114d9))
    * **api:** Add global search in workspace ([c49962b](c49962b))
    * **api:** Add max page size ([#377](#377)) ([ed18eb0](ed18eb0))
    * **cli:** Add functionality to operate on Environments ([#324](#324)) ([4c6f3f8](4c6f3f8))
    * **cli:** Quit on decryption failure ([#381](#381)) ([1349d15](1349d15))
    
    ### 🐛 Bug Fixes
    
    * **api-client:** Fixed broken export ([096df2c](096df2c))
    * **api:** Add NotFound exception on passing an invalid roleId while inviting user in workspace ([#408](#408)) ([ab441db](ab441db))
    * **cli:** Fixed missing module ([f7a091f](f7a091f))
    * **platform:**  Build failure in platform ([#385](#385)) ([90dcb2c](90dcb2c))
    
    ### 🔧 Miscellaneous Chores
    
    * Add api client build script and updated CI ([da0e27a](da0e27a))
    * **api:** Reorganized import using path alias ([d5befd1](d5befd1))
    * **ci:** Update CLI CI name ([8f4c456](8f4c456))
    * **cli:** Add Zod validation to parseInput function ([#362](#362)) ([34e6c39](34e6c39))
    * Fixed api client tests and rearranged controllers ([1307604](1307604))
    * Housekeeping ([c5f1330](c5f1330))
    * **platform:** Added strict null check ([072254f](072254f))
    * **web:** Added strict null check ([7e12b47](7e12b47))
    
    ### 🔨 Code Refactoring
    
    * **api:** Update logic for forking projects ([#398](#398)) ([4cf3838](4cf3838))
    Kiranchaudhary537 pushed a commit to Kiranchaudhary537/keyshade that referenced this pull request Oct 13, 2024
    ## [2.4.0](keyshade-xyz/keyshade@v2.3.0...v2.4.0) (2024-09-05)
    
    ### 🚀 Features
    
    * **api-client:** Create controller for Event module ([keyshade-xyz#399](keyshade-xyz#399)) ([122df35](keyshade-xyz@122df35))
    * **api-client:** Create controller for Integration module ([keyshade-xyz#397](keyshade-xyz#397)) ([697d38b](keyshade-xyz@697d38b))
    * **api-client:** Create controller for Project module ([keyshade-xyz#370](keyshade-xyz#370)) ([fa25866](keyshade-xyz@fa25866))
    * **api-client:** Create controller for Secret module ([keyshade-xyz#396](keyshade-xyz#396)) ([7e929c0](keyshade-xyz@7e929c0))
    * **api-client:** Create controller for Variable module ([keyshade-xyz#395](keyshade-xyz#395)) ([3e114d9](keyshade-xyz@3e114d9))
    * **api:** Add global search in workspace ([c49962b](keyshade-xyz@c49962b))
    * **api:** Add max page size ([keyshade-xyz#377](keyshade-xyz#377)) ([ed18eb0](keyshade-xyz@ed18eb0))
    * **cli:** Add functionality to operate on Environments ([keyshade-xyz#324](keyshade-xyz#324)) ([4c6f3f8](keyshade-xyz@4c6f3f8))
    * **cli:** Quit on decryption failure ([keyshade-xyz#381](keyshade-xyz#381)) ([1349d15](keyshade-xyz@1349d15))
    
    ### 🐛 Bug Fixes
    
    * **api-client:** Fixed broken export ([096df2c](keyshade-xyz@096df2c))
    * **api:** Add NotFound exception on passing an invalid roleId while inviting user in workspace ([keyshade-xyz#408](keyshade-xyz#408)) ([ab441db](keyshade-xyz@ab441db))
    * **cli:** Fixed missing module ([f7a091f](keyshade-xyz@f7a091f))
    * **platform:**  Build failure in platform ([keyshade-xyz#385](keyshade-xyz#385)) ([90dcb2c](keyshade-xyz@90dcb2c))
    
    ### 🔧 Miscellaneous Chores
    
    * Add api client build script and updated CI ([da0e27a](keyshade-xyz@da0e27a))
    * **api:** Reorganized import using path alias ([d5befd1](keyshade-xyz@d5befd1))
    * **ci:** Update CLI CI name ([8f4c456](keyshade-xyz@8f4c456))
    * **cli:** Add Zod validation to parseInput function ([keyshade-xyz#362](keyshade-xyz#362)) ([34e6c39](keyshade-xyz@34e6c39))
    * Fixed api client tests and rearranged controllers ([1307604](keyshade-xyz@1307604))
    * Housekeeping ([c5f1330](keyshade-xyz@c5f1330))
    * **platform:** Added strict null check ([072254f](keyshade-xyz@072254f))
    * **web:** Added strict null check ([7e12b47](keyshade-xyz@7e12b47))
    
    ### 🔨 Code Refactoring
    
    * **api:** Update logic for forking projects ([keyshade-xyz#398](keyshade-xyz#398)) ([4cf3838](keyshade-xyz@4cf3838))
    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.

    API: Update logic for forking projects
    2 participants