From 99fd5619d91572b665a2c496914cf8462b99fc4f Mon Sep 17 00:00:00 2001 From: yasmoradi Date: Fri, 31 Jul 2026 17:57:08 +0200 Subject: [PATCH 1/2] improve bit Boilerplate identity (#12792) --- .../.template.config/template.json | 3 + .../Pages/Identity/ConfirmPage.razor.cs | 24 ++- .../Pages/Identity/SignIn/SignInPanel.razor | 4 +- .../Identity/SignIn/SignInPanel.razor.cs | 15 +- .../Pages/NotAuthorizedPage.razor.cs | 8 +- .../IdentityController.EmailConfirmation.cs | 10 +- .../IdentityController.ExternalSignIn.cs | 24 ++- .../IdentityController.PhoneConfirmation.cs | 3 +- .../IdentityController.ResetPassword.cs | 6 +- ...RemoteAuthenticationOptionsConfigurator.cs | 48 +++-- .../Infrastructure/Services/PhoneService.cs | 19 +- .../Extensions/UriExtensions.cs | 60 ++++--- .../Tests/Features/Identity/BunitUITests.cs | 4 +- .../Identity/ConfirmPageTwoFactorTests.cs | 105 +++++++++++ .../Features/Identity/OneTimeTokenTests.cs | 162 +++++++++++++++++ .../PhoneNumberNormalizationUITests.cs | 14 +- .../Identity/ReturnUrlHardeningTests.cs | 170 ++++++++++++++++++ 17 files changed, 612 insertions(+), 67 deletions(-) create mode 100644 src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/ConfirmPageTwoFactorTests.cs create mode 100644 src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/OneTimeTokenTests.cs create mode 100644 src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/ReturnUrlHardeningTests.cs diff --git a/src/Templates/Boilerplate/Bit.Boilerplate/.template.config/template.json b/src/Templates/Boilerplate/Bit.Boilerplate/.template.config/template.json index 47da6004a7..3506c9f76c 100644 --- a/src/Templates/Boilerplate/Bit.Boilerplate/.template.config/template.json +++ b/src/Templates/Boilerplate/Bit.Boilerplate/.template.config/template.json @@ -541,6 +541,9 @@ "src/Tests/Features/Identity/TwoFactorAuthTests.cs", "src/Tests/Features/Identity/PrivilegedSessionTests.cs", "src/Tests/Features/Identity/RefreshTokenRotationTests.cs", + "src/Tests/Features/Identity/ReturnUrlHardeningTests.cs", + "src/Tests/Features/Identity/OneTimeTokenTests.cs", + "src/Tests/Features/Identity/ConfirmPageTwoFactorTests.cs", "src/Tests/Features/Identity/UserGroupFeatureManagementUITests.cs", "src/Tests/Features/Seo/**", "src/Tests/Features/Tenants/TenantInvitationUITests.cs", diff --git a/src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/ConfirmPage.razor.cs b/src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/ConfirmPage.razor.cs index f8e334db6c..bdcbbf53ba 100644 --- a/src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/ConfirmPage.razor.cs +++ b/src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/ConfirmPage.razor.cs @@ -31,6 +31,8 @@ public partial class ConfirmPage [Parameter, SupplyParameterFromQuery(Name = "phoneToken")] public string? PhoneTokenQueryString { get; set; } + private string GetSafeReturnUrl() => Uri.IsAppRelativeUrl(ReturnUrlQueryString, requireLeadingSlash: false) ? ReturnUrlQueryString : PageUrls.Home; + protected override async Task OnInitAsync() { @@ -84,9 +86,18 @@ await WrapRequest(async () => Token = emailModel.Token }, CurrentCancellationToken); + if (signInResponse.RequiresTwoFactor) + { + // The e-mail is confirmed, but the automatic sign-in needs a second factor this page cannot collect + // (ConfirmEmailRequestDto carries no code). Storing the response would overwrite the caller's tokens + // with nulls, so show the "confirmed, now sign in" panel instead. + isEmailConfirmed = true; + return; + } + await AuthManager.StoreTokens(signInResponse, true); - NavigationManager.NavigateTo(ReturnUrlQueryString ?? PageUrls.Home, replace: true); + NavigationManager.NavigateTo(GetSafeReturnUrl(), replace: true); isEmailConfirmed = true; }); @@ -98,7 +109,7 @@ private async Task ResendEmailToken() await WrapRequest(async () => { - await identityController.SendConfirmEmailToken(new() { Email = emailModel.Email, ReturnUrl = ReturnUrlQueryString }, CurrentCancellationToken); + await identityController.SendConfirmEmailToken(new() { Email = emailModel.Email, ReturnUrl = GetSafeReturnUrl() }, CurrentCancellationToken); }); } @@ -114,9 +125,16 @@ await WrapRequest(async () => PhoneNumber = phoneModel.PhoneNumber }, CurrentCancellationToken); + if (signInResponse.RequiresTwoFactor) + { + // See the note in ConfirmEmail above. + isPhoneConfirmed = true; + return; + } + await AuthManager.StoreTokens(signInResponse, true); - NavigationManager.NavigateTo(ReturnUrlQueryString ?? PageUrls.Home, replace: true); + NavigationManager.NavigateTo(GetSafeReturnUrl(), replace: true); isPhoneConfirmed = true; }); diff --git a/src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPanel.razor b/src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPanel.razor index 7b70823bd2..04b0a0f84a 100644 --- a/src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPanel.razor +++ b/src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPanel.razor @@ -68,7 +68,7 @@ @Localizer[nameof(AppStrings.Password)] - @Localizer[nameof(AppStrings.ForgotPasswordLink)] + @Localizer[nameof(AppStrings.ForgotPasswordLink)] @@ -138,7 +138,7 @@ { @Localizer[nameof(AppStrings.DontHaveAccountMessage)] - @Localizer[nameof(AppStrings.SignUp)] + @Localizer[nameof(AppStrings.SignUp)] } diff --git a/src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPanel.razor.cs b/src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPanel.razor.cs index e3c8afe3c6..cf1a9c8600 100644 --- a/src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPanel.razor.cs +++ b/src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPanel.razor.cs @@ -18,7 +18,12 @@ public partial class SignInPanel private SignInPanelType internalSignInPanelType; private readonly SignInRequestDto model = new(); private AppDataAnnotationsValidator? validatorRef; - private string GetReturnUrl() => ReturnUrl ?? ReturnUrlQueryString ?? PageUrls.Home; + private string GetSafeReturnUrl() + { + var returnUrl = ReturnUrl ?? ReturnUrlQueryString; + + return Uri.IsAppRelativeUrl(returnUrl, requireLeadingSlash: false) ? returnUrl : PageUrls.Home; + } [Parameter] public string? ReturnUrl { get; set; } @@ -143,7 +148,7 @@ private async Task DoSignIn() if (isNewUser is false) { - model.ReturnUrl = GetReturnUrl(); + model.ReturnUrl = GetSafeReturnUrl(); requiresTwoFactor = await AuthManager.SignIn(model, CurrentCancellationToken); @@ -187,7 +192,7 @@ private async Task DoSignIn() } else { - NavigationManager.NavigateTo(GetReturnUrl(), replace: true); + NavigationManager.NavigateTo(GetSafeReturnUrl(), replace: true); } } } @@ -248,7 +253,7 @@ private async Task ExternalSignIn(string provider) var port = localHttpServer.EnsureStarted(); - var redirectUrl = await identityController.GetExternalSignInUri(provider, GetReturnUrl(), port is -1 ? null : port, CurrentCancellationToken); + var redirectUrl = await identityController.GetExternalSignInUri(provider, GetSafeReturnUrl(), port is -1 ? null : port, CurrentCancellationToken); await externalNavigationService.NavigateTo(redirectUrl); } @@ -324,7 +329,7 @@ private async Task SendOtp(bool resend) var request = new IdentityRequestDto { UserName = model.UserName, Email = model.Email, PhoneNumber = model.PhoneNumber }; - await identityController.SendOtp(request, GetReturnUrl(), CurrentCancellationToken); + await identityController.SendOtp(request, GetSafeReturnUrl(), CurrentCancellationToken); isOtpSent = true; } diff --git a/src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/NotAuthorizedPage.razor.cs b/src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/NotAuthorizedPage.razor.cs index f53e3a21b3..0ba7646731 100644 --- a/src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/NotAuthorizedPage.razor.cs +++ b/src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/NotAuthorizedPage.razor.cs @@ -9,6 +9,7 @@ public partial class NotAuthorizedPage [SupplyParameterFromQuery(Name = "return-url"), Parameter] public string? ReturnUrl { get; set; } + private string GetSafeReturnUrl() => Uri.IsAppRelativeUrl(ReturnUrl, requireLeadingSlash: false) ? ReturnUrl : PageUrls.Home; [AutoInject] private SignInModalService signInModalService = default!; @@ -28,9 +29,10 @@ protected override async Task OnAfterFirstRenderAsync() var accessToken = await AuthManager.RefreshToken(requestedBy: nameof(NotAuthorizedPage)); if (string.IsNullOrEmpty(accessToken) is false && ReturnUrl is not null && ReturnUrl.Contains("try_refreshing_token=false", StringComparison.InvariantCulture) is false) { + var returnUrl = GetSafeReturnUrl(); // To prevent infinities redirect loop, let's append try_refreshing_token=false to the url, so we only redirect in case no try_refreshing_token=false is present - var @char = ReturnUrl.Contains('?') ? '&' : '?'; // The RedirectUrl may already include a query string. - NavigationManager.NavigateTo($"{ReturnUrl}{@char}try_refreshing_token=false", replace: true); + var @char = returnUrl.Contains('?') ? '&' : '?'; // The RedirectUrl may already include a query string. + NavigationManager.NavigateTo($"{returnUrl}{@char}try_refreshing_token=false", replace: true); } } @@ -48,7 +50,7 @@ protected override async Task OnAfterFirstRenderAsync() private async Task SignIn() { await AuthManager.SignOut(CurrentCancellationToken); - var returnUrl = ReturnUrl ?? NavigationManager.GetRelativePath(); + var returnUrl = ReturnUrl is null ? NavigationManager.GetRelativePath() : GetSafeReturnUrl(); await signInModalService.SignIn(returnUrl); // Alternatively, you can redirect the user to the sign-in page. diff --git a/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.EmailConfirmation.cs b/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.EmailConfirmation.cs index 63e888d3a7..426776d2a3 100644 --- a/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.EmailConfirmation.cs +++ b/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.EmailConfirmation.cs @@ -28,7 +28,8 @@ public async Task ConfirmEmail(ConfirmEmailRequestDto request, CancellationToken var user = await userManager.FindByEmailAsync(request.Email!) ?? throw new BadRequestException(Localizer[nameof(AppStrings.UserNotFound)]).WithData("Email", request.Email); - var expired = (TimeProvider.GetUtcNow() - user.EmailTokenRequestedOn) > AppSettings.Identity.EmailTokenLifetime; + var expired = user.EmailTokenRequestedOn is null || + (TimeProvider.GetUtcNow() - user.EmailTokenRequestedOn.Value) > AppSettings.Identity.EmailTokenLifetime; if (expired) throw new BadRequestException(nameof(AppStrings.ExpiredToken)).WithData("UserId", user.Id); @@ -39,7 +40,7 @@ public async Task ConfirmEmail(ConfirmEmailRequestDto request, CancellationToken throw new BadRequestException(Localizer[nameof(AppStrings.UserLockedOut), (TimeProvider.GetUtcNow() - user.LockoutEnd!).Value.Humanize(culture: CultureInfo.CurrentUICulture)]).WithData("UserId", user.Id).WithExtensionData("TryAgainIn", tryAgainIn); } - var tokenIsValid = await userManager.VerifyUserTokenAsync(user, TokenOptions.DefaultPhoneProvider, FormattableString.Invariant($"VerifyEmail:{request.Email},{user.EmailTokenRequestedOn?.ToUniversalTime()}"), request.Token!); + var tokenIsValid = await userManager.VerifyUserTokenAsync(user, TokenOptions.DefaultPhoneProvider, FormattableString.Invariant($"VerifyEmail:{user.Email},{user.EmailTokenRequestedOn?.ToUniversalTime()}"), request.Token!); if (tokenIsValid is false) { @@ -66,7 +67,10 @@ public async Task ConfirmEmail(ConfirmEmailRequestDto request, CancellationToken private async Task SendConfirmEmailToken(User user, string? returnUrl, CancellationToken cancellationToken) { - returnUrl ??= PageUrls.Home; + if (Uri.IsAppRelativeUrl(returnUrl, requireLeadingSlash: false) is false) + { + returnUrl = PageUrls.Home; + } var resendDelay = (TimeProvider.GetUtcNow() - user.EmailTokenRequestedOn) - AppSettings.Identity.EmailTokenLifetime; diff --git a/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.ExternalSignIn.cs b/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.ExternalSignIn.cs index 4486095c65..297e7e5cec 100644 --- a/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.ExternalSignIn.cs +++ b/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.ExternalSignIn.cs @@ -11,8 +11,9 @@ public partial class IdentityController /// /// Deliberately not cached: the returned URL embeds the caller's origin, which GetWebAppUrl resolves from the X-Origin - /// header. That header is part of neither the output cache's vary rules nor a CDN's cache key, so a cached response would - /// hand one origin's sign-in URL to a caller coming from another, sending the resulting sign-in link to the wrong origin. + /// header. The output cache does vary by that header (See AppResponseCachePolicy.CacheRequestAsync), but a CDN edge + /// does not unless it is configured to, so a shared-cached response would hand one origin's sign-in URL to a caller + /// coming from another, sending the resulting sign-in link to the wrong origin. /// [HttpGet] public async Task GetExternalSignInUri(string provider, string? returnUrl = null, int? localHttpPort = null, CancellationToken cancellationToken = default) @@ -38,6 +39,9 @@ public async Task ExternalSignInCallback(string? returnUrl = null, string? signInPageUri; ExternalLoginInfo? info = null; + if (localHttpPort is not null and (< 1 or > 65535)) + throw new BadRequestException().WithData("localHttpPort", localHttpPort); + try { info = await signInManager.GetExternalLoginInfoAsync() ?? throw new BadRequestException().WithData("Reason", "External login info is missing."); @@ -46,6 +50,8 @@ public async Task ExternalSignInCallback(string? returnUrl = null, var user = await userManager.FindByLoginAsync(info.LoginProvider, info.ProviderKey); + var isLinked = user is not null; + if (user is null && (string.IsNullOrEmpty(email) is false || string.IsNullOrEmpty(phoneNumber) is false)) { user = await userManager.FindUser(new() { Email = email, PhoneNumber = phoneNumber }); @@ -86,8 +92,14 @@ public async Task ExternalSignInCallback(string? returnUrl = null, // Therefore, we assign a role to these users by default. await userManager.CreateUserWithDemoRole(user); } + } - await userManager.AddLoginAsync(user, info); + if (isLinked is false) + { + var addLoginResult = await userManager.AddLoginAsync(user, info); + + if (addLoginResult.Succeeded is false) + throw new ResourceValidationException(addLoginResult.Errors.Select(e => new LocalizedString(e.Code, e.Description)).ToArray()).WithData("UserId", user.Id); } // Confirmation is only as good as the provider's own verification of the identifier @@ -130,7 +142,11 @@ public async Task ExternalSignInCallback(string? returnUrl = null, } catch (Exception exp) { - serverExceptionHandler.Handle(exp, new() { { "LoginProvider", info?.LoginProvider }, { "Principal", info?.Principal?.GetDisplayName() } }); + var principalName = info?.Principal?.FindFirstValue("preferred_username") + ?? info?.Principal?.GetEmail() + ?? info?.Principal?.FindFirstValue(ClaimTypes.NameIdentifier); + + serverExceptionHandler.Handle(exp, new() { { "LoginProvider", info?.LoginProvider }, { "Principal", principalName } }); signInPageUri = $"{PageUrls.SignIn}?error={Uri.EscapeDataString(exp is KnownException ? Localizer[exp.Message] : Localizer[nameof(AppStrings.UnknownException)])}"; } finally diff --git a/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.PhoneConfirmation.cs b/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.PhoneConfirmation.cs index 28ef99bf5f..5119abfa44 100644 --- a/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.PhoneConfirmation.cs +++ b/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.PhoneConfirmation.cs @@ -30,7 +30,8 @@ public async Task ConfirmPhone(ConfirmPhoneRequestDto request, CancellationToken var user = await userManager.FindByPhoneNumber(request.PhoneNumber!) ?? throw new BadRequestException(Localizer[nameof(AppStrings.UserNotFound)]).WithData("PhoneNumber", request.PhoneNumber); - var expired = (TimeProvider.GetUtcNow() - user.PhoneNumberTokenRequestedOn) > AppSettings.Identity.PhoneNumberTokenLifetime; + var expired = user.PhoneNumberTokenRequestedOn is null || + (TimeProvider.GetUtcNow() - user.PhoneNumberTokenRequestedOn.Value) > AppSettings.Identity.PhoneNumberTokenLifetime; if (expired) throw new BadRequestException(nameof(AppStrings.ExpiredToken)).WithData("UserId", user.Id); diff --git a/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.ResetPassword.cs b/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.ResetPassword.cs index c09ad99671..9e4736d6f3 100644 --- a/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.ResetPassword.cs +++ b/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.ResetPassword.cs @@ -39,7 +39,8 @@ public async Task SendResetPasswordToken(SendResetPasswordTokenRequestDto reques var token = await userManager.GenerateUserTokenAsync(user, TokenOptions.DefaultPhoneProvider, FormattableString.Invariant($"ResetPassword,{user.ResetPasswordTokenRequestedOn?.ToUniversalTime()}")); var isEmail = string.IsNullOrEmpty(request.Email) is false; var qs = $"{(isEmail ? "email" : "phoneNumber")}={Uri.EscapeDataString(isEmail ? request.Email! : request.PhoneNumber!)}"; - var url = $"{PageUrls.ResetPassword}?token={Uri.EscapeDataString(token)}&{qs}&culture={CultureInfo.CurrentUICulture.Name}&return-url={Uri.EscapeDataString(request.ReturnUrl ?? PageUrls.Home)}"; + var returnUrl = Uri.IsAppRelativeUrl(request.ReturnUrl, requireLeadingSlash: false) ? request.ReturnUrl : PageUrls.Home; + var url = $"{PageUrls.ResetPassword}?token={Uri.EscapeDataString(token)}&{qs}&culture={CultureInfo.CurrentUICulture.Name}&return-url={Uri.EscapeDataString(returnUrl)}"; var link = new Uri(HttpContext.Request.GetWebAppUrl(), url); List sendMessagesTasks = []; @@ -82,7 +83,8 @@ public async Task ResetPassword(ResetPasswordRequestDto request, CancellationTok request.PhoneNumber = phoneService.NormalizePhoneNumber(request.PhoneNumber); var user = await userManager.FindUser(request) ?? throw new ResourceNotFoundException(Localizer[nameof(AppStrings.UserNotFound)]).WithData("Identifier", request); - var expired = (TimeProvider.GetUtcNow() - user.ResetPasswordTokenRequestedOn) > AppSettings.Identity.ResetPasswordTokenLifetime; + var expired = user.ResetPasswordTokenRequestedOn is null || + (TimeProvider.GetUtcNow() - user.ResetPasswordTokenRequestedOn.Value) > AppSettings.Identity.ResetPasswordTokenLifetime; if (expired) throw new BadRequestException(nameof(AppStrings.ExpiredToken)).WithData("UserId", user.Id); diff --git a/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/Services/RemoteAuthenticationOptionsConfigurator.cs b/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/Services/RemoteAuthenticationOptionsConfigurator.cs index d5fa3ddbc3..641ccaffea 100644 --- a/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/Services/RemoteAuthenticationOptionsConfigurator.cs +++ b/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/Services/RemoteAuthenticationOptionsConfigurator.cs @@ -10,15 +10,25 @@ namespace Boilerplate.Server.Api.Features.Identity.Services; /// -/// Configures External Identity Providers BackchannelHttpHandler to use Microsoft.Extensions.HttpClient Factory +/// Routes every external identity provider's backchannel through Microsoft.Extensions.Http's client factory, so the +/// named-client configuration (See ConfigureHttpClientFactoryForExternalIdentityProviders) and everything +/// ConfigureHttpClientDefaults applies - resilience, service discovery, the SocketsHttpHandler settings - reach it. +/// +/// OpenIdConnect and Apple are configured through rather than +/// on purpose. Their own framework post-configure builds a +/// ConfigurationManager around whatever Backchannel it finds and keeps that instance for the process lifetime, +/// so it has to already be the factory's client by then; assigning it afterwards leaves the discovery-document and +/// JWKS fetches on a bare HttpClient while only the token/userinfo calls get the configured one. The OAuth-family +/// handlers capture nothing, so they stay on post-configure. +/// /// public class RemoteAuthenticationOptionsConfigurator(IHttpClientFactory httpClientFactory) - : IPostConfigureOptions, + : IConfigureNamedOptions, + IConfigureNamedOptions, IPostConfigureOptions, IPostConfigureOptions, IPostConfigureOptions, IPostConfigureOptions, - IPostConfigureOptions, IPostConfigureOptions { public void PostConfigure(string? name, RemoteAuthenticationOptions options) @@ -26,32 +36,42 @@ public void PostConfigure(string? name, RemoteAuthenticationOptions options) options.Backchannel = httpClientFactory.CreateClient(name!); } - public void PostConfigure(string? name, OpenIdConnectOptions options) + public void Configure(string? name, OpenIdConnectOptions options) { PostConfigure(name, (RemoteAuthenticationOptions)options); } - public void PostConfigure(string? name, GitHubAuthenticationOptions options) + public void Configure(OpenIdConnectOptions options) + { + Configure(Options.DefaultName, options); + } + + public void Configure(string? name, AppleAuthenticationOptions options) { PostConfigure(name, (RemoteAuthenticationOptions)options); } - public void PostConfigure(string? name, TwitterOptions options) + public void Configure(AppleAuthenticationOptions options) + { + Configure(Options.DefaultName, options); + } + + public void PostConfigure(string? name, GitHubAuthenticationOptions options) { PostConfigure(name, (RemoteAuthenticationOptions)options); } - public void PostConfigure(string? name, FacebookOptions options) + public void PostConfigure(string? name, TwitterOptions options) { PostConfigure(name, (RemoteAuthenticationOptions)options); } - public void PostConfigure(string? name, MicrosoftIdentityOptions options) + public void PostConfigure(string? name, FacebookOptions options) { PostConfigure(name, (RemoteAuthenticationOptions)options); } - public void PostConfigure(string? name, AppleAuthenticationOptions options) + public void PostConfigure(string? name, MicrosoftIdentityOptions options) { PostConfigure(name, (RemoteAuthenticationOptions)options); } @@ -96,6 +116,9 @@ public void ConfigureHttpClientFactoryForExternalIdentityProviders() }); + // Program.Services.cs also registers a "Keycloak" client (with its BaseAddress, which + // AppUserClaimsPrincipalFactory's relative POST depends on). AddHttpClient accumulates rather than + // replaces, so both configure actions run; this one exists so the scheme always has a named client. services.AddHttpClient("Keycloak", httpClient => { @@ -109,11 +132,14 @@ public void ConfigureHttpClientFactoryForExternalIdentityProviders() services.TryAddEnumerable(ServiceDescriptor.Singleton, RemoteAuthenticationOptionsConfigurator>()); services.TryAddEnumerable(ServiceDescriptor.Singleton, RemoteAuthenticationOptionsConfigurator>()); services.TryAddEnumerable(ServiceDescriptor.Singleton, RemoteAuthenticationOptionsConfigurator>()); - services.TryAddEnumerable(ServiceDescriptor.Singleton, RemoteAuthenticationOptionsConfigurator>()); services.TryAddEnumerable(ServiceDescriptor.Singleton, RemoteAuthenticationOptionsConfigurator>()); - services.TryAddEnumerable(ServiceDescriptor.Singleton, RemoteAuthenticationOptionsConfigurator>()); services.TryAddEnumerable(ServiceDescriptor.Singleton, RemoteAuthenticationOptionsConfigurator>()); + // Configure, not PostConfigure - see the note on RemoteAuthenticationOptionsConfigurator. The framework's + // own post-configure for these two captures Backchannel into a ConfigurationManager it keeps forever, so + // ours has to run first or the metadata/JWKS channel never sees the factory's client. + services.TryAddEnumerable(ServiceDescriptor.Singleton, RemoteAuthenticationOptionsConfigurator>()); + services.TryAddEnumerable(ServiceDescriptor.Singleton, RemoteAuthenticationOptionsConfigurator>()); } } } diff --git a/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Infrastructure/Services/PhoneService.cs b/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Infrastructure/Services/PhoneService.cs index 53c82bdb59..e366604cda 100644 --- a/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Infrastructure/Services/PhoneService.cs +++ b/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Infrastructure/Services/PhoneService.cs @@ -5,6 +5,7 @@ namespace Boilerplate.Server.Api.Infrastructure.Services; public partial class PhoneService { [AutoInject] private readonly ServerApiSettings appSettings = default!; + [AutoInject] private readonly IStringLocalizer localizer = default!; [AutoInject] private readonly PhoneNumberUtil phoneNumberUtil = default!; [AutoInject] private readonly IHostEnvironment hostEnvironment = default!; [AutoInject] private readonly ILogger phoneLogger = default!; @@ -21,9 +22,23 @@ public partial class PhoneService ? value.ToString() : new RegionInfo((CultureInfoManager.InvariantGlobalization is false ? CultureInfo.CurrentUICulture : CultureInfoManager.DefaultCulture).Name).TwoLetterISORegionName; - var parsedPhoneNumber = phoneNumberUtil.Parse(phoneNumber, region); + try + { + var parsedPhoneNumber = phoneNumberUtil.Parse(phoneNumber, region); - return phoneNumberUtil.Format(parsedPhoneNumber, PhoneNumberFormat.E164); + return phoneNumberUtil.Format(parsedPhoneNumber, PhoneNumberFormat.E164); + } + catch (NumberParseException exp) + { + // libphonenumber is far stricter than the [Phone] data annotation the DTOs carry, so a value that passed + // model validation can still land here - "+1" does. The region is caller-influenced too: Cloudflare sends + // XX for unknown geography and T1 for Tor, and neither is a region libphonenumber accepts, which makes any + // national-format number throw for a perfectly legitimate visitor. Unhandled this surfaces as a 500 logged + // at Critical on an anonymous endpoint; it is a bad request. + throw new BadRequestException(localizer[nameof(AppStrings.PhoneAttribute_ValidationError), localizer[nameof(AppStrings.PhoneNumber)]]) + .WithData("Reason", exp.Message) + .WithData("Region", region); + } } public virtual async Task SendSms(string messageText, string phoneNumber) diff --git a/src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Infrastructure/Extensions/UriExtensions.cs b/src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Infrastructure/Extensions/UriExtensions.cs index 5611100894..ea75b352b7 100644 --- a/src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Infrastructure/Extensions/UriExtensions.cs +++ b/src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Infrastructure/Extensions/UriExtensions.cs @@ -4,28 +4,6 @@ namespace System; public static partial class UriExtensions { - extension(Uri) - { - /// - /// True only for an app-rooted relative url such as /sign-in?otp=123. - ///
- /// with is NOT enough on its own: - /// a network-path reference like //attacker.com/path is a well-formed RELATIVE reference, and both - /// browsers and NavigationManager.NavigateTo resolve it to https://attacker.com/path. - /// Browsers treat /\host the same way, so the second character is rejected too. - ///
- /// Use this wherever a url arrives from outside the app and is then navigated to - the Blazor Hybrid local - /// HTTP server's external-sign-in callback and the sign-in / sign-up pages that consume it. - /// - public static bool IsAppRelativeUrl([NotNullWhen(true)] string? url) - { - return string.IsNullOrEmpty(url) is false - && url.StartsWith('/') - && (url.Length is 1 || url[1] is not ('/' or '\\')) - && Uri.IsWellFormedUriString(url, UriKind.Relative); - } - } - extension(Uri uri) { public string GetUrlWithoutQueryParameter(string key) @@ -96,5 +74,43 @@ public string GetPath() var uriBuilder = new UriBuilder(uri.GetUrlWithoutCulture()) { Query = string.Empty, Fragment = string.Empty }; return uriBuilder.Path; } + + /// + /// True only for a relative url that cannot leave this app's origin, such as /sign-in?otp=123. + ///
+ /// with is NOT enough on its own: + /// a network-path reference like //attacker.com/path is a well-formed RELATIVE reference, and both + /// browsers and NavigationManager.NavigateTo resolve it to https://attacker.com/path. + /// Browsers treat /\host and a leading backslash the same way, so those are rejected too. + ///
+ /// Use this wherever a url arrives from outside the app and is then navigated to - the Blazor Hybrid local + /// HTTP server's external-sign-in callback, and every return-url that reaches + /// NavigationManager.NavigateTo. + /// + /// + /// Keep the default when the url is expected to be app-rooted (/products/1). Pass false for a + /// return-url, because the app itself produces base-relative values without the leading slash - see + /// NavigationManagerExtensions.GetRelativePath, which AppShell and SignInModalService + /// feed straight into a return-url. Rootless values are equally origin-bound: a well-formed relative + /// reference carries no scheme and no authority, so it always resolves against the app's own base. + /// + public static bool IsAppRelativeUrl([NotNullWhen(true)] string? url, bool requireLeadingSlash = true) + { + if (string.IsNullOrEmpty(url)) + return false; + + if (url[0] is '\\') + return false; + + if (url[0] is '/') + { + if (url.Length > 1 && url[1] is '/' or '\\') + return false; + } + else if (requireLeadingSlash) + return false; + + return Uri.IsWellFormedUriString(url, UriKind.Relative); + } } } diff --git a/src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/BunitUITests.cs b/src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/BunitUITests.cs index cec257cbde..50189ea243 100644 --- a/src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/BunitUITests.cs +++ b/src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/BunitUITests.cs @@ -52,9 +52,9 @@ public async Task SignIn_Should_WorkAsExpected() // user is the seeded default account. var authenticationStateProvider = ctx.Services.GetRequiredService(); - cut.WaitForAssertion(() => + await cut.WaitForAssertionAsync(async () => { - var user = authenticationStateProvider.GetAuthenticationStateAsync().GetAwaiter().GetResult().User; + var user = (await authenticationStateProvider.GetAuthenticationStateAsync()).User; Assert.IsTrue(user.IsAuthenticated()); Assert.AreEqual(Guid.Parse("8ff71671-a1d6-4f97-abb9-d87d7b47d6e7"), user.GetUserId()); }, timeout: TimeSpan.FromSeconds(30)); diff --git a/src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/ConfirmPageTwoFactorTests.cs b/src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/ConfirmPageTwoFactorTests.cs new file mode 100644 index 0000000000..c1f896e80f --- /dev/null +++ b/src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/ConfirmPageTwoFactorTests.cs @@ -0,0 +1,105 @@ +using Bunit; +using Microsoft.EntityFrameworkCore; +using Boilerplate.Shared.Features.Identity; +using Boilerplate.Server.Api.Infrastructure.Data; +using Boilerplate.Tests.Infrastructure.Services; +using Boilerplate.Server.Api.Features.Identity.Models; +using Microsoft.AspNetCore.Components.Authorization; +using Boilerplate.Client.Core.Components.Pages.Identity; +using Boilerplate.Client.Core.Infrastructure.Services.Contracts; + +namespace Boilerplate.Tests.Features.Identity; + +/// +/// Confirming an identifier ends with an automatic sign-in, and that sign-in answers +/// { requiresTwoFactor: true } - HTTP 200, with null tokens and no session row - whenever the account has +/// two-factor enabled. ConfirmEmailRequestDto carries no second-factor code, so the flow cannot be completed +/// from this page at all; the only correct behaviour is to confirm and stop. +/// +/// Storing that response instead is the failure this pins, and it is worse than it looks: on the Web client the null +/// crosses JS interop and Storage.setItem coerces it, so access_token becomes the literal string +/// "null", which is non-empty enough to pass the token provider's guard and then throws while being parsed - an +/// interrupting error dialog on every auth-state evaluation until the tokens are cleared, on top of the sign-out. +/// +/// +[TestClass, TestCategory("UITest")] +public class ConfirmPageTwoFactorTests +{ + [TestMethod] + public async Task ConfirmEmail_Should_NotStoreTokens_When_TheAccountRequiresASecondFactor() + { + await using var server = new AppTestServer(); + await server.Build().Start(TestContext.CancellationToken); + + var (email, token) = await CreateUnconfirmedTwoFactorAccount(server); + + await using var ctx = server.CreateBunitContext(); + + var storageService = ctx.Services.GetRequiredService(); + await storageService.SetItem("access_token", ExistingSessionMarker); + + // [SupplyParameterFromQuery] values cannot be passed as bUnit parameters - the query string has to be real, so + // navigate to the confirmation link first. That is also exactly what clicking the e-mailed link does, and + // ConfirmPage.OnInitAsync then auto-submits ConfirmEmail. + var navigationManager = ctx.Services.GetRequiredService(); + var confirmUrl = $"{PageUrls.Confirm}?email={Uri.EscapeDataString(email)}&emailToken={Uri.EscapeDataString(token)}"; + navigationManager.NavigateTo(confirmUrl); + + var cut = ctx.Render(parameters => parameters.AddChildContent()); + + // The confirmation itself must still happen - this is not "reject the request", it is "do not pretend it + // signed you in". + await cut.WaitForAssertionAsync(async () => + { + Assert.IsTrue(await IsEmailConfirmed(server, email), "The e-mail must still be confirmed."); + }, timeout: TimeSpan.FromSeconds(30)); + + Assert.AreEqual(ExistingSessionMarker, await storageService.GetItem("access_token"), + "A requiresTwoFactor response carries null tokens; storing it would overwrite the caller's live session."); + + Assert.AreEqual(navigationManager.ToAbsoluteUri(confirmUrl).ToString(), navigationManager.Uri, + "Without tokens the user is not signed in, so the page must not navigate away as if it had succeeded."); + } + + /// + /// Creates an account whose e-mail is still unconfirmed while two-factor authentication is on, and returns the + /// confirmation code that was e-mailed to it. TwoFactorEnabled is set directly on the row because the + /// shipped flow for turning it on (UserController) needs an authenticator app. + /// + private async Task<(string email, string token)> CreateUnconfirmedTwoFactorAccount(AppTestServer server) + { + await using var scope = server.WebApp.Services.CreateAsyncScope(); + var identityController = scope.ServiceProvider.GetRequiredService(); + + var email = MagicLinkSignInUtils.NewTestEmail(); + + // A brand-new address auto-provisions an unconfirmed account and mails it a confirmation code. + await Assert.ThrowsExactlyAsync( + () => identityController.SendOtp(new() { Email = email }, null, TestContext.CancellationToken)); + + var captured = await server.WaitForCapturedEmail(email, e => e.Kind is CapturedEmailKind.EmailToken, TestContext.CancellationToken); + + await using var dbScope = server.WebApp.Services.CreateAsyncScope(); + var dbContext = dbScope.ServiceProvider.GetRequiredService(); + var normalizedEmail = email.ToUpperInvariant(); + await dbContext.Set() + .Where(u => u.NormalizedEmail == normalizedEmail) + .ExecuteUpdateAsync(setters => setters.SetProperty(u => u.TwoFactorEnabled, true), TestContext.CancellationToken); + + return (email, captured.Token!); + } + + private static async Task IsEmailConfirmed(AppTestServer server, string email) + { + await using var scope = server.WebApp.Services.CreateAsyncScope(); + var dbContext = scope.ServiceProvider.GetRequiredService(); + var normalizedEmail = email.ToUpperInvariant(); + + return await dbContext.Set().AnyAsync(u => u.NormalizedEmail == normalizedEmail && u.EmailConfirmed); + } + + /// A recognisable stand-in for the token of a session that already exists in this browser. + private const string ExistingSessionMarker = "existing-session-access-token"; + + public Microsoft.VisualStudio.TestTools.UnitTesting.TestContext TestContext { get; set; } = default!; +} diff --git a/src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/OneTimeTokenTests.cs b/src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/OneTimeTokenTests.cs new file mode 100644 index 0000000000..5e21ebf77f --- /dev/null +++ b/src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/OneTimeTokenTests.cs @@ -0,0 +1,162 @@ +using Microsoft.EntityFrameworkCore; +using Boilerplate.Shared.Features.Identity; +using Boilerplate.Server.Api.Infrastructure.Data; +using Boilerplate.Tests.Infrastructure.Services; +using Boilerplate.Server.Api.Features.Identity.Models; + +namespace Boilerplate.Tests.Features.Identity; + +/// +/// The one-time token flows (e-mail / phone confirmation and password reset) share one shape: a nullable +/// *TokenRequestedOn column, an expiry gate that subtracts it from now, and a token whose purpose string +/// embeds that same timestamp. Two things about that shape fail quietly, and both are pinned here. +/// +/// 1. The expiry gate is a lifted comparison. DateTimeOffset - DateTimeOffset? is TimeSpan?, and a +/// lifted > is false when the column is null - which is exactly the state a consumed token +/// leaves behind. Without an explicit null check the gate is skipped and the request runs on into a verification that +/// can never succeed, charging the user an AccessFailedAsync strike on the way. +/// +/// +/// 2. The purpose string must be built from the same value on both sides. The account lookup is +/// case-insensitive (it matches on NormalizedEmail) but the purpose is an HMAC input, so verifying against the +/// caller's casing while minting from the stored casing rejects the user's own valid code. +/// +/// Neither failure is visible from the code alone - both look like an ordinary "Invalid token." - which is why they +/// are worth the cost of a test. +/// +[TestClass, TestCategory("IntegrationTest")] +public class OneTimeTokenTests +{ + /// + /// Re-opening a confirmation link that was already used. ConfirmEmail nulls EmailTokenRequestedOn on + /// success, and ConfirmPage auto-submits whenever the link carries an emailToken, so a second device, + /// a second tab or a reload replays it. The replay must be rejected as expired without touching the + /// account's failed-access count: that count does not decay, so five such replays over an account's life would + /// lock the user out of password sign-in for something they did not do. + /// + [TestMethod] + public async Task ReplayingAConsumedConfirmationToken_Should_NotChargeAFailedAccessAttempt() + { + await using var server = new AppTestServer(); + await server.Build(services => services.AddIntegrationApiOnlyTestsServices()).Start(TestContext.CancellationToken); + + await using var scope = server.WebApp.Services.CreateAsyncScope(); + var identityController = scope.ServiceProvider.GetRequiredService(); + + var email = MagicLinkSignInUtils.NewTestEmail(); + + await Assert.ThrowsExactlyAsync( + () => identityController.SendOtp(new() { Email = email }, null, TestContext.CancellationToken)); + + var captured = await server.WaitForCapturedEmail(email, e => e.Kind is CapturedEmailKind.EmailToken, TestContext.CancellationToken); + + // First use succeeds and clears EmailTokenRequestedOn. + await identityController.ConfirmEmail(new() { Email = email, Token = captured.Token }, TestContext.CancellationToken); + + var failedCountBeforeReplay = await ReadAccessFailedCount(server, email); + + var replay = await Assert.ThrowsExactlyAsync( + () => identityController.ConfirmEmail(new() { Email = email, Token = captured.Token }, TestContext.CancellationToken), + "Replaying a consumed confirmation token must be rejected."); + + Assert.AreEqual(nameof(AppStrings.ExpiredToken), replay.Key, + "A consumed token has no outstanding request, so the expiry gate must reject it before the token check - " + + "reporting InvalidToken instead means the lifted null comparison let it through."); + + Assert.AreEqual(failedCountBeforeReplay, await ReadAccessFailedCount(server, email), + "Rejecting a provably unverifiable replay must not spend the user's lockout budget."); + } + + /// + /// The same gate on the phone flow, which the shipped seed data reaches directly: UserConfiguration seeds + /// PhoneNumberConfirmed = true while leaving PhoneNumberTokenRequestedOn null. + /// + [TestMethod] + public async Task ConfirmingAPhoneWithNoOutstandingToken_Should_ReportExpiredRatherThanInvalid() + { + await using var server = new AppTestServer(); + await server.Build(services => services.AddIntegrationApiOnlyTestsServices()).Start(TestContext.CancellationToken); + + await using var scope = server.WebApp.Services.CreateAsyncScope(); + var identityController = scope.ServiceProvider.GetRequiredService(); + + var exception = await Assert.ThrowsExactlyAsync( + () => identityController.ConfirmPhone(new() { PhoneNumber = SeededTestPhoneNumber, Token = "123456" }, TestContext.CancellationToken), + "There is no outstanding phone token for the seeded account, so this must not be accepted."); + + Assert.AreEqual(nameof(AppStrings.ExpiredToken), exception.Key, + "PhoneNumberTokenRequestedOn is null for the seeded account, so the expiry gate must fire first."); + } + + /// + /// A user who registered as John.Doe@Example.com and later types john.doe@example.com resolves to the + /// same account, because Identity looks accounts up by NormalizedEmail. The 6-digit code they were sent must + /// therefore still work - it is minted from the stored address, so verifying against the typed one would reject a + /// correctly delivered code and charge a strike for it. + /// + [TestMethod] + public async Task ConfirmEmail_Should_AcceptACaseVariantOfTheRegisteredAddress() + { + await using var server = new AppTestServer(); + await server.Build(services => services.AddIntegrationApiOnlyTestsServices()).Start(TestContext.CancellationToken); + + await using var scope = server.WebApp.Services.CreateAsyncScope(); + var identityController = scope.ServiceProvider.GetRequiredService(); + + var email = $"Mixed.Case.{Guid.NewGuid()}@BitPlatform.dev"; + + await Assert.ThrowsExactlyAsync( + () => identityController.SendOtp(new() { Email = email }, null, TestContext.CancellationToken)); + + var captured = await server.WaitForCapturedEmail(email, e => e.Kind is CapturedEmailKind.EmailToken, TestContext.CancellationToken); + + // The user types their address in a different case than they registered with. + await identityController.ConfirmEmail( + new() { Email = email.ToLowerInvariant(), Token = captured.Token }, TestContext.CancellationToken); + + await using var dbScope = server.WebApp.Services.CreateAsyncScope(); + var dbContext = dbScope.ServiceProvider.GetRequiredService(); + var normalizedEmail = email.ToUpperInvariant(); + + Assert.IsTrue(await dbContext.Set().AnyAsync(u => u.NormalizedEmail == normalizedEmail && u.EmailConfirmed, TestContext.CancellationToken), + "The code was correct and was delivered to that address, so a difference in casing must not reject it."); + } + + /// + /// [Phone] (the only validation the DTOs carry) is far more permissive than libphonenumber, which is what + /// actually parses the value: "+1" passes the attribute and throws inside PhoneService. On an + /// anonymous endpoint that difference is the difference between a 400 and a 500 logged at Critical. + /// + [TestMethod] + [DataRow("+1")] + [DataRow("+999999999999999999")] + public async Task AnUnparsablePhoneNumber_Should_BeABadRequestRatherThanAServerFault(string phoneNumber) + { + await using var server = new AppTestServer(); + await server.Build(services => services.AddIntegrationApiOnlyTestsServices()).Start(TestContext.CancellationToken); + + await using var scope = server.WebApp.Services.CreateAsyncScope(); + var identityController = scope.ServiceProvider.GetRequiredService(); + + await Assert.ThrowsExactlyAsync( + () => identityController.SendConfirmPhoneToken(new() { PhoneNumber = phoneNumber }, TestContext.CancellationToken), + $"'{phoneNumber}' passes the [Phone] attribute but libphonenumber cannot parse it; that is a bad request, not a server fault."); + } + + /// The phone number UserConfiguration seeds for the default test account. + private const string SeededTestPhoneNumber = "+31684207362"; + + private static async Task ReadAccessFailedCount(AppTestServer server, string email) + { + await using var scope = server.WebApp.Services.CreateAsyncScope(); + var dbContext = scope.ServiceProvider.GetRequiredService(); + var normalizedEmail = email.ToUpperInvariant(); + + return await dbContext.Set() + .Where(u => u.NormalizedEmail == normalizedEmail) + .Select(u => u.AccessFailedCount) + .SingleAsync(); + } + + public TestContext TestContext { get; set; } = default!; +} diff --git a/src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/PhoneNumberNormalizationUITests.cs b/src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/PhoneNumberNormalizationUITests.cs index f0126053fa..bf159cbb85 100644 --- a/src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/PhoneNumberNormalizationUITests.cs +++ b/src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/PhoneNumberNormalizationUITests.cs @@ -68,7 +68,7 @@ await server.Build(services => // proves the number was normalized (not merely passed through). var e164 = new Regex(@"^\+[1-9]\d{6,14}$"); - foreach (var attempt in attempts) + foreach (var (Typed, Normalized) in attempts) { // Each request starts from a fresh Sign in page. A hard reload is the simplest way back to the phone form: // the OTP panel is treated as a modal, but its NavigationLock only intercepts internal Blazor navigation @@ -82,17 +82,17 @@ await Page.GotoAsync(new Uri(serverAddress, PageUrls.SignIn).ToString(), // Type the de-normalized number into the BitPhoneInput's number box (its carries the // placeholder) and ask for the code. Send OTP stays disabled until the debounced value commits, so // Playwright's actionability wait covers the 500ms debounce. - await Page.GetByPlaceholder(AppStrings.PhoneNumberPlaceholder).FillAsync(attempt.Typed); + await Page.GetByPlaceholder(AppStrings.PhoneNumberPlaceholder).FillAsync(Typed); await Page.GetByRole(AriaRole.Button, new() { Name = AppStrings.SendOtpButtonText }).ClickAsync(); // The server registers the new account, texts the confirmation code (captured synchronously by the fake) and // answers "not confirmed", which reveals the OTP panel - a reliable signal that SendSms has already run. await Page.Locator(".bit-otp-inp").First.WaitForAsync(); - var sms = await WaitForSmsTo(attempt.Normalized, TestContext.CancellationToken); + var sms = await WaitForSmsTo(Normalized, TestContext.CancellationToken); - Assert.AreEqual(attempt.Normalized, sms.PhoneNumber, - $"The server should have normalized '{attempt.Typed}' to E.164 before calling SendSms."); + Assert.AreEqual(Normalized, sms.PhoneNumber, + $"The server should have normalized '{Typed}' to E.164 before calling SendSms."); Assert.MatchesRegex(e164, sms.PhoneNumber, "SendSms must receive a canonical E.164 number (leading '+', digits only, no formatting)."); } @@ -146,8 +146,8 @@ await Page.GotoAsync(new Uri(serverAddress, PageUrls.SignIn).ToString(), /// is exactly what the server normalized - which is the behavior under test. Registered per-test via /// configureTestServices (RemoveAll<PhoneService> then AddScoped<PhoneService, CapturingPhoneService>). /// -public partial class CapturingPhoneService(ServerApiSettings appSettings, IBackgroundJobClient backgroundJobClient, IHostEnvironment hostEnvironment, IHttpContextAccessor httpContextAccessor, ILogger phoneLogger, PhoneNumberUtil phoneNumberUtil) : - PhoneService(appSettings, backgroundJobClient, hostEnvironment, httpContextAccessor, phoneLogger, phoneNumberUtil) +public partial class CapturingPhoneService(ServerApiSettings appSettings, IBackgroundJobClient backgroundJobClient, IHostEnvironment hostEnvironment, IHttpContextAccessor httpContextAccessor, IStringLocalizer localizer, ILogger phoneLogger, PhoneNumberUtil phoneNumberUtil) : + PhoneService(appSettings, backgroundJobClient, hostEnvironment, httpContextAccessor, localizer, phoneLogger, phoneNumberUtil) { /// /// Every SendSms call as (message body, phone number). Static because DI owns the resolved instance's lifetime, so a diff --git a/src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/ReturnUrlHardeningTests.cs b/src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/ReturnUrlHardeningTests.cs new file mode 100644 index 0000000000..ad1cd48b41 --- /dev/null +++ b/src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/ReturnUrlHardeningTests.cs @@ -0,0 +1,170 @@ +using System.Web; +using Boilerplate.Shared.Features.Identity; +using Boilerplate.Tests.Infrastructure.Services; + +namespace Boilerplate.Tests.Features.Identity; + +/// +/// Guards the return-url trust boundary. The value travels a long way from an anonymous request body, through +/// a link the app itself e-mails, into NavigationManager.NavigateTo on the client - so anything that can leave +/// this origin has to be rejected somewhere, and is that somewhere. +/// +/// The predicate has two subtleties worth pinning: a network-path reference such as //attacker.com is a +/// perfectly well-formed relative reference that browsers resolve off-origin, and the app itself produces +/// base-relative return urls without a leading slash (See NavigationManagerExtensions.GetRelativePath, +/// which AppShell feeds straight into a return-url), so a rooted-only predicate would break the app's +/// own sign-in links. +/// +/// +[TestClass, TestCategory("UnitTest")] +public class ReturnUrlPredicateTests +{ + /// + /// Values the app itself generates, plus the rooted form the e-mailed links carry. All must survive. + /// + [TestMethod] + [DataRow("/")] + [DataRow("/products/1")] + [DataRow("/sign-in?otp=123456")] + [DataRow("/confirm?email=a%40b.c&emailToken=123456")] + // GetRelativePath() returns a BASE-relative path, i.e. no leading slash - this is what AppShell and + // SignInModalService put into a return-url, so rejecting it would break ordinary sign-in. + [DataRow("products/1")] + [DataRow("sign-in")] + [DataRow("products/1?a=b")] + public void IsAppRelativeUrl_Should_AcceptTheUrlsTheAppItselfProduces(string url) + { + Assert.IsTrue(Uri.IsAppRelativeUrl(url, requireLeadingSlash: false), + $"'{url}' is produced by the app itself and must not be rejected."); + } + + /// + /// Everything that can leave the origin. The // and /\ forms are the interesting ones: both are + /// well-formed RELATIVE references, so Uri.IsWellFormedUriString(_, UriKind.Relative) alone would let them + /// through and the browser would then resolve them to the attacker's host. + /// + [TestMethod] + [DataRow("//evil.example")] + [DataRow("//evil.example/path")] + [DataRow("///evil.example")] + [DataRow("/\\evil.example")] + [DataRow("\\\\evil.example")] + [DataRow("\\/evil.example")] + [DataRow("https://evil.example")] + [DataRow("http://evil.example/x")] + [DataRow("app://localhost/x")] + [DataRow("javascript:alert(1)")] + [DataRow("")] + public void IsAppRelativeUrl_Should_RejectAnythingThatCanLeaveTheOrigin(string url) + { + Assert.IsFalse(Uri.IsAppRelativeUrl(url, requireLeadingSlash: false), + $"'{url}' can escape this app's origin and must be rejected."); + Assert.IsFalse(Uri.IsAppRelativeUrl(url), + $"'{url}' must also be rejected by the stricter, rooted-only form."); + } + + /// + /// The default (rooted-only) form is what the Blazor Hybrid local HTTP server's callback uses, and it must stay + /// strict: a rootless value is origin-bound but is not a valid app-rooted url. + /// + [TestMethod] + public void IsAppRelativeUrl_Should_StillRequireALeadingSlashByDefault() + { + Assert.IsFalse(Uri.IsAppRelativeUrl("products/1"), "The default overload must keep requiring a leading slash."); + Assert.IsTrue(Uri.IsAppRelativeUrl("/products/1"), "A rooted url must pass the default overload."); + } +} + +/// +/// The server half of the same boundary: returnUrl arrives in an anonymous request body and is spliced +/// into a link the application e-mails from its own domain, so an off-origin value would turn the app's own +/// transactional mail into a redirector. These tests read the captured e-mail (See ) +/// and assert what actually left the building. +/// +[TestClass, TestCategory("IntegrationTest")] +public class ReturnUrlHardeningTests +{ + /// + /// SendOtp needs no pre-existing account - it auto-provisions - so this is reachable by an anonymous caller + /// against any address they choose, which is what makes it worth a test rather than a comment. + /// + [TestMethod] + public async Task SendOtp_Should_NotPutAnOffOriginReturnUrlIntoTheConfirmationEmail() + { + await using var server = new AppTestServer(); + await server.Build(services => services.AddIntegrationApiOnlyTestsServices()).Start(TestContext.CancellationToken); + + await using var scope = server.WebApp.Services.CreateAsyncScope(); + var identityController = scope.ServiceProvider.GetRequiredService(); + + var email = MagicLinkSignInUtils.NewTestEmail(); + + // A brand-new address: the account is created unconfirmed, so SendOtp sends the CONFIRMATION mail, and the + // attacker-chosen returnUrl is what would be embedded in its link. + await Assert.ThrowsExactlyAsync( + () => identityController.SendOtp(new() { Email = email, ReturnUrl = "https://evil.example/login" }, null, TestContext.CancellationToken), + "A brand-new account is unconfirmed, so SendOtp must answer UserIsNotConfirmed after mailing the confirmation link."); + + var captured = await server.WaitForCapturedEmail(email, e => e.Kind is CapturedEmailKind.EmailToken, TestContext.CancellationToken); + + var returnUrl = HttpUtility.ParseQueryString(captured.Link!.Query)["return-url"]; + + Assert.AreEqual(PageUrls.Home, returnUrl, + $"An off-origin return url must be replaced by the home page, but the e-mailed link carried '{returnUrl}'."); + } + + /// + /// The same boundary on the reset-password flow, which takes its return url from a different DTO and builds its + /// link in a different method - so it can (and did) regress independently. A network-path reference is the + /// interesting payload here: it is a well-formed RELATIVE url, so it slips past the obvious check. + /// + [TestMethod] + [DataRow("//evil.example/login", PageUrls.Home, "A network-path reference is a well-formed RELATIVE url, so it must be caught explicitly.")] + [DataRow("https://evil.example/login", PageUrls.Home, "An absolute url must never survive into an e-mailed link.")] + // The guard must not become "replace everything": an ordinary in-app return url has to survive end to end, or + // resetting a password from a product page would drop the user on the home page instead of bringing them back. + [DataRow("/settings/sessions", "/settings/sessions", "A legitimate app-relative return url must be preserved.")] + public async Task SendResetPasswordToken_Should_OnlyEmailAnAppRelativeReturnUrl(string requested, string expected, string because) + { + await using var server = new AppTestServer(); + await server.Build(services => services.AddIntegrationApiOnlyTestsServices()).Start(TestContext.CancellationToken); + + await using var scope = server.WebApp.Services.CreateAsyncScope(); + var identityController = scope.ServiceProvider.GetRequiredService(); + + // A per-run account, not the seeded one: SendResetPasswordToken is throttled per user for the token's whole + // lifetime, so sharing an account across these rows (they run in parallel) would fail on the resend delay. + var email = await CreateConfirmedAccount(server, identityController); + + await identityController.SendResetPasswordToken( + new() { Email = email, ReturnUrl = requested }, TestContext.CancellationToken); + + var captured = await server.WaitForCapturedEmail(email, e => e.Kind is CapturedEmailKind.ResetPassword, TestContext.CancellationToken); + + var returnUrl = HttpUtility.ParseQueryString(captured.Link!.Query)["return-url"]; + + Assert.AreEqual(expected, returnUrl, $"{because} The e-mailed link carried '{returnUrl}'."); + } + + /// + /// Registers and confirms a brand-new account, so the test owns its own fixture instead of mutating the seeded + /// one. SendOtp auto-provisions the (unconfirmed) account and mails the confirmation code; consuming that + /// code confirms it, which is what SendResetPasswordToken requires. + /// + private async Task CreateConfirmedAccount(AppTestServer server, IIdentityController identityController) + { + var email = MagicLinkSignInUtils.NewTestEmail(); + + await Assert.ThrowsExactlyAsync( + () => identityController.SendOtp(new() { Email = email }, null, TestContext.CancellationToken), + "A brand-new account is unconfirmed, so SendOtp answers UserIsNotConfirmed after mailing the confirmation link."); + + var captured = await server.WaitForCapturedEmail(email, e => e.Kind is CapturedEmailKind.EmailToken, TestContext.CancellationToken); + + await identityController.ConfirmEmail(new() { Email = email, Token = captured.Token }, TestContext.CancellationToken); + + return email; + } + + public TestContext TestContext { get; set; } = default!; +} From 555865206420fe81ec85721db6a9157adb42f0b1 Mon Sep 17 00:00:00 2001 From: yasmoradi Date: Fri, 31 Jul 2026 18:14:39 +0200 Subject: [PATCH 2/2] fix --- .../Pages/Identity/SignIn/SignInPanel.razor.cs | 2 +- .../Identity/IdentityController.ExternalSignIn.cs | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPanel.razor.cs b/src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPanel.razor.cs index cf1a9c8600..63ba077fc6 100644 --- a/src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPanel.razor.cs +++ b/src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPanel.razor.cs @@ -236,7 +236,7 @@ private async Task ExternalSignIn(string provider) queryParams.TryGetValue("return-url", out var returnUrl); var returnUrlValue = GetValue(returnUrl); - ReturnUrlQueryString = Uri.IsAppRelativeUrl(returnUrlValue) ? returnUrlValue : PageUrls.Home; + ReturnUrlQueryString = Uri.IsAppRelativeUrl(returnUrlValue, requireLeadingSlash: false) ? returnUrlValue : PageUrls.Home; queryParams.TryGetValue("userName", out var userName); UserNameQueryString = GetValue(userName); queryParams.TryGetValue("email", out var email); diff --git a/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.ExternalSignIn.cs b/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.ExternalSignIn.cs index 297e7e5cec..3a4e09c32c 100644 --- a/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.ExternalSignIn.cs +++ b/src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.ExternalSignIn.cs @@ -57,6 +57,8 @@ public async Task ExternalSignInCallback(string? returnUrl = null, user = await userManager.FindUser(new() { Email = email, PhoneNumber = phoneNumber }); } + var isNewUser = user is null; + if (user is null) { var name = info.Principal.FindFirstValue("preferred_username") ?? info.Principal.FindFirstValue(ClaimTypes.Name) ?? info.Principal.FindFirstValue(ClaimTypes.NameIdentifier) ?? info.Principal.FindFirstValue("name"); @@ -99,7 +101,17 @@ public async Task ExternalSignInCallback(string? returnUrl = null, var addLoginResult = await userManager.AddLoginAsync(user, info); if (addLoginResult.Succeeded is false) + { + if (isNewUser) + { + // Only this request could reach that row: a provider asserting neither an email nor a phone + // number leaves the identifier fallback above nothing to match it by, so without the login it + // would be unreachable forever and every retry would add another one. + await userManager.DeleteAsync(user); + } + throw new ResourceValidationException(addLoginResult.Errors.Select(e => new LocalizedString(e.Code, e.Description)).ToArray()).WithData("UserId", user.Id); + } } // Confirmation is only as good as the provider's own verification of the identifier