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

feat(api): Create endpoint for fetching all revisions of a secret #303

Merged
merged 5 commits into from
Jul 29, 2024

Conversation

anudeeps352
Copy link
Contributor

@anudeeps352 anudeeps352 commented Jun 27, 2024

User description

Description

Create endpoint for fetching all revisions of a secret

GET /api/secret/:secretId/revisions/:environmentId 

Fixes #272

Dependencies

Mention any dependencies/packages used

Future Improvements

Mention any improvements to be done in future related to any file/feature

Mentions

Mention and tag the people

Screenshots of relevant screens

Add screenshots of relevant screens

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

Enhancement, Tests


Description

  • Added a new endpoint GET /api/secret/:secretId/revisions/:environmentId to fetch all revisions of a secret.
  • Implemented the getRevisionsOfSecret method in the SecretController class.
  • Added getRevisionsOfSecret method to the SecretService class, including authority checks and retrieval logic.
  • Added end-to-end tests for the new endpoint, covering multiple scenarios including fetching multiple revisions, no revisions, non-existent secret, non-existent environment, and unauthorized access.

Changes walkthrough 📝

Relevant files
Enhancement
secret.controller.ts
Add endpoint to fetch all revisions of a secret                   

apps/api/src/secret/controller/secret.controller.ts

  • Added a new endpoint GET
    /api/secret/:secretId/revisions/:environmentId to fetch all revisions
    of a secret.
  • Implemented the getRevisionsOfSecret method in the SecretController
    class.
  • +14/-0   
    secret.service.ts
    Implement service method to fetch secret revisions             

    apps/api/src/secret/service/secret.service.ts

  • Added getRevisionsOfSecret method to the SecretService class.
  • Implemented authority checks and retrieval of secret revisions.
  • +29/-0   
    Tests
    secret.e2e.spec.ts
    Add tests for fetching all revisions of a secret endpoint

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

  • Added end-to-end tests for the new endpoint to fetch all revisions of
    a secret.
  • Included tests for various scenarios: fetching multiple revisions, no
    revisions, non-existent secret, non-existent environment, and
    unauthorized access.
  • +104/-0 

    💡 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 [1-5] 3
    🧪 Relevant tests Yes
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    Ensure that the getRevisionsOfSecret method in SecretService handles the case where the secret or environment does not exist before making database queries. This could prevent unnecessary database calls and improve error handling.
    Error Handling:
    Verify that the error messages returned are consistent and informative across different failure scenarios in the endpoint.

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Jun 27, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling to improve method robustness and error reporting

    Implement error handling for the getRevisionsOfSecret method to manage cases where the
    authority checks fail or the database operations throw an exception. This will improve the
    robustness of the method and provide clearer error messages to the client.

    apps/api/src/secret/service/secret.service.ts [499-511]

    -await this.authorityCheckerService.checkAuthorityOverSecret({
    -  userId: user.id,
    -  entity: { id: secretId },
    -  authority: Authority.READ_SECRET,
    -  prisma: this.prisma
    -})
    -await this.authorityCheckerService.checkAuthorityOverEnvironment({
    -  userId: user.id,
    -  entity: { id: environmentId },
    -  authority: Authority.READ_ENVIRONMENT,
    -  prisma: this.prisma
    -})
    +try {
    +  await this.authorityCheckerService.checkAuthorityOverSecret({
    +    userId: user.id,
    +    entity: { id: secretId },
    +    authority: Authority.READ_SECRET,
    +    prisma: this.prisma
    +  })
    +  await this.authorityCheckerService.checkAuthorityOverEnvironment({
    +    userId: user.id,
    +    entity: { id: environmentId },
    +    authority: Authority.READ_ENVIRONMENT,
    +    prisma: this.prisma
    +  })
    +} catch (error) {
    +  throw new HttpException('Failed to verify authorities or fetch data', HttpStatus.INTERNAL_SERVER_ERROR);
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: Implementing error handling for authority checks and database operations enhances the robustness of the method and provides clearer error messages to the client, which is crucial for reliability and user experience.

    10
    Enhancement
    Add parameter validation for secretId and environmentId to ensure they are well-formed UUIDs

    Consider validating the parameters secretId and environmentId to ensure they are not empty
    or malformed before proceeding with the database operations. This can help prevent
    unnecessary database queries and potential errors when invalid IDs are provided.

    apps/api/src/secret/controller/secret.controller.ts [110-111]

    -@Param('secretId') secretId: string,
    -@Param('environmentId') environmentId: string
    +@Param('secretId', ParseUUIDPipe) secretId: string,
    +@Param('environmentId', ParseUUIDPipe) environmentId: string
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding validation for secretId and environmentId ensures that only well-formed UUIDs are processed, preventing unnecessary database queries and potential errors. This is a significant improvement for robustness and security.

    9
    Performance
    Suggest adding a database index on secretId and environmentId to improve query performance

    To optimize the database query when fetching secret revisions, consider including an index
    on the secretId and environmentId fields in the secretVersion table. This can
    significantly improve the performance of the query, especially when dealing with a large
    number of records.

    apps/api/src/secret/service/secret.service.ts [514-517]

    +const revisions = await this.prisma.secretVersion.findMany({
    +  where: {
    +    secretId: secretId,
    +    environmentId: environmentId
    +  }
    +})
     
    -
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding an index on secretId and environmentId can significantly improve the performance of database queries, especially with large datasets. This is a valuable optimization for performance.

    8
    Security
    Implement a middleware to check API key authorities to centralize security checks

    To ensure that the API key provided has the necessary authorities, consider implementing a
    middleware that checks the API key before reaching the controller method. This can help
    centralize security checks and reduce redundancy across different endpoints.

    apps/api/src/secret/controller/secret.controller.ts [106-111]

    -@RequiredApiKeyAuthorities(Authority.READ_SECRET)
    +@UseGuards(ApiKeyAuthGuard)
     async getRevisionsOfSecret(
       @CurrentUser() user: User,
       @Param('secretId') secretId: string,
       @Param('environmentId') environmentId: string
     )
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using a middleware to check API key authorities can centralize security checks and reduce redundancy, improving maintainability. However, the current decorator approach is also valid, so this is more of a structural improvement.

    7

    @rajdip-b rajdip-b added the foss hack Clustering all the curated issues for Foss Hack 2024 label Jun 28, 2024
    @anudeeps352
    Copy link
    Contributor Author

    Hey btw since this pr won't be considered in foss hack,can this be merged ??

    @rajdip-b
    Copy link
    Member

    I'm totally fine if you are!

    apps/api/src/secret/service/secret.service.ts Outdated Show resolved Hide resolved
    apps/api/src/secret/service/secret.service.ts Outdated Show resolved Hide resolved
    apps/api/src/secret/service/secret.service.ts Outdated Show resolved Hide resolved
    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.

    The code looks good. Just fix the tests and we can merge it.

    @anudeeps352
    Copy link
    Contributor Author

    Hey I think this one would work fine

    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.

    Looks so clean! LGTM.

    @rajdip-b rajdip-b merged commit 1df4469 into keyshade-xyz:develop Jul 29, 2024
    4 checks passed
    rajdip-b pushed a commit that referenced this pull request Jul 29, 2024
    ## [2.3.0](v2.2.0...v2.3.0) (2024-07-29)
    
    ### 🚀 Features
    
    * **api:** Add pagination metadata to Environment module ([#382](#382)) ([9baa344](9baa344))
    * **api:** Add pagination metadata to Event module ([#394](#394)) ([60010b4](60010b4))
    * **api:** Add pagination metadata to Integration module ([#391](#391)) ([0372e36](0372e36))
    * **api:** Add pagination metadata to Project module ([#393](#393)) ([bc274fd](bc274fd))
    * **api:** Add pagination metadata to Secret module ([#389](#389)) ([c4cc667](c4cc667))
    * **api:** Add pagination metadata to Variable module ([#390](#390)) ([be6aabf](be6aabf))
    * **api:** Add pagination metadata to Workspace module  ([#387](#387)) ([a08c924](a08c924))
    * **api:** Add pagination metadata to Workspace Role module ([#388](#388)) ([d8e8f49](d8e8f49))
    * **api:** Create a paginate method ([#379](#379)) ([09576f1](09576f1))
    * **api:** Create endpoint for fetching all revisions of a [secure] ([#303](#303)) ([de2b602](de2b602))
    * **api:** Create endpoint for fetching all revisions of a variable ([#304](#304)) ([9abddc1](9abddc1))
    * **cli:** Improved the DX for list profile ([#334](#334)) ([6bff496](6bff496))
    * **platform:** Add warning sonner toast for invalid otp ([#335](#335)) ([21513f5](21513f5))
    
    ### 🐛 Bug Fixes
    
    * **cli:** Added parent directory check ([#359](#359)) ([538ea7f](538ea7f))
    * **platform:** Platform types fixes ([#374](#374)) ([8e9d9ff](8e9d9ff))
    
    ### 📚 Documentation
    
    * Added docker details in setting-things-up.md ([#358](#358)) ([ed5093a](ed5093a))
    * Update postman workspace link ([d6aba27](d6aba27))
    * Updated env and cli docs ([1213d2a](1213d2a))
    
    ### 🔧 Miscellaneous Chores
    
    * Added next backend url in .env.example ([5695254](5695254))
    * **api-client:** Added pagination structure ([a70e957](a70e957))
    * **api-client:** Fixed test script ([ad70819](ad70819))
    * **api-client:** Removed try-catch from tests in environment ([a64e48c](a64e48c))
    * **api:** Add user cache for optimization ([#386](#386)) ([8d730b5](8d730b5))
    * **api:** Alter cache rehydration interval ([f5f9eec](f5f9eec))
    * **api:** Fixed naming error in variable controller ([0c5a380](0c5a380))
    * **api:** Improve handling of edge cases for paginate module ([#402](#402)) ([8591487](8591487))
    * **api:** Minor updates to user service ([249d778](249d778))
    * **api:** Skip workspace creation when user is admin ([#376](#376)) ([13f6c59](13f6c59))
    * **ci:** Add docker check   ([#383](#383)) ([3119001](3119001))
    * **ci:** Add names to CI files ([1a7e5f6](1a7e5f6))
    * **ci:** Add validate CLI pipeline ([#373](#373)) ([a91df6c](a91df6c))
    * **ci:** Adding validate pipeline ([#372](#372)) ([23cf3b3](23cf3b3))
    * **ci:** Disabled platform and api deployments ([74d601a](74d601a))
    * **ci:** Fixed deployment scripts ([12e35db](12e35db))
    * **ci:** Fixed platform script ([d783f2a](d783f2a))
    * **CI:** Include migration deployment in API deploy pipeline ([dbd5222](dbd5222))
    * **CI:** Separated deployment and docker build jobs ([090e193](090e193))
    * **CI:** Setup inter-job dependency ([1756727](1756727))
    * **ci:** Update auto-assign.yaml ([#375](#375)) ([91e0ec1](91e0ec1))
    * **cli:** Changed objects to classes ([#306](#306)) ([c83f2db](c83f2db))
    * Removed Minio config ([8feb83a](8feb83a))
    * Updated deployment scripts and added health check in platform ([fcc1c3f](fcc1c3f))
    
    ### 🔨 Code Refactoring
    
    * **api:** Updated path of some endpoints in project controller ([9502678](9502678))
    * **api:** Updated Redis provider ([33491a1](33491a1))
    rajdip-b pushed a commit that referenced this pull request Jul 29, 2024
    ## [2.3.0](v2.2.0...v2.3.0) (2024-07-29)
    
    ### 🚀 Features
    
    * **api:** Add pagination metadata to Environment module ([#382](#382)) ([9baa344](9baa344))
    * **api:** Add pagination metadata to Event module ([#394](#394)) ([60010b4](60010b4))
    * **api:** Add pagination metadata to Integration module ([#391](#391)) ([0372e36](0372e36))
    * **api:** Add pagination metadata to Project module ([#393](#393)) ([bc274fd](bc274fd))
    * **api:** Add pagination metadata to Secret module ([#389](#389)) ([c4cc667](c4cc667))
    * **api:** Add pagination metadata to Variable module ([#390](#390)) ([be6aabf](be6aabf))
    * **api:** Add pagination metadata to Workspace module  ([#387](#387)) ([a08c924](a08c924))
    * **api:** Add pagination metadata to Workspace Role module ([#388](#388)) ([d8e8f49](d8e8f49))
    * **api:** Create a paginate method ([#379](#379)) ([09576f1](09576f1))
    * **api:** Create endpoint for fetching all revisions of a [secure] ([#303](#303)) ([de2b602](de2b602))
    * **api:** Create endpoint for fetching all revisions of a variable ([#304](#304)) ([9abddc1](9abddc1))
    * **cli:** Improved the DX for list profile ([#334](#334)) ([6bff496](6bff496))
    * **platform:** Add warning sonner toast for invalid otp ([#335](#335)) ([21513f5](21513f5))
    
    ### 🐛 Bug Fixes
    
    * **cli:** Added parent directory check ([#359](#359)) ([538ea7f](538ea7f))
    * **platform:** Platform types fixes ([#374](#374)) ([8e9d9ff](8e9d9ff))
    
    ### 📚 Documentation
    
    * Added docker details in setting-things-up.md ([#358](#358)) ([ed5093a](ed5093a))
    * Update postman workspace link ([d6aba27](d6aba27))
    * Updated env and cli docs ([1213d2a](1213d2a))
    
    ### 🔧 Miscellaneous Chores
    
    * Added next backend url in .env.example ([5695254](5695254))
    * **api-client:** Added pagination structure ([a70e957](a70e957))
    * **api-client:** Fixed test script ([ad70819](ad70819))
    * **api-client:** Removed try-catch from tests in environment ([a64e48c](a64e48c))
    * **api:** Add user cache for optimization ([#386](#386)) ([8d730b5](8d730b5))
    * **api:** Alter cache rehydration interval ([f5f9eec](f5f9eec))
    * **api:** Fixed naming error in variable controller ([0c5a380](0c5a380))
    * **api:** Improve handling of edge cases for paginate module ([#402](#402)) ([8591487](8591487))
    * **api:** Minor updates to user service ([249d778](249d778))
    * **api:** Skip workspace creation when user is admin ([#376](#376)) ([13f6c59](13f6c59))
    * **ci:** Add docker check   ([#383](#383)) ([3119001](3119001))
    * **ci:** Add names to CI files ([1a7e5f6](1a7e5f6))
    * **ci:** Add validate CLI pipeline ([#373](#373)) ([a91df6c](a91df6c))
    * **ci:** Adding validate pipeline ([#372](#372)) ([23cf3b3](23cf3b3))
    * **ci:** Disabled platform and api deployments ([74d601a](74d601a))
    * **ci:** Fixed deployment scripts ([12e35db](12e35db))
    * **ci:** Fixed platform script ([d783f2a](d783f2a))
    * **CI:** Include migration deployment in API deploy pipeline ([dbd5222](dbd5222))
    * **CI:** Separated deployment and docker build jobs ([090e193](090e193))
    * **CI:** Setup inter-job dependency ([1756727](1756727))
    * **ci:** Update auto-assign.yaml ([#375](#375)) ([91e0ec1](91e0ec1))
    * **cli:** Changed objects to classes ([#306](#306)) ([c83f2db](c83f2db))
    * Removed Minio config ([8feb83a](8feb83a))
    * Updated deployment scripts and added health check in platform ([fcc1c3f](fcc1c3f))
    
    ### 🔨 Code Refactoring
    
    * **api:** Updated path of some endpoints in project controller ([9502678](9502678))
    * **api:** Updated Redis provider ([33491a1](33491a1))
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    enhancement foss hack Clustering all the curated issues for Foss Hack 2024 Review effort [1-5]: 3 Tests
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    API: Create endpoint for fetching all revisions of a secret
    2 participants