From 84fa2f16c6928ea8baa0296e1bd76d6cbe6d6c57 Mon Sep 17 00:00:00 2001 From: James Gunn Date: Thu, 21 Sep 2023 11:39:40 +0100 Subject: [PATCH] Remove the IsComplete property from AuthenticationState --- .../AuthenticationState.cs | 16 ++------------- .../Controllers/AuthorizationController.cs | 7 ++++--- .../RequireAuthenticationStateFilter.cs | 18 ++++++----------- .../Journeys/CoreSignInJourney.cs | 20 +++++++++---------- .../Journeys/TrnTokenSignInJourney.cs | 14 +++++++------ .../src/TeacherIdentity.AuthServer/Program.cs | 2 +- .../UserClaimHelper.cs | 10 ++++------ 7 files changed, 35 insertions(+), 52 deletions(-) diff --git a/dotnet-authserver/src/TeacherIdentity.AuthServer/AuthenticationState.cs b/dotnet-authserver/src/TeacherIdentity.AuthServer/AuthenticationState.cs index 260d4732f..f60143b4c 100644 --- a/dotnet-authserver/src/TeacherIdentity.AuthServer/AuthenticationState.cs +++ b/dotnet-authserver/src/TeacherIdentity.AuthServer/AuthenticationState.cs @@ -189,9 +189,9 @@ public bool TryGetOAuthState([NotNullWhen(true)] out OAuthAuthorizationState? oA public IEnumerable GetInternalClaims() { - if (!IsComplete) + if (!UserId.HasValue) { - throw new InvalidOperationException("Cannot retrieve claims until authentication is complete."); + throw new InvalidOperationException("User is not signed in."); } return UserClaimHelper.GetInternalClaims(this); @@ -199,13 +199,6 @@ public IEnumerable GetInternalClaims() public UserType[] GetPermittedUserTypes() => UserRequirements.GetPermittedUserTypes(); - public bool IsComplete => - EmailAddressVerified && - (!UserRequirements.HasFlag(UserRequirements.TrnHolder) || - HasTrnToken || - TrnLookup == TrnLookupState.Complete) && - UserId.HasValue; - public bool HasExpired(DateTime utcNow) => (StartedAt + JourneyLifetime) <= utcNow; public void Reset(DateTime utcNow) @@ -655,11 +648,6 @@ public void OnTrnLookupCompleted(FindTeachersResponseResult? findTeachersResult, public async Task SignIn(HttpContext httpContext) { - if (!IsComplete) - { - throw new InvalidOperationException("Journey is not complete."); - } - var claims = GetInternalClaims(); return await httpContext.SignInCookies(claims, resetIssued: true, AuthCookieLifetime); } diff --git a/dotnet-authserver/src/TeacherIdentity.AuthServer/Controllers/AuthorizationController.cs b/dotnet-authserver/src/TeacherIdentity.AuthServer/Controllers/AuthorizationController.cs index a612f4b53..de1f11983 100644 --- a/dotnet-authserver/src/TeacherIdentity.AuthServer/Controllers/AuthorizationController.cs +++ b/dotnet-authserver/src/TeacherIdentity.AuthServer/Controllers/AuthorizationController.cs @@ -181,7 +181,9 @@ public async Task Authorize() authenticationState.Reset(_clock.UtcNow); } - if (!authenticationState.IsComplete) + var signInJourney = _signInJourneyProvider.GetSignInJourney(authenticationState, HttpContext); + + if (!signInJourney.IsCompleted()) { // If the client application requested promptless authentication, // return an error indicating that the user is not logged in. @@ -196,7 +198,6 @@ public async Task Authorize() })); } - var signInJourney = _signInJourneyProvider.GetSignInJourney(authenticationState, HttpContext); return Redirect(signInJourney.GetStartStepUrl()); } @@ -207,7 +208,7 @@ public async Task Authorize() await _trnTokenHelper.ApplyTrnTokenToUser(authenticationState.UserId, trnToken.TrnToken); } - Debug.Assert(authenticationState.IsComplete); + Debug.Assert(signInJourney.IsCompleted()); // It's possible that the user doesn't have a 'signed in' cookie but the sign in journey is completed // (if, say, the user retried the page that sent the response cookie and they never got it). diff --git a/dotnet-authserver/src/TeacherIdentity.AuthServer/Infrastructure/Filters/RequireAuthenticationStateFilter.cs b/dotnet-authserver/src/TeacherIdentity.AuthServer/Infrastructure/Filters/RequireAuthenticationStateFilter.cs index 8c53dbb97..405a9e988 100644 --- a/dotnet-authserver/src/TeacherIdentity.AuthServer/Infrastructure/Filters/RequireAuthenticationStateFilter.cs +++ b/dotnet-authserver/src/TeacherIdentity.AuthServer/Infrastructure/Filters/RequireAuthenticationStateFilter.cs @@ -1,32 +1,25 @@ using Microsoft.AspNetCore.Http.Extensions; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Filters; +using TeacherIdentity.AuthServer.Journeys; using TeacherIdentity.AuthServer.State; namespace TeacherIdentity.AuthServer.Infrastructure.Filters; -public class RequireAuthenticationStateFilterFactory : IFilterFactory -{ - public bool IsReusable => false; // RequireAuthenticationStateFilter needs an IClock which is transient - - public IFilterMetadata CreateInstance(IServiceProvider serviceProvider) - { - var filter = serviceProvider.GetRequiredService(); - return filter; - } -} - public class RequireAuthenticationStateFilter : IAuthorizationFilter { + private readonly SignInJourneyProvider _signInJourneyProvider; private readonly IdentityLinkGenerator _linkGenerator; private readonly ILogger _logger; private readonly IClock _clock; public RequireAuthenticationStateFilter( + SignInJourneyProvider signInJourneyProvider, IdentityLinkGenerator linkGenerator, ILogger logger, IClock clock) { + _signInJourneyProvider = signInJourneyProvider; _linkGenerator = linkGenerator; _logger = logger; _clock = clock; @@ -44,10 +37,11 @@ public void OnAuthorization(AuthorizationFilterContext context) } var authenticationState = authenticationStateFeature.AuthenticationState; + var signInJourney = _signInJourneyProvider.GetSignInJourney(authenticationState, context.HttpContext); // If the journey has been completed then forward to the completion page // (prevents going 'back' to amend submitted details) - if (authenticationState.IsComplete) + if (signInJourney.IsCompleted()) { if (context.HttpContext.GetEndpoint()?.Metadata.Contains(AllowCompletedAuthenticationJourneyMarker.Instance) != true) { diff --git a/dotnet-authserver/src/TeacherIdentity.AuthServer/Journeys/CoreSignInJourney.cs b/dotnet-authserver/src/TeacherIdentity.AuthServer/Journeys/CoreSignInJourney.cs index 868e0a4d1..c6d623cc0 100644 --- a/dotnet-authserver/src/TeacherIdentity.AuthServer/Journeys/CoreSignInJourney.cs +++ b/dotnet-authserver/src/TeacherIdentity.AuthServer/Journeys/CoreSignInJourney.cs @@ -37,11 +37,11 @@ public override bool CanAccessStep(string step) Steps.EmailConfirmation => AuthenticationState is { EmailAddressSet: true, EmailAddressVerified: false }, Steps.ResendEmailConfirmation => AuthenticationState is { EmailAddressSet: true, EmailAddressVerified: false }, Steps.InstitutionEmail => AuthenticationState is { EmailAddressSet: true, EmailAddressVerified: true, IsInstitutionEmail: true }, - Steps.EmailExists => AuthenticationState.IsComplete, + Steps.EmailExists => AuthenticationState.UserId is not null, Steps.Phone => AuthenticationState.EmailAddressVerified, Steps.PhoneConfirmation => AuthenticationState is { MobileNumberSet: true, MobileNumberVerified: false, EmailAddressVerified: true }, Steps.ResendPhoneConfirmation => AuthenticationState is { MobileNumberSet: true, MobileNumberVerified: false }, - Steps.PhoneExists => AuthenticationState.IsComplete, + Steps.PhoneExists => AuthenticationState.UserId is not null, Steps.Name => AuthenticationState.ContactDetailsVerified, Steps.PreferredName => AuthenticationState is { NameSet: true, ContactDetailsVerified: true }, Steps.DateOfBirth => AuthenticationState is { PreferredNameSet: true, ContactDetailsVerified: true }, @@ -74,20 +74,20 @@ public override string GetLastAccessibleStepUrl(string? requestedStep) return (currentStep, AuthenticationState) switch { (SignInJourney.Steps.Email, _) => SignInJourney.Steps.EmailConfirmation, - (SignInJourney.Steps.EmailConfirmation, { IsComplete: true }) => Steps.EmailExists, - (SignInJourney.Steps.EmailConfirmation, { IsComplete: false }) => shouldCheckAnswers ? Steps.CheckAnswers : Steps.NoAccount, + (SignInJourney.Steps.EmailConfirmation, { UserId: not null }) => Steps.EmailExists, + (SignInJourney.Steps.EmailConfirmation, _) => shouldCheckAnswers ? Steps.CheckAnswers : Steps.NoAccount, (Steps.NoAccount, _) => Steps.Phone, (Steps.Landing, _) => Steps.Email, (Steps.Email, _) => Steps.EmailConfirmation, - (Steps.EmailConfirmation, { IsComplete: true }) => Steps.EmailExists, - (Steps.EmailConfirmation, { IsComplete: false, IsInstitutionEmail: true }) => shouldCheckAnswers ? Steps.CheckAnswers : Steps.InstitutionEmail, - (Steps.EmailConfirmation, { IsComplete: false }) => shouldCheckAnswers ? Steps.CheckAnswers : Steps.Phone, + (Steps.EmailConfirmation, { UserId: not null }) => Steps.EmailExists, + (Steps.EmailConfirmation, { IsInstitutionEmail: true, UserId: null }) => shouldCheckAnswers ? Steps.CheckAnswers : Steps.InstitutionEmail, + (Steps.EmailConfirmation, _) => shouldCheckAnswers ? Steps.CheckAnswers : Steps.Phone, (Steps.ResendEmailConfirmation, _) => Steps.EmailConfirmation, (Steps.InstitutionEmail, { EmailAddressVerified: false }) => Steps.EmailConfirmation, (Steps.InstitutionEmail, { EmailAddressVerified: true }) => shouldCheckAnswers ? Steps.CheckAnswers : Steps.Phone, (Steps.Phone, _) => Steps.PhoneConfirmation, - (Steps.PhoneConfirmation, { IsComplete: true }) => Steps.PhoneExists, - (Steps.PhoneConfirmation, { IsComplete: false }) => shouldCheckAnswers ? Steps.CheckAnswers : Steps.Name, + (Steps.PhoneConfirmation, { UserId: not null }) => Steps.PhoneExists, + (Steps.PhoneConfirmation, _) => shouldCheckAnswers ? Steps.CheckAnswers : Steps.Name, (Steps.ResendPhoneConfirmation, _) => Steps.PhoneConfirmation, (Steps.Name, { ExistingAccountFound: true }) => Steps.AccountExists, (Steps.Name, { ExistingAccountFound: false }) => shouldCheckAnswers ? Steps.CheckAnswers : Steps.PreferredName, @@ -139,7 +139,7 @@ public override string GetLastAccessibleStepUrl(string? requestedStep) protected override string GetStartStep() => Steps.Landing; - protected override bool IsFinished() => AuthenticationState.IsComplete; + protected override bool IsFinished() => AuthenticationState.UserId.HasValue; protected override string GetStepUrl(string step) => step switch { diff --git a/dotnet-authserver/src/TeacherIdentity.AuthServer/Journeys/TrnTokenSignInJourney.cs b/dotnet-authserver/src/TeacherIdentity.AuthServer/Journeys/TrnTokenSignInJourney.cs index 5023365fa..52c2926d2 100644 --- a/dotnet-authserver/src/TeacherIdentity.AuthServer/Journeys/TrnTokenSignInJourney.cs +++ b/dotnet-authserver/src/TeacherIdentity.AuthServer/Journeys/TrnTokenSignInJourney.cs @@ -70,7 +70,7 @@ public override bool CanAccessStep(string step) SignInJourney.Steps.EmailConfirmation => AuthenticationState is { EmailAddressSet: true, EmailAddressVerified: false }, CoreSignInJourney.Steps.Phone => AuthenticationState.EmailAddressVerified, CoreSignInJourney.Steps.PhoneConfirmation => AuthenticationState is { MobileNumberSet: true, MobileNumberVerified: false, EmailAddressVerified: true }, - CoreSignInJourney.Steps.PhoneExists => AuthenticationState.IsComplete, + CoreSignInJourney.Steps.PhoneExists => AuthenticationState.UserId is not null, CoreSignInJourney.Steps.ResendPhoneConfirmation => AuthenticationState is { MobileNumberSet: true, MobileNumberVerified: false }, CoreSignInJourney.Steps.Email => AuthenticationState.MobileNumberVerified, CoreSignInJourney.Steps.EmailConfirmation => AuthenticationState is { EmailAddressSet: true, EmailAddressVerified: false, MobileNumberVerified: true }, @@ -97,11 +97,11 @@ public override bool CanAccessStep(string step) (Steps.Landing, { ExistingAccountFound: true }) => CoreSignInJourney.Steps.AccountExists, (Steps.Landing, { ExistingAccountFound: false }) => CoreSignInJourney.Steps.Phone, (SignInJourney.Steps.Email, _) => SignInJourney.Steps.EmailConfirmation, - (SignInJourney.Steps.EmailConfirmation, { IsComplete: true }) => CoreSignInJourney.Steps.EmailExists, - (SignInJourney.Steps.EmailConfirmation, { IsComplete: false }) => shouldCheckAnswers ? Steps.CheckAnswers : CoreSignInJourney.Steps.Phone, + (SignInJourney.Steps.EmailConfirmation, { UserId: not null }) => CoreSignInJourney.Steps.EmailExists, + (SignInJourney.Steps.EmailConfirmation, _) => shouldCheckAnswers ? Steps.CheckAnswers : CoreSignInJourney.Steps.Phone, (CoreSignInJourney.Steps.Phone, _) => CoreSignInJourney.Steps.PhoneConfirmation, - (CoreSignInJourney.Steps.PhoneConfirmation, { IsComplete: true }) => CoreSignInJourney.Steps.PhoneExists, - (CoreSignInJourney.Steps.PhoneConfirmation, { IsComplete: false }) => shouldCheckAnswers ? Steps.CheckAnswers : CoreSignInJourney.Steps.PreferredName, + (CoreSignInJourney.Steps.PhoneConfirmation, { UserId: not null }) => CoreSignInJourney.Steps.PhoneExists, + (CoreSignInJourney.Steps.PhoneConfirmation, _) => shouldCheckAnswers ? Steps.CheckAnswers : CoreSignInJourney.Steps.PreferredName, (CoreSignInJourney.Steps.Email, _) => CoreSignInJourney.Steps.EmailConfirmation, (CoreSignInJourney.Steps.EmailConfirmation, { IsInstitutionEmail: false }) => Steps.CheckAnswers, (CoreSignInJourney.Steps.EmailConfirmation, { IsInstitutionEmail: true }) => CoreSignInJourney.Steps.InstitutionEmail, @@ -170,7 +170,9 @@ AuthenticationState is protected override string GetStartStep() => Steps.Landing; - protected override bool IsFinished() => AuthenticationState.IsComplete; + protected override bool IsFinished() => + AuthenticationState.UserId.HasValue && + AuthenticationState.TrnLookupStatus == TrnLookupStatus.Found; protected override string GetStepUrl(string step) => step switch { diff --git a/dotnet-authserver/src/TeacherIdentity.AuthServer/Program.cs b/dotnet-authserver/src/TeacherIdentity.AuthServer/Program.cs index 620ed3a83..c24bd2d72 100644 --- a/dotnet-authserver/src/TeacherIdentity.AuthServer/Program.cs +++ b/dotnet-authserver/src/TeacherIdentity.AuthServer/Program.cs @@ -293,7 +293,7 @@ public static async Task Main(string[] args) "/SignIn", model => { - model.Filters.Add(new RequireAuthenticationStateFilterFactory()); + model.Filters.Add(new ServiceFilterAttribute(typeof(RequireAuthenticationStateFilter))); model.Filters.Add(new NoCachePageFilter()); }); diff --git a/dotnet-authserver/src/TeacherIdentity.AuthServer/UserClaimHelper.cs b/dotnet-authserver/src/TeacherIdentity.AuthServer/UserClaimHelper.cs index ac02ba443..cdc3d1094 100644 --- a/dotnet-authserver/src/TeacherIdentity.AuthServer/UserClaimHelper.cs +++ b/dotnet-authserver/src/TeacherIdentity.AuthServer/UserClaimHelper.cs @@ -21,9 +21,9 @@ public UserClaimHelper(TeacherIdentityServerDbContext dbContext, IDqtApiClient d public static IReadOnlyCollection GetInternalClaims(AuthenticationState authenticationState) { - if (!authenticationState.IsComplete) + if (!authenticationState.UserId.HasValue) { - throw new InvalidOperationException("Cannot retrieve claims until authentication is complete."); + throw new InvalidOperationException("User is not signed in."); } return GetInternalClaims( @@ -37,9 +37,8 @@ public static IReadOnlyCollection GetInternalClaims(AuthenticationState a authenticationState.StaffRoles); } - public static IReadOnlyCollection GetInternalClaims(User user) - { - return GetInternalClaims( + public static IReadOnlyCollection GetInternalClaims(User user) => + GetInternalClaims( user.UserId, user.EmailAddress, user.FirstName, @@ -48,7 +47,6 @@ public static IReadOnlyCollection GetInternalClaims(User user) user.Trn, user.UserType, user.StaffRoles); - } public static string MapUserTypeToClaimValue(UserType userType) => userType.ToString();