Skip to content

feat: validate tokens and repository access up front#341

Open
GuillaumeLagrange wants to merge 4 commits intomainfrom
cod-2628-cli-improve-token-validation-across-commands
Open

feat: validate tokens and repository access up front#341
GuillaumeLagrange wants to merge 4 commits intomainfrom
cod-2628-cli-improve-token-validation-across-commands

Conversation

@GuillaumeLagrange
Copy link
Copy Markdown
Contributor

@GuillaumeLagrange GuillaumeLagrange commented May 7, 2026

Catch invalid or mis-scoped tokens before any benchmarks run, instead of
surfacing a 401 from the upload endpoint after a full suite has executed.

The local provider now validates the token and the repository's
CREATE_LOCAL_RUN access in a single GraphQL round-trip when resolving
from a -r override or a detected git remote. The project-repository
fallback is unchanged. auth status uses the same combined query so it
doesn't double-roundtrip when a remote is detected, and auth login
validates the token via session() before persisting it.

The bulk of the diff is a prerequisite refactor: the api_client is now
the single source of truth for the auth token. Previously the token
lived in two places — on the api_client for GraphQL Authorization
headers, and on OrchestratorConfig/ExecutorConfig for upload
Authorization headers — and the two could drift, notably in CI with
OIDC where set_oidc_token mutated config but the api_client kept its
original token. CodSpeedAPIClient now owns the token through token()
and set_token(), the trait method becomes
refresh_token(&mut CodSpeedAPIClient), and &mut api_client is
plumbed through cli/run/exec → Orchestrator → upload_all. The token
field is deleted from both configs.

Reviewers: the two commits are independently reviewable. The first
(ref: make api_client the single source of truth…) is a pure
refactor with no behavior change. The second (feat: validate tokens…) is the user-facing change.

Refs COD-2628

Pin the dependency to a revision that surfaces partial `data` on
errored responses via `GraphQLError::data()`. Lets callers consume
populated sibling fields when a per-field error propagates up to a
nullable ancestor — used by the upcoming up-front token validation
path to read the session even when `repositoryOverview` errors with
REPOSITORY_NOT_FOUND.

Refs COD-2628
Co-Authored-By: Claude <noreply@anthropic.com>
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented May 7, 2026

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

✅ 4 untouched benchmarks
🆕 3 new benchmarks

Performance Changes

Mode Benchmark BASE HEAD Efficiency
🆕 Memory sleep 1 N/A 15.6 KB N/A
🆕 WallTime sleep 1 N/A 1 s N/A
🆕 Simulation sleep 1 N/A 126.3 µs N/A

Comparing cod-2628-cli-improve-token-validation-across-commands (7d22fc9) with main (fd8701d)

Open in CodSpeed

@GuillaumeLagrange GuillaumeLagrange force-pushed the cod-2628-cli-improve-token-validation-across-commands branch from 2eb80bd to 9d888b4 Compare May 7, 2026 13:55
GuillaumeLagrange and others added 2 commits May 7, 2026 16:02
Token state used to live in two places: the api_client carried it for
GraphQL Authorization headers, while OrchestratorConfig.token (and its
clone in ExecutorConfig.token) carried it for upload Authorization
headers. The two were filled from different paths and could drift —
notably in CI with OIDC, where set_oidc_token mutated config but the
api_client kept its original token.

Centralize on the api_client:
- CodSpeedAPIClient now exposes token() and set_token() and is the
  single mutation point for the credentials.
- RunEnvironmentProvider::set_oidc_token(&mut ExecutorConfig) becomes
  refresh_token(&mut CodSpeedAPIClient). GHA implements it; other
  providers inherit the no-op default.
- The Buildkite "token required" check moves from try_from(config) to
  check_oidc_configuration(api_client) — token presence is the
  api_client's authority now.
- Token resolution happens once at CLI entry in build_api_client(&cli):
  --token / CODSPEED_TOKEN takes precedence; the persisted CLI config is
  only loaded as fallback.
- token field deleted from OrchestratorConfig and ExecutorConfig. The
  uploader's Authorization header and the tokenless metadata flag both
  read from api_client.token().
- &mut CodSpeedAPIClient is plumbed through cli/run/exec → Orchestrator
  → upload_all so refresh_token can mutate in place.

Refs COD-2628
Co-Authored-By: Claude <noreply@anthropic.com>
Previously, an invalid or mis-scoped token only surfaced as a 401 from
the upload endpoint after the full benchmark suite had run. The local
provider also had to make two separate GraphQL calls — one to verify
the token and one to look up the repository — without a clean error
shape for the "missing repo / valid token" case.

Combine session validation and repository lookup into a single
`session_and_repository_overview` query, and use it for the local
provider's resolution path:

- `LocalProvider` now validates the token and the repository's
  `CREATE_LOCAL_RUN` access in one round-trip when resolving from a
  `-r` override or from a detected git remote. The project-repository
  fallback is unchanged and still relies on
  `get_or_create_project_repository` to surface auth errors.
- `auth status` now renders the detected git remote alongside the
  authentication state, using the same combined query so we don't
  double-roundtrip when a remote is detected.
- `auth login` validates the token via `session()` before persisting,
  so a malformed or expired token is rejected up front instead of
  being written to disk.
- `REPOSITORY_NOT_FOUND` is folded into the success path
  (`repository_overview: None`) by deserializing the partial-data
  payload — relies on the gql_client partial-data fork.
- `CurrentUser.gql` and `GetRepository.gql` are deleted; replaced by
  `Session.gql` and `SessionAndRepositoryOverview.gql`.

Refs COD-2628
Co-Authored-By: Claude <noreply@anthropic.com>
@GuillaumeLagrange GuillaumeLagrange force-pushed the cod-2628-cli-improve-token-validation-across-commands branch from 9d888b4 to 04f58f9 Compare May 7, 2026 14:04
@GuillaumeLagrange GuillaumeLagrange changed the title Improve token validation across commands feat: validate tokens and repository access up front May 7, 2026
Copy link
Copy Markdown
Member

@adriencaccia adriencaccia left a comment

Choose a reason for hiding this comment

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

I did not finish the review yet. Last two commits still left

Comment thread Cargo.toml
git2 = "0.20.4"
nestify = "0.3.3"
gql_client = { git = "https://github.com/CodSpeedHQ/gql-client-rs" }
gql_client = { git = "https://github.com/CodSpeedHQ/gql-client-rs", rev = "ff1be1b85139e7b7a1f2e9121ae99c6497112030" }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good idea to have a pinned commit here.
But you can merge your changes on the repo to the main branch instead of keeping it in a branch

/// All the validation has already been performed in `check_oidc_configuration`.
/// So if the oidc_config is None, we simply return.
async fn set_oidc_token(&self, config: &mut ExecutorConfig) -> Result<()> {
async fn refresh_token(&self, api_client: &mut CodSpeedAPIClient) -> Result<()> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I do not get why we are changing the name of this method. This method does not refresh any token.
It only sets an OIDC token on the api client if the provider is set up for it.
So let's have a name that reflects exactly this.

Comment on lines +254 to +257
// OIDC tokens (e.g. on GitHub Actions) are short-lived, so we
// refresh the api_client's credentials just before each upload.
// For other run environments this is a no-op.
self.provider.refresh_token(api_client).await?;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same thing here, let's keep the previous logic/comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants