feat(oauth): Add RFC 8707 resource indicator support#736
feat(oauth): Add RFC 8707 resource indicator support#736MariaChrysafis wants to merge 3 commits into
Conversation
MariaChrysafis
changed the title
WalkthroughAdds RFC 8707 resource indicator handling to OAuth flows: new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@ezynda3 for review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/transport/oauth_test.go (1)
23-89: Please fold the new resource-indicator scenarios into a table.These cases vary mainly by discovery inputs and expected
resource, so atests := []struct{...}table would remove most of the duplicatedhttptestscaffolding and make it easier to add the explicitAuthServerMetadataURL+ canonical-resource regression case that this change needs.As per coding guidelines, "Write table-driven tests using a tests := []struct{ name, ... } pattern".
Also applies to: 94-142, 146-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/transport/oauth_test.go` around lines 23 - 89, Refactor TestOAuthHandler_ResourceIndicator_RFC8707 into a table-driven test: create a tests := []struct{name string, discoveryURL string, expectedResource string, initialMetadata string, ...} and loop over them; for each case spin up the httptest server responses (the /.well-known document and /token handler) configured from the table row, then construct OAuthConfig, call NewOAuthHandler, use GetAuthorizationURL, ProcessAuthorizationResponse and RefreshToken and assert the expectedResource for authorization URL, token exchange and refresh. Ensure you include a case that passes an explicit AuthServerMetadataURL (canonical-resource regression) and any permutations of discovery inputs mentioned in the original test so common setup (httptest server, serverURL variable, tokenExchangeResource/refreshResource capture) is reused inside the table loop rather than duplicated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/transport/oauth.go`:
- Around line 317-325: getResourceURL currently only returns h.resourceURL if
populated, but h.resourceURL is only set via the protected-resource discovery
path so when AuthServerMetadataURL is provided and baseURL is configured the
handler never attempts discovery and may use baseURL as the audience; update
OAuthHandler initialization to perform a best-effort protected-resource lookup
even when AuthServerMetadataURL is set (populate h.resourceURL from the
protected-resource's advertised canonical identifier if available) and keep
getResourceURL unchanged so it prefers the discovered value over h.baseURL; add
a regression test exercising the case where AuthServerMetadataURL and baseURL
are both configured but the protected-resource metadata advertises a different
resource to ensure the client sends the discovered resource as the audience.
---
Nitpick comments:
In `@client/transport/oauth_test.go`:
- Around line 23-89: Refactor TestOAuthHandler_ResourceIndicator_RFC8707 into a
table-driven test: create a tests := []struct{name string, discoveryURL string,
expectedResource string, initialMetadata string, ...} and loop over them; for
each case spin up the httptest server responses (the /.well-known document and
/token handler) configured from the table row, then construct OAuthConfig, call
NewOAuthHandler, use GetAuthorizationURL, ProcessAuthorizationResponse and
RefreshToken and assert the expectedResource for authorization URL, token
exchange and refresh. Ensure you include a case that passes an explicit
AuthServerMetadataURL (canonical-resource regression) and any permutations of
discovery inputs mentioned in the original test so common setup (httptest
server, serverURL variable, tokenExchangeResource/refreshResource capture) is
reused inside the table loop rather than duplicated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4a067c00-c315-46da-9738-d9b16a1da0a7
📒 Files selected for processing (2)
client/transport/oauth.goclient/transport/oauth_test.go
| // getResourceURL returns the RFC 8707 resource indicator to send to the | ||
| // authorization server so it can audience-restrict issued tokens. It prefers | ||
| // the resource value advertised via RFC 9728 protected resource metadata and | ||
| // falls back to the MCP server base URL. Returns empty string if unknown. | ||
| func (h *OAuthHandler) getResourceURL() string { | ||
| if h.resourceURL != "" { | ||
| return h.resourceURL | ||
| } | ||
| return h.baseURL |
There was a problem hiding this comment.
Explicit auth-metadata config still misses the canonical resource.
getResourceURL() only prefers h.resourceURL, but that field is populated exclusively on the protected-resource discovery path. When AuthServerMetadataURL is set and baseURL is also configured, these new resource parameters fall back to baseURL and never see the protected resource’s advertised canonical identifier. If those differ, the client can send the wrong audience to the authorization server. Please make protected-resource lookup a best-effort step even when auth metadata is supplied directly, and add a regression test for that combination.
Also applies to: 431-434
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/transport/oauth.go` around lines 317 - 325, getResourceURL currently
only returns h.resourceURL if populated, but h.resourceURL is only set via the
protected-resource discovery path so when AuthServerMetadataURL is provided and
baseURL is configured the handler never attempts discovery and may use baseURL
as the audience; update OAuthHandler initialization to perform a best-effort
protected-resource lookup even when AuthServerMetadataURL is set (populate
h.resourceURL from the protected-resource's advertised canonical identifier if
available) and keep getResourceURL unchanged so it prefers the discovered value
over h.baseURL; add a regression test exercising the case where
AuthServerMetadataURL and baseURL are both configured but the protected-resource
metadata advertises a different resource to ensure the client sends the
discovered resource as the audience.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
client/transport/oauth.go (1)
147-147:⚠️ Potential issue | 🟠 MajorExplicit auth-metadata config still misses the canonical resource.
This still only populates
h.resourceURLon the protected-resource discovery path. WhenAuthServerMetadataURLis set, discovery is skipped,getResourceURL()falls back tobaseURL, and the client can send the wrong audience if the protected resource advertises a different canonical identifier. Please keep the direct metadata fetch, but do a best-effort protected-resource lookup to seedh.resourceURLand add a regression test for that combination.Also applies to: 317-324, 429-432
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/transport/oauth.go` at line 147, The handler currently only sets h.resourceURL during the protected-resource discovery path, so when AuthServerMetadataURL is provided discovery is skipped and getResourceURL() falls back to baseURL causing wrong audience; keep the explicit metadata fetch path when AuthServerMetadataURL is set (i.e., still call the direct metadata retrieval routine that parses protected_resource or resource indicators), then do a best-effort protected-resource lookup to seed h.resourceURL (prefer protected-resource discovery result but fall back to metadata's resource or baseURL), update getResourceURL() to prefer h.resourceURL if populated, and add a regression test covering AuthServerMetadataURL + protected-resource advertising a different canonical identifier to ensure h.resourceURL is set correctly; refer to symbols resourceURL, h.resourceURL, AuthServerMetadataURL, getResourceURL(), and the protected-resource discovery code paths when implementing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@client/transport/oauth.go`:
- Line 147: The handler currently only sets h.resourceURL during the
protected-resource discovery path, so when AuthServerMetadataURL is provided
discovery is skipped and getResourceURL() falls back to baseURL causing wrong
audience; keep the explicit metadata fetch path when AuthServerMetadataURL is
set (i.e., still call the direct metadata retrieval routine that parses
protected_resource or resource indicators), then do a best-effort
protected-resource lookup to seed h.resourceURL (prefer protected-resource
discovery result but fall back to metadata's resource or baseURL), update
getResourceURL() to prefer h.resourceURL if populated, and add a regression test
covering AuthServerMetadataURL + protected-resource advertising a different
canonical identifier to ensure h.resourceURL is set correctly; refer to symbols
resourceURL, h.resourceURL, AuthServerMetadataURL, getResourceURL(), and the
protected-resource discovery code paths when implementing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2bf947bf-efdd-45de-a2a9-459bd6b96fff
📒 Files selected for processing (1)
client/transport/oauth.go
|
@MariaChrysafis sorry please have a look at and fix the merge conflicts |
2 participants
Description
Adds the
resourceparameter (RFC 8707) to OAuth authorization, token exchange, and refresh requests so authorization servers can audience-restrict issued tokens per the MCP auth specification.Type of Change
Checklist
MCP Spec Compliance
Summary by CodeRabbit