refactor(api)!: Refactor environment, secret and variable functionality

rajdip-b commented 3 months ago

User description

Description

This PR introduces a lot of breaking changes. Listing them down below:

The approval feature is now removed
Variable and secrets now store the revisions in a clustered manner, grouped by environments.
Environments don't have a default tag anymore

PR Type

Enhancement, Tests

Description

Removed approval-related code and logic across multiple services.
Refactored secret and variable services to handle multiple environments.
Simplified project creation, update, and deletion logic.
Updated tests to reflect changes in services and removed approval-related tests.
Simplified authority checks by removing approval-related logic.

Changes walkthrough 📝

Relevant files

Enhancement

secret.service.ts `Refactor secret service to support multiple environments and remove` `approval logic.` apps/api/src/secret/service/secret.service.ts Removed approval-related code. Refactored secret creation to handle multiple environments. Updated secret update logic to handle multiple environments. Simplified secret deletion logic.	+316/-587
variable.service.ts `Refactor variable service to support multiple environments and remove` `approval logic.` apps/api/src/variable/service/variable.service.ts Removed approval-related code. Refactored variable creation to handle multiple environments. Updated variable update logic to handle multiple environments. Simplified variable deletion logic.	+281/-528
project.service.ts `Refactor project service to remove approval logic and handle multiple` `environments.` apps/api/src/project/service/project.service.ts Removed approval-related code. Updated project creation to handle multiple environments without default tags. Simplified project update and deletion logic.	+197/-368
authority-checker.service.ts `Simplify authority checker service by removing approval-related` `checks.` apps/api/src/common/authority-checker.service.ts Removed approval-related checks. Simplified authority checks for projects, environments, variables, and secrets.	+23/-107
create.environment.ts `Simplify environment creation DTO by removing unnecessary validation.` apps/api/src/environment/dto/create.environment/create.environment.ts - Removed `IsBoolean` import and usage.	+1/-5

Tests

secret.e2e.spec.ts `Update secret service tests to reflect refactored logic and multiple` `environments.` apps/api/src/secret/secret.e2e.spec.ts Updated tests to reflect changes in secret service. Removed tests related to approval logic. Added tests for handling multiple environments.	+87/-354
variable.e2e.spec.ts `Update variable service tests to reflect refactored logic and multiple` `environments.` apps/api/src/variable/variable.e2e.spec.ts Updated tests to reflect changes in variable service. Removed tests related to approval logic. Added tests for handling multiple environments.	+64/-311

💡 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 3 months ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	5
🧪 Relevant tests	Yes
🔒 Security concerns	No
⚡ Key issues to review	Possible Bug: The refactoring removes the concept of default environments and approval features. Ensure that this change aligns with the intended use cases and does not affect existing functionalities unexpectedly.
	Data Integrity: The migration script associated with this PR should be carefully reviewed to ensure that it handles data correctly and preserves the integrity of existing data during the transition.
	Dependency Check: The removal of approval-related features might affect other parts of the system that depend on these functionalities. Verify that all dependencies are correctly updated or removed.

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

PR Code Suggestions ✨

