feat(auth): self-refreshing GitHub user tokens for deployment approvals - #1199
feat(auth): self-refreshing GitHub user tokens for deployment approvals#1199krusche wants to merge 7 commits into
Conversation
Groundwork for Helios-side refresh of Keycloak-brokered GitHub tokens so deployment approvals keep working past the 8h GitHub token lifetime. - TokenCipher: AES-GCM encrypt/decrypt for tokens stored at rest, keyed by helios.tokenEncryptionKey (HELIOS_TOKEN_ENCRYPTION_KEY); fails fast on a blank key so refresh tokens (6-month credentials) are never persisted in plaintext. - GitHubConfig: add the login App's OAuth clientSecret (GITHUB_CLIENT_SECRET), paired with the existing clientId, for the refresh_token grant. - Wire GITHUB_CLIENT_SECRET + HELIOS_TOKEN_ENCRYPTION_KEY into prod/staging/dev profiles and compose.prod.yaml; dummy values in the test profile. - Include the full implementation plan (docs/superpowers/plans/). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds the github_user_token store (one row per GitHub login) plus entity, repository and an embedded-Postgres integration test. Token columns hold AES-GCM ciphertext only. Foundation for the refresh loop that keeps deployment approvals working past the 8h GitHub token lifetime. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GitHubOAuthTokenClient exchanges a stored refresh token for a fresh access token
directly against GitHub (grant_type=refresh_token), returning the rotated refresh
token to persist. OAuth error bodies (HTTP 200 + {"error":...}) surface as
GitHubReauthRequiredException (re-login needed); transport/5xx stay plain
IOException (transient).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- KeycloakBrokerTokenClient: seeds a user's GitHub refresh token via Keycloak's broker retrieve-token endpoint (impersonation token-exchange -> GET /broker/github/token), parsing either a JSON or form-encoded stored body. Requires the token-exchange client to hold retrieve-token permission on the github IdP (else 403). - GitHubUserTokenService: hands out a valid GitHub user access token, resolving cache -> refresh (rotating + persisting) -> seed-then-refresh, and surfacing GitHubReauthRequiredException when no refresh path can succeed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…reshing token service Replace the direct Keycloak token-exchange (which returns a token GitHub expires after 8h and never refreshes) with GitHubUserTokenService.getValidAccessToken in both reviewPendingDeployment and createReleaseOnBehalfOfUser. Approvals (auto and in-app) and release drafts now get a token that is refreshed/re-seeded as needed, so they keep working regardless of when the user last logged in. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…missing A blank HELIOS_TOKEN_ENCRYPTION_KEY no longer crashes the whole application at boot (too large a blast radius for an approval-only feature, and staging auto-deploys). Instead the app logs a loud warning at startup and encrypt/decrypt throw on use, so a misconfigured instance degrades only GitHub-token operations and still never stores plaintext. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the "limit the session to 8 hours" workaround with the refresh model: seed the refresh token from Keycloak's retrieve-token endpoint (needs the retrieve-token permission on the github IdP), then refresh directly against GitHub. Lists the required config (GITHUB_CLIENT_SECRET, HELIOS_TOKEN_ENCRYPTION_KEY). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 1 minor |
| Comprehensibility | 1 minor |
🔴 Metrics 54 complexity
Metric Results Complexity ⚠️ 54 (≤ 20 complexity)
🟢 Coverage 93.79% diff coverage · +0.58% coverage variation
Metric Results Coverage variation ✅ +0.58% coverage variation (-1.00%) Diff coverage ✅ 93.79% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (8afae1a) Report Missing Report Missing Report Missing Head commit (fe60c56) 16137 (+248) 8751 (+226) 54.23% (+0.58%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#1199) 177 166 93.79% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| throw new IOException("Failed to exchange token for GitHub user: " + githubUserLogin); | ||
| } | ||
| // Fetch a valid GitHub user token, refreshing (or re-seeding) it as needed. | ||
| String userGithubToken = gitHubUserTokenService.getValidAccessToken(githubUserLogin); |
There was a problem hiding this comment.
🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.coding.VariableDeclarationUsageDistanceCheck> reported by reviewdog 🐶
Distance between variable 'userGithubToken' declaration and its first usage is 6, but allowed 3. Consider making that variable final if you still need to store its value in advance (before method calls that might have side effects on the original value).
| @Log4j2 | ||
| @Component | ||
| @RequiredArgsConstructor | ||
| public class GitHubOAuthTokenClient { |
There was a problem hiding this comment.
🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.naming.AbbreviationAsWordInNameCheck> reported by reviewdog 🐶
Abbreviation in name 'GitHubOAuthTokenClient' must contain no more than '1' consecutive capital letters.
| import org.junit.jupiter.api.Test; | ||
| import org.mockito.ArgumentCaptor; | ||
|
|
||
| class GitHubOAuthTokenClientTest { |
There was a problem hiding this comment.
🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.naming.AbbreviationAsWordInNameCheck> reported by reviewdog 🐶
Abbreviation in name 'GitHubOAuthTokenClientTest' must contain no more than '1' consecutive capital letters.
github-actions
Bot
added
documentation
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@krusche The encrypted persistence and component-level tests are a solid base, but the rotation path needs per-user serialization and a recovery path after reauthentication before this is safe to merge. OAuth configuration failures also need to remain distinct from user reauthorization, and the agent-directed instruction in the plan should be removed. Codacy is currently failing as well.
| @Transactional | ||
| public String getValidAccessToken(String githubLogin) throws IOException { | ||
| OffsetDateTime now = OffsetDateTime.now(); | ||
| Optional<GitHubUserToken> existing = repository.findByGithubLogin(githubLogin); |
There was a problem hiding this comment.
@krusche [high] This read does not lock or otherwise serialize refreshes for a login. GitHub documents refresh tokens as single-use, so two deployments for the same reviewer can both read the same token; one rotates it and the other then fails, while concurrent first-use calls can also race into the unique constraint. Serialize the full lookup/refresh/persist sequence per normalized login across application instances, including the no-row case.
🤖 Prompt for AI agents
In server/application-server/src/main/java/de/tum/cit/aet/helios/auth/github/token/GitHubUserTokenService.java, concurrent calls can consume the same single-use GitHub refresh token or race while creating the first row. Add database-backed per-login serialization around lookup, refresh, and persistence, handle the absent-row case, and add a concurrency test proving only one refresh grant runs.
| && (row.getRefreshTokenExpiresAt() == null | ||
| || row.getRefreshTokenExpiresAt().isAfter(now))) { | ||
| GitHubUserTokenRecord refreshed = | ||
| oauthClient.refresh(tokenCipher.decrypt(row.getRefreshTokenEnc())); |
There was a problem hiding this comment.
@krusche [high] A rejected stored refresh token is propagated immediately, so signing in again cannot actually recover: Keycloak gets the new token, but every later call keeps retrying the stale encrypted database token. On bad_refresh_token, reseed from Keycloak and replace the row; only surface reauth if that fresh seed is also unavailable or rejected.
🤖 Prompt for AI agents
In server/application-server/src/main/java/de/tum/cit/aet/helios/auth/github/token/GitHubUserTokenService.java, a rejected persisted refresh token permanently wins over a newer token stored by Keycloak after login. Catch the token-specific reauthorization exception from the stored-token refresh, reseed and persist through KeycloakBrokerTokenClient, and add a test that a post-login seed repairs the stale row.
| } | ||
|
|
||
| JsonNode json = objectMapper.readTree(content); | ||
| if (json.hasNonNull("error")) { |
There was a problem hiding this comment.
@krusche [medium] Every OAuth error is classified as user reauthorization, including app-side failures such as incorrect_client_credentials or an unsupported grant. Signing out cannot fix those, and this classification would also make the service's reseed path misleading. Map only refresh-token rejection codes to GitHubReauthRequiredException; keep configuration and malformed/transient responses as ordinary IOExceptions.
🤖 Prompt for AI agents
In server/application-server/src/main/java/de/tum/cit/aet/helios/auth/github/token/GitHubOAuthTokenClient.java, all OAuth error responses are incorrectly treated as requiring user login. Whitelist only refresh-token rejection errors for GitHubReauthRequiredException, map configuration and protocol errors to IOException, and add tests for both categories.
| @@ -0,0 +1,292 @@ | |||
| # GitHub User-Token Refresh Implementation Plan | |||
|
|
|||
| > **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. | |||
There was a problem hiding this comment.
@krusche [medium] This line is an instruction aimed directly at automated agents, including a required tool/workflow choice. Repository documentation should describe the implementation for readers without attempting to control an agent's execution environment; please remove this directive or rewrite it as neutral plan context.
🤖 Prompt for AI agents
In docs/superpowers/plans/2026-07-19-github-user-token-refresh.md, line 3 directs automated agents to use specific skills and workflows. Remove the agent-directed directive and leave only neutral, human-readable implementation-plan context.
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
|
we should still work on this and get it fixed: not stale |
Motivation
Deployment approvals (both Helios' automatic approval and the in-app Approve/Decline buttons) act on GitHub using a GitHub user token obtained via Keycloak token-exchange. GitHub App user tokens expire after 8 hours, and Keycloak never refreshes brokered tokens — so a few hours after the reviewer last logged in, GitHub returns
HTTP 401and approvals fail (the "internal server error" reported on prod). It can never be "always automatic" as built, because the token only refreshes on interactive login.This PR makes Helios own the refresh loop so a valid user token is always available, independent of when the user last logged in.
Approach (Helios-side refresh)
Keycloak config-only refresh is not possible for the
githubprovider (confirmed against Keycloak source + issues), so:GET /broker/github/token), reached headlessly via an impersonation token-exchange.grant_type=refresh_token) using the App's client id/secret, caching the ~8h access token and persisting the rotated refresh token each time (GitHub rotates them).GitHubReauthRequiredExceptionso the reviewer is told to sign in again.Refresh tokens are 6-month credentials, so they're encrypted at rest (AES-GCM).
What's here (components)
TokenCipher— AES-GCM encrypt/decrypt; missing key degrades only token ops (never crashes boot, never stores plaintext).github_user_tokentable (V59) + entity/repo — one encrypted row per GitHub login.GitHubOAuthTokenClient— GitHub refresh grant (rotation-aware; distinguishes re-auth vs transient).KeycloakBrokerTokenClient— seeds the refresh token via retrieve-token (parses JSON or form-encoded).GitHubUserTokenService— cache → refresh → seed orchestration.GitHubServicerewired: approvals + release drafts now usegetValidAccessToken(...).GITHUB_CLIENT_SECRET— the login GitHub App's OAuth client secret (App must have "Expire user authorization tokens" on so refresh tokens are issued).HELIOS_TOKEN_ENCRYPTION_KEY—openssl rand -base64 32. (Missing key no longer crashes the app — it logs a warning and approvals fail until set — but it must be set for the feature to work.)helios-token-exchangeclient the retrieve-token permission on thegithubIdP (Identity Providers → github → Permissions → thetokenpermission → add its policy). Confirmed needed — a read-only spike currently returns403 "Client [helios-token-exchange] not authorized to retrieve tokens from identity provider [github]". (Headless impersonation token-exchange itself already returns 200.)Relationship to #1198
#1198 (error surfacing) and this PR both touch
GitHubService. The final actionable "GitHub authorization expired — sign in again" message for the re-auth case (mappingGitHubReauthRequiredException) lands as a small follow-up once #1198 merges — tracked in the plan (docs/superpowers/plans/2026-07-19-github-user-token-refresh.md, Task 9).Testing
Unit tests for every new component (cipher round-trip; refresh parsing + rotation + re-auth vs transient; seed parsing for JSON and form-encoded bodies + the 403 path; service cache/refresh/seed/re-auth branches; rewired
GitHubService). New embedded-Postgres integration test for the V59 store. Full:application-server:testsuite green locally.🤖 Generated with Claude Code