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): Added support for changing email of users #233

Merged
merged 12 commits into from
May 22, 2024

Conversation

rayaanoidPrime
Copy link
Contributor

@rayaanoidPrime rayaanoidPrime commented May 20, 2024

User description

Description

Give a summary of the change that you have made

Fixes #222

Dependencies

NA

Future Improvements

NA

Mentions

Screenshots of relevant screens

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


Description

  • Added support for changing user email with OTP validation.
  • Implemented endpoints for OTP validation and resending in UserController.
  • Introduced OTP generation, validation, and resending logic in UserService.
  • Created UserEmailChange table and defined its schema in Prisma.
  • Added end-to-end tests for email change OTP functionality.

Changes walkthrough 📝

Relevant files
Enhancement
user.controller.ts
Add OTP validation and resending endpoints in UserController

apps/api/src/user/controller/user.controller.ts

  • Added endpoints for OTP validation and resending.
  • Introduced validateOtp and resendOtp methods.
  • +13/-0   
    user.service.ts
    Implement OTP logic and email change handling in UserService

    apps/api/src/user/service/user.service.ts

  • Added logic for OTP generation, validation, and resending.
  • Introduced validateOtp and resendOtp methods.
  • Added email change handling with OTP.
  • +115/-1 
    migration.sql
    Create UserEmailChange table and add unique index               

    apps/api/src/prisma/migrations/20240519122223_add_user_email_change/migration.sql

    • Created UserEmailChange table.
    • Added unique index on userId.
    +13/-0   
    schema.prisma
    Define UserEmailChange model in Prisma schema                       

    apps/api/src/prisma/schema.prisma

  • Defined UserEmailChange model.
  • Added fields for userId, newEmail, otp, and createdOn.
  • +10/-0   
    Tests
    user.e2e.spec.ts
    Add end-to-end tests for email change OTP functionality   

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

  • Added tests for email change OTP functionality.
  • Included tests for OTP validation and resending.
  • +170/-0 

    💡 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 Description updated to latest commit (424505a)

    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR introduces several new features including OTP generation, validation, and email change functionality, which are spread across multiple files and involve both backend logic and database changes. The complexity of the changes and the need to ensure security and correctness in the OTP handling and email updating process require a thorough review.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The OTP validation logic does not account for the possibility of multiple OTP records for the same user due to concurrent operations. This could lead to unexpected behavior where an older OTP might still be considered valid if it hasn't expired.

    Performance Concern: The current implementation queries the database multiple times within the same request cycle, which could be optimized. For example, the user and email change records could be fetched in a single query.

    🔒 Security concerns

    No

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Hash OTPs before storing them in the database

    To enhance security, consider hashing the OTP before storing it in the database. This
    prevents exposure of plain-text OTPs in case of a data breach.

    apps/api/src/user/service/user.service.ts [59]

    -otp: randomUUID().slice(0, 6).toUpperCase()
    +otp: await bcrypt.hash(randomUUID().slice(0, 6).toUpperCase(), 10)
     
    Suggestion importance[1-10]: 10

    Why: Hashing OTPs before storing them significantly enhances security by preventing exposure of plain-text OTPs in case of a data breach. This is a critical security improvement.

    10
    Implement rate limiting for OTP generation to prevent abuse

    Implement a rate limiting mechanism on the OTP generation to prevent abuse through
    frequent requests, which can lead to resource exhaustion.

    apps/api/src/user/service/user.service.ts [55-60]

    +// Ensure rate limiting is checked before OTP generation
    +if (!await this.canGenerateOtp(user.id)) {
    +  throw new TooManyRequestsException('Too many OTP requests');
    +}
    +
     const otp = await this.prisma.userEmailChange.create({
       data: {
         newEmail: dto.email,
         userId: user.id,
         otp: randomUUID().slice(0, 6).toUpperCase()
       }
     })
     
    +// Implement this method in the UserService class
    +private async canGenerateOtp(userId: string): Promise<boolean> {
    +  // Add logic to check the number of OTP requests in a certain time frame
    +  return true; // Placeholder
    +}
    +
    Suggestion importance[1-10]: 8

    Why: Implementing rate limiting for OTP generation is important to prevent abuse through frequent requests, which can lead to resource exhaustion. This is a significant security enhancement.

    8
    Enhancement
    Add error handling for email sending failures

    Consider adding error handling for the sendOtp method in the mailService to manage
    potential failures in sending emails, which could leave the user without a way to complete
    their email change.

    apps/api/src/user/service/user.service.ts [63]

    -await this.mailService.sendOtp(dto.email, otp.otp)
    +try {
    +  await this.mailService.sendOtp(dto.email, otp.otp)
    +} catch (error) {
    +  this.log.error(`Failed to send OTP to ${dto.email}: ${error.message}`);
    +  throw new InternalServerErrorException('Failed to send OTP');
    +}
     
    Suggestion importance[1-10]: 9

    Why: Adding error handling for the sendOtp method is crucial to ensure that failures in sending emails are properly managed, preventing users from being left without a way to complete their email change. This enhances the robustness of the application.

    9
    Maintainability
    Refactor OTP generation into a separate method

    Refactor the OTP generation logic into a separate method to avoid code duplication and
    improve maintainability.

    apps/api/src/user/service/user.service.ts [59]

    -otp: randomUUID().slice(0, 6).toUpperCase()
    +otp: this.generateOtp()
     
    +// Add this method in the UserService class
    +private generateOtp(): string {
    +  return randomUUID().slice(0, 6).toUpperCase();
    +}
    +
    Suggestion importance[1-10]: 7

    Why: Refactoring the OTP generation logic into a separate method improves code maintainability and reduces duplication. While beneficial, it is not as critical as security or error handling improvements.

    7

    apps/api/src/prisma/schema.prisma Outdated Show resolved Hide resolved
    apps/api/src/user/controller/user.controller.ts Outdated Show resolved Hide resolved
    apps/api/src/user/controller/user.controller.ts Outdated Show resolved Hide resolved
    apps/api/src/user/controller/user.controller.ts Outdated Show resolved Hide resolved
    apps/api/src/user/controller/user.controller.ts Outdated Show resolved Hide resolved
    apps/api/src/user/service/user.service.ts Outdated Show resolved Hide resolved
    apps/api/src/user/service/user.service.ts Outdated Show resolved Hide resolved
    apps/api/src/user/service/user.service.ts Outdated Show resolved Hide resolved
    apps/api/src/user/user.e2e.spec.ts Outdated Show resolved Hide resolved
    apps/api/src/user/service/user.service.ts Outdated Show resolved Hide resolved
    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.

    As per the discussions in Discord thread, you would need to make a slight addition to this PR: you would need to change the user's auth provider to EMAIL_OTP if they successfully change their email.

    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 would need to update your branch with #230, where the method of generating otp has changed.

    apps/api/src/prisma/schema.prisma Outdated Show resolved Hide resolved
    apps/api/src/common/generate-otp.ts Outdated Show resolved Hide resolved
    apps/api/src/user/service/user.service.ts Outdated Show resolved Hide resolved
    apps/api/src/user/service/user.service.ts Outdated Show resolved Hide resolved
    apps/api/src/user/service/user.service.ts Outdated Show resolved Hide resolved
    apps/api/src/user/service/user.service.ts Outdated Show resolved Hide resolved
    Copy link

    codecov bot commented May 21, 2024

    Codecov Report

    Attention: Patch coverage is 88.00000% with 6 lines in your changes are missing coverage. Please review.

    Project coverage is 91.71%. Comparing base (7bb3d21) to head (ce50743).
    Report is 85 commits behind head on develop.

    Current head ce50743 differs from pull request most recent head 8a84f04

    Please upload reports for the commit 8a84f04 to get more accurate results.

    Files Patch % Lines
    apps/api/src/mail/services/mail.service.ts 0.00% 4 Missing ⚠️
    apps/api/src/user/service/user.service.ts 93.75% 2 Missing ⚠️
    Additional details and impacted files
    @@             Coverage Diff              @@
    ##           develop     #233       +/-   ##
    ============================================
    + Coverage    62.20%   91.71%   +29.50%     
    ============================================
      Files           76      111       +35     
      Lines         1503     2510     +1007     
      Branches       260      469      +209     
    ============================================
    + Hits           935     2302     +1367     
    + Misses         568      208      -360     
    Flag Coverage Δ
    api-e2e-tests 91.71% <88.00%> (+29.50%) ⬆️

    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.

    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.

    I think the code looks perfect! You just missed out creating the tests for the code you wrote. Codecov points to the lines in red that are currently not covered by the tests.

    @rayaanoidPrime
    Copy link
    Contributor Author

    Ahh okay. I'll get to the tests sometime in the evening. Just to be clear only the tests for the lines in the above two files as shown by Codecov is fine right ?

    @rajdip-b
    Copy link
    Member

    Yeap! Just write the tests for user.service.ts. You can leave the other one out.

    Copy link

    sonarcloud bot commented May 22, 2024

    Quality Gate Passed 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

    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.

    Awesome work man!

    @rajdip-b rajdip-b merged commit 5ea9a10 into keyshade-xyz:develop May 22, 2024
    5 checks passed
    @rayaanoidPrime rayaanoidPrime deleted the 222-user-email-change branch May 23, 2024 17:40
    rajdip-b pushed a commit that referenced this pull request May 24, 2024
    ## [1.4.0](v1.3.0...v1.4.0) (2024-05-24)
    
    ### 🚀 Features
    
    * add example for health and email auth ([b834d25](b834d25))
    * **api:** Add `minio-client` provider ([#237](#237)) ([cd71c5a](cd71c5a))
    * **api:** Add feature to fork projects ([#239](#239)) ([3bab653](3bab653))
    * **api:** Added feedback form module ([#210](#210)) ([ae1efd8](ae1efd8))
    * **api:** Added Project Level Access  ([#221](#221)) ([564f5ed](564f5ed))
    * **api:** Added support for changing email of users ([#233](#233)) ([5ea9a10](5ea9a10))
    * implemented auth, ui for most, and fixed cors ([#217](#217)) ([feace86](feace86))
    * **platfrom:** add delete method in api client ([#225](#225)) ([55cf09f](55cf09f))
    * **postman:** add example for get_self and update_self ([e015acf](e015acf))
    * **web:** Add and link privacy and tnc page ([#226](#226)) ([ec81eb9](ec81eb9))
    
    ### 🐛 Bug Fixes
    
    * **web:** docker next config not found ([#228](#228)) ([afe3160](afe3160))
    
    ### 📚 Documentation
    
    * Added docs regarding postman, and refactored architecture diagrams ([f1c9777](f1c9777))
    * Fix typo in organization-of-code.md ([#234](#234)) ([11244a2](11244a2))
    
    ### 🔧 Miscellaneous Chores
    
    * **api:** Get feedback forward email from process.env ([#236](#236)) ([204c9d1](204c9d1))
    * **postman:** Initialized postman ([bb76384](bb76384))
    * **release:** Update changelog config ([af91283](af91283))
    * Remove swagger docs ([#220](#220)) ([7640299](7640299))
    
    ### 🔨 Code Refactoring
    
    * **api:** Replaced OTP code from alphanumeric to numeric ([#230](#230)) ([f16162a](f16162a))
    @rajdip-b
    Copy link
    Member

    🎉 This PR is included in version 1.4.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.

    API: Support for changing email for users
    3 participants