From ada4125582ced53e44586caa85230ff7132805d6 Mon Sep 17 00:00:00 2001 From: Frode Hus Date: Sat, 19 Sep 2026 11:24:52 +0200 Subject: [PATCH] Let a role behind an authentication context actually activate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A role whose policy carries AuthenticationContext_EndUser_Assignment could not be activated at all: the request was refused, the browser step-up was completed, and the retry was refused again with the same message, however many times it was repeated. Two faults on the path between the step-up and the retry, and both had to go. Elevate never declared the cp1 client capability, so its tokens carried no xms_cc claim. PIM will not honour an authentication context from a client that has not said it understands a claims challenge: it answers with RoleAssignmentRequestAcrsValidationFailed and re-issues the same challenge, even for a token that plainly carries the context — confirmed against a live tenant, where a token with acrs=c10, amr=fido,rsa,mfa and the right scopes was refused until cp1 was declared. 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 asking MSAL silently for "the" token for those scopes instead. 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 same live trace showed it plainly: the held token carried acrs=c10, MSAL's silent call for the same account and scopes returned one without it. StepUpTokenCache holds the step-up token for its own lifetime and serves it to the retry, which also spares the second and third role behind the same context their own browser round trip. A forced refresh still goes past it, as it goes past MSAL's cache, so the propagation probes are unchanged. Verified against a live tenant on Windows: the activation that had been refused indefinitely now succeeds. NOT COMPILED on macOS. Written on a Windows machine with no Swift toolchain, so 'swift test' has never run against any of it. The Windows and CLI suites pass (965 tests) and the Windows app was built and driven by hand; the macOS half mirrors it and needs a run on a Mac. The loopback change there — the capability on every token request — is the one part no test exercises against a real server. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 20 ++++ cli/src/Elevate.Cli/Auth/MsalCliProvider.cs | 28 ++++++ .../ElevateApp/MSAL/MSALTokenProvider.swift | 21 +++++ .../ElevateCore/Auth/AccessTokenClaims.swift | 6 ++ .../Auth/AuthorizationCodeClient.swift | 5 + .../ElevateCore/Auth/StepUpTokenCache.swift | 54 +++++++++++ .../Networking/ClaimsChallenge.swift | 18 +++- .../AccessTokenClaimsTests.swift | 9 ++ .../AuthorizationCodeClientTests.swift | 11 +++ .../StepUpTokenCacheTests.swift | 65 +++++++++++++ .../Auth/FirstPartyTokenProvider.cs | 7 ++ .../Auth/MsalProviderBase.cs | 30 ++++++ .../Auth/MsalTokenProvider.cs | 8 ++ .../Elevate.Core/Auth/AccessTokenClaims.cs | 12 +++ .../src/Elevate.Core/Auth/StepUpTokenCache.cs | 92 ++++++++++++++++++ .../AccessTokenClaimsTests.cs | 12 +++ .../StepUpTokenCacheTests.cs | 93 +++++++++++++++++++ 17 files changed, 490 insertions(+), 1 deletion(-) create mode 100644 macos/Sources/ElevateCore/Auth/StepUpTokenCache.swift create mode 100644 macos/Tests/ElevateCoreTests/StepUpTokenCacheTests.swift create mode 100644 windows/src/Elevate.Core/Auth/StepUpTokenCache.cs create mode 100644 windows/tests/Elevate.Core.Tests/StepUpTokenCacheTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 965b46c5..b45fe3aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -99,6 +99,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"; + } +}