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): Add resend otp implementation #445

Merged
merged 10 commits into from
Oct 13, 2024

Conversation

Prakhargarg-2010196
Copy link
Contributor

@Prakhargarg-2010196 Prakhargarg-2010196 commented Sep 17, 2024

User description

Description

Base implemented by using present functions to resend otp to the user.
Tried using the already present sendotp function. But as the errors returned are different in the resend otp took few other functions and called those instead of calling sendOtp().

Fixes #326

Dependencies

Mention any dependencies/packages used

Future Improvements

  • Throttler fails on test as well not implements even applying on global as async module

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 are breaking another fix/feature of the project

Documentation Update


PR Type

enhancement, dependencies


Description

  • Implemented a new resendOtp endpoint in the AuthController with throttling to limit the number of OTP resend requests.
  • Added ThrottlerModule and ThrottlerGuard to the application for rate limiting API requests.
  • Updated AuthService to include a resendOtp method with error handling.
  • Introduced new environment variables THROTTLE_TTL and THROTTLE_LIMIT for configuring throttling behavior.
  • Added @nestjs/throttler to the project dependencies and updated the lock file accordingly.

Changes walkthrough 📝

Relevant files
Enhancement
app.module.ts
Add throttling configuration and guard to AppModule           

apps/api/src/app/app.module.ts

  • Added ThrottlerModule configuration for rate limiting.
  • Introduced ThrottlerGuard as a global guard.
  • +16/-2   
    auth.controller.ts
    Implement resend OTP endpoint with throttling                       

    apps/api/src/auth/controller/auth.controller.ts

  • Added resendOtp endpoint with throttling.
  • Applied ThrottlerGuard to resendOtp endpoint.
  • +12/-1   
    auth.service.ts
    Add resend OTP functionality to AuthService                           

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

  • Implemented resendOtp method in AuthService.
  • Added error logging for OTP resend failures.
  • +29/-5   
    Configuration changes
    .env.example
    Add throttling configuration variables to .env.example     

    .env.example

    • Added THROTTLE_TTL and THROTTLE_LIMIT environment variables.
    +6/-1     
    Dependencies
    package.json
    Update package.json with throttler dependency                       

    apps/api/package.json

  • Added @nestjs/throttler as a dependency.
  • Reordered reflect-metadata in devDependencies.
  • +2/-1     
    pnpm-lock.yaml
    Update pnpm-lock.yaml for new dependencies                             

    pnpm-lock.yaml

  • Updated lock file to include @nestjs/throttler.
  • Reflected changes in dependencies and versions.
  • +237/-83

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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Configuration Concern
    The ThrottlerModule is configured with environment variables, but there's no validation or default values set. This could lead to runtime errors if the variables are not properly set.

    Error Handling
    The resendOtp method catches all errors and only logs them. This could hide important errors and make debugging difficult.

    Rate Limiting Configuration
    The resendOtp endpoint has a hard-coded rate limit that might not be flexible for different environments or use cases.

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Sep 17, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Input validation
    Add input validation for the email parameter in the resendOtp method

    Consider adding input validation for the email parameter in the resendOtp method.
    This can help prevent unnecessary processing and improve security by ensuring only
    valid email addresses are processed.

    apps/api/src/auth/controller/auth.controller.ts [49-58]

     @Public()
     @Post('resend-otp/:email')
     @UseGuards(ThrottlerGuard)
     @Throttle({ default: { ttl: seconds(1), limit: 2 } })
     async resendOtp(
    -  @Param('email')
    +  @Param('email', new ParseEmailPipe())
       email: string
     ): Promise<void> {
       return await this.authService.resendOtp(email)
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding input validation for the email parameter enhances security and prevents unnecessary processing of invalid data, which is crucial for maintaining the integrity of the application.

    9
    Error handling
    Implement more specific error handling in the catch block

    Consider handling specific error types separately in the catch block of the
    resendOtp method. This would allow for more precise error handling and logging,
    potentially improving the ability to debug and respond to different types of errors.

    apps/api/src/auth/service/auth.service.ts [68-74]

     try {
       const user = await getUserByEmailOrId(email, this.prisma)
       const otp = await generateOtp(email, user.id, this.prisma)
       await this.mailService.sendOtp(email, otp.code)
     } catch (e) {
    -  this.logger.error(`Resending OTP failed ${e}`)
    +  if (e instanceof PrismaClientKnownRequestError) {
    +    this.logger.error(`Database error while resending OTP: ${e.message}`)
    +  } else if (e instanceof MailServiceError) {
    +    this.logger.error(`Mail service error while resending OTP: ${e.message}`)
    +  } else {
    +    this.logger.error(`Unexpected error while resending OTP: ${e}`)
    +  }
    +  throw new InternalServerErrorException('Failed to resend OTP')
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion improves error handling by distinguishing between different error types, which enhances debugging and provides more informative logging. It also introduces an InternalServerErrorException, which is a good practice for handling unexpected errors.

    8
    Enhancement
    ✅ Provide default values for throttling configuration

    Add default values for the THROTTLE_TTL and THROTTLE_LIMIT variables to provide
    guidance for users setting up the environment.

    .env.example [54-56]

     # Enter time in secs
    -THROTTLE_TTL=
    -THROTTLE_LIMIT=
    +THROTTLE_TTL=60
    +THROTTLE_LIMIT=10
     

    [Suggestion has been applied]

    Suggestion importance[1-10]: 8

    Why: Adding default values for environment variables can guide users in setting up their environment correctly and ensure that the application has sensible defaults, improving usability and reducing setup errors.

    8
    Code cleanup
    ✅ Remove TODO comments and rely on controller-level throttling

    Remove the TODO comments and implement the actual throttling logic in the resendOtp
    method. The throttling should be handled at the controller level using the @Throttle
    decorator, as shown in the auth.controller.ts file.

    apps/api/src/auth/service/auth.service.ts [64-67]

    -/**
    - *TODO Resend the otp based on another function send otp but
    - *TODO with a throttler to rate limit the api
    - */
    +// Throttling is handled at the controller level
     

    [Suggestion has been applied]

    Suggestion importance[1-10]: 7

    Why: Removing the TODO comments clarifies that throttling is already handled at the controller level, which improves code readability and reduces confusion about where throttling logic should be implemented.

    7
    Best practice
    Specify exact version for "@nestjs/throttler" package

    Consider adding a specific version for the "@nestjs/throttler" package instead of
    using the caret (^) to ensure consistency across environments.

    apps/api/package.json [35]

    -"@nestjs/throttler": "^6.2.1",
    +"@nestjs/throttler": "6.2.1",
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Specifying an exact version can help ensure consistency across different environments, reducing the risk of unexpected behavior due to version changes. However, it may limit flexibility in receiving minor updates and patches.

    7
    Configuration management
    ✅ Use type-safe configuration retrieval for throttle settings

    Consider using environment variables or a configuration file for the throttle limit
    and TTL values instead of hardcoding them. This would make the application more
    flexible and easier to configure in different environments.

    apps/api/src/app/app.module.ts [41-50]

     ThrottlerModule.forRootAsync({
       imports: [ConfigModule],
       inject: [ConfigService],
       useFactory: (config: ConfigService) => [
         {
    -      ttl: seconds(config.get('THROTTLE_TTL')),
    -      limit: config.get('THROTTLE_LIMIT')
    +      ttl: seconds(config.get<number>('THROTTLE_TTL')),
    +      limit: config.get<number>('THROTTLE_LIMIT')
         }
       ]
     }),
     

    [Suggestion has been applied]

    Suggestion importance[1-10]: 6

    Why: Using type-safe configuration retrieval is a minor improvement that enhances code safety and maintainability, ensuring that the configuration values are of the expected type. However, the impact is relatively small compared to other suggestions.

    6

    💡 Need additional feedback ? start a PR chat

    apps/api/src/auth/service/auth.service.ts Outdated Show resolved Hide resolved
    apps/api/src/auth/service/auth.service.ts Outdated Show resolved Hide resolved
    .env.example Outdated Show resolved Hide resolved
    .env.example Outdated Show resolved Hide resolved
    Fixes the throttle feature and moved the env variables to env.schema for
    validation of the env variables needed for throttle configuration.
    @Prakhargarg-2010196
    Copy link
    Contributor Author

    @rajdip-b Can you review this PR again? As of now the throttler is working as expected.
    I have moved the throttler config from app.module.ts to auth.module.ts instead. Also as i was unable to find any env/env.ts file in the directory, I created two variables under api/common/env/env.schema.ts for the throttle config.

    @rajdip-b
    Copy link
    Member

    rajdip-b commented Oct 4, 2024

    @rajdip-b Can you review this PR again? As of now the throttler is working as expected. I have moved the throttler config from app.module.ts to auth.module.ts instead. Also as i was unable to find any env/env.ts file in the directory, I created two variables under api/common/env/env.schema.ts for the throttle config.

    Yeah, your changes are valid.

    @Prakhargarg-2010196

    This comment has been minimized.

    @rajdip-b
    Copy link
    Member

    rajdip-b commented Oct 6, 2024

    You PR still has merge conflicts. Once they are removed, we can merge this

    @Prakhargarg-2010196
    Copy link
    Contributor Author

    You PR still has merge conflicts. Once they are removed, we can merge this

    Sure will do

    @Prakhargarg-2010196
    Copy link
    Contributor Author

    Screenshot_2024-10-07-11-06-57-080_com.github.android.jpg
    Hey @rajdip-b I have resolved the conflict although this i am seeing in the github checks can you tell me how to resolve this?

    The current auth.controller.spec.ts wasn't working with the
    throttler Guard, so created default/empty mocks which can be modified
    further.
    .env.example Outdated Show resolved Hide resolved
    Copy link

    codecov bot commented Oct 13, 2024

    Codecov Report

    Attention: Patch coverage is 42.85714% with 8 lines in your changes missing coverage. Please review.

    Project coverage is 87.64%. Comparing base (ce50743) to head (dc4b166).
    Report is 195 commits behind head on develop.

    Files with missing lines Patch % Lines
    apps/api/src/auth/service/auth.service.ts 0.00% 4 Missing ⚠️
    apps/api/src/auth/controller/auth.controller.ts 50.00% 2 Missing ⚠️
    apps/api/src/common/env/env.schema.ts 0.00% 2 Missing ⚠️
    Additional details and impacted files
    @@             Coverage Diff             @@
    ##           develop     #445      +/-   ##
    ===========================================
    - Coverage    91.71%   87.64%   -4.08%     
    ===========================================
      Files          111      105       -6     
      Lines         2510     2743     +233     
      Branches       469      415      -54     
    ===========================================
    + Hits          2302     2404     +102     
    - Misses         208      339     +131     
    Flag Coverage Δ
    api-e2e-tests 87.64% <42.85%> (-4.08%) ⬇️

    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.

    @rajdip-b rajdip-b merged commit 4dc6aa1 into keyshade-xyz:develop Oct 13, 2024
    9 of 10 checks passed
    @rajdip-b rajdip-b added the hacktoberfest-accepted Your PR has this = Congrats! label Oct 13, 2024
    Kiranchaudhary537 pushed a commit to Kiranchaudhary537/keyshade that referenced this pull request Oct 13, 2024
    rajdip-b pushed a commit that referenced this pull request Oct 24, 2024
    ## [2.6.0](v2.5.0...v2.6.0) (2024-10-24)
    
    ### 🚀 Features
    
    * **api:**  Add icon and remove description field from workspace ([#435](#435)) ([a99c0db](a99c0db))
    * **api-client:** Added workspace-membership and related tests ([#452](#452)) ([6a1c091](6a1c091))
    * **api-client:** Create controller for User module ([#484](#484)) ([f9d8e83](f9d8e83))
    * **api:** Add prod env schema in env file ([#436](#436)) ([21c3004](21c3004))
    * **api:** Add resend otp implementation ([#445](#445)) ([4dc6aa1](4dc6aa1))
    * **api:** Fetch total count of environments, [secure]s and variables in project ([#434](#434)) ([0c9e50a](0c9e50a))
    * **api:** Replace `projectId` with `name` and `slug` in workspace-role response.  ([#432](#432)) ([af06071](af06071))
    * **cli:** Add functionality to operate on Secrets ([#504](#504)) ([1b4bf2f](1b4bf2f))
    * **cli:** Add project command ([#451](#451)) ([70448e1](70448e1))
    * **cli:** Add workspace operations ([#441](#441)) ([ed38d22](ed38d22))
    * **cli:** implement commands to get, list, update, and delete, workspace roles ([#469](#469)) ([957ea8d](957ea8d))
    * **cli:** Implemented pagination support ([#453](#453)) ([feb1806](feb1806))
    * **cli:** Secret scan ([#438](#438)) ([85cb8ab](85cb8ab))
    * **cli:** Update environment command outputs ([f4af874](f4af874))
    * **platform:** Clearing email field after waitlisting the user email ([#481](#481)) ([256d659](256d659))
    * Remove project IDs from workspace role export data and update tests ([#448](#448)) ([8fdb328](8fdb328))
    * **web:** Configured extra check for waitlisted users already in the list and created toast message for them ([#492](#492)) ([2ddd0ef](2ddd0ef))
    * **web:** show the toast only when email add successfully ([#490](#490)) ([783c411](783c411))
    
    ### 🐛 Bug Fixes
    
    * **api,api-client:** Add environmentSlug in multiple places across the variable module ([#468](#468)) ([d970aff](d970aff))
    * **api:** Replace the id with slug in the global-search service ([#455](#455)) ([74804b1](74804b1))
    * **platform:** Fixed duplicate Google Logo UI fix  ([#450](#450)) ([fb0d6f7](fb0d6f7))
    * resolve footer website name cut-off or overlap issue ([#444](#444)) ([fe03ba2](fe03ba2))
    * **web:** Horizontal Scrolling issue on the website ([#440](#440)) ([655177b](655177b))
    
    ### 📚 Documentation
    
    * Add documentation for environment in CLI ([#462](#462)) ([dad7394](dad7394))
    * Add documentation for project in CLI ([#466](#466)) ([341fb32](341fb32))
    * Add documentation for scan in CLI ([#461](#461)) ([72281e6](72281e6))
    * Add documentation for workspace command ([#464](#464)) ([4aad8a2](4aad8a2))
    * Add instructions for resetting the local Prisma database ([#495](#495)) ([#501](#501)) ([b07ea17](b07ea17))
    * Added docker support documentation ([#465](#465)) ([bc04be4](bc04be4))
    * Added documentation for running the platform ([#473](#473)) ([8b8386b](8b8386b))
    * Added missing mappings to pages ([5de9fd8](5de9fd8))
    * Fix Documentation Hyperlink and update expired Discord invite link ([#496](#496)) ([5a10e39](5a10e39))
    * Updated CLI docs ([#460](#460)) ([c7e0f13](c7e0f13))
    
    ### 🔧 Miscellaneous Chores
    
    * Add more logging to Sentry init ([#470](#470)) ([de4925d](de4925d))
    * **api:** Optimise API docker image size ([#360](#360)) ([ea40dc1](ea40dc1))
    * **api:** Updated lockfile ([a968e78](a968e78))
    * **CI:** Add [secure] scan validation ([f441262](f441262))
    * **cli:** Update controller invocation in environment commands ([#477](#477)) ([596bd1a](596bd1a))
    * Minor changes to variables ([fe01ca6](fe01ca6))
    * **[secure]-scan:** Failing lint issues ([#507](#507)) ([48f45df](48f45df))
    * **[secure]-scan:** Formatted files ([5884833](5884833))
    * Update .env.example ([70ad4f7](70ad4f7))
    * Updated scripts ([9eb76a7](9eb76a7))
    * **web:** email validation ([#487](#487)) ([e8e737a](e8e737a))
    @rajdip-b
    Copy link
    Member

    🎉 This PR is included in version 2.6.0 🎉

    The release is available on GitHub release

    Your semantic-release bot 📦🚀

    @Prakhargarg-2010196 Prakhargarg-2010196 mentioned this pull request Nov 4, 2024
    2 tasks
    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: Resend OTP endpoint
    2 participants