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

[PM-5518] Refactor Email Token Providers #3784

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

kspearrin
Copy link
Member

@kspearrin kspearrin commented Feb 9, 2024

Type of change

- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Refactor email token providers to use persistent IDistributedCache as the backing store for randomly generated tokens. This replaces existing TOTP functionality for email token, which has been determined to be inappropriate for our use cases of emailed tokens.

Code changes

  • EmailTokenProvider.cs: Get rid of calls into user service and keep all implementation details inside the token provider. Generate and validate randomly generated alpha and/or numeric strings using persistent IDistributedCache. Remove code related to the two-factor use case.
  • EmailTwoFactorTokenProvider.cs: Inherited implementation of EmailTokenProvider for two factor use cases. Does true token generation and validation instead of faking generation results with redacted emails like before.
  • UserService.cs: Call the UserManager (base class) generate/validate two-factor token methods, which in turn calls into our new EmailTwoFactorTokenProvider.
  • CoreHelpers.cs: Moved email redaction code into here.
  • BaseRequestValidator.cs: Generate redacted email from core helper rather than getting it from token provider results.
  • appsettings.Development.json: Add mail config to Identity dev project environment. This mirrors what we do in Api. Need this to send 2FA emails on login.
  • ServiceCollectionExtensions.cs: Register our custom token providers for CoreHelpers.CustomProviderName(TwoFactorProviderType.Email) and TokenOptions.DefaultEmailProvider instead of using the ASP.NET EmailTokenProvider.

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • If making database changes - make sure you also update Entity Framework queries and/or migrations
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

@bitwarden-bot
Copy link

bitwarden-bot commented Feb 9, 2024

Logo
Checkmarx One – Scan Summary & Details4537a7f0-9f02-425e-9c9b-ec2cf6184475

No New Or Fixed Issues Found

@kspearrin kspearrin changed the title [PM-5518] new email token providers [PM-5518] Refactor Email Token Providers Feb 12, 2024
@kspearrin kspearrin marked this pull request as ready for review February 12, 2024 17:05
@kspearrin kspearrin requested a review from a team as a code owner February 12, 2024 17:05
}

public async Task<bool> ValidateAsync(string purpose, string token, UserManager<User> manager, User user)
{
var cacheKey = string.Format(CacheKeyFormat, user.Id, token);
var cacheKey = string.Format(CacheKeyFormat, user.Id, user.SecurityStamp, purpose);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mirroring some of the implementation details from ASP.NET Core's EmailTokenProvider, which includes security stamp as a parameter in the token generation. We can include it as part of the cache key to get similar benefits.

@kspearrin
Copy link
Member Author

kspearrin commented Feb 12, 2024

One issue that I thought about here is self hosted customers who are using memory-based IDistributedCache (the default) will run into issues when token validation is needed across projects. For example, when using the "Send verification code email again" option, which happens from the API, but validation is done in Identity. Perhaps we should move that API endpoint to Identity? Or expect customers to start using a persistent cache implementation. Perhaps we could use SQL Server as a backing store.

@kspearrin
Copy link
Member Author

One issue that I thought about here is self hosted customers who are using memory-based IDistributedCache (the default) will run into issues when token validation is needed across projects. For example, when using the "Send verification code email again" option, which happens from the API, but validation is done in Identity. Perhaps we should move that API endpoint to Identity? Or expect customers to start using a persistent cache implementation. Perhaps we could use SQL Server as a backing store.

This concern is being addressed with #3791

Copy link
Member

@jlf0dev jlf0dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good to me so far, just some failing tests

Copy link

codecov bot commented Feb 20, 2024

Codecov Report

Attention: 49 lines in your changes are missing coverage. Please review.

Comparison is base (a661ffd) 36.36% compared to head (a895906) 36.36%.

Files Patch % Lines
src/Core/Utilities/CoreHelpers.cs 0.00% 25 Missing ⚠️
src/Core/Auth/Identity/EmailTokenProvider.cs 26.66% 11 Missing ⚠️
.../Core/Auth/Identity/EmailTwoFactorTokenProvider.cs 69.23% 7 Missing and 1 partial ⚠️
...rc/Identity/IdentityServer/BaseRequestValidator.cs 0.00% 3 Missing ⚠️
src/Core/Services/Implementations/UserService.cs 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3784      +/-   ##
==========================================
- Coverage   36.36%   36.36%   -0.01%     
==========================================
  Files        1157     1158       +1     
  Lines       55884    55905      +21     
  Branches     5376     5377       +1     
==========================================
+ Hits        20324    20329       +5     
- Misses      34614    34631      +17     
+ Partials      946      945       -1     

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

@kspearrin
Copy link
Member Author

This is looking good to me so far, just some failing tests

Fixed tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants