From 627dfd809a63016f7044f0368ba0343cce4bdfd3 Mon Sep 17 00:00:00 2001 From: Doug Waldron <737326+dougwaldron@users.noreply.github.com> Date: Tue, 5 Nov 2024 15:04:26 -0500 Subject: [PATCH] Improve user authentication procedure (#125) * Update the user email if changed in the external login provider * Search for user by Object ID before email address * Add auditing dates to `ApplicationUser` * Track most recent login date * Mask private data when logging --- src/AppServices/AppServices.csproj | 1 + .../Helpers/PrincipalExtensions.cs | 10 +- src/AppServices/Staff/ClaimConstants.cs | 103 ------------------ src/AppServices/Staff/Dto/StaffViewDto.cs | 1 + src/AppServices/Staff/StaffService.cs | 5 +- src/Domain/Identity/ApplicationUser.cs | 6 + .../Pages/Account/ExternalLogin.cshtml.cs | 95 ++++++++++------ .../{ErrorLogging => Logging}/ErrorLogger.cs | 2 +- src/WebApp/Platform/Logging/Redaction.cs | 20 ++++ .../{ErrorLogging => Logging}/ShortId.cs | 2 +- src/WebApp/Program.cs | 2 +- src/WebApp/WebApp.csproj | 1 - 12 files changed, 104 insertions(+), 144 deletions(-) delete mode 100644 src/AppServices/Staff/ClaimConstants.cs rename src/WebApp/Platform/{ErrorLogging => Logging}/ErrorLogger.cs (97%) create mode 100644 src/WebApp/Platform/Logging/Redaction.cs rename src/WebApp/Platform/{ErrorLogging => Logging}/ShortId.cs (90%) diff --git a/src/AppServices/AppServices.csproj b/src/AppServices/AppServices.csproj index 4f91fc4..30ad372 100644 --- a/src/AppServices/AppServices.csproj +++ b/src/AppServices/AppServices.csproj @@ -13,6 +13,7 @@ + runtime; build; native; contentfiles; analyzers; buildtransitive all diff --git a/src/AppServices/Permissions/Helpers/PrincipalExtensions.cs b/src/AppServices/Permissions/Helpers/PrincipalExtensions.cs index 7d56321..3807dc9 100644 --- a/src/AppServices/Permissions/Helpers/PrincipalExtensions.cs +++ b/src/AppServices/Permissions/Helpers/PrincipalExtensions.cs @@ -8,8 +8,14 @@ namespace MyApp.AppServices.Permissions.Helpers; public static class PrincipalExtensions { - public static string? GetUserIdValue(this ClaimsPrincipal principal) => - principal.FindFirstValue(ClaimTypes.NameIdentifier); + public static string? GetEmail(this ClaimsPrincipal principal) => + principal.FindFirstValue(ClaimTypes.Email); + + public static string GetGivenName(this ClaimsPrincipal principal) => + principal.FindFirstValue(ClaimTypes.GivenName) ?? string.Empty; + + public static string GetFamilyName(this ClaimsPrincipal principal) => + principal.FindFirstValue(ClaimTypes.Surname) ?? string.Empty; public static bool HasRealClaim(this ClaimsPrincipal principal, string type, [NotNullWhen(true)] string? value) => value is not null && principal.HasClaim(type, value); diff --git a/src/AppServices/Staff/ClaimConstants.cs b/src/AppServices/Staff/ClaimConstants.cs deleted file mode 100644 index 1d49533..0000000 --- a/src/AppServices/Staff/ClaimConstants.cs +++ /dev/null @@ -1,103 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. -// Original namespace Microsoft.Identity.Web - -namespace MyApp.AppServices.Staff; - -/// -/// Constants for claim types. -/// -public static class ClaimConstants -{ - /// - /// Name claim: "name". - /// - public const string Name = "name"; - - /// - /// Old Object Id claim: http://schemas.microsoft.com/identity/claims/objectidentifier. - /// - public const string ObjectId = "http://schemas.microsoft.com/identity/claims/objectidentifier"; - - /// - /// New Object id claim: "oid". - /// - public const string Oid = "oid"; - - /// - /// PreferredUserName: "preferred_username". - /// - public const string PreferredUserName = "preferred_username"; - - /// - /// Old TenantId claim: "http://schemas.microsoft.com/identity/claims/tenantid". - /// - public const string TenantId = "http://schemas.microsoft.com/identity/claims/tenantid"; - - /// - /// New Tenant Id claim: "tid". - /// - public const string Tid = "tid"; - - /// - /// ClientInfo claim: "client_info". - /// - public const string ClientInfo = "client_info"; - - /// - /// UniqueObjectIdentifier: "uid". - /// Home Object Id. - /// - public const string UniqueObjectIdentifier = "uid"; - - /// - /// UniqueTenantIdentifier: "utid". - /// Home Tenant Id. - /// - public const string UniqueTenantIdentifier = "utid"; - - /// - /// Older scope claim: "http://schemas.microsoft.com/identity/claims/scope". - /// - public const string Scope = "http://schemas.microsoft.com/identity/claims/scope"; - - /// - /// Newer scope claim: "scp". - /// - public const string Scp = "scp"; - - /// - /// New Roles claim = "roles". - /// - public const string Roles = "roles"; - - /// - /// Old Role claim: "http://schemas.microsoft.com/ws/2008/06/identity/claims/role". - /// - public const string Role = "http://schemas.microsoft.com/ws/2008/06/identity/claims/role"; - - /// - /// Subject claim: "sub". - /// - public const string Sub = "sub"; - - /// - /// Acr claim: "acr". - /// - public const string Acr = "acr"; - - /// - /// UserFlow claim: "http://schemas.microsoft.com/claims/authnclassreference". - /// - public const string UserFlow = "http://schemas.microsoft.com/claims/authnclassreference"; - - /// - /// Tfp claim: "tfp". - /// - public const string Tfp = "tfp"; - - /// - /// Name Identifier ID claim: "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/nameidentifier". - /// - public const string NameIdentifierId = "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/nameidentifier"; -} diff --git a/src/AppServices/Staff/Dto/StaffViewDto.cs b/src/AppServices/Staff/Dto/StaffViewDto.cs index 50fc578..c1b0db2 100644 --- a/src/AppServices/Staff/Dto/StaffViewDto.cs +++ b/src/AppServices/Staff/Dto/StaffViewDto.cs @@ -19,6 +19,7 @@ public record StaffViewDto : INamedEntity public string? PhoneNumber { get; init; } public OfficeViewDto? Office { get; init; } public bool Active { get; init; } + public string? ObjectIdentifier { get; init; } // Display properties [JsonIgnore] diff --git a/src/AppServices/Staff/StaffService.cs b/src/AppServices/Staff/StaffService.cs index 26888f4..dd32591 100644 --- a/src/AppServices/Staff/StaffService.cs +++ b/src/AppServices/Staff/StaffService.cs @@ -4,13 +4,13 @@ using GaEpd.AppLibrary.Pagination; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Identity; +using Microsoft.Identity.Web; using MyApp.AppServices.Permissions; using MyApp.AppServices.Permissions.Helpers; using MyApp.AppServices.Staff.Dto; using MyApp.AppServices.UserServices; using MyApp.Domain.Entities.Offices; using MyApp.Domain.Identity; -using System.Security.Claims; namespace MyApp.AppServices.Staff; @@ -125,7 +125,7 @@ async Task UpdateUserRoleAsync(ApplicationUser u, string r, bool public async Task UpdateAsync(string id, StaffUpdateDto resource) { var principal = userService.GetCurrentPrincipal()!; - if (id != principal.FindFirstValue(ClaimConstants.NameIdentifierId) && + if (id != principal.GetNameIdentifierId() && !await authorization.Succeeded(principal, Policies.UserAdministrator).ConfigureAwait(false)) { throw new InsufficientPermissionsException(nameof(Policies.UserAdministrator)); @@ -139,6 +139,7 @@ public async Task UpdateAsync(string id, StaffUpdateDto resource ? null : await officeRepository.GetAsync(resource.OfficeId.Value).ConfigureAwait(false); user.Active = resource.Active; + user.ProfileUpdatedAt = DateTimeOffset.UtcNow; return await userManager.UpdateAsync(user).ConfigureAwait(false); } diff --git a/src/Domain/Identity/ApplicationUser.cs b/src/Domain/Identity/ApplicationUser.cs index 651238d..fb9feb2 100644 --- a/src/Domain/Identity/ApplicationUser.cs +++ b/src/Domain/Identity/ApplicationUser.cs @@ -45,6 +45,12 @@ public class ApplicationUser : IdentityUser, IEntity [StringLength(36)] public string? ObjectIdentifier { get; set; } + // Auditing properties + public DateTimeOffset? AccountCreatedAt { get; init; } + public DateTimeOffset? AccountUpdatedAt { get; set; } + public DateTimeOffset? ProfileUpdatedAt { get; set; } + public DateTimeOffset? MostRecentLogin { get; set; } + // Display properties public string SortableFullName => string.Join(", ", new[] { FamilyName, GivenName }.Where(s => !string.IsNullOrEmpty(s))); diff --git a/src/WebApp/Pages/Account/ExternalLogin.cshtml.cs b/src/WebApp/Pages/Account/ExternalLogin.cshtml.cs index c415cef..8b33c94 100644 --- a/src/WebApp/Pages/Account/ExternalLogin.cshtml.cs +++ b/src/WebApp/Pages/Account/ExternalLogin.cshtml.cs @@ -1,13 +1,16 @@ using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authentication.OpenIdConnect; using Microsoft.AspNetCore.Identity; +using Microsoft.EntityFrameworkCore; +using Microsoft.Identity.Web; +using MyApp.AppServices.Permissions.Helpers; using MyApp.AppServices.Staff; using MyApp.AppServices.Staff.Dto; using MyApp.Domain.Identity; using MyApp.WebApp.Models; +using MyApp.WebApp.Platform.Logging; using MyApp.WebApp.Platform.PageModelHelpers; using MyApp.WebApp.Platform.Settings; -using System.Security.Claims; namespace MyApp.WebApp.Pages.Account; @@ -82,18 +85,27 @@ public async Task OnGetCallbackAsync(string? returnUrl = null, st if (externalLoginInfo?.Principal is null) return RedirectToLoginPageWithError("Error loading work account information."); - var preferredUserName = externalLoginInfo.Principal.FindFirstValue(ClaimConstants.PreferredUserName); - if (preferredUserName is null) + var userTenant = externalLoginInfo.Principal.GetTenantId(); + var userEmail = externalLoginInfo.Principal.GetEmail(); + if (userEmail is null || userTenant is null) return RedirectToLoginPageWithError("Error loading detailed work account information."); - if (!preferredUserName.IsValidEmailDomain()) + if (!userEmail.IsValidEmailDomain()) { - logger.LogWarning("User {UserName} with invalid email domain attempted signin", preferredUserName); + logger.LogWarning("User {UserName} with invalid email domain attempted signin", userEmail.MaskEmail()); return RedirectToPage("./Unavailable"); } - // Determine if a user account already exists. - var user = await userManager.FindByNameAsync(preferredUserName); + logger.LogInformation("User {UserName} in tenant {TenantID} successfully authenticated", userEmail.MaskEmail(), + userTenant); + + // Determine if a user account already exists with the Object ID. + // If not, then determine if a user account already exists with the given username. + var user = AppSettings.DevSettings.UseInMemoryData + ? await userManager.FindByNameAsync(userEmail) + : await userManager.Users.SingleOrDefaultAsync(u => + u.ObjectIdentifier == externalLoginInfo.Principal.GetObjectId()) ?? + await userManager.FindByNameAsync(userEmail); // If the user does not have a local account yet, then create one and sign in. if (user is null) @@ -102,7 +114,7 @@ public async Task OnGetCallbackAsync(string? returnUrl = null, st // If user has been marked as inactive, don't sign in. if (!user.Active) { - logger.LogWarning("Inactive user {UserName} attempted signin", preferredUserName); + logger.LogWarning("Inactive user {Email} attempted signin", userEmail.MaskEmail()); return RedirectToPage("./Unavailable"); } @@ -138,37 +150,38 @@ private async Task CreateUserAndSignInAsync(ExternalLoginInfo inf { var user = new ApplicationUser { - UserName = info.Principal.FindFirstValue(ClaimConstants.PreferredUserName), - Email = info.Principal.FindFirstValue(ClaimTypes.Email) ?? - info.Principal.FindFirstValue(ClaimConstants.PreferredUserName), - GivenName = info.Principal.FindFirstValue(ClaimTypes.GivenName) ?? "", - FamilyName = info.Principal.FindFirstValue(ClaimTypes.Surname) ?? "", - ObjectIdentifier = info.Principal.FindFirstValue(ClaimConstants.ObjectId), + UserName = info.Principal.GetDisplayName(), + Email = info.Principal.GetEmail(), + GivenName = info.Principal.GetGivenName(), + FamilyName = info.Principal.GetFamilyName(), + ObjectIdentifier = info.Principal.GetObjectId(), + AccountCreatedAt = DateTimeOffset.Now, + MostRecentLogin = DateTimeOffset.Now, }; // Create the user in the backing store. var createUserResult = await userManager.CreateAsync(user); if (!createUserResult.Succeeded) { - logger.LogWarning("Failed to create new user {UserName}", user.UserName); + logger.LogWarning("Failed to create new user {UserName}", user.Email.MaskEmail()); return await FailedLoginAsync(createUserResult, user); } - logger.LogInformation("Created new user {UserName} with object ID {ObjectId}", user.UserName, + logger.LogInformation("Created new user {Email} with object ID {ObjectId}", user.Email.MaskEmail(), user.ObjectIdentifier); // Add new user to application Roles if seeded in app settings or local admin user setting is enabled. var seedAdminUsers = configuration.GetSection("SeedAdminUsers").Get(); if (AppSettings.DevSettings.LocalUserIsStaff) { - logger.LogInformation("Seeding staff role for new user {UserName}", user.UserName); + logger.LogInformation("Seeding staff role for new user {Email}", user.Email.MaskEmail()); await userManager.AddToRoleAsync(user, RoleName.Staff); } if (AppSettings.DevSettings.LocalUserIsAdmin || (seedAdminUsers != null && seedAdminUsers.Contains(user.Email, StringComparer.InvariantCultureIgnoreCase))) { - logger.LogInformation("Seeding all roles for new user {UserName}", user.UserName); + logger.LogInformation("Seeding all roles for new user {Email}", user.Email.MaskEmail()); foreach (var role in AppRole.AllRoles) await userManager.AddToRoleAsync(user, role.Key); } @@ -181,13 +194,31 @@ private async Task CreateUserAndSignInAsync(ExternalLoginInfo inf // Update local store with from external provider. private async Task RefreshUserInfoAndSignInAsync(ApplicationUser user, ExternalLoginInfo info) { - logger.LogInformation("Existing user {UserName} logged in with {LoginProvider} provider", - user.UserName, info.LoginProvider); - user.Email = info.Principal.FindFirstValue(ClaimTypes.Email) ?? - info.Principal.FindFirstValue(ClaimConstants.PreferredUserName); - user.GivenName = info.Principal.FindFirstValue(ClaimTypes.GivenName) ?? user.GivenName; - user.FamilyName = info.Principal.FindFirstValue(ClaimTypes.Surname) ?? user.FamilyName; + logger.LogInformation("Existing user {Email} logged in with {LoginProvider} provider", + user.Email.MaskEmail(), info.LoginProvider); + + var previousValues = new ApplicationUser + { + UserName = user.UserName, + Email = user.Email, + GivenName = user.GivenName, + FamilyName = user.FamilyName, + }; + + user.UserName = info.Principal.GetDisplayName(); + user.Email = info.Principal.GetEmail(); + user.GivenName = info.Principal.GetGivenName(); + user.FamilyName = info.Principal.GetFamilyName(); + user.MostRecentLogin = DateTimeOffset.Now; + + if (user.UserName != previousValues.UserName || user.Email != previousValues.Email || + user.GivenName != previousValues.GivenName || user.FamilyName != previousValues.FamilyName) + { + user.AccountUpdatedAt = DateTimeOffset.Now; + } + await userManager.UpdateAsync(user); + await signInManager.RefreshSignInAsync(user); return LocalRedirectOrHome(); } @@ -200,19 +231,17 @@ private async Task AddLoginProviderAndSignInAsync( if (!addLoginResult.Succeeded) { - logger.LogWarning("Failed to add login provider {LoginProvider} for user {UserName}", - info.LoginProvider, user.UserName); + logger.LogWarning("Failed to add login provider {LoginProvider} for user {Email}", + info.LoginProvider, user.Email.MaskEmail()); return await FailedLoginAsync(addLoginResult, user); } - if (user.ObjectIdentifier == null) - { - user.ObjectIdentifier = info.Principal.FindFirstValue(ClaimConstants.ObjectId); - await userManager.UpdateAsync(user); - } + user.ObjectIdentifier ??= info.Principal.GetObjectId(); + user.MostRecentLogin = DateTimeOffset.Now; + await userManager.UpdateAsync(user); - logger.LogInformation("Login provider {LoginProvider} added for user {UserName} with object ID {ObjectId}", - info.LoginProvider, user.UserName, user.ObjectIdentifier); + logger.LogInformation("Login provider {LoginProvider} added for user {Email} with object ID {ObjectId}", + info.LoginProvider, user.Email.MaskEmail(), user.ObjectIdentifier); // Include the access token in the properties. var props = new AuthenticationProperties(); diff --git a/src/WebApp/Platform/ErrorLogging/ErrorLogger.cs b/src/WebApp/Platform/Logging/ErrorLogger.cs similarity index 97% rename from src/WebApp/Platform/ErrorLogging/ErrorLogger.cs rename to src/WebApp/Platform/Logging/ErrorLogger.cs index 3831835..3f2114d 100644 --- a/src/WebApp/Platform/ErrorLogging/ErrorLogger.cs +++ b/src/WebApp/Platform/Logging/ErrorLogger.cs @@ -4,7 +4,7 @@ using MyApp.WebApp.Platform.Settings; using System.Collections; -namespace MyApp.WebApp.Platform.ErrorLogging; +namespace MyApp.WebApp.Platform.Logging; public class ErrorLogger(IFileService fileService, IServiceProvider serviceProvider) : IErrorLogger { diff --git a/src/WebApp/Platform/Logging/Redaction.cs b/src/WebApp/Platform/Logging/Redaction.cs new file mode 100644 index 0000000..567e83f --- /dev/null +++ b/src/WebApp/Platform/Logging/Redaction.cs @@ -0,0 +1,20 @@ +using System.Text.RegularExpressions; + +namespace MyApp.WebApp.Platform.Logging; + +public static partial class Redaction +{ + public static string MaskEmail(this string? email) + { + if (string.IsNullOrEmpty(email)) return string.Empty; + + var atIndex = email.IndexOf('@'); + if (atIndex <= 1) return email; + + var maskedEmail = MyRegex().Replace(email[..atIndex], "*"); + return string.Concat(maskedEmail, email.AsSpan(atIndex)); + } + + [GeneratedRegex(".(?=.{2})")] + private static partial Regex MyRegex(); +} diff --git a/src/WebApp/Platform/ErrorLogging/ShortId.cs b/src/WebApp/Platform/Logging/ShortId.cs similarity index 90% rename from src/WebApp/Platform/ErrorLogging/ShortId.cs rename to src/WebApp/Platform/Logging/ShortId.cs index 4aa9185..8715e64 100644 --- a/src/WebApp/Platform/ErrorLogging/ShortId.cs +++ b/src/WebApp/Platform/Logging/ShortId.cs @@ -1,6 +1,6 @@ using System.Text; -namespace MyApp.WebApp.Platform.ErrorLogging; +namespace MyApp.WebApp.Platform.Logging; public static class ShortId { diff --git a/src/WebApp/Program.cs b/src/WebApp/Program.cs index f116b8f..3658b10 100644 --- a/src/WebApp/Program.cs +++ b/src/WebApp/Program.cs @@ -6,7 +6,7 @@ using MyApp.AppServices.ErrorLogging; using MyApp.AppServices.RegisterServices; using MyApp.WebApp.Platform.AppConfiguration; -using MyApp.WebApp.Platform.ErrorLogging; +using MyApp.WebApp.Platform.Logging; using MyApp.WebApp.Platform.Settings; using System.Reflection; diff --git a/src/WebApp/WebApp.csproj b/src/WebApp/WebApp.csproj index ac47d62..db83c74 100644 --- a/src/WebApp/WebApp.csproj +++ b/src/WebApp/WebApp.csproj @@ -14,7 +14,6 @@ -