Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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 @@ -3,6 +3,7 @@
import com.comet.opik.domain.mcpoauth.McpOAuthService;
import com.comet.opik.domain.mcpoauth.McpOAuthTokenUtils;
import com.comet.opik.domain.mcpoauth.ValidatedToken;
import com.comet.opik.podam.PodamFactoryUtils;
import com.comet.opik.utils.JsonUtils;
import io.dropwizard.testing.junit5.DropwizardExtensionsSupport;
import io.dropwizard.testing.junit5.ResourceExtension;
Expand All @@ -17,9 +18,11 @@
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.NullSource;
import org.junit.jupiter.params.provider.ValueSource;
import uk.co.jemos.podam.api.PodamFactory;

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

import static com.comet.opik.domain.mcpoauth.OAuthConstants.OAUTH_VALIDATE_TOKEN_RESOURCE_BASE_PATH;
import static org.assertj.core.api.Assertions.assertThat;
Expand All @@ -38,6 +41,7 @@ class OAuthValidateTokenResourceTest {
+ RandomStringUtils.secure().nextAlphanumeric(24);

private static final McpOAuthService mcpOAuthService = mock(McpOAuthService.class);
private final PodamFactory factory = PodamFactoryUtils.newPodamFactory();

private static final ResourceExtension EXT = ResourceExtension.builder()
.setMapper(JsonUtils.getMapper())
Expand Down Expand Up @@ -89,11 +93,8 @@ void rejectsUnvalidatedAccessToken() {
@ValueSource(strings = {"Bearer ", "bearer ", "BEARER "})
@DisplayName("accepts a valid token and returns the validated identity, case-insensitively on the scheme")
void acceptsValidToken(String schemePrefix) {
var validated = ValidatedToken.builder()
.userName(RandomStringUtils.secure().nextAlphanumeric(10))
.workspaceId(UUID.randomUUID().toString())
.workspaceName(RandomStringUtils.secure().nextAlphanumeric(10))
.resource("http://localhost/api/v1/mcp/%s".formatted(RandomStringUtils.secure().nextAlphanumeric(8)))
var validated = factory.manufacturePojo(ValidatedToken.class).toBuilder()
.expiresAt(Instant.now().plus(Duration.ofHours(1)))
.build();
when(mcpOAuthService.validateAccessToken(ACCESS_TOKEN)).thenReturn(Optional.of(validated));

Expand All @@ -102,4 +103,20 @@ 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 validated = factory.manufacturePojo(ValidatedToken.class).toBuilder()
.expiresAt(Instant.parse("2026-09-04T10:15:30.123Z"))
.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\"");
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
package com.comet.opik.domain.mcpoauth;

import com.comet.opik.api.resources.utils.ClickHouseContainerUtils;
import com.comet.opik.api.resources.utils.MigrationUtils;
import com.comet.opik.api.resources.utils.MySQLContainerUtils;
import com.comet.opik.api.resources.utils.RedisContainerUtils;
import com.comet.opik.api.resources.utils.TestDropwizardAppExtensionUtils;
import com.comet.opik.api.resources.utils.TestDropwizardAppExtensionUtils.AppContextConfig;
import com.comet.opik.api.resources.utils.TestDropwizardAppExtensionUtils.CustomConfig;
import com.comet.opik.api.resources.utils.TestUtils;
import com.comet.opik.api.resources.utils.resources.OAuthResourceClient;
import com.comet.opik.extensions.DropwizardAppExtensionProvider;
import com.comet.opik.extensions.RegisterApp;
import com.redis.testcontainers.RedisContainer;
import jakarta.ws.rs.client.Entity;
import jakarta.ws.rs.core.HttpHeaders;
import jakarta.ws.rs.core.Response;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
import org.junit.jupiter.api.extension.ExtendWith;
import org.testcontainers.clickhouse.ClickHouseContainer;
import org.testcontainers.containers.GenericContainer;
import org.testcontainers.lifecycle.Startables;
import org.testcontainers.mysql.MySQLContainer;
import ru.vyarus.dropwizard.guice.test.ClientSupport;
import ru.vyarus.dropwizard.guice.test.jupiter.ext.TestDropwizardAppExtension;
import ru.vyarus.guicey.jdbi3.tx.TransactionTemplate;

import java.time.Instant;
import java.util.List;

import static com.comet.opik.api.resources.utils.ClickHouseContainerUtils.DATABASE_NAME;
import static com.comet.opik.domain.mcpoauth.OAuthConstants.OAUTH_VALIDATE_TOKEN_RESOURCE_BASE_PATH;
import static org.assertj.core.api.Assertions.assertThat;

/**
* Drives the real OAuth flow (register → consent → code exchange) against the app and MySQL, then introspects the
* minted access token through {@code POST /opik/auth-oauth}. The mocked resource test proves the wire format of a
* hand-built DTO; this one proves the value on the wire is the persisted row's expiry, which is what a resource
* server (opik-mcp, OPIK-8252) caches against.
*/
@TestInstance(TestInstance.Lifecycle.PER_CLASS)
@ExtendWith(DropwizardAppExtensionProvider.class)
@DisplayName("OAuth Validate Token Integration Test")
class OAuthValidateTokenIntegrationTest {

private static final String REDIRECT_URI = "http://localhost:1234/callback";
private static final String RESOURCE_URI = "http://localhost:8080/api/v1/mcp";

private final RedisContainer REDIS = RedisContainerUtils.newRedisContainer();
private final GenericContainer<?> ZOOKEEPER = ClickHouseContainerUtils.newZookeeperContainer();
private final ClickHouseContainer CLICKHOUSE = ClickHouseContainerUtils.newClickHouseContainer(ZOOKEEPER);
private final MySQLContainer MYSQL = MySQLContainerUtils.newMySQLContainer();

@RegisterApp
private final TestDropwizardAppExtension app;

{
Startables.deepStart(REDIS, CLICKHOUSE, MYSQL, ZOOKEEPER).join();

var databaseAnalyticsFactory = ClickHouseContainerUtils.newDatabaseAnalyticsFactory(CLICKHOUSE, DATABASE_NAME);

MigrationUtils.runMysqlDbMigration(MYSQL);
MigrationUtils.runClickhouseDbMigration(CLICKHOUSE);

// Local auth, so consent resolves to the default workspace without a session. A one-hour access token
// keeps the minted token alive for the whole test.
app = TestDropwizardAppExtensionUtils.newTestDropwizardAppExtension(
AppContextConfig.builder()
.jdbcUrl(MYSQL.getJdbcUrl())
.databaseAnalyticsFactory(databaseAnalyticsFactory)
.redisUrl(REDIS.getRedisURI())
.customConfigs(List.of(
new CustomConfig("mcpOAuth.enabled", "true"),
new CustomConfig("mcpOAuth.baseUrl", "http://localhost:8080"),
new CustomConfig("mcpOAuth.mcpResourceUri", RESOURCE_URI),
new CustomConfig("mcpOAuth.accessTokenTtl", "PT1H")))
.build());
}

private String baseURI;
private ClientSupport client;
private TransactionTemplate transactionTemplate;
private OAuthResourceClient oauthClient;

@BeforeAll
void setUpAll(ClientSupport clientSupport, TransactionTemplate transactionTemplate) {
this.client = clientSupport;
this.baseURI = TestUtils.getBaseUrl(clientSupport);
this.transactionTemplate = transactionTemplate;
this.oauthClient = new OAuthResourceClient(clientSupport, baseURI, REDIRECT_URI, RESOURCE_URI);
}

@Test
@DisplayName("introspection reports the persisted expiry of a token minted through the OAuth endpoints")
void introspectionReportsPersistedExpiry() {
var minted = oauthClient.mintArtifacts();
String accessToken = minted.tokens().accessToken();
Instant persistedExpiry = fetchTokenExpiry(McpOAuthTokenUtils.hash(accessToken));
assertThat(persistedExpiry).isNotNull();

try (Response response = client.target(baseURI + OAUTH_VALIDATE_TOKEN_RESOURCE_BASE_PATH).request()
.header(HttpHeaders.AUTHORIZATION, "Bearer " + accessToken)
.post(Entity.json(""))) {
assertThat(response.getStatus()).isEqualTo(Response.Status.OK.getStatusCode());
var validated = response.readEntity(ValidatedToken.class);
assertThat(validated.expiresAt()).isEqualTo(persistedExpiry);
assertThat(validated.resource()).isEqualTo(RESOURCE_URI);
assertThat(validated.workspaceName()).isEqualTo(minted.tokens().workspaceName());
Comment on lines +108 to +111

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.

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?

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/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.

}
}

private Instant fetchTokenExpiry(String tokenHash) {
return transactionTemplate.inTransaction(
handle -> handle.attach(McpOAuthTokenDAO.class).fetch(tokenHash).map(McpOAuthToken::expiresAt)
.orElse(null));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9729,6 +9729,9 @@ components:
type: string
resource:
type: string
expires_at:
type: string
format: date-time
AnalyticsQueryResponse:
type: object
properties:
Expand Down
3 changes: 3 additions & 0 deletions sdks/code_generation/fern/openapi/openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9729,6 +9729,9 @@ components:
type: string
resource:
type: string
expires_at:
type: string
format: date-time
AnalyticsQueryResponse:
type: object
properties:
Expand Down
Loading