[OPIK-6666] [BE] feat: MCP OAuth dynamic client registration — RFC 7591 (PR 5/7)#6996
Open
LifeXplorer wants to merge 9 commits into
Open
Conversation
PR 5 of 7 (chained on PR 4). - OAuthRegisterResource: POST /oauth/register — self-registration for AI hosts, IP-bucketed rate limiting via RateLimitService - ClientRegistrationResponse DTO - McpOAuthBundle: also disables OAuthRegisterResource when flag is off - OAuthRegisterResourceTest Adapted to PR 1's slimmed McpOAuthClient (no createdAt): client_id_issued_at is optional per RFC 7591 §3.2.1 and is omitted from the response. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
github-actions
Bot
added
java
Contributor
Open
2 tasks
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
…dau/OPIK-6666-oauth-as-chunked-5
…dau/OPIK-6666-oauth-as-chunked-5
…onfig Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
LifeXplorer
commented
Jun 8, 2026
LifeXplorer
left a comment
LifeXplorer
left a comment
Contributor
Author
There was a problem hiding this comment.
Opik reviewer (mined from your team's review history)
6 findings — 0 high · 2 medium · 4 low. Suppressed by team conventions: see suppressed.md.
React 👍/👎 on each comment — your feedback helps tune what it flags.
- Fix OAuthRegisterResourceTest: build resource in setUp() after stubbing config (was NPEing under @Injectmocks since LimitConfig moved to ctor) - Key rate-limit bucket on last X-Forwarded-For hop (nginx-appended, unspoofable) instead of the attacker-controlled first hop - Assert full ClientRegistrationResponse and 429 OAuthError body - Emit Retry-After header on 429 from remaining TTL - Extract RATE_LIMIT_BUCKET_PREFIX to dedupe the bucket literal - Rename register_clientIdIssuedAtAlwaysOmitted; document throttle is intentionally independent of the global rateLimit.enabled flag Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…hunked-4' into avinahradau/OPIK-6666-oauth-as-chunked-5 # Conflicts: # apps/opik-backend/src/main/java/com/comet/opik/domain/mcpoauth/ClientRegistrationRequest.java
Base merge renamed McpOAuthClient.clientId to id; update the register resource, response mapper, and test to the new accessor/builder. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
|
||
| McpOAuthClient client = clientService.register(request); | ||
| ClientRegistrationResponse body = ClientRegistrationResponseMapper.INSTANCE.toResponse(client); | ||
| return Response.created(URI.create("/admin/mcp-oauth-clients/" + client.id())) |
Contributor
There was a problem hiding this comment.
Response.created(URI.create("/admin/mcp-oauth-clients/" + client.id())) returns a relative Location; should we build it from uriInfo.getAbsolutePathBuilder().path(client.id()).build() so it’s absolute and deterministic?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/main/java/com/comet/opik/api/resources/oauth/OAuthRegisterResource.java
around line 83, the create endpoint returns
Response.created(URI.create("/admin/mcp-oauth-clients/" + client.id())) which sets the
201 Location header to a relative URI. Refactor this so Location is an absolute,
deterministic URI built from the current request context—use
uriInfo.getAbsolutePathBuilder() (and append the client id path segment) to build the
URI for the Location header. Ensure the Location value is absolute (includes
scheme/host) and matches the API’s intended `/admin/mcp-oauth-clients/{id}` route.
Match the @Tag/@Operation/@apiresponse pattern used by the sibling OAuth resources so the registration endpoint appears in the API docs. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
PR 5 of 7 splitting #6900 per review.
OAuthRegisterResourcePOST /oauth/register— RFC 7591 dynamic client registration for AI hosts; IP-bucketed rate limiting viaRateLimitServiceClientRegistrationResponseMcpOAuthBundleOAuthRegisterResourcewhen flag offOAuthRegisterResourceTestNote: adapted to PR-1's slimmed
McpOAuthClient(nocreatedAt).client_id_issued_atis optional (RFC 7591 §3.2.1) and is omitted from the response; the test asserts that.Change checklist
Testing
mvn -o test-compile— green.Documentation
No user-facing documentation changes required — internal MCP OAuth registration endpoint, not part of the public API surface.
Issues
🤖 Generated with Claude Code