chore(API): Refactor authority check functions in API

[szabodaniel995]

User description

Description

Refactored the authority checking functionalities into a service
Renamed the lookup functions adhering to the requested new names
Annotated the input of the lookup functions with a common interface
Added requested new lookup functionality

Fixes #180

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.

---

Type

enhancement, bug_fix

---

Description

• Introduced AuthorityCheckerService to centralize authority checks across various services.
• Refactored ApprovalService, EnvironmentService, ProjectService, SecretService, VariableService, and WorkspaceService to use the new AuthorityCheckerService.
• Removed old authority check functions, replacing them with calls to AuthorityCheckerService.
• Updated CommonModule to include AuthorityCheckerService in its providers and exports.

---

Changes walkthrough

	Relevant files
Enhancement	approval.service.ts Refactor Approval Service to Use AuthorityCheckerService  apps/api/src/approval/service/approval.service.ts Refactored authority checks to use AuthorityCheckerService instead of standalone functions. Updated method calls to use new service methods for authority validation. +16/-14  authority-checker.service.ts Add AuthorityCheckerService for Centralized Authority Checks apps/api/src/common/authority-checker.service.ts Introduced a new service AuthorityCheckerService to handle authority checks. Implemented methods to validate user authority over workspaces, projects, environments, variables, and secrets. +380/-0  environment.service.ts Refactor Environment Service to Use AuthorityCheckerService apps/api/src/environment/service/environment.service.ts Refactored to use AuthorityCheckerService for authority checks. Removed old authority check functions and replaced them with service method calls. +39/-33  project.service.ts Integrate AuthorityCheckerService in Project Service          apps/api/src/project/service/project.service.ts Integrated AuthorityCheckerService for authority validations. Removed old project authority check functions. +39/-33  secret.service.ts Utilize AuthorityCheckerService in Secret Service                apps/api/src/secret/service/secret.service.ts Utilized AuthorityCheckerService for authority checks in secret management. Removed old secret authority check functions. +61/-58  variable.service.ts Adopt AuthorityCheckerService in Variable Service                apps/api/src/variable/service/variable.service.ts Adopted AuthorityCheckerService for authority checks in variable management. Removed old variable authority check functions. +64/-58  workspace.service.ts Integrate AuthorityCheckerService in Workspace Service      apps/api/src/workspace/service/workspace.service.ts Integrated AuthorityCheckerService for workspace-related authority checks. Removed old workspace authority check functions. +86/-74
Configuration changes	common.module.ts Update CommonModule to Include AuthorityCheckerService      apps/api/src/common/common.module.ts Added AuthorityCheckerService to the providers and exports of the CommonModule. +3/-2

---

✨ 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]

PR Description updated to latest commit (2935c5b)

[codiumai-pr-agent-free]

PR Review

⏱️ Estimated effort to review [1-5]	4, because the PR involves significant refactoring and centralization of authority checking functions across multiple services. It requires a thorough understanding of the existing codebase and the changes to ensure that the new implementation correctly handles all the authority checks without introducing regressions or security issues.
🧪 Relevant tests	Yes
🔍 Possible issues	Possible Bug: The refactoring introduces a centralized AuthorityCheckerService which replaces multiple existing functions. If not all use cases are covered by the new service, it could lead to authorization bugs.
	Performance Concern: The centralized authority checks could introduce a single point of failure and potential performance bottlenecks if not properly optimized, especially in high-load environments.
🔒 Security concerns	No

---

✨ Review tool usage guide:

---

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

• When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

• With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

[codiumai-pr-agent-free]

PR Code Suggestions

