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, schema): Add preview field in API Key #680

Merged
merged 3 commits into from
Jan 30, 2025

Conversation

rajdip-b
Copy link
Member

@rajdip-b rajdip-b commented Jan 30, 2025

User description

Description

  • Add a preview field in API Key model that stores the api key value in the format ks_****<last 4 chars>
  • Added DB migrations
  • Updates schema package to reflect the changes
  • Add tests

Screenshots

  • Creating API key
image
  • Get API key:
image

PR Type

Enhancement, Tests


Description

  • Added a preview field to the API Key model for masked key representation.

  • Updated API key creation logic to generate and store the preview field.

  • Modified schema and database migrations to include the preview field.

  • Enhanced tests to validate the preview field functionality.


Changes walkthrough 📝

Relevant files
Tests
api-key.e2e.spec.ts
Add tests for `preview` field in API Key                                 

apps/api/src/api-key/api-key.e2e.spec.ts

  • Added tests to validate the preview field format.
  • Verified the last 4 characters of value and preview match.
  • Included preview in API key response validation.
  • +6/-0     
    Enhancement
    api-key.service.ts
    Implement `preview` field generation in API Key service   

    apps/api/src/api-key/service/api-key.service.ts

  • Added logic to generate preview field during API key creation.
  • Logged the creation of the preview field.
  • Updated API key selection to include preview.
  • +9/-0     
    environment.service.ts
    Refactor secret and variable count logic                                 

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

  • Refactored secret and variable count retrieval logic.
  • Simplified asynchronous operations for better readability.
  • +4/-6     
    index.ts
    Update schema to include `preview` field                                 

    packages/schema/src/api-key/index.ts

    • Added preview field to API Key schema definition.
    +1/-0     
    migration.sql
    Add migration for `preview` field in API Key                         

    apps/api/src/prisma/migrations/20250130160517_add_preview_in_api_key/migration.sql

  • Added a database migration to include preview field in ApiKey table.
  • +2/-0     
    schema.prisma
    Update Prisma schema for `preview` field                                 

    apps/api/src/prisma/schema.prisma

    • Updated Prisma schema to include preview field in ApiKey model.
    +1/-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.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 Security concerns

    Sensitive information exposure:
    The preview field and logging of API key previews (ks_****1234) exposes the last 4 characters of the actual API key. While this is a common practice, it reduces the key's entropy and could make brute force attacks slightly easier. Consider logging only the key name without the preview value.

    ⚡ Recommended focus areas for review

    Performance Regression

    Removing Promise.all when fetching secret and variable counts could impact performance by making the operations sequential instead of parallel

    const secretCount = await this.getSecretCount(environment.id)
    const variableCount = await this.getVariableCount(environment.id)
    environment['secrets'] = secretCount
    environment['variables'] = variableCount
    Logging Concern

    Logging the preview key which contains part of the actual API key value could expose sensitive information in logs

    this.logger.log(
      `User ${user.id} created API key ${previewKey} with name ${dto.name}`
    )

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    Remove sensitive data from logs

    Remove sensitive information (API key preview) from logs to prevent potential
    security risks. Use a more generic message or mask the key preview in logs.

    apps/api/src/api-key/service/api-key.service.ts [48-50]

     this.logger.log(
    -  `User ${user.id} created API key ${previewKey} with name ${dto.name}`
    +  `User ${user.id} created new API key with name ${dto.name}`
     )
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Logging sensitive information like API key previews poses a security risk as logs may be exposed. The suggestion correctly identifies a security best practice to avoid logging sensitive data.

    9
    General
    Optimize parallel async operations

    Revert back to using Promise.all() for parallel execution of secret and variable
    count retrieval. The current sequential execution can lead to performance issues
    when dealing with multiple environments.

    apps/api/src/environment/service/environment.service.ts [328-332]

     for (const environment of items) {
    -  const secretCount = await this.getSecretCount(environment.id)
    -  const variableCount = await this.getVariableCount(environment.id)
    -  environment['secrets'] = secretCount
    -  environment['variables'] = variableCount
    +  const [secrets, variables] = await Promise.all([
    +    this.getSecretCount(environment.id),
    +    this.getVariableCount(environment.id)
    +  ])
    +  environment['secrets'] = secrets
    +  environment['variables'] = variables
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using Promise.all() for parallel execution of independent async operations is more efficient than sequential execution, especially when dealing with multiple environments. This change could improve performance.

    7

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Jan 30, 2025

    CI Feedback 🧐

    (Feedback updated until commit e08f1cf)

    A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

    Action: Validate API

    Failed stage: E2E tests [❌]

    Failed test name: project.e2e.spec.ts

    Failure summary:

    The action failed due to a database unique constraint violation in the project service:

  • A duplicate environment name was attempted to be created for the same project
  • The error occurred in project.service.ts line 990 during environment creation
  • The database enforces unique constraint on the combination of (projectId,name) fields
  • This caused one of the e2e tests to fail, leading to the overall action failure

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    872:  └─ migration.sql
    873:  └─ 20250109110640_add_admin_api_key_authority/
    874:  └─ migration.sql
    875:  └─ 20250123150220_add_rotate_after_in_secret/
    876:  └─ migration.sql
    877:  └─ 20250130160517_add_preview_in_api_key/
    878:  └─ migration.sql
    879:  All migrations have been successfully applied.
    880:  �[31m[Nest] 3605  - �[39m01/30/2025, 4:54:17 PM �[31m  ERROR�[39m �[38;5;3m[ExceptionsHandler] �[39m�[31m
    881:  Invalid `this.prisma.environment.create()` invocation in
    882:  /home/runner/work/keyshade/keyshade/apps/api/src/project/service/project.service.ts:990:33
    883:  987 envNameToIdMap[environment.name] = newEnvironmentId
    884:  988 
    885:  989 createEnvironmentOps.push(
    886:  → 990   this.prisma.environment.create(
    887:  Unique constraint failed on the fields: (`projectId`,`name`)�[39m
    888:  PrismaClientKnownRequestError: 
    889:  Invalid `this.prisma.environment.create()` invocation in
    890:  /home/runner/work/keyshade/keyshade/apps/api/src/project/service/project.service.ts:990:33
    891:  987 envNameToIdMap[environment.name] = newEnvironmentId
    892:  988 
    893:  989 createEnvironmentOps.push(
    894:  → 990   this.prisma.environment.create(
    895:  Unique constraint failed on the fields: (`projectId`,`name`)
    896:  at Ln.handleRequestError (/home/runner/work/keyshade/keyshade/node_modules/.pnpm/@[email protected][email protected]/node_modules/@prisma/client/runtime/library.js:121:7753)
    897:  at Ln.handleAndLogRequestError (/home/runner/work/keyshade/keyshade/node_modules/.pnpm/@[email protected][email protected]/node_modules/@prisma/client/runtime/library.js:121:7061)
    ...
    
    975:  console.info
    976:  secp256k1 unavailable, reverting to browser version
    977:  at Object.<anonymous> (../../node_modules/.pnpm/[email protected]/node_modules/eccrypto/index.js:23:13)
    978:  PASS api src/api-key/api-key.e2e.spec.ts
    979:  ● Console
    980:  console.info
    981:  secp256k1 unavailable, reverting to browser version
    982:  at Object.<anonymous> (../../node_modules/.pnpm/[email protected]/node_modules/eccrypto/index.js:23:13)
    983:  �[31m[Nest] 3605  - �[39m01/30/2025, 4:55:21 PM �[31m  ERROR�[39m �[38;5;3m[AuthService] �[39m�[31mInvalid email address: �[39m
    984:  �[31m[Nest] 3605  - �[39m01/30/2025, 4:55:21 PM �[31m  ERROR�[39m �[38;5;3m[AuthService] �[39m�[31mInvalid email address: abcdef�[39m
    985:  �[31m[Nest] 3605  - �[39m01/30/2025, 4:55:21 PM �[31m  ERROR�[39m �[38;5;3m[AuthService] �[39m�[31mInvalid login code for [email protected]: 123456�[39m
    ...
    
    993:  console.info
    994:  secp256k1 unavailable, reverting to browser version
    995:  at Object.<anonymous> (../../node_modules/.pnpm/[email protected]/node_modules/eccrypto/index.js:23:13)
    996:  PASS api src/app/app.e2e.spec.ts
    997:  ● Console
    998:  console.info
    999:  secp256k1 unavailable, reverting to browser version
    1000:  at Object.<anonymous> (../../node_modules/.pnpm/[email protected]/node_modules/eccrypto/index.js:23:13)
    1001:  Test Suites: 1 failed, 13 passed, 14 total
    1002:  Tests:       1 failed, 359 passed, 360 total
    1003:  Snapshots:   0 total
    1004:  Time:        105.648 s
    1005:  Ran all test suites.
    1006:  Force exiting Jest: Have you considered using `--detectOpenHandles` to detect async operations that kept running after all tests finished?
    1007:  /home/runner/work/keyshade/keyshade/apps/api:
    1008:  ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL  @keyshade/[email protected] e2e: `pnpm run e2e:prepare && cross-env NODE_ENV='e2e' DATABASE_URL='***localhost:5432/tests' jest --runInBand --config=jest.e2e-config.ts && pnpm run e2e:teardown`
    1009:  Exit status 1
    1010:  ELIFECYCLE  Command failed with exit code 1.
    1011:  ##[error]Process completed with exit code 1.
    

    @rajdip-b rajdip-b merged commit 06d8c44 into develop Jan 30, 2025
    5 of 6 checks passed
    @rajdip-b rajdip-b deleted the feat/api-key-preview branch January 30, 2025 17:00
    rajdip-b pushed a commit that referenced this pull request Jan 30, 2025
    ## [2.11.0-stage.8](v2.11.0-stage.7...v2.11.0-stage.8) (2025-01-30)
    
    ### 🚀 Features
    
    * **api, schema:** Add preview field in API Key ([#680](#680)) ([06d8c44](06d8c44))
    @rajdip-b
    Copy link
    Member Author

    🎉 This PR is included in version 2.11.0-stage.8 🎉

    The release is available on GitHub release

    Your semantic-release bot 📦🚀

    rajdip-b pushed a commit that referenced this pull request Feb 10, 2025
    ## [2.11.0](v2.10.0...v2.11.0) (2025-02-10)
    
    ### 🚀 Features
    
    * **api, schema:** Add preview field in API Key ([#680](#680)) ([06d8c44](06d8c44))
    * **api:** Workspace-membership invitationAccepted included ([#665](#665)) ([3877249](3877249))
    * **platform:** Add CopySVG icon to the Slug component and update imports ([#677](#677)) ([2ad93ba](2ad93ba))
    * **platform:** Add Google OAuth ([#689](#689)) ([ad3a3d2](ad3a3d2))
    * **platform:** Add new access level SVGs and integrate into ProjectCard component ([#678](#678)) ([cc3ef77](cc3ef77))
    * **platform:** Add new design for slug ([#675](#675)) ([2b8985c](2b8985c))
    * **platform:** Add SVGs to projectTabs ([#673](#673)) ([37bfddf](37bfddf))
    * **platform:** Added the feature for deleting a [secure] ([#674](#674)) ([37e7960](37e7960))
    * **platform:** Edit [secure] in project ([#684](#684)) ([1e34030](1e34030))
    * **platform:** Restructure workspace settings and user settings ([#682](#682)) ([cd0013a](cd0013a))
    * **platform:** Update table ui and change variable to accordion ([#676](#676)) ([71e9ae9](71e9ae9))
    * **project:** Edit project feature ([#685](#685)) ([a906920](a906920))
    * Update details in listing [secure]s ([#686](#686)) ([84aa5f4](84aa5f4))
    * Variables listing revamp ([#735](#735)) ([38b42fa](38b42fa))
    
    ### 🐛 Bug Fixes
    
    * **api:** Convert email to lowercase ([#694](#694)) ([b41db33](b41db33))
    * **api:** Github OAuth redirect not working ([#692](#692)) ([3495f8a](3495f8a))
    * **api:** Project hard sync existing entities deleted ([#660](#660)) ([3632217](3632217))
    * **cli:** Version flag causing errors ([#679](#679)) ([65bb70b](65bb70b))
    * **platform:** ContextMenu not working on variable card ([#688](#688)) ([fbb147a](fbb147a))
    * **platform:** Fixed the typo in query params ([#723](#723)) ([6c6bb7f](6c6bb7f))
    
    ### 📚 Documentation
    
    * Added section for building packages ([#720](#720)) ([ecfde92](ecfde92))
    
    ### 🔧 Miscellaneous Chores
    
    * **api:** Remove failing environment tests ([d1b9767](d1b9767))
    * **cli:** Bumped CLI to v2.5.1 ([2d39815](2d39815))
    * **platform:** Fixed formatting ([81f86de](81f86de))
    * **release:** 2.11.0-stage.1 [skip ci] ([b442fe0](b442fe0)), closes [#675](#675)
    * **release:** 2.11.0-stage.10 [skip ci] ([cf34066](cf34066)), closes [#665](#665)
    * **release:** 2.11.0-stage.11 [skip ci] ([1344cf1](1344cf1)), closes [#660](#660)
    * **release:** 2.11.0-stage.12 [skip ci] ([92cecfc](92cecfc)), closes [#686](#686)
    * **release:** 2.11.0-stage.13 [skip ci] ([c91d48a](c91d48a)), closes [#684](#684)
    * **release:** 2.11.0-stage.14 [skip ci] ([5d20407](5d20407)), closes [#688](#688)
    * **release:** 2.11.0-stage.15 [skip ci] ([110e265](110e265)), closes [#685](#685)
    * **release:** 2.11.0-stage.16 [skip ci] ([2a7cba6](2a7cba6)), closes [#689](#689)
    * **release:** 2.11.0-stage.17 [skip ci] ([e071a74](e071a74)), closes [#692](#692) [#690](#690)
    * **release:** 2.11.0-stage.18 [skip ci] ([94f3938](94f3938)), closes [#694](#694)
    * **release:** 2.11.0-stage.19 [skip ci] ([f9b095c](f9b095c)), closes [#723](#723) [#720](#720)
    * **release:** 2.11.0-stage.2 [skip ci] ([f9d05de](f9d05de)), closes [#673](#673)
    * **release:** 2.11.0-stage.20 [skip ci] ([02972f2](02972f2)), closes [#735](#735)
    * **release:** 2.11.0-stage.3 [skip ci] ([c2398a6](c2398a6)), closes [#677](#677)
    * **release:** 2.11.0-stage.4 [skip ci] ([6c7e41a](6c7e41a)), closes [#676](#676)
    * **release:** 2.11.0-stage.5 [skip ci] ([defdbcd](defdbcd)), closes [#678](#678)
    * **release:** 2.11.0-stage.6 [skip ci] ([5060fe7](5060fe7)), closes [#679](#679)
    * **release:** 2.11.0-stage.7 [skip ci] ([b2be010](b2be010)), closes [#674](#674)
    * **release:** 2.11.0-stage.8 [skip ci] ([972e55b](972e55b)), closes [#680](#680)
    * **release:** 2.11.0-stage.9 [skip ci] ([fd92c3b](fd92c3b)), closes [#682](#682)
    
    ### 🔨 Code Refactoring
    
    * **platform:** Updated the [secure] table and changed edit variable dialog to a sheet ([#690](#690)) ([f51ad34](f51ad34))
    @rajdip-b
    Copy link
    Member Author

    🎉 This PR is included in version 2.11.0 🎉

    The release is available on GitHub release

    Your semantic-release bot 📦🚀

    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.

    1 participant