From f37c27bfc50c65bc409aa5507fa1907a6fb9d269 Mon Sep 17 00:00:00 2001 From: Oleksii Holub <1935960+Tyrrrz@users.noreply.github.com> Date: Tue, 28 Nov 2023 13:23:08 +0200 Subject: [PATCH] Route Passwordless API errors in `PasswordlessService` (#84) * Route Passwordless API errors in `PasswordlessService` * Remove test for an aspect that no longer exists --- .../Implementations/PasswordlessService.cs | 275 +++++++++++------- .../Program.cs | 1 - .../EndpointTests.cs | 2 +- .../OldTests/PasswordlessServiceTests.cs | 13 - 4 files changed, 175 insertions(+), 116 deletions(-) diff --git a/src/Passwordless.AspNetCore/Services/Implementations/PasswordlessService.cs b/src/Passwordless.AspNetCore/Services/Implementations/PasswordlessService.cs index 570f39e..674b225 100644 --- a/src/Passwordless.AspNetCore/Services/Implementations/PasswordlessService.cs +++ b/src/Passwordless.AspNetCore/Services/Implementations/PasswordlessService.cs @@ -17,6 +17,9 @@ namespace Passwordless.AspNetCore.Services.Implementations; public class PasswordlessService : PasswordlessService where TUser : class, new() { + /// + /// Initializes an instance of . + /// public PasswordlessService( IPasswordlessClient passwordlessClient, IUserStore userStore, @@ -37,6 +40,9 @@ public class PasswordlessService { private readonly ILogger> _logger; + /// + /// Initializes an instance of . + /// public PasswordlessService( IPasswordlessClient passwordlessClient, IUserStore userStore, @@ -66,70 +72,95 @@ public PasswordlessService( protected AuthenticationOptions? AuthenticationOptions { get; } protected UserManager? UserManager { get; } - public virtual async Task AddCredentialAsync(PasswordlessAddCredentialRequest request, ClaimsPrincipal claimsPrincipal, CancellationToken cancellationToken) + /// + public virtual async Task AddCredentialAsync( + PasswordlessAddCredentialRequest request, + ClaimsPrincipal claimsPrincipal, + CancellationToken cancellationToken) { - var userId = await GetUserIdAsync(claimsPrincipal, cancellationToken); - - if (string.IsNullOrEmpty(userId)) + try { - return Unauthorized(); - } - - - var user = await UserStore.FindByIdAsync(userId, cancellationToken); + var userId = await GetUserIdAsync(claimsPrincipal, cancellationToken); - if (user is null) - { - _logger.LogDebug("Could not find user with id {UserId} while attempting to add credential", userId); - return Unauthorized(); - } + if (string.IsNullOrEmpty(userId)) + { + return Unauthorized(); + } - _logger.LogInformation("Found user {UserId} while attempting to add credential", userId); + var user = await UserStore.FindByIdAsync(userId, cancellationToken); - var username = await UserStore.GetUserNameAsync(user, cancellationToken); + if (user is null) + { + _logger.LogDebug("Could not find user with id {UserId} while attempting to add credential", userId); + return Unauthorized(); + } - if (string.IsNullOrEmpty(username)) - { - return Unauthorized(); - } + _logger.LogInformation("Found user {UserId} while attempting to add credential", userId); - UserInformation userInformation; - if (IdentityOptions?.User.RequireUniqueEmail is true && UserStore is IUserEmailStore emailStore) - { - var email = await emailStore.GetEmailAsync(user, cancellationToken); + var username = await UserStore.GetUserNameAsync(user, cancellationToken); - if (string.IsNullOrEmpty(email)) + if (string.IsNullOrEmpty(username)) { return Unauthorized(); } - userInformation = new UserInformation(username, request.DisplayName, new HashSet { email }); - } - else - { - userInformation = new UserInformation(username, request.DisplayName, null); - } + UserInformation userInformation; + if (IdentityOptions?.User.RequireUniqueEmail is true && UserStore is IUserEmailStore emailStore) + { + var email = await emailStore.GetEmailAsync(user, cancellationToken); - var registerOptions = CreateRegisterOptions(userId, userInformation); + if (string.IsNullOrEmpty(email)) + { + return Unauthorized(); + } - // I could check if the customize service is the noop one and not allocate this context class - // which would make this a bit more pay-for-play. - var customizeContext = new CustomizeRegisterOptionsContext(false, registerOptions); - await CustomizeRegisterOptions.CustomizeAsync(customizeContext, cancellationToken); + userInformation = new UserInformation(username, request.DisplayName, new HashSet { email }); + } + else + { + userInformation = new UserInformation(username, request.DisplayName, null); + } - if (customizeContext.Options is null) - { - return Unauthorized(); - } + var registerOptions = CreateRegisterOptions(userId, userInformation); + + // I could check if the customize service is the noop one and not allocate this context class + // which would make this a bit more pay-for-play. + var customizeContext = new CustomizeRegisterOptionsContext(false, registerOptions); + await CustomizeRegisterOptions.CustomizeAsync(customizeContext, cancellationToken); + + if (customizeContext.Options is null) + { + return Unauthorized(); + } - var registerTokenResponse = await PasswordlessClient.CreateRegisterTokenAsync(customizeContext.Options, cancellationToken); + var registerTokenResponse = + await PasswordlessClient.CreateRegisterTokenAsync(customizeContext.Options, cancellationToken); - _logger.LogDebug("Successfully created a register token for user {UserId}", userId); + _logger.LogDebug("Successfully created a register token for user {UserId}", userId); - return Ok(registerTokenResponse); + return Ok(registerTokenResponse); + } + // Route Passwordless API errors to the corresponding result + catch (PasswordlessApiException ex) + { + _logger.LogDebug( + "Passwordless API responded with an error while attempting to add credential: {Error}", + ex.Details + ); + + return Problem( + ex.Details.Detail, + ex.Details.Instance, + ex.Details.Status, + ex.Details.Title, + ex.Details.Type + ); + } } - protected virtual ValueTask GetUserIdAsync(ClaimsPrincipal claimsPrincipal, CancellationToken cancellationToken) + protected virtual ValueTask GetUserIdAsync( + ClaimsPrincipal claimsPrincipal, + CancellationToken cancellationToken) { // Do we want to check if the Identity is authenticated? They could have multiple identities and the first one isn't authenticated if (claimsPrincipal.Identity?.IsAuthenticated is not true) @@ -150,89 +181,131 @@ public virtual async Task AddCredentialAsync(PasswordlessAddCredentialR return ValueTask.FromResult(userId); } - public virtual async Task RegisterUserAsync(TRegisterRequest request, CancellationToken cancellationToken) + /// + public virtual async Task RegisterUserAsync( + TRegisterRequest request, + CancellationToken cancellationToken) { - var user = new TUser(); - var result = await CreateUserAsync(user, request, cancellationToken); - - if (!result.Succeeded) + try { - return ValidationProblem(result.Errors.ToDictionary(e => e.Code, e => new[] { e.Description })); ; - } + var user = new TUser(); + var result = await CreateUserAsync(user, request, cancellationToken); - var userId = await UserStore.GetUserIdAsync(user, cancellationToken); - _logger.LogDebug("Registering user with id: {Id}", userId); + if (!result.Succeeded) + { + return ValidationProblem(result.Errors.ToDictionary(e => e.Code, e => new[] { e.Description })); + } - var aliases = request.Aliases ?? new HashSet(); + var userId = await UserStore.GetUserIdAsync(user, cancellationToken); + _logger.LogDebug("Registering user with id: {Id}", userId); - if (IdentityOptions?.User.RequireUniqueEmail is true && UserStore is IUserEmailStore emailStore) - { - if (string.IsNullOrEmpty(request.Email)) + var aliases = request.Aliases ?? new HashSet(); + + if (IdentityOptions?.User.RequireUniqueEmail is true && UserStore is IUserEmailStore emailStore) { - return ValidationProblem(new Dictionary + if (string.IsNullOrEmpty(request.Email)) { - { "invalid_email", new [] { "Email cannot be null or empty." }} - }); + return ValidationProblem(new Dictionary + { + { "invalid_email", new[] { "Email cannot be null or empty." } } + }); + } + + aliases.Add(request.Email); + } + else if (!string.IsNullOrEmpty(request.Email)) + { + _logger.LogWarning( + "An email was provided for {UserId}, but IdentityOptions.User.RequireUniqueEmail was not set to true so the email will not be used as an alias for the passkey.", + userId); } - aliases.Add(request.Email); - } - else if (!string.IsNullOrEmpty(request.Email)) - { - _logger.LogWarning("An email was provided for {UserId}, but IdentityOptions.User.RequireUniqueEmail was not set to true so the email will not be used as an alias for the passkey.", - userId); - } + var registerOptions = CreateRegisterOptions(userId, + new UserInformation(request.Username, request.DisplayName, aliases)); - var registerOptions = CreateRegisterOptions(userId, - new UserInformation(request.Username, request.DisplayName, aliases)); + // Customize register options + var customizeContext = new CustomizeRegisterOptionsContext(true, registerOptions); + await CustomizeRegisterOptions.CustomizeAsync(customizeContext, cancellationToken); - // Customize register options - var customizeContext = new CustomizeRegisterOptionsContext(true, registerOptions); - await CustomizeRegisterOptions.CustomizeAsync(customizeContext, cancellationToken); + if (customizeContext.Options is null) + { + // Is this the best result? + return Unauthorized(); + } - if (customizeContext.Options is null) + var token = await PasswordlessClient.CreateRegisterTokenAsync(customizeContext.Options, cancellationToken); + return Ok(token); + } + // Route Passwordless API errors to the corresponding result + catch (PasswordlessApiException ex) { - // Is this the best result? - return Unauthorized(); + _logger.LogDebug( + "Passwordless API responded with an error while attempting to register user: {Error}", + ex.Details + ); + + return Problem( + ex.Details.Detail, + ex.Details.Instance, + ex.Details.Status, + ex.Details.Title, + ex.Details.Type + ); } - - var token = await PasswordlessClient.CreateRegisterTokenAsync(customizeContext.Options, cancellationToken); - return Ok(token); } - public virtual async Task LoginUserAsync(PasswordlessLoginRequest loginRequest, CancellationToken cancellationToken) + /// + public virtual async Task LoginUserAsync( + PasswordlessLoginRequest loginRequest, + CancellationToken cancellationToken) { - var verifiedUser = await PasswordlessClient.VerifyTokenAsync(loginRequest.Token, cancellationToken); - - if (verifiedUser is null) + try { - _logger.LogDebug("User could not be verified with token {Token}", loginRequest.Token); - return Unauthorized(); - } + var verifiedUser = await PasswordlessClient.VerifyTokenAsync(loginRequest.Token, cancellationToken); - _logger.LogDebug("Attempting to find user in store by id {UserId}.", verifiedUser.UserId); - var user = await UserStore.FindByIdAsync(verifiedUser.UserId, cancellationToken); + _logger.LogDebug("Attempting to find user in store by id {UserId}.", verifiedUser.UserId); + var user = await UserStore.FindByIdAsync(verifiedUser.UserId, cancellationToken); - if (user is null) - { - _logger.LogDebug("Could not find user."); - return Unauthorized(); - } + if (user is null) + { + _logger.LogDebug("Could not find user."); + return Unauthorized(); + } - var claimsPrincipal = await UserClaimsPrincipalFactory.CreateAsync(user); + var claimsPrincipal = await UserClaimsPrincipalFactory.CreateAsync(user); - // First try our own scheme, then optionally try built in options but null is still allowed because it - // will then fallback to the default scheme. - var scheme = Options.SignInScheme - ?? AuthenticationOptions?.DefaultSignInScheme; + // First try our own scheme, then optionally try built in options but null is still allowed because it + // will then fallback to the default scheme. + var scheme = Options.SignInScheme + ?? AuthenticationOptions?.DefaultSignInScheme; - _logger.LogInformation("Signing in user with scheme {Scheme} and {NumberOfClaims} claims", - scheme, claimsPrincipal.Claims.Count()); + _logger.LogInformation("Signing in user with scheme {Scheme} and {NumberOfClaims} claims", + scheme, claimsPrincipal.Claims.Count()); - return SignIn(claimsPrincipal, authenticationScheme: scheme); + return SignIn(claimsPrincipal, authenticationScheme: scheme); + } + // Route Passwordless API errors to the corresponding result + catch (PasswordlessApiException ex) + { + _logger.LogDebug( + "Passwordless API responded with an error while attempting to login user: {Error}", + ex.Details + ); + + return Problem( + ex.Details.Detail, + ex.Details.Instance, + ex.Details.Status, + ex.Details.Title, + ex.Details.Type + ); + } } - protected virtual async Task CreateUserAsync(TUser user, TRegisterRequest request, CancellationToken cancellationToken) + protected virtual async Task CreateUserAsync( + TUser user, + TRegisterRequest request, + CancellationToken cancellationToken) { await UserStore.SetUserNameAsync(user, request.Username, cancellationToken); diff --git a/tests/Passwordless.AspNetCore.Tests.Dummy/Program.cs b/tests/Passwordless.AspNetCore.Tests.Dummy/Program.cs index b3a2cb6..ba22f44 100644 --- a/tests/Passwordless.AspNetCore.Tests.Dummy/Program.cs +++ b/tests/Passwordless.AspNetCore.Tests.Dummy/Program.cs @@ -3,7 +3,6 @@ using Microsoft.AspNetCore.Identity.EntityFrameworkCore; using Microsoft.Data.Sqlite; using Microsoft.EntityFrameworkCore; -using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; namespace Passwordless.AspNetCore.Tests.Dummy; diff --git a/tests/Passwordless.AspNetCore.Tests/EndpointTests.cs b/tests/Passwordless.AspNetCore.Tests/EndpointTests.cs index 46f4e6b..c7373af 100644 --- a/tests/Passwordless.AspNetCore.Tests/EndpointTests.cs +++ b/tests/Passwordless.AspNetCore.Tests/EndpointTests.cs @@ -107,7 +107,7 @@ public async Task I_can_define_a_register_endpoint_and_it_will_reject_duplicate_ responses.Skip(1).Should().OnlyContain(r => r.StatusCode == HttpStatusCode.BadRequest); } - [Fact(Skip = "Bug: this currently does not return 400 status code. Task: PAS-260")] + [Fact] public async Task I_can_define_a_signin_endpoint_and_it_will_reject_invalid_signin_attempts() { // Arrange diff --git a/tests/Passwordless.AspNetCore.Tests/OldTests/PasswordlessServiceTests.cs b/tests/Passwordless.AspNetCore.Tests/OldTests/PasswordlessServiceTests.cs index 3011766..1361e12 100644 --- a/tests/Passwordless.AspNetCore.Tests/OldTests/PasswordlessServiceTests.cs +++ b/tests/Passwordless.AspNetCore.Tests/OldTests/PasswordlessServiceTests.cs @@ -312,19 +312,6 @@ public async Task LoginUserAsync_TriesAuthenticationOptionsIfOursIsNull() Assert.Equal("auth_options_scheme", signInResult.AuthenticationScheme); } - [Fact] - public async Task LoginUserAsync_PasswordlessClientReturnsNull_ReturnsUnauthorized() - { - _mockPasswordlessClient - .Setup(s => s.VerifyTokenAsync("test_token", default)) - .Returns(Task.FromResult(null!)); - - var sut = CreateSut(); - - var result = await sut.LoginUserAsync(new PasswordlessLoginRequest("test_token"), CancellationToken.None); - Assert.IsAssignableFrom(result); - } - [Fact] public async Task LoginUserAsync_UserDoesNotExist_ReturnsUnauthorized() {