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
118 stars 55 forks source link

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

Open anudeeps352 opened 5 days ago

anudeeps352 commented 5 days ago

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

If changes are made in the code:

Documentation Update

PR Type

Enhancement, Tests

Description

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

    codiumai-pr-agent[bot] commented 5 days ago

    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.
    codiumai-pr-agent[bot] commented 5 days ago

    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]](https://github.com/keyshade-xyz/keyshade/pull/303/files#diff-dee38177617754972e3d3f727d7f1536566d7a784b7ffdda74aa97d6eec4cbc5R499-R511) ```diff -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]](https://github.com/keyshade-xyz/keyshade/pull/303/files#diff-504a3762c691dc3814c053b3815ff9b1f1a8eb54cde844d882d9ea06779eca84R110-R111) ```diff -@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]](https://github.com/keyshade-xyz/keyshade/pull/303/files#diff-dee38177617754972e3d3f727d7f1536566d7a784b7ffdda74aa97d6eec4cbc5R514-R517) ```diff +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]](https://github.com/keyshade-xyz/keyshade/pull/303/files#diff-504a3762c691dc3814c053b3815ff9b1f1a8eb54cde844d882d9ea06779eca84R106-R111) ```diff -@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