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
208 stars 105 forks source link

refactor(API): Refactor workspace-membership into a separate module #421

Closed unamdev0 closed 2 months ago

unamdev0 commented 2 months ago

User description

Description

Created seperate modulr for workspace membership

Fixes #319

Documentation Update

PR Type

enhancement, tests

Description

Changes walkthrough 📝

Relevant files
Enhancement
6 files
workspace.service.ts
Refactor workspace service to separate membership logic   
apps/api/src/workspace/service/workspace.service.ts
  • Removed workspace membership-related methods.
  • Simplified workspace service by focusing on workspace creation and
    updates.
  • Refactored to use the new workspace-membership module.
    		•  +82/-1057
    workspace-membership.service.ts
    Implement workspace membership management service               
    apps/api/src/workspace-membership/service/workspace-membership.service.ts
  • Implemented service for managing workspace memberships.
  • Added methods for transferring ownership and managing invitations.
  • Included role updates and membership checks.
    		•  +1011/-0
    workspace-membership.controller.ts
    Create controller for workspace membership operations       
    apps/api/src/workspace-membership/controller/workspace-membership.controller.ts
  • Created controller for workspace membership operations.
  • Added endpoints for managing membership and invitations.
  • Integrated with workspace membership service.
    		•  +160/-0 
    create.workspace-membership.ts
    Define DTO for workspace member creation                                 
    apps/api/src/workspace-membership/dto/create.workspace/create.workspace-membership.ts
  • Defined DTO for creating workspace members.
  • Included validation for email and role slugs.
    		•  +12/-0   
    workspace-membership.module.ts
    Create module for workspace membership management               
    apps/api/src/workspace-membership/workspace-membership.module.ts
  • Created module for workspace membership.
  • Registered service and controller for membership management.
    		•  +9/-0     
    create.workspace.ts
    Simplify workspace DTO by removing membership details       
    apps/api/src/workspace/dto/create.workspace/create.workspace.ts
  • Removed WorkspaceMemberDTO interface.
  • Focused DTO on workspace creation details.
    		•  +1/-6     
    Tests
    5 files
    workspace-membership.e2e.spec.ts
    Add end-to-end tests for workspace membership                       
    apps/api/src/workspace-membership/workspace-membership.e2e.spec.ts
  • Added end-to-end tests for workspace membership functionalities.
  • Tested ownership transfer, user invitations, and membership updates.
  • Verified event creation for membership actions.
    		•  +1047/-0
    project.e2e.spec.ts
    Integrate workspace membership service in project tests   
    apps/api/src/project/project.e2e.spec.ts
  • Integrated workspace membership service into project tests.
  • Updated tests to use new membership module.
    		•  +8/-2     
    workspace-membership.controller.spec.ts
    Add unit tests for workspace membership controller             
    apps/api/src/workspace-membership/controller/workspace-membership.controller.spec.ts
  • Added unit tests for workspace membership controller.
  • Verified controller initialization and basic functionality.
    		•  +38/-0   
    workspace-membership.service.spec.ts
    Add unit tests for workspace membership service                   
    apps/api/src/workspace-membership/service/workspace-membership.service.spec.ts
  • Added unit tests for workspace membership service.
  • Ensured service methods are defined and functional.
    		•  +34/-0   
    create.workspace-membership.spec.ts
    Add test for CreateWorkspaceMember DTO                                     
    apps/api/src/workspace-membership/dto/create.workspace/create.workspace-membership.spec.ts
  • Added test for CreateWorkspaceMember DTO.
  • Verified DTO definition and validation.
    		•  +7/-0     

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

    rajdip-b commented 2 months ago

    Do fill up the description about with the pointers whenever you can

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

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Code Removal
    Large portions of code related to workspace membership functionality have been removed from this file. Ensure all removed functionality is properly implemented in the new workspace-membership module. New Service Implementation
    This new service file contains a lot of complex logic related to workspace membership. Carefully review all methods for correctness, especially around invitation handling, role updates, and ownership transfers. Test Coverage
    This new end-to-end test file is quite extensive. Ensure it covers all critical paths and edge cases for the new workspace membership functionality.
    codiumai-pr-agent-free[bot] commented 2 months ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use a transaction for workspace deletion to ensure atomicity ___ **Consider using a transaction for the delete operation to ensure atomicity. This can
    prevent partial deletions if an error occurs during the process.** [apps/api/src/workspace/service/workspace.service.ts [157-162]](https://github.com/keyshade-xyz/keyshade/pull/421/files#diff-a92613f23cf2834d52eaedb87c4e8c460d48b6fca8df4ccc968b2a8af19a9c3fR157-R162) ```diff // Delete the workspace -await this.prisma.workspace.delete({ - where: { - id: workspace.id - } -}) +await this.prisma.$transaction(async (prisma) => { + // Delete related entities first (if any) + await prisma.workspaceMember.deleteMany({ where: { workspaceId: workspace.id } }); + // Delete the workspace + await prisma.workspace.delete({ + where: { + id: workspace.id + } + }); +}); ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 9 Why: Using a transaction for deletion ensures atomicity and prevents partial deletions, which is crucial for maintaining data consistency. This is a significant improvement, hence a high score.
    		9
    Use a more descriptive variable name for workspace roles to improve readability ___ **In the 'beforeEach' function, consider using a more descriptive variable name for
    the workspace roles, such as memberWorkspaceRole instead of memberRole, to improve
    code readability.** [apps/api/src/workspace-membership/workspace-membership.e2e.spec.ts [150-157]](https://github.com/keyshade-xyz/keyshade/pull/421/files#diff-3d2af810cbc31b6ae710d387ba6e8d72d9aaf5e8021ce6266fe5b438c6115c1aR150-R157) ```diff -memberRole = await prisma.workspaceRole.create({ +memberWorkspaceRole = await prisma.workspaceRole.create({ data: { name: 'Member', slug: 'member', workspaceId: workspace1.id, authorities: [Authority.READ_WORKSPACE] } }) ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 5 Why: Using a more descriptive variable name can improve code readability, but the current name is already fairly descriptive, so the improvement is minor.
    		5
    Enhancement
    Add an assertion to verify the removal of the old role after updating a member's role ___ **In the 'should be able to update the role of a member' test, consider adding an
    assertion to check that the old role (adminRole) is no longer associated with the
    user after the update.** [apps/api/src/workspace-membership/workspace-membership.e2e.spec.ts [595-599]](https://github.com/keyshade-xyz/keyshade/pull/421/files#diff-3d2af810cbc31b6ae710d387ba6e8d72d9aaf5e8021ce6266fe5b438c6115c1aR595-R599) ```diff expect(membership.roles).toEqual([ { roleId: memberRole.id } ]) +expect(membership.roles).not.toContainEqual({ + roleId: adminRole.id +}) ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: Adding an assertion to verify the removal of the old role after updating a member's role is a valuable enhancement, as it ensures that the role update process is functioning correctly and completely.
    		8
    Provide a more informative error message for BadRequestException ___ **Consider using a more specific error message in the BadRequestException. Instead of
    using an empty string, provide a clear explanation of why the request is invalid.** [apps/api/src/workspace/service/workspace.service.ts [152-153]](https://github.com/keyshade-xyz/keyshade/pull/421/files#diff-a92613f23cf2834d52eaedb87c4e8c460d48b6fca8df4ccc968b2a8af19a9c3fR152-R153) ```diff throw new BadRequestException( + 'Invalid request: Unable to perform the requested action on the workspace.' ) ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: The suggestion improves the clarity of error messages, which enhances user experience and debugging. However, it does not address a critical issue, so it receives a moderate score.
    		7
    Add assertions to check the structure of returned member objects in the test ___ **In the 'should be able to get all the members of the workspace' test, consider
    adding an assertion to check the structure of the returned member object, ensuring
    it contains expected properties like id, email, or name.** [apps/api/src/workspace-membership/workspace-membership.e2e.spec.ts [1014-1016]](https://github.com/keyshade-xyz/keyshade/pull/421/files#diff-3d2af810cbc31b6ae710d387ba6e8d72d9aaf5e8021ce6266fe5b438c6115c1aR1014-R1016) ```diff expect(response.statusCode).toBe(200) expect(response.json().items).toBeInstanceOf(Array) expect(response.json().items).toHaveLength(1) +expect(response.json().items[0]).toHaveProperty('id') +expect(response.json().items[0]).toHaveProperty('email') +expect(response.json().items[0]).toHaveProperty('name') ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: Adding assertions to check the structure of the returned member object enhances the test by ensuring that the response contains the expected properties, improving test robustness and reliability.
    		7
    Add a method to resend invitation emails for pending invitations ___ **Consider adding a method to resend invitation emails for pending invitations, which
    could be useful if the original invitation email was not received or expired.** [apps/api/src/workspace-membership/service/workspace-membership.service.ts [1011]](https://github.com/keyshade-xyz/keyshade/pull/421/files#diff-510c8dfb0963173c1ce7e47b2433e04050f9e999e298ba59d3fd771ceceb7419R1011-R1011) ```diff } +async resendInvitation( + user: User, + workspaceSlug: Workspace['slug'], + inviteeEmail: User['email'] +): Promise { + const workspace = await this.authorityCheckerService.checkAuthorityOverWorkspace({ + userId: user.id, + entity: { slug: workspaceSlug }, + authorities: [Authority.ADD_USER], + prisma: this.prisma + }) + + const invitee = await this.prisma.user.findUnique({ + where: { email: inviteeEmail } + }) + + if (!invitee) { + throw new NotFoundException(`User with email ${inviteeEmail} not found`) + } + + const membership = await this.prisma.workspaceMember.findUnique({ + where: { + workspaceId_userId: { + workspaceId: workspace.id, + userId: invitee.id + } + } + }) + + if (!membership || membership.invitationAccepted) { + throw new BadRequestException(`No pending invitation found for ${inviteeEmail}`) + } + + await this.mailService.workspaceInvitationMailForUsers( + inviteeEmail, + workspace.name, + `${process.env.WORKSPACE_FRONTEND_URL}/workspace/${workspace.id}/join`, + user.name, + true + ) + + this.log.debug(`Resent invitation to ${inviteeEmail} for workspace ${workspace.name}`) +} + ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 6 Why: Adding a method to resend invitations is a useful enhancement for user experience, but it is not critical to the current functionality and could be implemented later as an additional feature.
    		6
    Possible issue
    Add a check for workspace existence before updating ___ **Consider adding a check for the existence of the workspace before attempting to
    update it. This can prevent potential errors if the workspace doesn't exist.** [apps/api/src/workspace/service/workspace.service.ts [94-110]](https://github.com/keyshade-xyz/keyshade/pull/421/files#diff-a92613f23cf2834d52eaedb87c4e8c460d48b6fca8df4ccc968b2a8af19a9c3fR94-R110) ```diff +const existingWorkspace = await this.prisma.workspace.findUnique({ + where: { id: workspace.id } +}); +if (!existingWorkspace) { + throw new NotFoundException(`Workspace with ID ${workspace.id} not found`); +} const updatedWorkspace = await this.prisma.workspace.update({ where: { id: workspace.id }, data: { name: dto.name, slug: dto.name ? await generateEntitySlug(dto.name, 'WORKSPACE', this.prisma) : undefined, description: dto.description, lastUpdatedBy: { connect: { id: user.id } } } }) ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: Adding a check for workspace existence before updating is a good practice to prevent errors and ensure data integrity. This suggestion addresses a potential issue, making it valuable.
    		8
    Security
    Add input validation for the update workspace method ___ **Consider adding input validation for the dto parameter in the updateWorkspace
    method. This can help prevent potential issues with invalid or unexpected input.** [apps/api/src/workspace/service/workspace.service.ts [71-75]](https://github.com/keyshade-xyz/keyshade/pull/421/files#diff-a92613f23cf2834d52eaedb87c4e8c460d48b6fca8df4ccc968b2a8af19a9c3fR71-R75) ```diff async updateWorkspace( user: User, workspaceSlug: Workspace['slug'], dto: UpdateWorkspace ) { + if (!dto || (typeof dto.name !== 'undefined' && dto.name.trim() === '')) { + throw new BadRequestException('Invalid workspace update data'); + } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: Input validation is essential for preventing invalid data from causing errors or security issues. This suggestion enhances the robustness of the code, making it an important improvement.
    		8
    Implement rate limiting for the invitation system ___ **Consider implementing rate limiting for the inviteUsersToWorkspace method to prevent
    potential abuse of the invitation system.** [apps/api/src/workspace-membership/service/workspace-membership.service.ts [186-190]](https://github.com/keyshade-xyz/keyshade/pull/421/files#diff-510c8dfb0963173c1ce7e47b2433e04050f9e999e298ba59d3fd771ceceb7419R186-R190) ```diff +@RateLimit({ points: 10, duration: 60 }) // Example rate limit: 10 invitations per minute async inviteUsersToWorkspace( user: User, workspaceSlug: Workspace['slug'], members: CreateWorkspaceMember[] ): Promise { ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: Implementing rate limiting can prevent abuse and enhance security, but the suggestion lacks details on how to integrate it with the existing system, which limits its immediate applicability.
    		7
    Input validation
    Add input validation for pagination and sorting parameters ___ **Consider adding input validation for the page, limit, sort, order, and search
    parameters in the getAllMembersOfWorkspace method to ensure they are within
    acceptable ranges or formats.** [apps/api/src/workspace-membership/service/workspace-membership.service.ts [436-444]](https://github.com/keyshade-xyz/keyshade/pull/421/files#diff-510c8dfb0963173c1ce7e47b2433e04050f9e999e298ba59d3fd771ceceb7419R436-R444) ```diff async getAllMembersOfWorkspace( user: User, workspaceSlug: Workspace['slug'], page: number, limit: number, sort: string, order: string, search: string ) { + if (page < 0) throw new BadRequestException('Page must be non-negative') + if (limit < 1 || limit > 100) throw new BadRequestException('Limit must be between 1 and 100') + if (!['asc', 'desc'].includes(order.toLowerCase())) throw new BadRequestException('Order must be "asc" or "desc"') + // Add more validations as needed ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: Adding input validation for pagination and sorting parameters is a good practice to prevent invalid inputs and potential errors, enhancing the robustness of the method.
    		8
    Error handling
    Improve error handling by using a more specific error type ___ **Consider using a more specific error type instead of InternalServerErrorException in
    the transferOwnership method. This could provide more meaningful error information
    to the client.** [apps/api/src/workspace-membership/service/workspace-membership.service.ts [149-153]](https://github.com/keyshade-xyz/keyshade/pull/421/files#diff-510c8dfb0963173c1ce7e47b2433e04050f9e999e298ba59d3fd771ceceb7419R149-R153) ```diff } catch (e) { this.log.error('Error in transaction', e) - throw new InternalServerErrorException('Error in transaction') + throw new Error('Failed to transfer ownership: ' + e.message) } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 4 Why: The suggestion to use a more specific error type could improve error clarity, but replacing `InternalServerErrorException` with a generic `Error` does not provide a more specific error type. The suggestion does not enhance the specificity of the error handling.
    		4
    rajdip-b commented 2 months ago

    :tada: This PR is included in version 2.5.0 :tada:

    The release is available on GitHub release

    Your semantic-release bot :package::rocket: