feat: Adds On-Behalf of Token Exchange support - #1094
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1094 +/- ##
==========================================
+ Coverage 25.77% 25.82% +0.04%
==========================================
Files 3209 3213 +4
Lines 151191 151283 +92
Branches 10220 10228 +8
==========================================
+ Hits 38970 39063 +93
+ Misses 109845 109844 -1
Partials 2376 2376
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| if (cache is null || !ReferenceEquals(cache.Token, token)) | ||
| { | ||
| cache = new ActorCache(token, ActClaimReader.ReadActor(token)); | ||
| Volatile.Write(ref actorCache, cache); |
There was a problem hiding this comment.
There is a write-write race here when AccessToken is reassigned on one thread while another thread is inside GetActor().
Scenario: thread A snapshots token = "v1" and is about to write the cache. Thread B reassigns AccessToken = "v2", computes ActorCache("v2", actor_v2), and writes it. Then thread A resumes and overwrites with ActorCache("v1", actor_v1). The cache now holds the actor for the old token. A third concurrent caller will read the wrong actor for "v2" until the cache is invalidated on the next call.
The fix is to replace Volatile.Write with Interlocked.CompareExchange(ref actorCache, cache, null) - or a snapshot-based CAS - so only the first writer wins and a stale overwrite is silently discarded. The concurrent-read test covers a stable token, but there is no test for this concurrent-mutation path.
There was a problem hiding this comment.
The scenario is real, it is near impossible to occur given that this method is on a token response.
Also, there will be no scenario where a wrong actor can be read since we are checking that the token in the cache is matching the token in the request. The worst possible outcome is that there could be a recompute of the value for the token that gets over-ridden.
No changes required.
| /// not contain a <c>sub</c>. | ||
| /// </summary> | ||
| [JsonPropertyName("sub")] | ||
| public string? Subject { get; set; } |
There was a problem hiding this comment.
Subject and Act are both settable ({ get; set; }). GetDelegationChain() returns the same Actor instance that is held inside the cached ActorCache, so a caller can do response.GetDelegationChain()!.Subject = "something" and permanently corrupt the cached actor. The next call to GetCurrentActor() will then return the wrong value.
Since Actor is only ever constructed by JSON deserialization or by ActClaimReader, changing both properties to { get; init; } would make the type immutable without breaking anything. That way the cached instance is safe to hand out directly.
| TokenType = response.TokenType, | ||
| ExpiresIn = response.ExpiresIn, | ||
| Scope = response.Scope, | ||
| IssuedTokenType = response.IssuedTokenType |
There was a problem hiding this comment.
RefreshToken and IdToken from the inner AccessTokenResponse are not mapped here and are silently dropped. The PR description says OBO issues no refresh or ID token, which is correct today, so this is fine in practice.
Just worth a note: if the server ever returns an id_token in this flow, it will actually be validated inside the GetTokenAsync(TokenExchangeTokenRequest) call above (via AssertIdTokenValidIfExisting), but the caller will never see it because OnBehalfOfTokenResponse has no IdToken property. A quick comment or a doc note on the response class saying these fields are intentionally excluded would help future maintainers understand this was a deliberate choice and not an oversight.
There was a problem hiding this comment.
This is by design. The OBO flow will not issue id_token or refresh_token today and if at it does, there should be a conscious change from our side on the SDK to expose it rather expose it automatically.
2 participants
Changes
This PR adds On-Behalf-Of (OBO) Token Exchange support to the Authentication API client, letting a service (for example, an MCP server) exchange an incoming user access token for a short-lived, audience-scoped access token that preserves the user's identity and actor attribution.
GetTokenOnBehalfOfAsyncmethod to the Authentication API client that performs the exchange. The caller supplies the incoming user token, the target audience, and optional scope/organization; the grant type and subject-token type are set internally.README.mdandExamples.md.References
Testing
Unit tests cover the exchange request (correct parameters sent), input validation, response mapping, and the actor helpers.
This change adds unit test coverage
This change adds integration test coverage
This change has been tested on the latest version of the platform/language or why not
Checklist