Category	Suggestions
Enhancement	Add error handling for database access in the checkAuthorityOverWorkspace function. Consider adding error handling for the findUnique and findFirst methods in the checkAuthorityOverWorkspace function to handle potential database errors more gracefully. apps/api/src/common/authority-checker.service.ts [36-40] -workspace = await prisma.workspace.findUnique({ - where: { - id: entity.id +try { + workspace = await prisma.workspace.findUnique({ + where: { + id: entity.id + } + }); + if (!workspace) { + throw new NotFoundException(`Workspace with id ${entity.id} not found`); } -}) +} catch (error) { + throw new InternalServerErrorException('Database access error'); +}
	Improve the exception message for better debugging and user feedback. Use a more specific exception message in checkAuthorityOverWorkspace when the workspace is not found, including both id and name when available. apps/api/src/common/authority-checker.service.ts [54] -throw new NotFoundException(`Workspace with id ${entity.id} not found`) +throw new NotFoundException(`Workspace not found with id: ${entity.id} or name: ${entity.name}`)
	Refactor authority checks into a single reusable method to reduce code duplication. Refactor the repeated code for authority checks into a separate method within the SecretService class. This method could take parameters like entityId, authorityType, and entityType to generalize the authority check process, reducing code duplication. apps/api/src/secret/service/secret.service.ts [67-84] -const project = - await this.authorityCheckerService.checkAuthorityOverProject({ +const project = await this.checkAuthority('project', projectId, Authority.CREATE_SECRET); +... +const environment = await this.checkAuthority('environment', environmentId, Authority.READ_ENVIRONMENT); +... +private async checkAuthority(entityType: string, entityId: string, authorityType: Authority): Promise<any> { + return await this.authorityCheckerService[`checkAuthorityOver${entityType}`]({ userId: user.id, - entity: { id: projectId }, - authority: Authority.CREATE_SECRET, + entity: { id: entityId }, + authority: authorityType, prisma: this.prisma }); -... -const environment = - await this.authorityCheckerService.checkAuthorityOverEnvironment({ - userId: user.id, - entity: { id: environmentId }, - authority: Authority.READ_ENVIRONMENT, - prisma: this.prisma - }); +}
Maintainability	Refactor permission checks into a separate method to reduce code duplication. Refactor the repeated code for checking permissions in checkAuthorityOverWorkspace into a separate method to improve code maintainability and reduce duplication. apps/api/src/common/authority-checker.service.ts [64-70] -if ( - !permittedAuthorities.has(authority) && - !permittedAuthorities.has(Authority.WORKSPACE_ADMIN) -) { - throw new UnauthorizedException( - `User ${userId} does not have the required authorities to perform the action` - ); -} +this.verifyPermissions(permittedAuthorities, authority, userId);
	Use destructuring for the user object to improve code clarity. To improve code readability and maintainability, consider destructuring the user object to directly access the id property when it is used multiple times. apps/api/src/secret/service/secret.service.ts [69] -userId: user.id, +const { id: userId } = user; +... +userId: userId,
Bug	Add null checks for entity to prevent potential runtime errors. To prevent potential null reference errors, add null checks for entity before accessing entity.id or entity.name in the checkAuthorityOverWorkspace function. apps/api/src/common/authority-checker.service.ts [35-39] -if (entity?.id) { +if (entity && entity.id) { workspace = await prisma.workspace.findUnique({ where: { id: entity.id } }); }
	Add a null check for the project variable after its asynchronous assignment. To ensure that the project variable is always defined before it's used, especially in asynchronous operations, consider adding a null check right after its assignment. This can prevent runtime errors in subsequent code that assumes the project exists. apps/api/src/secret/service/secret.service.ts [67-73] const project = await this.authorityCheckerService.checkAuthorityOverProject({ userId: user.id, entity: { id: projectId }, authority: Authority.CREATE_SECRET, prisma: this.prisma }); +if (!project) { + throw new Error('Project not found or access denied'); +}
Performance	Combine conditions into a single query to optimize database access. Optimize the database query in checkAuthorityOverWorkspace by combining the conditions for id and name into a single query using OR logic, reducing the need for separate queries. apps/api/src/common/authority-checker.service.ts [35-46] -if (entity?.id) { - workspace = await prisma.workspace.findUnique({ - where: { - id: entity.id - } - }); -} else { - workspace = await prisma.workspace.findFirst({ - where: { - name: entity?.name - } - }); -} +workspace = await prisma.workspace.findFirst({ + where: { + OR: [ + { id: entity?.id }, + { name: entity?.name } + ] + } +});
Best practice	Add error handling for asynchronous authority check operations. Consider using a try-catch block around the asynchronous call to checkAuthorityOverProject to handle potential exceptions, such as network issues or database errors, which could otherwise lead to unhandled promise rejections. apps/api/src/secret/service/secret.service.ts [67-73] -const project = - await this.authorityCheckerService.checkAuthorityOverProject({ +let project; +try { + project = await this.authorityCheckerService.checkAuthorityOverProject({ userId: user.id, entity: { id: projectId }, authority: Authority.CREATE_SECRET, prisma: this.prisma - }) + }); +} catch (error) { + // Handle error appropriately + console.error('Failed to check authority over project', error); +}
	Add error handling for the getDefaultEnvironmentOfProject call to manage potential failures gracefully. To avoid potential issues with unhandled promise rejections, consider adding error handling for the getDefaultEnvironmentOfProject call. This function is likely to interact with external systems or databases, which might fail. apps/api/src/secret/service/secret.service.ts [87] -environment = await getDefaultEnvironmentOfProject(projectId, this.prisma) +try { + environment = await getDefaultEnvironmentOfProject(projectId, this.prisma); +} catch (error) { + console.error('Failed to get default environment:', error); + // Handle error appropriately +}
	Make the authorityCheckerService property immutable by adding the readonly modifier. Consider using TypeScript's readonly modifier for authorityCheckerService to prevent reassignment and ensure the service's immutability within the class. apps/api/src/environment/service/environment.service.ts [31] -public authorityCheckerService: AuthorityCheckerService +public readonly authorityCheckerService: AuthorityCheckerService
	Enhance encapsulation by changing the authorityCheckerService access modifier to private. To ensure encapsulation and data integrity, change the access modifier of authorityCheckerService from public to private since it does not seem to be used outside of this class. apps/api/src/socket/change-notifier.socket.ts [54] -public authorityCheckerService: AuthorityCheckerService +private authorityCheckerService: AuthorityCheckerService
	Improve class encapsulation by setting authorityCheckerService to private. It's a good practice to ensure that all injected services in a constructor are either private or protected unless they need to be exposed, which enhances encapsulation. Change authorityCheckerService to private. apps/api/src/workspace-role/service/workspace-role.service.ts [32] -public authorityCheckerService: AuthorityCheckerService +private authorityCheckerService: AuthorityCheckerService
	Ensure the immutability of authorityCheckerService by using the readonly modifier. To maintain consistency and ensure that services are not reassigned after their initial assignment, add the readonly modifier to authorityCheckerService. apps/api/src/event/service/event.service.ts [10] -public authorityCheckerService: AuthorityCheckerService +public readonly authorityCheckerService: AuthorityCheckerService
	Change authorityCheckerService to private to enhance data encapsulation. For better encapsulation and to prevent external modifications, consider changing authorityCheckerService from public to private since it does not appear to be used outside of this class. apps/api/src/approval/service/approval.service.ts [47] -public authorityCheckerService: AuthorityCheckerService +private authorityCheckerService: AuthorityCheckerService
	Mock the AuthorityCheckerService in the test setup to ensure isolation and reliability of the tests. Ensure that the AuthorityCheckerService is mocked in the testing module to prevent actual authority checks during testing, which can lead to unpredictable test outcomes and dependencies on external factors. apps/api/src/environment/controller/environment.controller.spec.ts [14] -providers: [EnvironmentService, PrismaService, AuthorityCheckerService] +providers: [EnvironmentService, PrismaService, { provide: AuthorityCheckerService, useValue: mockDeep<AuthorityCheckerService>() }]

---

✨ Improve tool usage guide:

---

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

• When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

• With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.

[rajdip-b]

I'll review this as soon as I can!

[rajdip-b]

Went through the PR, had a few suggestions that I have listed on the lines itself. Had a few general suggestions which I'm listing in here.

• I feel we should use the existing file-based approach rather than class. This is because the code is maintainable that way.
• We will need to add the members check that to each and every entity - secret, variable, environment, project and workspace. Mentioned the details in the review.
• For entities like secret, variable, environment and project that do not have a members attribute, we will need to drill into the parent. Eg: secret will have secret.environment.project.workspace.

[szabodaniel995]

Went through the PR, had a few suggestions that I have listed on the lines itself. Had a few general suggestions which I'm listing in here.

• I feel we should use the existing file-based approach rather than class. This is because the code is maintainable that way.
• We will need to add the members check that to each and every entity - secret, variable, environment, project and workspace. Mentioned the details in the review.
• For entities like secret, variable, environment and project that do not have a members attribute, we will need to drill into the parent. Eg: secret will have secret.environment.project.workspace.

Thank you for the review

• Resolved most
• See my comment on your last suggestion

I'm not sure the existing tests cover example cases for the results of these queries. It might be a good idea to write some e2e tests simulating real business use cases

[rajdip-b]

Agreed on your suggestions. We do need to add a few tests to cover these scenarios. But there are two blockers:

• Our test suites need to be refactored a bit.
• It falls out of the scope of this PR.

I will be happy to open up another issue in case you feel like fixing it. But that might take some time since I need to refactor the tests.

And, you will still need to make a few updates to the code!

• you can move the custom logger that you implemented into the common module.
• if not already present, add a @ Global() to the common module to export it's providers globally
• Remove all provider imports(if any) that you have made for authority checker and logger.
• Add readonly to all occurrences of private authorityCheckerservice

[szabodaniel995]

Agreed on your suggestions. We do need to add a few tests to cover these scenarios. But there are two blockers:

• Our test suites need to be refactored a bit.
• It falls out of the scope of this PR.

I will be happy to open up another issue in case you feel like fixing it. But that might take some time since I need to refactor the tests.

And, you will still need to make a few updates to the code!

• you can move the custom logger that you implemented into the common module.
• if not already present, add a @ Global() to the common module to export it's providers globally
• Remove all provider imports(if any) that you have made for authority checker and logger.
• Add readonly to all occurrences of private authorityCheckerservice

Updated the code.

Thank you for the opportunity, but I believe that these tests should be created by a person with good insight into the business logic and in possession of some real use cases.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[rajdip-b]

Thanks for working on this man! Much appreciated.

[szabodaniel995]

Thanks for working on this man! Much appreciated.

Glad to have helped.

Thank you :-)

[rajdip-b]

🎉 This PR is included in version 1.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