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): Workspace ip blacklisting #687

Merged
merged 2 commits into from
Feb 19, 2025

Conversation

csehatt741
Copy link
Contributor

@csehatt741 csehatt741 commented Feb 6, 2025

User description

Description

Implemented workspace IP blacklisting in order to enforce security of workspaces by blocking users from accessing them when their IP addresses are in the blacklist.

Fixes #127

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, Configuration changes


Description

  • Introduced workspace IP blacklisting to enhance security by restricting access based on IP addresses.

  • Replaced AuthorityCheckerService with a new centralized AuthorizationService across multiple services and controllers.

  • Added endpoints and DTOs for managing blacklisted IP addresses in workspaces.

  • Updated user type to AuthenticatedUser with support for IP address handling.

  • Enhanced authorization logic and streamlined service operations for better maintainability.

  • Added end-to-end and unit tests to verify the new IP blacklisting functionality and updated authorization logic.

  • Updated Prisma schema and added a migration to include blacklistedIpAddresses in the Workspace model.


Changes walkthrough 📝

Relevant files
Enhancement
31 files
authority-checker.service.ts
Refactor authority checker service for improved workspace handling.

apps/api/src/auth/service/authority-checker.service.ts

  • Refactored methods to use AuthorizationParams instead of
    AuthorityInput.
  • Added a new private method getWorkspace for fetching workspace
    details.
  • Updated methods to utilize PrismaService directly instead of passing
    it as a parameter.
  • Enhanced error handling and logging for workspace-related operations.
  • +191/-111
    project.service.ts
    Migrate project service to use AuthorizationService.         

    apps/api/src/project/service/project.service.ts

  • Replaced AuthorityCheckerService with AuthorizationService for
    authorization checks.
  • Updated user type from User to AuthenticatedUser throughout the
    service.
  • Simplified method calls by removing prisma parameter.
  • Improved code readability and consistency in authorization logic.
  • +60/-66 
    variable.service.ts
    Refactor variable service to use AuthorizationService.     

    apps/api/src/variable/service/variable.service.ts

  • Replaced AuthorityCheckerService with AuthorizationService for
    authorization checks.
  • Updated user type from User to AuthenticatedUser.
  • Streamlined authorization logic for variable operations.
  • +46/-52 
    workspace.service.ts
    Add IP blacklisting and refactor workspace service.           

    apps/api/src/workspace/service/workspace.service.ts

  • Added methods to get and update blacklisted IP addresses for
    workspaces.
  • Replaced AuthorityCheckerService with AuthorizationService.
  • Updated user type from User to AuthenticatedUser.
  • Enhanced workspace management with IP blacklisting functionality.
  • +109/-43
    secret.service.ts
    Refactor secret service to use AuthorizationService.         

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

  • Replaced AuthorityCheckerService with AuthorizationService.
  • Updated user type from User to AuthenticatedUser.
  • Improved authorization logic for secret operations.
  • +36/-43 
    workspace-membership.service.ts
    Refactor workspace membership service to use AuthorizationService.

    apps/api/src/workspace-membership/service/workspace-membership.service.ts

  • Replaced AuthorityCheckerService with AuthorizationService.
  • Updated user type from User to AuthenticatedUser.
  • Simplified workspace membership operations with improved authorization
    logic.
  • +39/-48 
    integration.service.ts
    Refactor integration service to use AuthorizationService.

    apps/api/src/integration/service/integration.service.ts

  • Replaced AuthorityCheckerService with AuthorizationService.
  • Updated user type from User to AuthenticatedUser.
  • Improved integration operations with streamlined authorization checks.
  • +41/-44 
    authorization.service.ts
    Add centralized AuthorizationService with IP restrictions.

    apps/api/src/auth/service/authorization.service.ts

  • Introduced a new AuthorizationService for centralized authorization
    logic.
  • Added methods to handle authorization for workspaces, projects,
    environments, variables, secrets, and integrations.
  • Implemented IP-based access restrictions for workspaces.
  • +152/-0 
    environment.service.ts
    Refactor environment service to use AuthorizationService.

    apps/api/src/environment/service/environment.service.ts

  • Replaced AuthorityCheckerService with AuthorizationService.
  • Updated user type from User to AuthenticatedUser.
  • Enhanced environment operations with improved authorization logic.
  • +30/-29 
    workspace-role.service.ts
    Refactor workspace role service to use AuthorizationService.

    apps/api/src/workspace-role/service/workspace-role.service.ts

  • Replaced AuthorityCheckerService with AuthorizationService.
  • Updated user type from User to AuthenticatedUser.
  • Improved workspace role management with streamlined authorization
    logic.
  • +24/-28 
    workspace-membership.controller.ts
    Update workspace membership controller for new authorization.

    apps/api/src/workspace-membership/controller/workspace-membership.controller.ts

  • Updated controller methods to use AuthenticatedUser type.
  • Adjusted method calls to utilize AuthorizationService.
  • +11/-10 
    project.controller.ts
    Update project controller for new authorization.                 

    apps/api/src/project/controller/project.controller.ts

  • Updated controller methods to use AuthenticatedUser type.
  • Adjusted method calls to utilize AuthorizationService.
  • +11/-10 
    auth.guard.ts
    Update AuthGuard to include user IP address.                         

    apps/api/src/auth/guard/auth/auth.guard.ts

  • Enhanced AuthGuard to include IP address in AuthenticatedUser context.
  • Improved user authentication logic for API key and JWT.
  • +30/-17 
    workspace.controller.ts
    Add IP blacklisting endpoints to workspace controller.     

    apps/api/src/workspace/controller/workspace.controller.ts

  • Added endpoints for managing blacklisted IP addresses.
  • Updated controller methods to use AuthenticatedUser type.
  • Integrated AuthorizationService for workspace operations.
  • +37/-9   
    variable.controller.ts
    Update variable controller for new authorization.               

    apps/api/src/variable/controller/variable.controller.ts

  • Updated controller methods to use AuthenticatedUser type.
  • Adjusted method calls to utilize AuthorizationService.
  • +9/-8     
    secret.controller.ts
    Update secret controller for new authorization.                   

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

  • Updated controller methods to use AuthenticatedUser type.
  • Adjusted method calls to utilize AuthorizationService.
  • +8/-7     
    change-notifier.socket.ts
    Refactor change notifier to use AuthorizationService.       

    apps/api/src/socket/change-notifier.socket.ts

  • Replaced AuthorityCheckerService with AuthorizationService.
  • Updated socket handling to use AuthenticatedUser type.
  • +14/-16 
    workspace-role.controller.ts
    Update workspace role controller for new authorization.   

    apps/api/src/workspace-role/controller/workspace-role.controller.ts

  • Updated controller methods to use AuthenticatedUser type.
  • Adjusted method calls to utilize AuthorizationService.
  • +8/-7     
    update.blacklistedIpAddresses.ts
    Add DTO for workspace blacklisted IP addresses.                   

    apps/api/src/workspace/dto/update.blacklistedIpAddresses/update.blacklistedIpAddresses.ts

  • Added DTO for updating blacklisted IP addresses in workspaces.
  • Validates IP address format and ensures non-empty array.
  • +8/-0     
    integration.controller.ts
    Update integration controller to use `AuthenticatedUser`.

    apps/api/src/integration/controller/integration.controller.ts

  • Replaced User with AuthenticatedUser in method parameters.
  • Added import for AuthenticatedUser.
  • +7/-6     
    environment.controller.ts
    Update environment controller to use `AuthenticatedUser`.

    apps/api/src/environment/controller/environment.controller.ts

  • Replaced User with AuthenticatedUser in method parameters.
  • Added import for AuthenticatedUser.
  • +7/-6     
    environment.ts
    Refactor environment utilities to use `AuthorizationService`.

    apps/api/src/common/environment.ts

  • Replaced AuthorityCheckerService with AuthorizationService.
  • Updated user type to AuthenticatedUser.
  • +7/-7     
    auth.module.ts
    Enhance auth module with authorization services.                 

    apps/api/src/auth/auth.module.ts

  • Added AuthorizationService and AuthorityCheckerService as providers.
  • Exported AuthorizationService.
  • Marked module as global.
  • +8/-2     
    event.service.ts
    Refactor event service to use `AuthorizationService`.       

    apps/api/src/event/service/event.service.ts

  • Replaced AuthorityCheckerService with AuthorizationService.
  • Updated user type to AuthenticatedUser.
  • +7/-8     
    event.controller.ts
    Update event controller to use `AuthenticatedUser`.           

    apps/api/src/event/controller/event.controller.ts

    • Updated user type to AuthenticatedUser.
    +3/-2     
    event.ts
    Update event utilities for `AuthenticatedUser`.                   

    apps/api/src/common/event.ts

    • Updated user type to AuthenticatedUser.
    +2/-1     
    user.decorator.ts
    Update `CurrentUser` decorator for `AuthenticatedUser`.   

    apps/api/src/decorators/user.decorator.ts

  • Updated decorator to use AuthenticatedUser instead of
    UserWithWorkspace.
  • +2/-2     
    common.module.ts
    Simplify common module by removing unused service.             

    apps/api/src/common/common.module.ts

    • Removed AuthorityCheckerService from providers and exports.
    +2/-3     
    authorization.types.ts
    Add types for authorization service.                                         

    apps/api/src/auth/service/authorization.types.ts

    • Added types for authorization parameters.
    +8/-0     
    auth.types.ts
    Extend auth types with IP address.                                             

    apps/api/src/auth/auth.types.ts

    • Added ipAddress to AuthenticatedUserContext.
    +1/-0     
    user.types.ts
    Add `AuthenticatedUser` type definition.                                 

    apps/api/src/user/user.types.ts

    • Added AuthenticatedUser interface with ipAddress.
    +4/-0     
    Tests
    30 files
    workspace.e2e.spec.ts
    Add e2e tests for workspace IP blacklisting.                         

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

  • Added end-to-end tests for blacklisted IP address functionality.
  • Updated test cases to use AuthenticatedUser type.
  • Verified IP-based access restrictions in workspace operations.
  • +109/-4 
    project.e2e.spec.ts
    Update project e2e tests for new authorization.                   

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

  • Updated test cases to use AuthenticatedUser type.
  • Verified project operations with new authorization logic.
  • +12/-8   
    variable.e2e.spec.ts
    Update variable e2e tests for new authorization.                 

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

  • Updated test cases to use AuthenticatedUser type.
  • Verified variable operations with new authorization logic.
  • +6/-4     
    integration.e2e.spec.ts
    Modify integration e2e tests for `AuthenticatedUser`.       

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

  • Updated user types to AuthenticatedUser.
  • Added ipAddress to user objects.
  • +14/-11 
    secret.e2e.spec.ts
    Modify secret e2e tests for `AuthenticatedUser`.                 

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

  • Updated user types to AuthenticatedUser.
  • Added ipAddress to user objects.
  • +6/-4     
    environment.e2e.spec.ts
    Modify environment e2e tests for `AuthenticatedUser`.       

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

  • Updated user types to AuthenticatedUser.
  • Added ipAddress to user objects.
  • +6/-4     
    event.e2e.spec.ts
    Modify event e2e tests for `AuthenticatedUser`.                   

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

  • Updated user type to AuthenticatedUser.
  • Added ipAddress to user object.
  • +7/-3     
    workspace-role.e2e.spec.ts
    Modify workspace role e2e tests for `AuthenticatedUser`. 

    apps/api/src/workspace-role/workspace-role.e2e.spec.ts

  • Updated user types to AuthenticatedUser.
  • Added ipAddress to user objects.
  • +9/-7     
    workspace-membership.e2e.spec.ts
    Modify workspace membership e2e tests for `AuthenticatedUser`.

    apps/api/src/workspace-membership/workspace-membership.e2e.spec.ts

  • Updated user types to AuthenticatedUser.
  • Added ipAddress to user objects.
  • +9/-5     
    secret.controller.spec.ts
    Update secret controller tests for authorization changes.

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

    • Replaced AuthorityCheckerService with AuthorizationService.
    +3/-7     
    variable.controller.spec.ts
    Update variable controller tests for authorization changes.

    apps/api/src/variable/controller/variable.controller.spec.ts

    • Replaced AuthorityCheckerService with AuthorizationService.
    +3/-7     
    user.e2e.spec.ts
    Add IP address handling in user e2e tests.                             

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

    • Added ipAddress to user objects in tests.
    +6/-2     
    secret.service.spec.ts
    Update secret service tests for authorization changes.     

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

    • Replaced AuthorityCheckerService with AuthorizationService.
    +3/-7     
    variable.service.spec.ts
    Update variable service tests for authorization changes. 

    apps/api/src/variable/service/variable.service.spec.ts

    • Replaced AuthorityCheckerService with AuthorizationService.
    +3/-7     
    integration.controller.spec.ts
    Update integration controller tests for authorization changes.

    apps/api/src/integration/controller/integration.controller.spec.ts

    • Replaced AuthorityCheckerService with AuthorizationService.
    +8/-2     
    environment.controller.spec.ts
    Update environment controller tests for authorization changes.

    apps/api/src/environment/controller/environment.controller.spec.ts

    • Replaced AuthorityCheckerService with AuthorizationService.
    +8/-2     
    event.controller.spec.ts
    Update event controller tests for authorization changes. 

    apps/api/src/event/controller/event.controller.spec.ts

    • Replaced AuthorityCheckerService with AuthorizationService.
    +8/-2     
    project.controller.spec.ts
    Update project controller tests for authorization changes.

    apps/api/src/project/controller/project.controller.spec.ts

    • Replaced AuthorityCheckerService with AuthorizationService.
    +3/-1     
    project.service.spec.ts
    Update project service tests for authorization changes.   

    apps/api/src/project/service/project.service.spec.ts

    • Replaced AuthorityCheckerService with AuthorizationService.
    +3/-1     
    integration.service.spec.ts
    Update integration service tests for authorization changes.

    apps/api/src/integration/service/integration.service.spec.ts

    • Replaced AuthorityCheckerService with AuthorizationService.
    +8/-2     
    environment.service.spec.ts
    Update environment service tests for authorization changes.

    apps/api/src/environment/service/environment.service.spec.ts

    • Replaced AuthorityCheckerService with AuthorizationService.
    +8/-2     
    workspace-role.controller.spec.ts
    Update workspace role controller tests for authorization changes.

    apps/api/src/workspace-role/controller/workspace-role.controller.spec.ts

    • Replaced AuthorityCheckerService with AuthorizationService.
    +3/-4     
    workspace-membership.controller.spec.ts
    Update workspace membership controller tests for authorization
    changes.

    apps/api/src/workspace-membership/controller/workspace-membership.controller.spec.ts

    • Replaced AuthorityCheckerService with AuthorizationService.
    +3/-1     
    event.service.spec.ts
    Update event service tests for authorization changes.       

    apps/api/src/event/service/event.service.spec.ts

    • Replaced AuthorityCheckerService with AuthorizationService.
    +8/-2     
    workspace-role.service.spec.ts
    Update workspace role service tests for authorization changes.

    apps/api/src/workspace-role/service/workspace-role.service.spec.ts

    • Replaced AuthorityCheckerService with AuthorizationService.
    +3/-4     
    workspace-membership.service.spec.ts
    Update workspace membership service tests for authorization changes.

    apps/api/src/workspace-membership/service/workspace-membership.service.spec.ts

    • Replaced AuthorityCheckerService with AuthorizationService.
    +3/-1     
    authorization.service.spec.ts
    Add unit tests for `AuthorizationService`.                             

    apps/api/src/auth/service/authorization.service.spec.ts

    • Added unit tests for AuthorizationService.
    +22/-0   
    workspace.controller.spec.ts
    Update workspace controller tests for authorization changes.

    apps/api/src/workspace/controller/workspace.controller.spec.ts

    • Replaced AuthorityCheckerService with AuthorizationService.
    +3/-1     
    workspace.service.spec.ts
    Update workspace service tests for authorization changes.

    apps/api/src/workspace/service/workspace.service.spec.ts

    • Replaced AuthorityCheckerService with AuthorizationService.
    +3/-1     
    update.blacklistedIpAddresses.spec.ts
    Add unit test for blacklisted IP addresses DTO.                   

    apps/api/src/workspace/dto/update.blacklistedIpAddresses/update.blacklistedIpAddresses.spec.ts

    • Added unit test for UpdateBlacklistedIpAddresses DTO.
    +7/-0     
    Formatting
    1 files
    instrumentation.ts
    Minor formatting changes in instrumentation file.               

    apps/web/src/instrumentation.ts

    • Updated imports to remove semicolons.
    +4/-4     
    Configuration changes
    2 files
    schema.prisma
    Extend workspace schema with blacklisted IP addresses.     

    apps/api/src/prisma/schema.prisma

    • Added blacklistedIpAddresses field to Workspace model.
    +10/-9   
    migration.sql
    Add migration for blacklisted IP addresses in workspace. 

    apps/api/src/prisma/migrations/20250206070313_add_blacklisted_ip_addresses_in_workspace/migration.sql

    • Added migration to include blacklistedIpAddresses in Workspace.
    +2/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @csehatt741 csehatt741 marked this pull request as draft February 6, 2025 10:31
    @rajdip-b
    Copy link
    Member

    rajdip-b commented Feb 6, 2025

    I'll review this once you are done.

    @csehatt741 csehatt741 marked this pull request as ready for review February 7, 2025 05:57
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    127 - Partially compliant

    Compliant requirements:

    • Update workspace schema to hold a list of blacklisted IP addresses
    • Allow workspace administrators to update the blacklist

    Non-compliant requirements:

    • Create a guard that will check if authenticated user's IP is blacklisted when accessing workspace resources
    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Input Validation

    The updateBlacklistedIpAddresses method accepts IP addresses without validation. Should validate that inputs are valid IPv4/IPv6 addresses before saving.

    async updateBlacklistedIpAddresses(
      user: AuthenticatedUser,
      workspaceSlug: Workspace['slug'],
      dto: UpdateBlacklistedIpAddresses
    ) {
      // Fetch the workspace
      const workspace =
        await this.authorizationService.authorizeUserAccessToWorkspace({
          user: user,
          entity: { slug: workspaceSlug },
          authorities: [Authority.WORKSPACE_ADMIN]
        })
    
      // Update blacklisted IP addresses
      const updatedWorkspace = await this.prisma.workspace.update({
        where: {
          id: workspace.id
        },
        data: {
          blacklistedIpAddresses: dto.ipAddresses
        }
      })
    
      this.log.debug(
        `Updated workspace blacklisted IP addresses ${workspace.name} (${workspace.id})`
      )
    
      await createEvent(
        {
          triggeredBy: user,
          entity: workspace,
          type: EventType.WORKSPACE_UPDATED,
          source: EventSource.WORKSPACE,
          title: `Workspace blacklisted IP addresses updated`,
          metadata: {
            workspaceId: workspace.id,
            name: workspace.name
          },
          workspaceId: workspace.id
        },
        this.prisma
      )
    
      return updatedWorkspace.blacklistedIpAddresses
    }
    Code Duplication

    The getWorkspace method contains duplicated workspace fetching logic that exists in multiple places. Consider extracting common workspace fetching logic into a shared service.

    private async getWorkspace(
      userId: User['id'],
      filter: {
        workspaceId?: Workspace['id']
        workspaceSlug?: Workspace['slug']
        workspaceName?: Workspace['name']
      }
    ): Promise<Workspace> {
      let workspace: Workspace
    
      try {
        if (filter.workspaceId) {
          workspace = await this.prisma.workspace.findUnique({
            where: {
              id: filter.workspaceId
            }
          })
        } else if (filter.workspaceSlug) {
          workspace = await this.prisma.workspace.findUnique({
            where: {
              slug: filter.workspaceSlug
            }
          })
        } else if (filter.workspaceName) {
          workspace = await this.prisma.workspace.findFirst({
            where: {
              name: filter.workspaceName,
              members: { some: { userId: userId } }
            }
          })
        }
      } catch (error) {
        this.customLoggerService.error(error)
        throw new InternalServerErrorException(error)
      }
    
      if (!workspace) {
        throw new NotFoundException(
          `Workspace ${filter.workspaceId ?? filter.workspaceSlug ?? filter.workspaceName} not found`
        )
      }
    
      return workspace
    }

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Feb 7, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Security
    Enforce IP blacklist restrictions

    Add IP address validation against workspace blacklist before allowing access.
    Since the PR adds IP tracking, it's critical to enforce blacklist restrictions
    at the auth guard level.

    apps/api/src/auth/guard/auth/auth.guard.ts [157-159]

     // If the user is not active, we throw an UnauthorizedException.
     if (!userContext.isActive) {
       throw new UnauthorizedException('User is not active')
     }
     
    +// Check if user IP is blacklisted for workspace
    +const isIpBlacklisted = await this.workspaceService.isIpBlacklisted(
    +  userContext.defaultWorkspace?.slug,
    +  userContext.ipAddress
    +);
    +if (isIpBlacklisted) {
    +  throw new ForbiddenException('IP address is blacklisted');
    +}
    +

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 9

    __

    Why: Critical security enhancement that enforces IP blacklist restrictions at the authentication level, preventing access from blocked IP addresses before any operations can occur. This is especially important given the PR's addition of IP tracking functionality.

    High
    Possible issue
    Handle missing workspace filter case

    Add error handling for the case where workspace is undefined in getWorkspace
    method. Currently, the code assumes workspace will be defined after the try
    block, but it might not be if none of the conditions match.

    apps/api/src/auth/service/authority-checker.service.ts [479-502]

     try {
       if (filter.workspaceId) {
         workspace = await this.prisma.workspace.findUnique({
           where: {
             id: filter.workspaceId
           }
         })
       } else if (filter.workspaceSlug) {
         workspace = await this.prisma.workspace.findUnique({
           where: {
             slug: filter.workspaceSlug
           }
         })
       } else if (filter.workspaceName) {
         workspace = await this.prisma.workspace.findFirst({
           where: {
             name: filter.workspaceName,
             members: { some: { userId: userId } }
           }
         })
    +  } else {
    +    throw new BadRequestException('No valid workspace filter provided')
       }
     } catch (error) {
       this.customLoggerService.error(error)
       throw new InternalServerErrorException(error)
     }

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 8

    __

    Why: The suggestion identifies a critical edge case where no workspace filter is provided, which could lead to undefined behavior. Adding explicit error handling improves robustness and provides clearer error messages.

    Medium
    Validate IP addresses before blacklisting

    Add input validation to ensure IP addresses in dto.ipAddresses are in valid
    format before updating the blacklist. This prevents storing invalid IP addresses
    that could cause issues with blacklisting functionality.

    apps/api/src/workspace/service/workspace.service.ts [547-555]

    +// Validate IP addresses
    +const validIpRegex = /^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$/;
    +if (!dto.ipAddresses.every(ip => validIpRegex.test(ip))) {
    +  throw new BadRequestException('Invalid IP address format');
    +}
    +
     // Update blacklisted IP addresses
     const updatedWorkspace = await this.prisma.workspace.update({
       where: {
         id: workspace.id
       },
       data: {
         blacklistedIpAddresses: dto.ipAddresses
       }
     })

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 8

    __

    Why: Adding IP address validation is critical for security and functionality - it prevents storing invalid IP addresses that could break blacklisting features or create security vulnerabilities.

    Medium

    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.

    You are using user: user at multiple places. We can replace it with just user. You can do a grep search to find out such instances.

    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.

    Instead of having a separate endpoint and dto for the blacklisted ip modification, we should use a single update endpoint.

    • If a non-empty array is passed, we update the list
    • If an empty or undefined array is passed, we skip updating the list

    @csehatt741
    Copy link
    Contributor Author

    Instead of having a separate endpoint and dto

    To my understanding updateWorkspace and updateBlacklistedIpAddresses have different authorization levels:

    • updateWorkspace requires Authority.UPDATE_WORKSPACE
    • according to the requirements of Workspace administrators would be able to update the blacklist of the workspaces., updateBlacklistedIpAddresses needs Authority.WORKSPACE_ADMIN

    @csehatt741 csehatt741 requested a review from rajdip-b February 7, 2025 14:20
    @rajdip-b
    Copy link
    Member

    rajdip-b commented Feb 8, 2025

    Yeah, I didn't think about this perspective before

    @rajdip-b
    Copy link
    Member

    rajdip-b commented Feb 8, 2025

    We should be good to merge once you get the error fixed.

    @csehatt741
    Copy link
    Contributor Author

    csehatt741 commented Feb 8, 2025

    We should be good to merge once you get the error fixed.

    I`ve already seen this issue before, but then could not reproduce it locally.

    Now after executing e2e tests locally, a completely different one failed in workspace-role with this exact same error.

    There seems to be some kind of race condition in e2e tests. I`ll look it up!

    @rajdip-b
    Copy link
    Member

    rajdip-b commented Feb 8, 2025

    Thanks!!

    @csehatt741
    Copy link
    Contributor Author

    @kriptonian1, could you take a look at this PR too, please?

    @csehatt741
    Copy link
    Contributor Author

    Thanks!!

    I cannot reproduce it locally. So far it has only occurred in the CI.

    @rajdip-b
    Copy link
    Member

    Thanks!!

    I cannot reproduce it locally. So far it has only occurred in the CI.

    Alright i'll look into it

    Copy link

    codecov bot commented Feb 11, 2025

    Codecov Report

    Attention: Patch coverage is 88.49105% with 45 lines in your changes missing coverage. Please review.

    Project coverage is 87.36%. Comparing base (ce50743) to head (48c3119).
    Report is 447 commits behind head on develop.

    Files with missing lines Patch % Lines
    .../api/src/auth/service/authority-checker.service.ts 78.99% 25 Missing ⚠️
    apps/api/src/auth/guard/auth/auth.guard.ts 70.58% 5 Missing ⚠️
    ...pps/api/src/secret/controller/secret.controller.ts 88.57% 4 Missing ⚠️
    apps/api/src/socket/change-notifier.socket.ts 42.85% 4 Missing ⚠️
    ...api/src/variable/controller/variable.controller.ts 87.87% 4 Missing ⚠️
    apps/api/src/auth/service/authorization.service.ts 93.87% 3 Missing ⚠️

    ❌ Your patch check has failed because the patch coverage (88.49%) is below the target coverage (98.00%). You can increase the patch coverage or adjust the target coverage.

    Additional details and impacted files
    @@             Coverage Diff             @@
    ##           develop     #687      +/-   ##
    ===========================================
    - Coverage    91.71%   87.36%   -4.36%     
    ===========================================
      Files          111      118       +7     
      Lines         2510     3062     +552     
      Branches       469      452      -17     
    ===========================================
    + Hits          2302     2675     +373     
    - Misses         208      387     +179     
    Flag Coverage Δ
    api-e2e-tests 87.36% <88.49%> (-4.36%) ⬇️

    Flags with carried forward coverage won't be shown. Click here to find out more.

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    @csehatt741
    Copy link
    Contributor Author

    @rajdip-b, what can I do here to have this PR merged?

    @rajdip-b
    Copy link
    Member

    I'm not getting much time to sit an have a look at it. i dont think there is much to be done from your end. maybe in the meantime you can go through the test coverage and add more tests if needed.

    @rajdip-b
    Copy link
    Member

    i think if you can just resolve the conflicts, we would be good to merge. i dont see any test failures

    @csehatt741 csehatt741 force-pushed the workspace-ip-blacklisting branch 3 times, most recently from 431bab0 to d93f71f Compare February 18, 2025 04:07
    @csehatt741 csehatt741 requested a review from rajdip-b February 18, 2025 09:13
    @csehatt741 csehatt741 force-pushed the workspace-ip-blacklisting branch 2 times, most recently from 387f904 to c78524b Compare February 18, 2025 09:27
    @csehatt741 csehatt741 force-pushed the workspace-ip-blacklisting branch from c78524b to 7eb3c78 Compare February 19, 2025 02:24
    @rajdip-b rajdip-b merged commit bc5f6a5 into keyshade-xyz:develop Feb 19, 2025
    4 checks passed
    rajdip-b pushed a commit that referenced this pull request Feb 19, 2025
    ## [2.12.0-stage.6](v2.12.0-stage.5...v2.12.0-stage.6) (2025-02-19)
    
    ### 🚀 Features
    
    * **api:** Workspace ip blacklisting ([#687](#687)) ([bc5f6a5](bc5f6a5))
    @rajdip-b
    Copy link
    Member

    🎉 This PR is included in version 2.12.0-stage.6 🎉

    The release is available on GitHub release

    Your semantic-release bot 📦🚀

    @csehatt741 csehatt741 deleted the workspace-ip-blacklisting branch February 19, 2025 08:04
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    API: IP Blacklisting on workspaces
    2 participants