[OPIK-8252] [BE] feat: Report expires_at from MCP OAuth token introspection - #8153
[OPIK-8252] [BE] feat: Report expires_at from MCP OAuth token introspection#8153awkoy wants to merge 2 commits into
Conversation
…ection opik-mcp, the resource server for the hosted MCP connector, now validates every OAuth access token against POST /opik/auth-oauth and caches a "valid" answer for a short TTL. Without knowing when the token actually expires it can only guess, and a token that dies inside that window is still forwarded once. ValidatedToken now carries the token row's expiresAt (ISO-8601 on the wire as expires_at), so the resource server caches until the real expiry and that window disappears. Purely additive: resource servers that predate the field ignore it. The OpenAPI spec and generated SDKs pick the field up through the usual automated update. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LwQuKKW95kFTeHAW3jVyTj
github-actions
Bot
added
java
⏱️ pre-commit per-hook timing
⏭️ 42 skipped (no matching files changed)
|
Backend Tests - Integration Group 14 30 files - 21 30 suites - 21 4m 26s ⏱️ +23s Results for commit 5759852. ± Comparison against base commit d53961c. This pull request removes 263 and adds 203 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| .workspaceId(row.workspaceId()) | ||
| .workspaceName(row.workspaceName()) | ||
| .resource(row.resource()) | ||
| .expiresAt(row.expiresAt()) |
There was a problem hiding this comment.
Generated clients omit token expiry
expiresAt is now emitted in the backend response, but the checked-in OpenAPI ValidatedToken schema isn't updated, so Fern-generated models lack expires_at and typed SDK consumers can't access it — should we update the API definition and regenerate the SDK artifacts as part of this change?
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/domain/mcpoauth/McpOAuthService.java
around lines 186-186, the validateAccessToken response now includes expiresAt without
updating the checked-in OpenAPI ValidatedToken schema. Update the API definition to
declare the expiry field, then regenerate the Fern TypeScript and Python SDK artifacts
so they expose expires_at and the TypeScript serializer maps it correctly. Include the
schema and generated-client changes in the same release change and run the relevant
generation or validation checks.
There was a problem hiding this comment.
Commit 5759852 addressed this comment by adding expires_at to both checked-in OpenAPI schemas as an ISO date-time field. Generated SDK artifact changes are not present in the diff.
There was a problem hiding this comment.
Added expires_at (string, date-time) to ValidatedToken in both checked-in OpenAPI specs in 5759852. SDK regeneration is left to the scheduled Fern workflow, as with previous DTO changes in this package.
There was a problem hiding this comment.
Thanks, noted—the specs now include expires_at, and SDK regeneration follows the scheduled Fern workflow. I’ll save this context to memory once the PR is merged.
| .workspaceId(row.workspaceId()) | ||
| .workspaceName(row.workspaceName()) | ||
| .resource(row.resource()) | ||
| .expiresAt(row.expiresAt()) |
There was a problem hiding this comment.
Persisted expiry propagation lacks integration coverage
The expires_at assignment is only covered by mocked resource tests, so a regression here would go undetected — should we add real backend integration coverage (per AGENTS.md Tests and Organization and apps/opik-backend/AGENTS.md Tests) that mints/seeds a real MySQL token, POSTs /opik/auth-oauth through the real McpOAuthService, and asserts the response expires_at matches the stored row value?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
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/domain/mcpoauth/McpOAuthService.java
around lines 186-186, add backend integration coverage for the `validateAccessToken`
response mapping that now includes `expiresAt`. Seed or mint a real MySQL token, POST to
`/opik/auth-oauth` through the real `McpOAuthService`, and assert that the response's
`expires_at` exactly matches the stored token row value; do not rely on the existing
mocked resource tests.
There was a problem hiding this comment.
Commit 5759852 addressed this comment by adding a real MySQL-backed integration test that mints a token, calls /opik/auth-oauth, and verifies expires_at matches the persisted row expiry.
There was a problem hiding this comment.
Added OAuthValidateTokenIntegrationTest in 5759852: real register → consent → code exchange against MySQL, then POST /opik/auth-oauth, asserting expires_at equals the persisted token row's expiry.
| var validated = ValidatedToken.builder() | ||
| .userName(RandomStringUtils.secure().nextAlphanumeric(10)) | ||
| .workspaceId(UUID.randomUUID().toString()) | ||
| .workspaceName(RandomStringUtils.secure().nextAlphanumeric(10)) | ||
| .resource("http://localhost/api/v1/mcp") | ||
| .expiresAt(expiresAt) |
There was a problem hiding this comment.
New fixture bypasses Podam convention
The new wire-format and modified valid-token fixtures manually populate ValidatedToken identity fields; should we build them with PodamFactoryUtils.newPodamFactory() and override only expiresAt plus assertion-required fields, as .agents/skills/opik-backend/testing.md requires?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/test/java/com/comet/opik/api/resources/oauth/OAuthValidateTokenResourceTest.java
around lines 116-121, refactor the `reportsExpiresAtOnTheWire` fixture to create
`ValidatedToken` through `PodamFactoryUtils.newPodamFactory()` or its utilities,
overriding only `expiresAt` and any fields genuinely required by the assertion. Apply
the same fixture convention to the `acceptsValidToken` setup around lines 95-101,
retaining only scenario-relevant overrides and removing manual random generation of
identity fields.
There was a problem hiding this comment.
Commit 5759852 addressed this comment by using PodamFactoryUtils.newPodamFactory() for both fixtures and overriding only expiresAt.
There was a problem hiding this comment.
Switched both ValidatedToken fixtures to PodamFactoryUtils.newPodamFactory().manufacturePojo(...) with only expiresAt overridden, in 5759852.
| .resource("http://localhost/api/v1/mcp") | ||
| .expiresAt(expiresAt) | ||
| .build(); | ||
| when(mcpOAuthService.validateAccessToken(ACCESS_TOKEN)).thenReturn(Optional.of(validated)); |
There was a problem hiding this comment.
Token expiry mapping lacks regression coverage
The new wire-format test mocks mcpOAuthService.validateAccessToken, so it only verifies Jackson serialization of a hand-built DTO and can pass if McpOAuthService.validateAccessToken omits or mis-maps the persisted token row’s expiresAt — should we add a service/integration regression test asserting ValidatedToken.expiresAt from a token row, or use the real service path here?
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/test/java/com/comet/opik/api/resources/oauth/OAuthValidateTokenResourceTest.java`
around line 123, update `reportsExpiresAtOnTheWire` because mocking
`mcpOAuthService.validateAccessToken` bypasses the expiry mapping under test. Exercise
the real service path, or add a service/integration regression test that persists a
token with a known `expiresAt` and asserts the returned `ValidatedToken.expiresAt`
before verifying the serialized `expires_at` field.
There was a problem hiding this comment.
Commit 5759852 addressed this comment by adding an integration regression test that exercises the real OAuth flow, reads the persisted token expiry, and asserts ValidatedToken.expiresAt matches it. The service now maps row.expiresAt() into the DTO.
There was a problem hiding this comment.
Covered by the new OAuthValidateTokenIntegrationTest in 5759852, which reads the value through the real validateAccessToken path rather than a mock.
TS SDK E2E Tests - Node 22317 tests - 1 315 ✅ +2 17m 38s ⏱️ + 3m 24s Results for commit 5759852. ± Comparison against base commit d53961c. This pull request removes 1 test.♻️ This comment has been updated with latest results. |
… schema Addresses the review on the previous commit. The mocked resource test only proved the wire format of a hand-built DTO; OAuthValidateTokenIntegrationTest now drives the real register → consent → code-exchange flow against MySQL and asserts that POST /opik/auth-oauth reports exactly the persisted row's expiry (plus resource and workspace). ValidatedToken fixtures in the resource test come from Podam like the rest of the suite. The checked-in OpenAPI schemas gain expires_at (string, date-time) so generated clients stop lagging the backend. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
github-actions
Bot
added
documentation
|
Already covered by a test in this PR. Additive also touches Backend (Java API / internal) Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review. |
|
🌿 Preview your docs: https://opik-preview-01a06caa-240f-74f4-9736-abe789af79df.docs.buildwithfern.com/docs/opik No broken links found Unverified links (timeout / rate-limited / server error — not failing the check)• https://aistudio.google.com/apikey (401) 📌 Results for commit 6f883db |
| var validated = response.readEntity(ValidatedToken.class); | ||
| assertThat(validated.expiresAt()).isEqualTo(persistedExpiry); | ||
| assertThat(validated.resource()).isEqualTo(RESOURCE_URI); | ||
| assertThat(validated.workspaceName()).isEqualTo(minted.tokens().workspaceName()); |
There was a problem hiding this comment.
Identity regressions pass integration tests
The integration test verifies only expiresAt, resource, and workspaceName from ValidatedToken, so regressions in the other identity fields still pass unnoticed — should we compare the complete returned object with an expected ValidatedToken built from the minted artifacts?
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/test/java/com/comet/opik/domain/mcpoauth/OAuthValidateTokenIntegrationTest.java`
around lines 108-111, update `introspectionReportsPersistedExpiry` so it does not verify
only `expiresAt`, `resource`, and `workspaceName`. Build an expected `ValidatedToken`
from the minted artifacts and persisted expiry, then compare the complete returned
object, or explicitly assert every meaningful field, including the user identity fields.
3 participants
Details
opik-mcp, the resource server for the hosted MCP connector, now validates every OAuth access token against
POST /opik/auth-oauthand caches a "valid" answer for a short TTL (comet-ml/opik-mcp#182). Without knowing when the token actually expires it can only guess, and a token that dies inside that window is still forwarded once.ValidatedTokennow carries the token row'sexpiresAt, serialized as ISO-8601expires_at, so the resource server caches until the real expiry and that window disappears.Purely additive: the field is
NON_NULL-included, every other construction site leaves it unset and every consumer (AuthFilter,AuthService.authorizeOAuth) reads onlyuserName/workspaceName. Resource servers that predate the field ignore it. The OpenAPI spec and generated SDKs pick the field up through the scheduled "Update automatically OpenAPI spec and Fern code" workflow, as with previous DTO changes in this package.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
expiresAtfield, its population inMcpOAuthService.validateAccessToken, and the two resource-test changesTesting
mvn -Dtest=OAuthValidateTokenResourceTest test(JDK 25) → 11 run, 0 failures, including the newreportsExpiresAtOnTheWireasserting"expires_at":"2026-09-04T10:15:30.123Z"on the wire.mvn spotless:checkclean.expires_inon the token endpoint andexpires_athere derive from the sameaccessTokenTtlin the same transaction, so they agree by construction.Documentation
None: the field is consumed by opik-mcp, not by end users.
🤖 Generated with Claude Code