-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[OPIK-8252] [BE] feat: Report expires_at from MCP OAuth token introspection #8153
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -183,6 +183,7 @@ public Optional<ValidatedToken> validateAccessToken(@NonNull String token) { | |
| .workspaceId(row.workspaceId()) | ||
| .workspaceName(row.workspaceName()) | ||
| .resource(row.resource()) | ||
| .expiresAt(row.expiresAt()) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Persisted expiry propagation lacks integration coverageThe Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by Other fix methodsPrompt for AI Agents
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added |
||
| .build()); | ||
| }); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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; | ||
|
|
@@ -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()) | ||
|
|
@@ -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)); | ||
|
|
||
|
|
@@ -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)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Token expiry mapping lacks regression coverageThe new wire-format test mocks Want Baz to fix this for you? Activate Fixer Other fix methodsPrompt for AI Agents
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Covered by the new |
||
|
|
||
| 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Identity regressions pass integration testsThe integration test verifies only Want Baz to fix this for you? Activate Fixer Other fix methodsPrompt for AI Agents |
||
| } | ||
| } | ||
|
|
||
| private Instant fetchTokenExpiry(String tokenHash) { | ||
| return transactionTemplate.inTransaction( | ||
| handle -> handle.attach(McpOAuthTokenDAO.class).fetch(tokenHash).map(McpOAuthToken::expiresAt) | ||
| .orElse(null)); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
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
expiresAtis now emitted in the backend response, but the checked-in OpenAPIValidatedTokenschema isn't updated, so Fern-generated models lackexpires_atand 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
There was a problem hiding this comment.
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_atto 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.
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) toValidatedTokenin 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.
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.