[PM-6666] Two factor Validator refactor #13
Conversation
…thub.com/bitwarden/server into auth/pm-6666/twofactorvalidator-refactor
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
12 file(s) reviewed, 25 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -165,7 +159,6 @@ protected async Task ValidateAsync(T context, ValidatedTokenRequest request, | |||
| } | |||
| } | |||
|
|
|||
| // Returns true if can finish validation process | |||
| if (await IsValidAuthTypeAsync(user, request.GrantType)) | |||
There was a problem hiding this comment.
style: IsValidAuthTypeAsync might be better named as IsNonSsoAuthTypeAllowedAsync for clarity
| /// </summary> | ||
| /// <param name="user">The user is assumed NOT null, still going to check though</param> | ||
| /// <param name="request">Duende Validated Request that contains the data to create the device object</param> | ||
| /// <returns>Returns null if user or device is malformed; The existing device if already in DB; a new device login</returns> | ||
| public async Task<Device> SaveDeviceAsync(User user, ValidatedTokenRequest request) |
There was a problem hiding this comment.
style: Consider adding null checks for user and request parameters
| IOrganizationUserRepository organizationUserRepository, | ||
| IOrganizationRepository organizationRepository, | ||
| IDataProtectorTokenFactory<SsoEmail2faSessionTokenable> ssoEmail2faSessionTokeFactory, | ||
| ICurrentContext currentContext) : ITwoFactorAuthenticationValidator |
There was a problem hiding this comment.
syntax: There's a typo in the parameter name 'ssoEmail2faSessionTokeFactory'. It should be 'ssoEmail2faSessionTokenFactory'.
| private readonly IApplicationCacheService _applicationCacheService = applicationCacheService; | ||
| private readonly IOrganizationUserRepository _organizationUserRepository = organizationUserRepository; | ||
| private readonly IOrganizationRepository _organizationRepository = organizationRepository; | ||
| private readonly IDataProtectorTokenFactory<SsoEmail2faSessionTokenable> _ssoEmail2faSessionTokeFactory = ssoEmail2faSessionTokeFactory; |
There was a problem hiding this comment.
syntax: The typo in the field name '_ssoEmail2faSessionTokeFactory' should be corrected to '_ssoEmail2faSessionTokenFactory'.
| return null; | ||
| // await BuildErrorResultAsync("No two-step providers enabled.", false, context, user); | ||
| } |
There was a problem hiding this comment.
style: The commented-out code should be removed or implemented properly. If it's meant to handle the case when no providers are enabled, consider throwing an exception or returning an appropriate error response.
| /// <summary> | ||
| /// Modify this value to mock the responses from UserManager.GetValidTwoFactorProvidersAsync() | ||
| /// </summary> | ||
| public IList<string> TWO_FACTOR_PROVIDERS { get; set; } = []; |
There was a problem hiding this comment.
style: Consider using IReadOnlyList for better encapsulation
| public override async Task<bool> GetTwoFactorEnabledAsync(TUser user) | ||
| { | ||
| return TWO_FACTOR_ENABLED; | ||
| } |
There was a problem hiding this comment.
style: async keyword unnecessary for synchronous method
| public override async Task<IList<string>> GetValidTwoFactorProvidersAsync(TUser user) | ||
| { | ||
| return TWO_FACTOR_PROVIDERS; | ||
| } |
There was a problem hiding this comment.
style: async keyword unnecessary for synchronous method
| public override async Task<string> GenerateTwoFactorTokenAsync(TUser user, string tokenProvider) | ||
| { | ||
| return TWO_FACTOR_TOKEN; | ||
| } |
There was a problem hiding this comment.
style: async keyword unnecessary for synchronous method
| public override async Task<bool> VerifyTwoFactorTokenAsync(TUser user, string tokenProvider, string token) | ||
| { | ||
| return TWO_FACTOR_TOKEN_VERIFIED; | ||
| } |
There was a problem hiding this comment.
style: async keyword unnecessary for synchronous method
🎟️ Tracking
PM-6666
📔 Objective
Goal of this PR is to create a testable service for TwoFactorAuthentication by refactoring out the TwoFactor logic from the BaseRequestValidator.
There were already TwoFactor integration tests this PR is meant to add Unit Tests.
📸 Screenshots
Two Factor smoke tests to verify refactor hasn't changed core function
firefox_jnYuNYxoGn.mp4
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changesGreptile Summary
This pull request refactors the two-factor authentication logic into a dedicated service, improving testability and separation of concerns.
TwoFactorAuthenticationValidatorclass insrc/Identity/IdentityServer/TwoFactorValidator.csBaseRequestValidatorand updated related classes to use the new validatortest/Identity.Test/IdentityServer/TwoFactorAuthenticationValidatorTests.csUserManagerTestWrapperintest/Identity.Test/Wrappers/UserManagerTestWrapper.csto facilitate mocking of two-factor operationsServiceCollectionExtensionsto register the new two-factor authentication validator service