diff --git a/CHANGELOG.md b/CHANGELOG.md index b13941e8..29f09f21 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -133,6 +133,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- macOS, Windows and CLI: **a role behind a Conditional Access authentication context could not be + activated at all.** The activation was refused, the browser step-up was completed, and the retry + was refused again with the same message — "the sign-in did not satisfy it" — however many times + it was repeated. Two faults, both on the path between the step-up and the retry: + + Elevate never declared the `cp1` client capability, so its tokens carried no `xms_cc` claim. PIM + will not honour an authentication context (`acrs`) from a client that has not said it understands + a claims challenge: it answered every activation with `RoleAssignmentRequestAcrsValidationFailed` + and re-issued the same challenge, even for a token that plainly carried the context. Elevate has + always handled claims challenges, so it now says so — through MSAL's client configuration on + macOS, Windows and the CLI, and as an `xms_cc` claims request on every token the loopback + providers mint themselves, refreshes included, since a refreshed token would otherwise lose it. + + And the token the step-up produced was thrown away: the retry asked MSAL silently for "the" token + for those scopes, and MSAL bypasses its access-token cache whenever a claims request is specified + and makes no promise to write the result back into it, so the retry could go out with the token + from *before* the step-up. The step-up token is now held for its own lifetime and is the one the + retry sends, which also spares the second and third role behind the same context their own + browser round trip. + - macOS: the panel's confirmations (Remove tenant, Sign out, Delete profile) did nothing and dismissed the panel on click. A `confirmationDialog` opens its own window, which took key focus from the menu bar panel and closed it before the click reached a button. They now confirm inline, diff --git a/cli/src/Elevate.Cli/Auth/MsalCliProvider.cs b/cli/src/Elevate.Cli/Auth/MsalCliProvider.cs index 4c52dd8e..ad6eee79 100644 --- a/cli/src/Elevate.Cli/Auth/MsalCliProvider.cs +++ b/cli/src/Elevate.Cli/Auth/MsalCliProvider.cs @@ -34,6 +34,13 @@ public sealed class MsalCliProvider : ITokenProvider private readonly Action _say; private readonly Lazy _registered; + /// + /// Tokens from a claims (step-up) acquisition. MSAL bypasses its own access-token cache when a + /// claims request is specified, so the silent call that follows the step-up can hand back the + /// token from before it; the retry would then be refused for the same missing claim. + /// + private readonly StepUpTokenCache _stepUp = new(); + public MsalCliProvider(SignInMethod method, string clientId, TokenCacheStore cache, InteractiveGate gate, InteractiveFlow flow, Action say) { ArgumentNullException.ThrowIfNull(cache); @@ -77,6 +84,7 @@ public async Task SignInAsync(SignInMethod method, CancellationToken c public async Task SignOutAsync(Identity identity, CancellationToken ct = default) { ArgumentNullException.ThrowIfNull(identity); + _stepUp.Forget(identity.Id); await _registered.Value.ConfigureAwait(false); if (await FindAccountAsync(identity).ConfigureAwait(false) is { } account) { @@ -114,6 +122,13 @@ public Task FreshAccessTokenAsync(Identity identity, string tenantId, IR private async Task SilentAsync(Identity identity, string tenantId, IReadOnlyList scopes, bool forceRefresh, CancellationToken ct) { ArgumentNullException.ThrowIfNull(identity); + + // A forced refresh wants a token minted now, so it skips the step-up token as it skips MSAL's cache. + if (!forceRefresh && _stepUp.Token(identity.Id, tenantId, Requested(scopes)) is { } stepped) + { + return stepped; + } + await _registered.Value.ConfigureAwait(false); var account = await FindAccountAsync(identity).ConfigureAwait(false) ?? throw new PimException(PimErrorKind.InteractionRequired); @@ -131,6 +146,13 @@ public async Task AcquireInteractivelyAsync(Identity identity, string te var account = await FindAccountAsync(identity).ConfigureAwait(false); var result = await _gate.RunAsync( () => InteractiveAsync(Requested(scopes), account, tenantId, claims, ct), ct).ConfigureAwait(false); + + // Only a claims acquisition is worth holding: a plain one has nothing MSAL's cache lacks. + if (!string.IsNullOrEmpty(claims)) + { + _stepUp.Store(result.AccessToken, identity.Id, tenantId, Requested(scopes)); + } + return result.AccessToken; } @@ -227,6 +249,12 @@ private static IPublicClientApplication Build(string clientId) return PublicClientApplicationBuilder.Create(clientId) .WithAuthority(AzureCloudInstance.AzurePublic, "organizations") .WithRedirectUri("http://localhost") + // "cp1" tells Entra, and the resource, that this client understands a claims + // challenge and will re-acquire against it. PIM refuses to honour an authentication + // context (`acrs`) from a client that has not said so: it answers the activation + // with RoleAssignmentRequestAcrsValidationFailed and re-issues the same challenge, + // however many times the token is re-minted. MSAL turns this into `xms_cc`. + .WithClientCapabilities(["cp1"]) .Build(); } catch (MsalException e) diff --git a/macos/Sources/ElevateApp/MSAL/MSALTokenProvider.swift b/macos/Sources/ElevateApp/MSAL/MSALTokenProvider.swift index c3f6429f..098d3c6e 100644 --- a/macos/Sources/ElevateApp/MSAL/MSALTokenProvider.swift +++ b/macos/Sources/ElevateApp/MSAL/MSALTokenProvider.swift @@ -12,6 +12,10 @@ final class MSALTokenProvider: TokenProviding, @unchecked Sendable { private let anchor: AuthAnchorWindow /// Shared with the loopback providers so an MSAL webview and a browser flow cannot run at once. private let gate: InteractiveGate + /// Tokens from a claims (step-up) acquisition. MSAL bypasses its own access-token cache when a + /// claims request is specified, so the silent call that follows the step-up can hand back the + /// token from before it; the retry would then be refused for the same missing claim. + private let stepUp = StepUpTokenCache() /// The method stamped on the identities this provider returns: `.ownApp` for the Settings /// registration, `.pinnedApp` for a registration an account was added with. let method: SignInMethod @@ -21,6 +25,12 @@ final class MSALTokenProvider: TokenProviding, @unchecked Sendable { precondition(method.isOwnApp, "MSAL serves only Entra app registrations, got \(method)") let authority = try MSALAADAuthority(url: URL(string: "https://login.microsoftonline.com/organizations")!) let config = MSALPublicClientApplicationConfig(clientId: clientId, redirectUri: redirectUri, authority: authority) + // "cp1" tells Entra, and the resource, that this client understands a claims challenge and + // will re-acquire against it. PIM refuses to honour an authentication context (`acrs`) from + // a client that has not said so: it answers the activation with + // RoleAssignmentRequestAcrsValidationFailed and re-issues the same challenge, however many + // times the token is re-minted. MSAL turns this into the token's `xms_cc` claim. + config.clientApplicationCapabilities = [ClaimsChallenge.clientCapability] app = try MSALPublicClientApplication(configuration: config) self.anchor = anchor self.gate = gate @@ -40,6 +50,7 @@ final class MSALTokenProvider: TokenProviding, @unchecked Sendable { } func signOut(_ identity: Identity) async throws { + await stepUp.forget(identityId: identity.id) guard let account = try? app.account(forIdentifier: identity.id) else { return } try await gate.run { [self] in try await withCheckedThrowingContinuation { (cont: CheckedContinuation) in @@ -61,6 +72,7 @@ final class MSALTokenProvider: TokenProviding, @unchecked Sendable { /// sign-out would only interrupt the user. func removeCachedAccounts(_ identities: [Identity]) throws { for identity in identities { + Task { await stepUp.forget(identityId: identity.id) } guard let account = try? app.account(forIdentifier: identity.id) else { continue } try app.remove(account) } @@ -78,6 +90,11 @@ final class MSALTokenProvider: TokenProviding, @unchecked Sendable { var canForceRefresh: Bool { true } func accessToken(identity: Identity, tenantId: String, scopes: [String], forceRefresh: Bool) async throws -> String { + // A forced refresh wants a token minted now, so it skips the step-up token as it skips MSAL's cache. + if !forceRefresh, + let stepped = await stepUp.token(identityId: identity.id, tenantId: tenantId, scopes: scopes) { + return stepped + } guard let account = try? app.account(forIdentifier: identity.id) else { throw PIMError.interactionRequired } let params = MSALSilentTokenParameters(scopes: scopes, account: account) params.authority = try MSALAADAuthority(url: URL(string: "https://login.microsoftonline.com/\(tenantId)")!) @@ -94,6 +111,10 @@ final class MSALTokenProvider: TokenProviding, @unchecked Sendable { let account = try? app.account(forIdentifier: identity.id) return try await gate.run { [self] in let result = try await interactive(account: account, tenantId: tenantId, scopes: scopes, claims: claims, prompt: .promptIfNecessary) + // Only a claims acquisition is worth holding: a plain one has nothing MSAL's cache lacks. + if claims != nil { + await stepUp.store(result.accessToken, identityId: identity.id, tenantId: tenantId, scopes: scopes) + } return result.accessToken } } diff --git a/macos/Sources/ElevateCore/Auth/AccessTokenClaims.swift b/macos/Sources/ElevateCore/Auth/AccessTokenClaims.swift index a1bad54f..d5a4c472 100644 --- a/macos/Sources/ElevateCore/Auth/AccessTokenClaims.swift +++ b/macos/Sources/ElevateCore/Auth/AccessTokenClaims.swift @@ -15,6 +15,12 @@ public enum AccessTokenClaims { return Set(scp.split(separator: " ").map(String.init)) } + /// When the token expires (`exp`), or nil when the token is opaque or carries no expiry. + public static func expiry(_ accessToken: String) -> Date? { + guard let exp = payload(accessToken)?["exp"] as? NSNumber else { return nil } + return Date(timeIntervalSince1970: exp.doubleValue) + } + /// The caller's object id in the token's tenant (`oid`), or nil when the token is opaque. public static func objectId(_ accessToken: String) -> String? { payload(accessToken)?["oid"] as? String diff --git a/macos/Sources/ElevateCore/Auth/AuthorizationCodeClient.swift b/macos/Sources/ElevateCore/Auth/AuthorizationCodeClient.swift index 64ff61d0..0744a96e 100644 --- a/macos/Sources/ElevateCore/Auth/AuthorizationCodeClient.swift +++ b/macos/Sources/ElevateCore/Auth/AuthorizationCodeClient.swift @@ -108,6 +108,11 @@ public struct AuthorizationCodeClient: Sendable { private func post(tenant: String, form: [String: String]) async throws -> HTTPResponse { let url = URL(string: "https://login.microsoftonline.com/\(tenant)/oauth2/v2.0/token")! + // Every token this client mints declares the client capability, the way MSAL declares it + // from its configuration: a token without `xms_cc` is one PIM will not accept an + // authentication context from, and a refreshed token would otherwise quietly lose it. + var form = form + form["claims"] = ClaimsChallenge.clientCapabilities let body = form.sorted { $0.key < $1.key }.map { "\($0.key)=\(Self.formEncode($0.value))" }.joined(separator: "&") return try await http.send(HTTPRequest(method: "POST", url: url, headers: ["Content-Type": "application/x-www-form-urlencoded", "Accept": "application/json"], body: Data(body.utf8))) } diff --git a/macos/Sources/ElevateCore/Auth/StepUpTokenCache.swift b/macos/Sources/ElevateCore/Auth/StepUpTokenCache.swift new file mode 100644 index 00000000..14cd8c56 --- /dev/null +++ b/macos/Sources/ElevateCore/Auth/StepUpTokenCache.swift @@ -0,0 +1,54 @@ +import Foundation + +/// Holds the access token a step-up sign-in just produced, so the call that follows uses *that* +/// token rather than whatever the SDK's cache still holds. +/// +/// MSAL bypasses its access-token cache whenever a claims request is specified, and does not +/// promise to write the token it hands back into that cache. Asking it silently for a token right +/// after a step-up can therefore return the pre-step-up one — which carries no `acrs` claim, so the +/// service refuses the retry for exactly the reason it refused the first attempt, and the user is +/// told their verification did not count. `OAuthSession` already caches the loopback providers' +/// tokens itself; this is the same guarantee for MSAL. +public actor StepUpTokenCache { + struct Key: Hashable { let identityId: String; let tenantId: String; let scopes: String } + struct Entry { let token: String; let expiresAt: Date } + + /// Discarded this long before the token's own expiry, so a token that expires mid-call is not handed out. + static let skew: TimeInterval = 60 + /// Lifetime assumed for a token whose expiry cannot be read (an opaque, non-JWT token). Long + /// enough to cover the retry the step-up was made for, short enough to be no one's cache. + static let opaqueLifetime: TimeInterval = 300 + + private var entries: [Key: Entry] = [:] + private let now: @Sendable () -> Date + + public init(now: @escaping @Sendable () -> Date = { Date() }) { self.now = now } + + /// Remembers a token acquired with a claims request. Call only for a claims acquisition: a + /// plain interactive token has nothing the SDK's own cache lacks. + public func store(_ token: String, identityId: String, tenantId: String, scopes: [String]) { + let expiry = AccessTokenClaims.expiry(token) ?? now().addingTimeInterval(Self.opaqueLifetime) + entries[Self.key(identityId, tenantId, scopes)] = Entry(token: token, expiresAt: expiry) + } + + /// The stored token for this identity, tenant and scope set, or nil once it is gone or too near expiry. + public func token(identityId: String, tenantId: String, scopes: [String]) -> String? { + let key = Self.key(identityId, tenantId, scopes) + guard let entry = entries[key] else { return nil } + guard entry.expiresAt.timeIntervalSince(now()) > Self.skew else { + entries[key] = nil + return nil + } + return entry.token + } + + /// Drops everything held for one identity: it signed out, or its client id changed. + public func forget(identityId: String) { + entries = entries.filter { $0.key.identityId != identityId } + } + + /// Scope order is the caller's accident, not part of the identity of a token. + private static func key(_ identityId: String, _ tenantId: String, _ scopes: [String]) -> Key { + Key(identityId: identityId, tenantId: tenantId, scopes: scopes.sorted().joined(separator: " ")) + } +} diff --git a/macos/Sources/ElevateCore/Networking/ClaimsChallenge.swift b/macos/Sources/ElevateCore/Networking/ClaimsChallenge.swift index 741b0470..0cdbeed2 100644 --- a/macos/Sources/ElevateCore/Networking/ClaimsChallenge.swift +++ b/macos/Sources/ElevateCore/Networking/ClaimsChallenge.swift @@ -15,9 +15,25 @@ public enum ClaimsChallenge { public static let multiFactor = #"{"access_token":{"amr":{"values":["mfa"]}}}"# /// Claims request for a Conditional Access authentication context (`acrs`), for roles whose - /// policy carries `AuthenticationContext_EndUser_Assignment`. + /// policy carries `AuthenticationContext_EndUser_Assignment`. It asks for the context alone: + /// MSAL merges the client capability in from its own configuration, and a second `xms_cc` here + /// would collide with it. The loopback providers, which have no MSAL to do that, send + /// `clientCapabilities` on their own acquisitions instead. public static func authenticationContext(_ id: String) -> String { let escaped = id.replacingOccurrences(of: "\\", with: "\\\\").replacingOccurrences(of: "\"", with: "\\\"") return #"{"access_token":{"acrs":{"essential":true,"value":""# + escaped + #""}}}"# } + + /// The capability that says this client understands a claims challenge and will re-acquire + /// against it. Without it Entra omits the `xms_cc` claim, and PIM refuses to honour an + /// authentication context the token plainly carries: it answers the activation with + /// `RoleAssignmentRequestAcrsValidationFailed` and re-issues the same challenge for as long as + /// the client keeps re-minting tokens. MSAL declares it from the client configuration; the + /// loopback providers, which speak to the token endpoint themselves, send it as a claims + /// request on every acquisition. + public static let clientCapability = "cp1" + + /// The claims request that declares nothing but the client capability, for a token acquisition + /// that carries no challenge of its own. + public static let clientCapabilities = #"{"access_token":{"xms_cc":{"values":[""# + clientCapability + #""]}}}"# } diff --git a/macos/Tests/ElevateCoreTests/AccessTokenClaimsTests.swift b/macos/Tests/ElevateCoreTests/AccessTokenClaimsTests.swift index 1e2aa5b6..2f4f2c1d 100644 --- a/macos/Tests/ElevateCoreTests/AccessTokenClaimsTests.swift +++ b/macos/Tests/ElevateCoreTests/AccessTokenClaimsTests.swift @@ -27,6 +27,15 @@ import Foundation } } + @Test func readsExpiryFromExpClaim() { + let body = try! JSONSerialization.data(withJSONObject: ["exp": 1_700_000_000]) + let b64 = body.base64EncodedString().replacingOccurrences(of: "+", with: "-") + .replacingOccurrences(of: "/", with: "_").trimmingCharacters(in: CharacterSet(charactersIn: "=")) + #expect(AccessTokenClaims.expiry("eyJhbGciOiJub25lIn0.\(b64).sig") == Date(timeIntervalSince1970: 1_700_000_000)) + #expect(AccessTokenClaims.expiry("not-a-jwt") == nil) + #expect(AccessTokenClaims.expiry(token(scp: nil)) == nil) + } + @Test func opaqueOrScopelessTokenIsUnknown() { #expect(AccessTokenClaims.permitsEntraActivation("not-a-jwt") == nil) #expect(AccessTokenClaims.permitsEntraActivation(token(scp: nil)) == nil) diff --git a/macos/Tests/ElevateCoreTests/AuthorizationCodeClientTests.swift b/macos/Tests/ElevateCoreTests/AuthorizationCodeClientTests.swift index bad7f66a..e5cc353e 100644 --- a/macos/Tests/ElevateCoreTests/AuthorizationCodeClientTests.swift +++ b/macos/Tests/ElevateCoreTests/AuthorizationCodeClientTests.swift @@ -83,4 +83,15 @@ import Foundation #expect(AuthorizationCodeClient.resourceScope(for: ["https://graph.microsoft.com/User.Read", "https://graph.microsoft.com/RoleEligibilitySchedule.Read.Directory"]) == "https://graph.microsoft.com/.default") #expect(AuthorizationCodeClient.resourceScope(for: ["https://management.azure.com/user_impersonation"]) == "https://management.azure.com/.default") } + + @Test func everyTokenRequestDeclaresTheClientCapability() async throws { + // Without `xms_cc` in the token, PIM refuses to honour an authentication context, so the + // capability has to ride on the refresh too, not only on the sign-in. + let http = StubHTTPClient() + await http.on("POST", "/tenant-2/oauth2/v2.0/token", body: Data(#"{"token_type":"Bearer","expires_in":100,"access_token":"AT2"}"#.utf8)) + _ = try await AuthorizationCodeClient(http: http).refresh(refreshToken: "RT", clientId: clientId, tenant: "tenant-2", scopes: ["https://management.azure.com/user_impersonation"]) + let form = String(decoding: (await http.requests.first!).body!, as: UTF8.self) + #expect(form.contains("claims=\(AuthorizationCodeClient.formEncode(ClaimsChallenge.clientCapabilities))")) + #expect(ClaimsChallenge.clientCapabilities.contains(#""xms_cc""#) && ClaimsChallenge.clientCapabilities.contains("cp1")) + } } diff --git a/macos/Tests/ElevateCoreTests/StepUpTokenCacheTests.swift b/macos/Tests/ElevateCoreTests/StepUpTokenCacheTests.swift new file mode 100644 index 00000000..7331da80 --- /dev/null +++ b/macos/Tests/ElevateCoreTests/StepUpTokenCacheTests.swift @@ -0,0 +1,65 @@ +import Testing +import Foundation +@testable import ElevateCore + +@Suite struct StepUpTokenCacheTests { + private func token(exp: Date?) -> String { + var payload: [String: Any] = ["aud": "https://graph.microsoft.com", "acrs": "c10"] + if let exp { payload["exp"] = Int(exp.timeIntervalSince1970) } + let body = try! JSONSerialization.data(withJSONObject: payload) + let b64 = body.base64EncodedString().replacingOccurrences(of: "+", with: "-") + .replacingOccurrences(of: "/", with: "_").trimmingCharacters(in: CharacterSet(charactersIn: "=")) + return "eyJhbGciOiJub25lIn0.\(b64).sig" + } + + private let scopes = ["https://graph.microsoft.com/User.Read", "https://graph.microsoft.com/RoleManagementPolicy.Read.Directory"] + + @Test func handsBackTheStepUpTokenForTheSameIdentityTenantAndScopes() async { + let cache = StepUpTokenCache() + let stepped = token(exp: .now.addingTimeInterval(3600)) + await cache.store(stepped, identityId: "a", tenantId: "t", scopes: scopes) + #expect(await cache.token(identityId: "a", tenantId: "t", scopes: scopes.reversed()) == stepped) + } + + @Test func holdsNothingForAnotherIdentityTenantOrScopeSet() async { + let cache = StepUpTokenCache() + await cache.store(token(exp: .now.addingTimeInterval(3600)), identityId: "a", tenantId: "t", scopes: scopes) + #expect(await cache.token(identityId: "b", tenantId: "t", scopes: scopes) == nil) + #expect(await cache.token(identityId: "a", tenantId: "other", scopes: scopes) == nil) + #expect(await cache.token(identityId: "a", tenantId: "t", scopes: ["https://management.azure.com/user_impersonation"]) == nil) + } + + @Test func dropsTheTokenOnceItsOwnExpiryIsNear() async { + let clock = Clock() + let cache = StepUpTokenCache(now: { clock.now }) + await cache.store(token(exp: clock.now.addingTimeInterval(600)), identityId: "a", tenantId: "t", scopes: scopes) + clock.advance(600 - StepUpTokenCache.skew + 1) + #expect(await cache.token(identityId: "a", tenantId: "t", scopes: scopes) == nil) + } + + @Test func anOpaqueTokenIsHeldOnlyLongEnoughForTheRetry() async { + let clock = Clock() + let cache = StepUpTokenCache(now: { clock.now }) + await cache.store("not-a-jwt", identityId: "a", tenantId: "t", scopes: scopes) + #expect(await cache.token(identityId: "a", tenantId: "t", scopes: scopes) == "not-a-jwt") + clock.advance(StepUpTokenCache.opaqueLifetime) + #expect(await cache.token(identityId: "a", tenantId: "t", scopes: scopes) == nil) + } + + @Test func signingOutForgetsWhatWasHeldForThatIdentityAlone() async { + let cache = StepUpTokenCache() + let kept = token(exp: .now.addingTimeInterval(3600)) + await cache.store(token(exp: .now.addingTimeInterval(3600)), identityId: "a", tenantId: "t", scopes: scopes) + await cache.store(kept, identityId: "b", tenantId: "t", scopes: scopes) + await cache.forget(identityId: "a") + #expect(await cache.token(identityId: "a", tenantId: "t", scopes: scopes) == nil) + #expect(await cache.token(identityId: "b", tenantId: "t", scopes: scopes) == kept) + } + + private final class Clock: @unchecked Sendable { + private let lock = NSLock() + private var date = Date(timeIntervalSince1970: 1_700_000_000) + var now: Date { lock.withLock { date } } + func advance(_ seconds: TimeInterval) { lock.withLock { date += seconds } } + } +} diff --git a/windows/src/Elevate.App.Model/Auth/FirstPartyTokenProvider.cs b/windows/src/Elevate.App.Model/Auth/FirstPartyTokenProvider.cs index b36bbece..61af4af1 100644 --- a/windows/src/Elevate.App.Model/Auth/FirstPartyTokenProvider.cs +++ b/windows/src/Elevate.App.Model/Auth/FirstPartyTokenProvider.cs @@ -57,6 +57,13 @@ private static IPublicClientApplication Build(SignInMethod method, Func return PublicClientApplicationBuilder.Create(clientId.Trim()) .WithAuthority(AzureCloudInstance.AzurePublic, "organizations") .WithRedirectUri(AppSettings.LoopbackRedirectUri) + // "cp1" tells Entra, and the resource, that this client understands a claims + // challenge and will re-acquire against it. PIM refuses to honour an authentication + // context (`acrs`) from a client that has not said so: it answers the activation + // with RoleAssignmentRequestAcrsValidationFailed and re-issues the same challenge, + // however many times the token is re-minted. MSAL turns this into the token's + // `xms_cc` claim. + .WithClientCapabilities(["cp1"]) .WithParentActivityOrWindow(parentWindow) .Build(); } diff --git a/windows/src/Elevate.App.Model/Auth/MsalProviderBase.cs b/windows/src/Elevate.App.Model/Auth/MsalProviderBase.cs index 0321f1d8..97c61d34 100644 --- a/windows/src/Elevate.App.Model/Auth/MsalProviderBase.cs +++ b/windows/src/Elevate.App.Model/Auth/MsalProviderBase.cs @@ -15,6 +15,13 @@ public abstract class MsalProviderBase : ITokenProvider private readonly InteractiveGate _gate; private readonly Lazy _registered; + /// + /// Tokens from a claims (step-up) acquisition. MSAL bypasses its own access-token cache when a + /// claims request is specified, so the silent call that follows the step-up can hand back the + /// token from before it; the retry would then be refused for the same missing claim. + /// + private readonly StepUpTokenCache _stepUp = new(); + protected MsalProviderBase(IPublicClientApplication app, TokenCache cache, InteractiveGate gate) { ArgumentNullException.ThrowIfNull(app); @@ -53,6 +60,7 @@ public async Task SignInAsync(SignInMethod method, CancellationToken c public async Task SignOutAsync(Identity identity, CancellationToken ct) { ArgumentNullException.ThrowIfNull(identity); + ForgetStepUpToken(identity); await EnsureCacheAsync().ConfigureAwait(false); if (await FindAccountAsync(identity).ConfigureAwait(false) is { } account) { @@ -77,6 +85,14 @@ public async Task AccessTokenAsync( Identity identity, string tenantId, IReadOnlyList scopes, bool forceRefresh, CancellationToken ct) { ArgumentNullException.ThrowIfNull(identity); + ArgumentNullException.ThrowIfNull(scopes); + + // A forced refresh wants a token minted now, so it skips the step-up token as it skips MSAL's cache. + if (!forceRefresh && _stepUp.Token(identity.Id, tenantId, Requested(scopes)) is { } stepped) + { + return stepped; + } + await EnsureCacheAsync().ConfigureAwait(false); var account = await FindAccountAsync(identity).ConfigureAwait(false) ?? throw new PimException(PimErrorKind.InteractionRequired); @@ -95,11 +111,25 @@ public async Task AcquireInteractivelyAsync(Identity identity, string te var result = await _gate.RunAsync( () => Interactive(Requested(scopes), account, tenantId, claims, account is null ? Prompt.SelectAccount : Prompt.NoPrompt, ct), ct) .ConfigureAwait(false); + + // Only a claims acquisition is worth holding: a plain one has nothing MSAL's cache lacks. + if (!string.IsNullOrEmpty(claims)) + { + _stepUp.Store(result.AccessToken, identity.Id, tenantId, Requested(scopes)); + } + return result.AccessToken; } protected Task EnsureCacheAsync() => _registered.Value; + /// Drops any step-up token held for an identity that is going away. + protected void ForgetStepUpToken(Identity identity) + { + ArgumentNullException.ThrowIfNull(identity); + _stepUp.Forget(identity.Id); + } + protected async Task FindAccountAsync(Identity identity) { try diff --git a/windows/src/Elevate.App.Model/Auth/MsalTokenProvider.cs b/windows/src/Elevate.App.Model/Auth/MsalTokenProvider.cs index f832b3de..a3d71be7 100644 --- a/windows/src/Elevate.App.Model/Auth/MsalTokenProvider.cs +++ b/windows/src/Elevate.App.Model/Auth/MsalTokenProvider.cs @@ -49,6 +49,7 @@ public async Task RemoveCachedAccountsAsync(IEnumerable identities, Ca await EnsureCacheAsync().ConfigureAwait(false); foreach (var identity in identities) { + ForgetStepUpToken(identity); if (await FindAccountAsync(identity).ConfigureAwait(false) is { } account) { await Run(() => App.RemoveAsync(account)).ConfigureAwait(false); @@ -71,6 +72,13 @@ private static IPublicClientApplication Build(string clientId, Func pare // The broker uses ms-appx-web://microsoft.aad.brokerplugin/{clientId}; localhost is the browser fallback. .WithRedirectUri(AppSettings.LoopbackRedirectUri) .WithBroker(new BrokerOptions(BrokerOptions.OperatingSystems.Windows) { Title = "Elevate" }) + // "cp1" tells Entra, and the resource, that this client understands a claims + // challenge and will re-acquire against it. PIM refuses to honour an authentication + // context (`acrs`) from a client that has not said so: it answers the activation + // with RoleAssignmentRequestAcrsValidationFailed and re-issues the same challenge, + // however many times the token is re-minted. MSAL turns this into the token's + // `xms_cc` claim. + .WithClientCapabilities(["cp1"]) .WithParentActivityOrWindow(parentWindow) .Build(); } diff --git a/windows/src/Elevate.Core/Auth/AccessTokenClaims.cs b/windows/src/Elevate.Core/Auth/AccessTokenClaims.cs index 843dc29a..99e79a62 100644 --- a/windows/src/Elevate.Core/Auth/AccessTokenClaims.cs +++ b/windows/src/Elevate.Core/Auth/AccessTokenClaims.cs @@ -26,6 +26,18 @@ public static class AccessTokenClaims return new HashSet(scp.Split(' ', StringSplitOptions.RemoveEmptyEntries), StringComparer.Ordinal); } + /// When the token expires (exp), or null when the token is opaque or carries no expiry. + public static DateTimeOffset? Expiry(string accessToken) + { + using var payload = Payload(accessToken); + return payload is not null + && payload.RootElement.TryGetProperty("exp", out var value) + && value.ValueKind == JsonValueKind.Number + && value.TryGetInt64(out var seconds) + ? DateTimeOffset.FromUnixTimeSeconds(seconds) + : null; + } + /// The caller's object id in the token's tenant (oid), or null when the token is opaque. public static string? ObjectId(string accessToken) => Claim(accessToken, "oid"); diff --git a/windows/src/Elevate.Core/Auth/StepUpTokenCache.cs b/windows/src/Elevate.Core/Auth/StepUpTokenCache.cs new file mode 100644 index 00000000..5d3dd391 --- /dev/null +++ b/windows/src/Elevate.Core/Auth/StepUpTokenCache.cs @@ -0,0 +1,92 @@ +namespace Elevate.Core.Auth; + +/// +/// Holds the access token a step-up sign-in just produced, so the call that follows uses +/// that token rather than whatever MSAL's cache still holds. Port of the Swift +/// StepUpTokenCache. +/// +/// +/// MSAL bypasses its access-token cache whenever a claims request is specified, and does not +/// promise to write the token it hands back into that cache. Asking it silently for a token right +/// after a step-up can therefore return the pre-step-up one — which carries no acrs claim, +/// so the service refuses the retry for exactly the reason it refused the first attempt, and the +/// user is told their verification did not count. +/// +public sealed class StepUpTokenCache +{ + /// Discarded this long before the token's own expiry, so a token that expires mid-call is not handed out. + public static readonly TimeSpan Skew = TimeSpan.FromSeconds(60); + + /// + /// Lifetime assumed for a token whose expiry cannot be read (an opaque, non-JWT token). Long + /// enough to cover the retry the step-up was made for, short enough to be no one's cache. + /// + public static readonly TimeSpan OpaqueLifetime = TimeSpan.FromMinutes(5); + + private readonly Dictionary _entries = []; + private readonly Lock _gate = new(); + private readonly Func _now; + + /// The clock, for tests; the system clock by default. + public StepUpTokenCache(Func? now = null) => _now = now ?? (() => DateTimeOffset.UtcNow); + + /// + /// Remembers a token acquired with a claims request. Call only for a claims acquisition: a + /// plain interactive token has nothing MSAL's own cache lacks. + /// + public void Store(string token, string identityId, string tenantId, IReadOnlyList scopes) + { + var expiry = AccessTokenClaims.Expiry(token) ?? _now() + OpaqueLifetime; + lock (_gate) + { + _entries[KeyFor(identityId, tenantId, scopes)] = new Entry(token, expiry); + } + } + + /// + /// The stored token for this identity, tenant and scope set, or null once it is gone or too + /// near expiry. + /// + public string? Token(string identityId, string tenantId, IReadOnlyList scopes) + { + var key = KeyFor(identityId, tenantId, scopes); + lock (_gate) + { + if (!_entries.TryGetValue(key, out var entry)) + { + return null; + } + + if (entry.ExpiresAt - _now() <= Skew) + { + _entries.Remove(key); + return null; + } + + return entry.Token; + } + } + + /// Drops everything held for one identity: it signed out, or its client id changed. + public void Forget(string identityId) + { + lock (_gate) + { + foreach (var key in _entries.Keys.Where(k => k.IdentityId == identityId).ToList()) + { + _entries.Remove(key); + } + } + } + + /// Scope order is the caller's accident, not part of the identity of a token. + private static Key KeyFor(string identityId, string tenantId, IReadOnlyList scopes) + { + ArgumentNullException.ThrowIfNull(scopes); + return new Key(identityId, tenantId, string.Join(' ', scopes.Order(StringComparer.Ordinal))); + } + + private readonly record struct Key(string IdentityId, string TenantId, string Scopes); + + private readonly record struct Entry(string Token, DateTimeOffset ExpiresAt); +} diff --git a/windows/tests/Elevate.Core.Tests/AccessTokenClaimsTests.cs b/windows/tests/Elevate.Core.Tests/AccessTokenClaimsTests.cs index c41e13d9..9b96a4f4 100644 --- a/windows/tests/Elevate.Core.Tests/AccessTokenClaimsTests.cs +++ b/windows/tests/Elevate.Core.Tests/AccessTokenClaimsTests.cs @@ -67,4 +67,16 @@ public void EntitlementScopeSetHoldsOneUserConsentableScope() Scopes.EntitlementAll.Should().Equal("https://graph.microsoft.com/EntitlementMgmt-SubjectAccess.ReadWrite"); Scopes.EntitlementClaim.Should().Be("EntitlementMgmt-SubjectAccess.ReadWrite"); } + + [Fact] + public void ReadsExpiryFromExpClaim() + { + var body = JsonSerializer.SerializeToUtf8Bytes(new Dictionary { ["exp"] = 1_700_000_000 }); + var b64 = Convert.ToBase64String(body).Replace('+', '-').Replace('/', '_').TrimEnd('='); + + AccessTokenClaims.Expiry($"eyJhbGciOiJub25lIn0.{b64}.sig") + .Should().Be(DateTimeOffset.FromUnixTimeSeconds(1_700_000_000)); + AccessTokenClaims.Expiry("not-a-jwt").Should().BeNull(); + AccessTokenClaims.Expiry(Token(null)).Should().BeNull(); + } } diff --git a/windows/tests/Elevate.Core.Tests/StepUpTokenCacheTests.cs b/windows/tests/Elevate.Core.Tests/StepUpTokenCacheTests.cs new file mode 100644 index 00000000..b18e64c2 --- /dev/null +++ b/windows/tests/Elevate.Core.Tests/StepUpTokenCacheTests.cs @@ -0,0 +1,93 @@ +using System.Text.Json; +using Elevate.Core.Auth; +using FluentAssertions; + +namespace Elevate.Core.Tests; + +/// Port of the Swift StepUpTokenCacheTests. +public class StepUpTokenCacheTests +{ + private static readonly string[] Scopes = + [ + "https://graph.microsoft.com/User.Read", + "https://graph.microsoft.com/RoleManagementPolicy.Read.Directory", + ]; + + private static readonly DateTimeOffset Start = DateTimeOffset.FromUnixTimeSeconds(1_700_000_000); + + [Fact] + public void HandsBackTheStepUpTokenForTheSameIdentityTenantAndScopes() + { + var cache = new StepUpTokenCache(); + var stepped = Token(DateTimeOffset.UtcNow.AddHours(1)); + + cache.Store(stepped, "a", "t", Scopes); + + cache.Token("a", "t", [.. Scopes.Reverse()]).Should().Be(stepped); + } + + [Fact] + public void HoldsNothingForAnotherIdentityTenantOrScopeSet() + { + var cache = new StepUpTokenCache(); + cache.Store(Token(DateTimeOffset.UtcNow.AddHours(1)), "a", "t", Scopes); + + cache.Token("b", "t", Scopes).Should().BeNull(); + cache.Token("a", "other", Scopes).Should().BeNull(); + cache.Token("a", "t", ["https://management.azure.com/user_impersonation"]).Should().BeNull(); + } + + [Fact] + public void DropsTheTokenOnceItsOwnExpiryIsNear() + { + var now = Start; + var cache = new StepUpTokenCache(() => now); + cache.Store(Token(Start.AddMinutes(10)), "a", "t", Scopes); + + now = Start.AddMinutes(10) - StepUpTokenCache.Skew; + + cache.Token("a", "t", Scopes).Should().BeNull(); + } + + [Fact] + public void AnOpaqueTokenIsHeldOnlyLongEnoughForTheRetry() + { + var now = Start; + var cache = new StepUpTokenCache(() => now); + cache.Store("not-a-jwt", "a", "t", Scopes); + + cache.Token("a", "t", Scopes).Should().Be("not-a-jwt"); + + now = Start + StepUpTokenCache.OpaqueLifetime; + + cache.Token("a", "t", Scopes).Should().BeNull(); + } + + [Fact] + public void SigningOutForgetsWhatWasHeldForThatIdentityAlone() + { + var cache = new StepUpTokenCache(); + var kept = Token(DateTimeOffset.UtcNow.AddHours(1)); + cache.Store(Token(DateTimeOffset.UtcNow.AddHours(1)), "a", "t", Scopes); + cache.Store(kept, "b", "t", Scopes); + + cache.Forget("a"); + + cache.Token("a", "t", Scopes).Should().BeNull(); + cache.Token("b", "t", Scopes).Should().Be(kept); + } + + /// A JWT-shaped token that expires at the given moment. + private static string Token(DateTimeOffset expiry) + { + var payload = new Dictionary + { + ["aud"] = "https://graph.microsoft.com", + ["acrs"] = "c10", + ["exp"] = expiry.ToUnixTimeSeconds(), + }; + var b64 = Convert.ToBase64String(JsonSerializer.SerializeToUtf8Bytes(payload)) + .Replace('+', '-').Replace('/', '_').TrimEnd('='); + return $"eyJhbGciOiJub25lIn0.{b64}.sig"; + } +}