feat: Add Enterprise Managed Authorization (SEP-990) support#1305
feat: Add Enterprise Managed Authorization (SEP-990) support#1305aniket-okta wants to merge 4 commits into
Conversation
|
|
||
| var tokens = await provider.GetAccessTokenAsync( | ||
| resourceUrl: new Uri("https://mcp-server.example.com"), | ||
| authorizationServerUrl: new Uri("https://auth.mcp-server.example.com")); |
There was a problem hiding this comment.
We experimented with adding some of these OAuth flows in #1178, but we haven't added it yet since it's an Auth Extension rather than a primary part of the spec. See https://github.com/modelcontextprotocol/ext-auth/blob/5c50488e96e242b4d09c2b97ae47fa152d96a836/specification/draft/enterprise-managed-authorization.mdx and https://github.com/modelcontextprotocol/conformance/blob/066b2d70800a8bd6bca82e000007c9877b443e5b/src/scenarios/client/auth/index.ts#L50-L55.
I do like that this doesn't pollute the ClientOAuthOptions like #1178 did, but I wonder if this wouldn't be better as a separate package. It looks like you could use these APIs to get the access token for any HttpClient needing to support these OAuth flows. It seems like something with more features like Duende.AccessTokenManagement.OpenIdConnect might be a better general solution for this than something tied specifically to the MCP C# SDK.
There was a problem hiding this comment.
Thanks for the review @halter73 ! Good points - I want to address the packaging question.
Why I placed this in ModelContextProtocol.Core:
- This was requested in Implement SEP-990: Enterprise Managed Authorization (Extension) #949, filed by the MCP spec team and tracked on the project board. Both the Go SDK (go-sdk#770) and TS SDK (typescript-sdk#1531) include this in their main packages - not as separate packages.
- While the underlying RFCs (8693, 7523) are generic, the implementation is MCP-specific - it uses the urn:ietf:params:oauth:token-type:id-jag token type, MCP resource URIs, and the EnterpriseAuthProvider is built around the MCP orchestrator context (ResourceUrl, AuthorizationServerUrl).
- Re: Duende - it handles OIDC token lifecycle for ASP.NET Core apps, but doesn't implement the ID-JAG token exchange or the MCP-specific JWT bearer grant pattern. Users would still need to orchestrate the full flow manually.
- There are conformance tests (CrossAppAccessCompleteFlowScenario) for this flow already.
That said, I'm happy to move this to a separate package (e.g. ModelContextProtocol.Auth.Enterprise) if the team prefers that pattern for auth extensions. The code is self-contained with no dependencies on SDK internals, so it would be a straightforward move. What do you think?
There was a problem hiding this comment.
Makes sense. Are APIs that just provide the access token that we can then use to create a new client sufficient? I know for the normal OAuth flows we need a bit deeper integration to support scope step up and token refresh. I suppose scope step up would be hard without an interactive user and a browser, but refresh seems like it could still be useful. I suppose you could always refresh manually and call ResumeSessionAsync with the new access token. What do the other SDKs do for token refresh in this case?
There was a problem hiding this comment.
As for the separate package, I don't think that's necessary. My primary concern would be not making ClientOAuthOptions overly confusing, but this PR avoids that by providing completely independent APIs.
There was a problem hiding this comment.
Thanks for the great questions, @halter73!
On scope step-up:
Correct - scope step-up is fundamentally incompatible with non-interactive enterprise flows. The whole point of SEP-990 is that the user has already authenticated once via their enterprise IdP (SSO), so there is no browser/user available at MCP-request time. Scope step-up would require re-prompting the user, which this flow is designed to eliminate. So no step-up support is by design, and that matches the behavior in both the Go and TS SDKs.
On token refresh:
Neither the Go SDK (go-sdk#770) nor the TS SDK (typescript-sdk#1531) implement explicit refresh token handling for this flow. The reason is structural: the ID-JAG itself is short-lived by design (it's a one-time assertion, not a long-lived credential). When the access token expires, the correct action is to re-run the full flow — re-request a fresh ID-JAG from the IdP, then exchange it for a new access token. This is effectively a "refresh" but through the full two-step exchange rather than a
refresh_tokengrant.
There was a problem hiding this comment.
In this PR, that is already supported via:
- Automatic re-execution:
EnterpriseAuthProvider.GetAccessTokenAsync()checks_cachedTokens.IsExpiredand re-runs the full flow when the token expires. - Manual invalidation: Callers can call
InvalidateCache()to force a fresh exchange on the next call.
Your suggestion of calling ResumeSessionAsync with a new access token after InvalidateCache() would work well for the HttpClient-based integration. However, a more seamless pattern might be to expose a CreateAuthorizationHeaderAsync() / GetAccessTokenAsync() method that can be wired directly into an HttpMessageHandler (similar to how the TS SDK's CrossAppAccessProvider plugs into withOAuth). This would give transparent token refresh on 401 without the caller needing to manage ResumeSessionAsync manually. I can add an HttpMessageHandler-based integration helper to EnterpriseAuthProvider if that would be the preferred approach, happy to follow whatever pattern the team is converging on for auth handlers.
Let me know if you'd like me to add refresh-on-401 integration or any deeper transport hookup in this PR, or if the current GetAccessTokenAsync + manual ResumeSessionAsync pattern is sufficient for the initial merge.
|
Hey @halter73, just a friendly ping on this one! It's been a few weeks since our last exchange and I wanted to check if you had any further thoughts on the token refresh / For context, both the Go (go-sdk#770) and TS (typescript-sdk#1531) PRs are still in review too, so I'm happy to align on whatever approach the team lands on across SDKs. I can also keep this PR scoped to the current API surface and follow up with a separate PR for the Happy to make any adjustments - just let me know! |
|
@halter73 Quick update: The TypeScript SDK's SEP-990 PR (typescript-sdk#1531) just merged! 🎉 This confirms the pattern we've implemented here is solid - token refresh via full flow re-execution, no scope step-up support, and caching with expiry checks. All aligned with how the TS SDK handles it. |
aniket-okta
force-pushed
the
feature/enterprise-managed-authorization
branch
from
c945019 to
ed87d4b
Compare
|
Would it be possible to add OAuthTestBase integration test for this? You might need to add features to the TestOAuthServetlr for it, but I don't think it should be too hard. It's not like it needs to be hardened or anything |
|
Hi @halter73, done! Added 3 One question: do we need a conformance test for this as well, or are the integration tests sufficient? |
halter73
left a comment
halter73
left a comment
There was a problem hiding this comment.
I wonder if we need top-level documentation for this in https://csharp.sdk.modelcontextprotocol.io/concepts/index.html. Or do we assume that each authorization provider (Entra, Okta, etc.) will provide their own docs for each SDK that's specific to their platform?
@mikekistler @jeffhandley Do you have any thoughts on this PR?
|
On top-level docs: On CI (no checks running): |
|
The Build and Test / build (windows-latest, Debug) failure seems like a transient CI infrastructure issue. The log shows: All tests still ran and passed despite the timeout - 0 failures across all assemblies (1922 passed in ModelContextProtocol.Tests, 336 passed in ModelContextProtocol.AspNetCore.Tests, 57 passed in Analyzers.Tests). The @halter73 Could you please re-run the failed Windows Debug job? Nothing in our code changes would cause this. |
|
This PR adds 8 public types here (this static class with 4 URN constants + 3 methods, the provider, its options, the assertion context, the exception, and three layer-2 option DTOs). For comparison:
I think we should try to reduce, so we can get close to Go. Concrete cuts:
Lands us at 3 required public types ( Trade-off: the current "bring your own JAG" extension point goes away unless we re-expose helpers later. Given no one is asking for that today and we can promote back to |
halter73
left a comment
halter73
left a comment
There was a problem hiding this comment.
Thanks for sticking with this! Two things before merge:
- A couple of earlier review threads were marked "Done" but weren't actually finished:
- One top-level class per file -
EnterpriseAuthProvider.csstill defines three top-level public types (EnterpriseAuthAssertionContext,EnterpriseAuthProvider,EnterpriseAuthProviderOptions), andEnterpriseAuth.csstill defines three internal response classes alongside the static helper. See inline comments for the splits. - Remove SEP-990 references — still present in
EnterpriseAuth.cs:51,McpJsonUtilities.cs:190, andREADME.mdlines 34/36. Inline comments below.
- Naming + public surface area.. Picking up the thread I left at #1305 (comment), I'd like to land on
CrossApplicationAccess*(TS-aligned, no abbreviations) and significantly shrink the public surface to match the Go SDK's 2-type shape. Details inline onEnterpriseAuth.cs.
Also withdrawing my earlier "consider this authentication, not authorization" aside — see the inline comment for why.
Requesting changes on the naming + visibility decision so we don't ship a public API we can't easily walk back.
…old IdP config into options
aniket-okta
force-pushed
the
feature/enterprise-managed-authorization
branch
from
dc12435 to
db67570
Compare
|
Hey @halter73, I've addressed all of your review feedback. Here's a summary of what changed: Naming / file structure
Public surface reduction
Callback simplification
SEP-990 / spec reference cleanup
Context type
Let me know if anything else needs adjusting! |
There was a problem hiding this comment.
Thanks for the rename and the file split. The public surface (CrossApplicationAccess*) reads much better now.
That said, the PR has grown rather than shrunk in this round, and most of the new weight is testing internals rather than behavior. Before another pass I'd like to see the surface area pulled back and a few correctness/lifecycle issues addressed. Grouping the asks below so it's easier to triage:
Please remove
InternalsVisibleTofor both test assemblies inModelContextProtocol.Core.csproj. We intentionally do not use IVT in this repo (see #6 — it was one of the first things we removed after migrating to the modelcontextprotocol org). If a test can't be expressed against the public API, just delete it.- The nested
Throwhelper class inCrossApplicationAccess.cs.src/Common/Throw.csis already linked into Core (<Compile Include="..\Common\Throw.cs" Link="Throw.cs" />) and providesThrow.IfNull(with[NotNull]+CallerArgumentExpression) andThrow.IfNullOrWhiteSpace. Use those everywhere instead — including replacing the manualstring.IsNullOrEmpty(...) → throw new ArgumentException(...)blocks in theCrossApplicationAccessProviderconstructor. - The unused
DiscoverAndRequestJwtAuthorizationGrantAsyncoverload (and itsDiscoverAndRequestJwtAuthGrantOptionsDTO, and the 4 unit tests targeting it). WithCrossApplicationAccessnowinternal, that method has no callers — the production flow goes throughCrossApplicationAccessProvider.ResolveIdpTokenEndpointAsyncinstead. Once IVT is gone, the corresponding tests must go too. - The leftover "Enterprise" wording in
README.md(heading on line 34) and theasynclambda on line 57 that doesn'tawait.
Please change
HttpClientshould be a required, non-nullable constructor parameter onCrossApplicationAccessProvider, notHttpClient? httpClient = nullwith a?? new HttpClient()fallback. The owning caller (e.g.HttpClientTransport) is responsible for theHttpClientlifetime — this is exactly the patternClientOAuthProvideruses (seeClientOAuthProvider.cs:62). Apply the same fix to the internalCrossApplicationAccesshelpers that also dohttpClient ?? new HttpClient().- Don't echo raw HTTP response bodies into
CrossApplicationAccessException.Message. Keep the exception message to a short, sanitized summary (status code + endpoint). - The new docs roughly double the README size. Please move the bulk of it under docs/ (see the inline suggestion on line 65 for where).
Out of scope for this PR
The cache fields on CrossApplicationAccessProvider aren't thread-safe — but neither are ClientOAuthProvider._tokenCache / _authServerMetadata, nor the equivalents in the TS (CrossAppAccessProvider._tokens) and Go (EnterpriseHandler.tokenSource) SDKs. Since this is a pre-existing gap shared across providers (and SDKs) rather than something this PR introduces, I'd rather address it as a single follow-up that covers both C# providers and aligns with whatever TS/Go land on. Tracked in #1595 — no action needed in this PR.
Once the surface is trimmed and the HttpClient lifecycle matches ClientOAuthProvider, this should be in good shape.
|
|
||
| private static class Throw | ||
| { | ||
| public static void IfNull<T>(T value, [System.Runtime.CompilerServices.CallerArgumentExpression(nameof(value))] string? name = null) where T : class |
There was a problem hiding this comment.
Do you know why you were unable to rely on the Throw.IfNull/IfNullOrWhiteSpace that ships with latest .NET versions and the polyfill in src/Common/Throw.cs?
csharp-sdk/src/Common/Throw.cs
Line 12 in 712a06b
That polyfill is already linked into ModelContextProtocol.Core (see the entry in ModelContextProtocol.Core.csproj) and is used in dozens of files here. I'd rather use it than duplicate, and IfNullOrWhiteSpace is also a stricter check than the new IfNullOrEmpty for these OAuth parameters anyway.
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <AssemblyAttribute Include="System.Runtime.CompilerServices.InternalsVisibleTo"> |
There was a problem hiding this comment.
We very intentionally do not use InternalsVisibleTo in our tests. Removing that was one of the first things we did after migrating the SDK to the modelcontextprotocol org. See #6
If there's no way to write an equivalent test using just the public API, just delete it.
| - [Protocol Specification](https://modelcontextprotocol.io/specification/) | ||
| - [GitHub Organization](https://github.com/modelcontextprotocol) | ||
|
|
||
| ## Enterprise Auth / Enterprise Managed Authorization |
There was a problem hiding this comment.
| ## Enterprise Auth / Enterprise Managed Authorization | |
| ## Cross-Application Access (Identity Assertion Authorization Grant flow) |
I think we should de-emphasize "Enterprise" now that it's no longer part of the type names. This appears to align better with the TS SDK's conceptual docs.
| /// Discovers the IDP's token endpoint via OAuth/OIDC metadata, then requests a JWT Authorization Grant. | ||
| /// Convenience wrapper over <see cref="RequestJwtAuthorizationGrantAsync"/>. | ||
| /// </summary> | ||
| public static async Task<string> DiscoverAndRequestJwtAuthorizationGrantAsync( |
There was a problem hiding this comment.
This method has no production callers. It's only callares are the 4 unit tests in CrossApplicationAccessTests.cs (lines 451, 500, 534, 553) reference it. CrossApplicationAccessProvider.ResolveIdpTokenEndpointAsync (lines 187–211 of CrossApplicationAccessProvider.cs) already does the same IdP discovery inline. Since CrossApplicationAccess is now internal, no external caller is possible either.
Please delete:
- This
DiscoverAndRequestJwtAuthorizationGrantAsyncmethod (lines 149–184) - The whole
DiscoverAndRequestJwtAuthGrantOptions.csfile - The
#region DiscoverAndRequestJwtAuthorizationGrantAsync Testsblock inCrossApplicationAccessTests.cs(lines 448–566)
| /// <exception cref="ArgumentException">Required option values are missing.</exception> | ||
| public CrossApplicationAccessProvider( | ||
| CrossApplicationAccessProviderOptions options, | ||
| HttpClient? httpClient = null, |
There was a problem hiding this comment.
Please match the ClientOAuthProvider pattern instead of httpClient ?? new HttpClient(). See src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs:62 — HttpClient is a required, non-nullable constructor parameter; the caller owns its lifetime, and the provider never news one up. That avoids the classic socket-exhaustion anti-pattern and makes the IHttpClientFactory / DI story work without any changes here.
Concrete changes:
- Change this parameter to
HttpClient httpClient(required, non-nullable, no default). Validate withThrow.IfNull(httpClient). - Drop the
?? new HttpClient()on line 95. - Remove the
HttpClient? HttpClientproperty fromRequestJwtAuthGrantOptions.csandExchangeJwtBearerGrantOptions.cs, and drop the?? new HttpClient()fallbacks atCrossApplicationAccess.cs:86and:203. The provider should pass its own_httpClientinto those internal helpers explicitly. - Update the integration test and any unit tests to pass an
HttpClient(the integration test inCrossApplicationAccessIntegrationTests.csalready does —httpClient: HttpClient— so this is mostly removing the fallback path).
If you want to make this easier for the caller, we should find a way to use the HttpClient passed to the HttpClientTransport constructor, similar to what ClientOAuthProvider does. However, since unlike ClientOAuthProvider, this is a lower level API where public callers are meant to construct the provider themselves, I think just making it a required constructor parameter is fine.
| IdTokenCallback = async (context, ct) => | ||
| { | ||
| // Return the OIDC ID token from your SSO session | ||
| return myIdToken; |
There was a problem hiding this comment.
This callback is async but has no await, which will compile-warn (CS1998). Either drop async and return Task.FromResult(myIdToken), or make the example realistically asynchronous by showing the ID-token fetch:
| return myIdToken; | |
| // Fetch a fresh ID token from your SSO session. | |
| return await mySsoClient.GetIdTokenAsync(cancellationToken); |
|
|
||
| if (response is null) | ||
| { | ||
| throw new CrossApplicationAccessException($"Failed to parse token exchange response: {responseBody}"); |
There was a problem hiding this comment.
Embedding the raw response body in the exception message risks leaking token material or PII into caller logs if the IdP returns something unexpected. Same on line 256 of this file.
Please drop the body from the message and instead set it on Exception.Data, e.g.:
var ex = new CrossApplicationAccessException("Failed to parse token exchange response.");
ex.Data["ResponseBody"] = responseBody;
throw ex;Or add a special parameter to CrossApplicationAccessException to get the body if you think it's valuable, but it should not be serialized directly into Exception.Message. That keeps the diagnostic information available to callers who explicitly opt in to inspecting it, without pushing it into the default ToString() output.
| $"Token exchange response issued_token_type must be '{TokenTypeIdJag}', got '{response.IssuedTokenType}'."); | ||
| } | ||
|
|
||
| if (!string.Equals(response.TokenType, TokenTypeNotApplicable, StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
RFC 8693 §2.2.1 specifies the literal string "N_A". Please use StringComparison.Ordinal here to match — and to be consistent with the Ordinal comparison used for issued_token_type 5 lines above. (The bearer comparison on line 269 is correctly case-insensitive per RFC 7523, leave that one alone.)
| if (!string.Equals(response.TokenType, TokenTypeNotApplicable, StringComparison.OrdinalIgnoreCase)) | |
| if (!string.Equals(response.TokenType, TokenTypeNotApplicable, StringComparison.Ordinal)) |
| if (string.IsNullOrEmpty(options.ClientId)) | ||
| { | ||
| throw new ArgumentException("ClientId is required.", nameof(options)); | ||
| } |
There was a problem hiding this comment.
Throwing ArgumentException with paramName: nameof(options) for each individual missing property (lines 77, 82, 87, 92) hides the actual missing field in stack traces — every error just says the options arg is invalid. Please use Throw.IfNullOrWhiteSpace(options.ClientId), so the property name shows up:
| if (string.IsNullOrEmpty(options.ClientId)) | |
| { | |
| throw new ArgumentException("ClientId is required.", nameof(options)); | |
| } | |
| Throw.IfNullOrWhiteSpace(options.ClientId) |
Or if you really want the message to be "ClientId" is required:
| if (string.IsNullOrEmpty(options.ClientId)) | |
| { | |
| throw new ArgumentException("ClientId is required.", nameof(options)); | |
| } | |
| if (string.IsNullOrEmpty(options.ClientId)) | |
| { | |
| throw new ArgumentException("ClientId is required.", $"{nameof(options)}.{nameof(options.ClientId)}"); | |
| } |
I think the one-liner is probably nicer though.
|
And I hate to circle back on the naming, but I don't love But now I notice that Go went with A couple of options to consider, in rough order of my preference:
I do see that Edit: To be clear, I'm not going to hold up the PR on this. I'll merge with the current naming if we can't agree on something better, but I do think we can come up with something better. |
| resourceUrl: new Uri("https://mcp-server.example.com"), | ||
| authorizationServerUrl: new Uri("https://auth.mcp-server.example.com")); | ||
| ``` | ||
|
|
There was a problem hiding this comment.
I thought this was in a conceptual doc. This doubles the size of our README. We try to keep this pretty slim. I would move this to somewhere under the docs/ directory. We don't have client OAuth documented yet, and I'm not going to ask you to do that. Feel free to add it as a section to transports.md, and we'll move it to a dedicated client OAuth doc once we create that.
If you want to go the extra mile, you could add this flow to ProtectedMcpServer and ProtectedMcpClient. These both run against the TestOAuthServer that you've already updated, so I think you're already most of the way there. It could be helpful for referencing from the /docs too. We already do that in identity.md for the server side at least.
2 participants
Implements Enterprise Managed Authorization (SEP-990) for the C# MCP SDK, enabling MCP Clients to leverage enterprise Identity Providers for seamless authorization without per-server user authentication.
Closes #949
Flow
Design
Layer 2:
EnterpriseAuthstatic class : standalone utilities (~680 lines)RequestJwtAuthorizationGrantAsync(): RFC 8693 token exchange (ID Token → ID-JAG)DiscoverAndRequestJwtAuthorizationGrantAsync(): convenience wrapper with IdP discoveryExchangeJwtBearerGrantAsync(): RFC 7523 JWT bearer grant (ID-JAG → Access Token)DiscoverAuthServerMetadataAsync(): OAuth authorization server metadata discoveryRequestJwtAuthGrantOptions,DiscoverAndRequestJwtAuthGrantOptions,ExchangeJwtBearerGrantOptionsJagTokenExchangeResponse,JwtBearerAccessTokenResponse,OAuthErrorResponseLayer 3:
EnterpriseAuthProvider: high-level provider with caching (~230 lines)InvalidateCache()EnterpriseAuthProviderOptionsfor configuration (ClientId, ClientSecret, Scope, AssertionCallback)Tests
36 unit tests covering both layers, passing on net8.0, net9.0, and net10.0.
Related