Category	Suggestion	Score
Security	Ensure that the environment belongs to the project during variable creation ___ Implement a check to ensure that the `environmentId` provided in each entry of `dto.entries` actually belongs to the project associated with `projectId`. This prevents users from mistakenly or maliciously attempting to create variable versions in environments not associated with the given project. [apps/api/src/variable/service/variable.service.ts [62-69]](https://github.com/keyshade-xyz/keyshade/pull/270/files#diff-db1895137a9c036373abefb8c924150d4f2022fd6d4f5271cc6c95c443bf01cfR62-R69) ```diff const environmentIds = dto.entries.map((entry) => entry.environmentId) for (const environmentId of environmentIds) { + const environment = await this.prisma.environment.findUnique({ + where: { id: environmentId }, + select: { projectId: true } + }); + if (environment.projectId !== projectId) { + throw new ForbiddenException(`Environment ${environmentId} is not part of the project ${projectId}`); + } await this.authorityCheckerService.checkAuthorityOverEnvironment({ userId: user.id, entity: { id: environmentId }, authority: Authority.READ_ENVIRONMENT, prisma: this.prisma }) } ``` Suggestion importance[1-10]: 10 Why: This suggestion addresses a significant security concern by ensuring that environments are correctly associated with the project, preventing unauthorized access or manipulation.	10
Data integrity	Verify existence of workspace before project creation to maintain data integrity ___ Implement a check to ensure that the `workspaceId` provided exists in the database before proceeding with project creation. This will prevent the creation of projects in non-existent workspaces, thus maintaining data integrity. [apps/api/src/project/service/project.service.ts [47-48]](https://github.com/keyshade-xyz/keyshade/pull/270/files#diff-d85f483eff15c28a4b3e5f7a54c9b159c71003f8a95ef7a4be00eb80ec72b2faR47-R48) ```diff +const workspaceExists = await this.prisma.workspace.findUnique({ where: { id: workspaceId } }); +if (!workspaceExists) { + throw new Error('Workspace does not exist.'); +} const workspace = ... ``` Suggestion importance[1-10]: 10 Why: Implementing a check to ensure that the `workspaceId` exists in the database before proceeding with project creation is essential for maintaining data integrity. This prevents the creation of projects in non-existent workspaces, which could lead to data inconsistencies.	10
Possible issue	Add error handling to the `createSecret` method to improve robustness ___ Consider adding error handling for the `createSecret` method to manage cases where the secret creation fails due to database issues or other exceptions. This will improve the robustness of the method. [apps/api/src/secret/service/secret.service.ts [47-115]](https://github.com/keyshade-xyz/keyshade/pull/270/files#diff-dee38177617754972e3d3f727d7f1536566d7a784b7ffdda74aa97d6eec4cbc5R47-R115) ```diff -const secret = await this.prisma.secret.create({ - data: { - name: dto.name, - note: dto.note, - rotateAt: addHoursToDate(dto.rotateAfter), - versions: { - createMany: { - data: await Promise.all( - dto.entries.map(async (entry) => ({ - value: await encrypt(project.publicKey, entry.value), - version: 1, - createdById: user.id, - environmentId: entry.environmentId - })) - ) +let secret; +try { + secret = await this.prisma.secret.create({ + data: { + name: dto.name, + note: dto.note, + rotateAt: addHoursToDate(dto.rotateAfter), + versions: { + createMany: { + data: await Promise.all( + dto.entries.map(async (entry) => ({ + value: await encrypt(project.publicKey, entry.value), + version: 1, + createdById: user.id, + environmentId: entry.environmentId + })) + ) + } + }, + project: { + connect: { + id: projectId + } } }, - project: { - connect: { - id: projectId + include: { + project: { + select: { + workspaceId: true + } + }, + versions: { + select: { + environmentId: true, + value: true + } } } - }, - include: { - project: { - select: { - workspaceId: true - } - }, - versions: { - select: { - environmentId: true, - value: true - } - } - } -}) + }); +} catch (error) { + this.logger.error(`Failed to create secret: ${error}`); + throw new InternalServerErrorException(`Failed to create secret`); +} ``` Suggestion importance[1-10]: 9 Why: The suggestion correctly identifies a potential issue with the `createSecret` method where database or other exceptions could cause the method to fail without proper error handling. Adding a try-catch block and logging the error improves the robustness and maintainability of the code.	9
	Add error handling for key pair generation to improve method robustness ___ Consider adding error handling for the `createKeyPair()` function to manage potential failures in key generation. This will ensure the robustness of the `createProject` method by handling scenarios where key generation might fail. [apps/api/src/project/service/project.service.ts [62-63]](https://github.com/keyshade-xyz/keyshade/pull/270/files#diff-d85f483eff15c28a4b3e5f7a54c9b159c71003f8a95ef7a4be00eb80ec72b2faR62-R63) ```diff -const { publicKey, privateKey } = createKeyPair() +let publicKey, privateKey; +try { + ({ publicKey, privateKey } = createKeyPair()); +} catch (error) { + throw new Error('Failed to generate key pair: ' + error.message); +} ``` Suggestion importance[1-10]: 9 Why: Adding error handling for the `createKeyPair()` function is crucial for robustness. It ensures that any failure in key generation is properly managed, preventing potential runtime errors and improving the reliability of the `createProject` method.	9
	Add validation to check if `environment2.id` exists before using it ___ Consider checking if `environment2.id` exists before using it in the `entries` array for creating secrets. This ensures that the environment is valid and prevents potential runtime errors or misconfigurations. [apps/api/src/secret/secret.e2e.spec.ts [163-164]](https://github.com/keyshade-xyz/keyshade/pull/270/files#diff-598db59202ce7ecc8d2fb84528577afd14eac8275a30e174d704d6fc2b7f5c3bR163-R164) ```diff -entries: [ - { - environmentId: environment2.id, - value: 'Secret 1 value' - } -] +if (environment2 && environment2.id) { + entries: [ + { + environmentId: environment2.id, + value: 'Secret 1 value' + } + ] +} ``` Suggestion importance[1-10]: 8 Why: This suggestion adds a validation step to ensure `environment2.id` exists before using it, which can prevent potential runtime errors or misconfigurations. This is a good practice for improving code robustness.	8
	Check usage of `defaultWorkspace` before deletion to avoid potential issues ___ When deleting properties from an object, consider if other parts of the application might still be using them. If `defaultWorkspace` is used elsewhere, this operation might lead to undefined behavior or errors. [apps/api/src/secret/secret.e2e.spec.ts [105]](https://github.com/keyshade-xyz/keyshade/pull/270/files#diff-598db59202ce7ecc8d2fb84528577afd14eac8275a30e174d704d6fc2b7f5c3bR105-R105) ```diff -delete createUser1.defaultWorkspace +// Ensure no other parts of the application require `defaultWorkspace` before deleting +if (canDeleteDefaultWorkspace(createUser1)) { + delete createUser1.defaultWorkspace +} ``` Suggestion importance[1-10]: 6 Why: This suggestion ensures that `defaultWorkspace` is not used elsewhere before deletion, which can prevent undefined behavior or errors. It is a good practice but not crucial for the current context.	6
Robustness	Add error handling for the variable creation process ___ Consider adding error handling for the case where the variable creation fails due to database constraints or other issues. This will improve the robustness of the `createVariable` method. [apps/api/src/variable/service/variable.service.ts [73-87]](https://github.com/keyshade-xyz/keyshade/pull/270/files#diff-db1895137a9c036373abefb8c924150d4f2022fd6d4f5271cc6c95c443bf01cfR73-R87) ```diff -const variable = await this.prisma.variable.create({ - data: { - name: dto.name, - note: dto.note, - versions: { - createMany: { - data: dto.entries.map((entry) => ({ - value: entry.value, - createdById: user.id, - environmentId: entry.environmentId - })) - } - }, - project: { - connect: { - id: projectId +let variable; +try { + variable = await this.prisma.variable.create({ + data: { + name: dto.name, + note: dto.note, + versions: { + createMany: { + data: dto.entries.map((entry) => ({ + value: entry.value, + createdById: user.id, + environmentId: entry.environmentId + })) + } + }, + project: { + connect: { + id: projectId + } } } - } -}) + }); +} catch (error) { + throw new InternalServerErrorException(`Failed to create variable: ${error.message}`); +} ``` Suggestion importance[1-10]: 9 Why: Adding error handling for the variable creation process is crucial for robustness, ensuring that any issues during the database operation are properly caught and managed.	9
Robustness	Implement a rollback mechanism for update operations ___ To maintain consistency and ensure data integrity, consider implementing a rollback mechanism in the `updateVariable` method to handle any failures during the update operations. [apps/api/src/variable/service/variable.service.ts [230-232]](https://github.com/keyshade-xyz/keyshade/pull/270/files#diff-db1895137a9c036373abefb8c924150d4f2022fd6d4f5271cc6c95c443bf01cfR230-R232) ```diff -const tx = await this.prisma.$transaction(op) -const updatedVariable = tx[0] +let updatedVariable; +try { + const tx = await this.prisma.$transaction(op); + updatedVariable = tx[0]; +} catch (error) { + await this.prisma.$rollback(); + throw new InternalServerErrorException(`Update failed: ${error.message}`); +} ``` Suggestion importance[1-10]: 8 Why: Implementing a rollback mechanism enhances robustness by ensuring data integrity in case of failures during update operations, although the existing transaction mechanism already provides some level of atomicity.	8
Best practice	Implement error handling for the secret creation process ___ Add error handling for the `createSecret` method to manage exceptions or failed secret creation attempts effectively. [apps/api/src/secret/secret.e2e.spec.ts [155-169]](https://github.com/keyshade-xyz/keyshade/pull/270/files#diff-598db59202ce7ecc8d2fb84528577afd14eac8275a30e174d704d6fc2b7f5c3bR155-R169) ```diff -secret1 = (await secretService.createSecret( - user1, - { - name: 'Secret 1', - rotateAfter: '24', - note: 'Secret 1 note', - entries: [ - { - environmentId: environment2.id, - value: 'Secret 1 value' - } - ] - }, - project1.id -)) as Secret +try { + secret1 = (await secretService.createSecret( + user1, + { + name: 'Secret 1', + rotateAfter: '24', + note: 'Secret 1 note', + entries: [ + { + environmentId: environment2.id, + value: 'Secret 1 value' + } + ] + }, + project1.id + )) as Secret +} catch (error) { + console.error('Failed to create secret:', error); +} ``` Suggestion importance[1-10]: 9 Why: Adding error handling for the `createSecret` method is a best practice that can help manage exceptions or failed attempts effectively, improving the robustness and reliability of the code.	9
Best practice	Use specific types instead of `any` to enhance type safety and maintainability ___ Replace the generic type `any` with a more specific type or interface for the `data` object in the `createProject` method to improve type safety and code maintainability. [apps/api/src/project/service/project.service.ts [66-67]](https://github.com/keyshade-xyz/keyshade/pull/270/files#diff-d85f483eff15c28a4b3e5f7a54c9b159c71003f8a95ef7a4be00eb80ec72b2faR66-R67) ```diff -const data: any = { +interface ProjectData { + name: string; + description: string; + storePrivateKey: boolean; + publicKey: string; + accessLevel: string; +} +const data: ProjectData = { name: dto.name, description: dto.description, ... } ``` Suggestion importance[1-10]: 8 Why: Replacing the generic type `any` with a more specific type or interface for the `data` object improves type safety and code maintainability. This change helps catch type-related errors at compile time and makes the code more readable and maintainable.	8
Data validation	Validate `dto` properties to ensure all required fields are provided and valid ___ Add validation for the `dto` object properties in the `createProject` method to ensure that all required fields are provided and valid before proceeding with project creation. [apps/api/src/project/service/project.service.ts [66-67]](https://github.com/keyshade-xyz/keyshade/pull/270/files#diff-d85f483eff15c28a4b3e5f7a54c9b159c71003f8a95ef7a4be00eb80ec72b2faR66-R67) ```diff +if (!dto.name \|\| !dto.description) { + throw new Error('Missing required project details.'); +} const data: any = { name: dto.name, description: dto.description, ... } ``` Suggestion importance[1-10]: 9 Why: Adding validation for the `dto` object properties ensures that all required fields are provided and valid before proceeding with project creation. This prevents potential runtime errors and ensures that the project creation process is robust and reliable.	9
Performance	Optimize environment authority checks by using asynchronous batch processing ___ Optimize the loop that checks for authority over environments by batching the requests or restructuring the logic to reduce the number of await calls inside the loop, which can improve performance. [apps/api/src/variable/service/variable.service.ts [154-161]](https://github.com/keyshade-xyz/keyshade/pull/270/files#diff-db1895137a9c036373abefb8c924150d4f2022fd6d4f5271cc6c95c443bf01cfR154-R161) ```diff -for (const environmentId of environmentIds) { - await this.authorityCheckerService.checkAuthorityOverEnvironment({ +await Promise.all(environmentIds.map(environmentId => + this.authorityCheckerService.checkAuthorityOverEnvironment({ userId: user.id, entity: { id: environmentId }, authority: Authority.READ_ENVIRONMENT, prisma: this.prisma }) -} +)); ``` Suggestion importance[1-10]: 7 Why: This optimization can improve performance by reducing the number of await calls inside the loop, but it is a minor enhancement compared to the other suggestions.	7
Enhancement	Clarify the time unit in the `rotateAfter` property ___ Ensure that the `rotateAfter` property is correctly formatted as a time unit (e.g., hours, days). Currently, it's just a string '24', which might be ambiguous or incorrect depending on the expected format. [apps/api/src/secret/secret.e2e.spec.ts [159]](https://github.com/keyshade-xyz/keyshade/pull/270/files#diff-598db59202ce7ecc8d2fb84528577afd14eac8275a30e174d704d6fc2b7f5c3bR159-R159) ```diff -rotateAfter: '24' +rotateAfter: '24h' # Assuming 'h' stands for hours ``` Suggestion importance[1-10]: 7 Why: Clarifying the time unit in the `rotateAfter` property improves code readability and prevents ambiguity. However, it is a minor enhancement rather than a critical fix.	7

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 92.95775% with 20 lines in your changes missing coverage. Please review.

Project coverage is 88.33%. Comparing base (ce50743) to head (2fc5dcf). Report is 28 commits behind head on develop.

Files	Patch %	Lines
apps/api/src/secret/service/secret.service.ts	90.80%	8 Missing :warning:
...rc/variable/dto/create.variable/create.variable.ts	54.54%	5 Missing :warning:
apps/api/src/variable/service/variable.service.ts	94.44%	4 Missing :warning:
.../api/src/secret/dto/create.secret/create.secret.ts	70.00%	3 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## develop #270 +/- ## =========================================== - Coverage 91.71% 88.33% -3.39% =========================================== Files 111 106 -5 Lines 2510 2271 -239 Branches 469 351 -118 =========================================== - Hits 2302 2006 -296 - Misses 208 265 +57 ``` | [Flag](https://app.codecov.io/gh/keyshade-xyz/keyshade/pull/270/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keyshade-xyz) | Coverage Δ | | |---|---|---| | [api-e2e-tests](https://app.codecov.io/gh/keyshade-xyz/keyshade/pull/270/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keyshade-xyz) | `88.33% <92.95%> (-3.39%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keyshade-xyz#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.