Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ public Optional<ValidatedToken> validateAccessToken(@NonNull String token) {
.workspaceId(row.workspaceId())
.workspaceName(row.workspaceName())
.resource(row.resource())
.expiresAt(row.expiresAt())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Severity

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

Fix in Cursor

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.build());
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,21 @@
import com.fasterxml.jackson.databind.annotation.JsonNaming;
import lombok.Builder;

import java.time.Instant;

/**
* What the introspection endpoint ({@code POST /opik/auth-oauth}) tells a resource server about a live access
* token. {@code expiresAt} is the token row's expiry: opik-mcp caches a "valid" answer until then instead of
* re-asking on a fixed timer, which closes the window in which an expired token is still forwarded (OPIK-8252).
* Additive — resource servers that predate the field ignore it.
*/
@Builder(toBuilder = true)
@JsonInclude(JsonInclude.Include.NON_NULL)
@JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class)
public record ValidatedToken(
String userName,
String workspaceId,
String workspaceName,
String resource) {
String resource,
Instant expiresAt) {
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
import org.junit.jupiter.params.provider.NullSource;
import org.junit.jupiter.params.provider.ValueSource;

import java.time.Duration;
import java.time.Instant;
import java.time.temporal.ChronoUnit;
import java.util.Optional;
import java.util.UUID;

Expand Down Expand Up @@ -94,6 +97,7 @@ void acceptsValidToken(String schemePrefix) {
.workspaceId(UUID.randomUUID().toString())
.workspaceName(RandomStringUtils.secure().nextAlphanumeric(10))
.resource("http://localhost/api/v1/mcp/%s".formatted(RandomStringUtils.secure().nextAlphanumeric(8)))
.expiresAt(Instant.now().plus(Duration.ofHours(1)).truncatedTo(ChronoUnit.MILLIS))
.build();
when(mcpOAuthService.validateAccessToken(ACCESS_TOKEN)).thenReturn(Optional.of(validated));

Expand All @@ -102,4 +106,25 @@ void acceptsValidToken(String schemePrefix) {
assertThat(response.readEntity(ValidatedToken.class)).isEqualTo(validated);
}
}

@Test
@DisplayName("reports the token expiry as an ISO-8601 expires_at so resource servers can cache until then")
void reportsExpiresAtOnTheWire() {
// opik-mcp caches a "valid" answer until this instant (OPIK-8252); an
// epoch number or a camelCase key would silently fall back to its TTL.
var expiresAt = Instant.parse("2026-09-04T10:15:30.123Z");
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Severity

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

Fix in Cursor

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit 5759852 addressed this comment by using PodamFactoryUtils.newPodamFactory() for both fixtures and overriding only expiresAt.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched both ValidatedToken fixtures to PodamFactoryUtils.newPodamFactory().manufacturePojo(...) with only expiresAt overridden, in 5759852.

.build();
when(mcpOAuthService.validateAccessToken(ACCESS_TOKEN)).thenReturn(Optional.of(validated));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Covered by the new OAuthValidateTokenIntegrationTest in 5759852, which reads the value through the real validateAccessToken path rather than a mock.


try (Response response = validate("Bearer " + ACCESS_TOKEN)) {
assertThat(response.getStatus()).isEqualTo(Response.Status.OK.getStatusCode());
assertThat(response.readEntity(String.class)).contains("\"expires_at\":\"2026-09-04T10:15:30.123Z\"");
}
}
}
Loading