WIP OCPBUGS-71237: Persist console sessions across pod restarts - #16911
WIP OCPBUGS-71237: Persist console sessions across pod restarts#16911jhadvig wants to merge 4 commits into
Conversation
Store the actual encrypted OAuth refresh token in the browser cookie instead of a reference ID that maps to an in-memory store. This allows any console pod to recover a user's session after a restart by decrypting the cookie and exchanging the refresh token with the OAuth server. To enable cross-pod cookie decryption, accept shared encryption keys from files (managed by the console-operator via a session-secret Secret) instead of generating random keys per process. The existing recovery mechanism in getLoginState() already handles the token refresh — the only gap was making the refresh token available from the cookie. Backward compatible: old-format cookies with reference IDs are still accepted via fallback lookup during rolling upgrades. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@jhadvig: This pull request references Jira Issue OCPBUGS-71237, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/invalid-bug
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughOpenShift authentication now accepts paired cookie keys. Session cookies store actual refresh tokens and retain legacy refresh-token ID lookup. Recovery cookies restore missing sessions. Playwright coverage verifies console session persistence after pod recovery and plugin rollout. ChangesOpenShift session persistence
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Playwright
participant Console
participant CombinedSessionStore
participant OpenShiftAuthenticator
participant ConsoleOperator
participant ConsoleDeployment
Playwright->>Console: authenticate and open dashboard
Console->>OpenShiftAuthenticator: request login state
OpenShiftAuthenticator->>CombinedSessionStore: read session or recovery cookie
CombinedSessionStore-->>OpenShiftAuthenticator: return authenticated state
Playwright->>ConsoleDeployment: delete console pods
ConsoleDeployment-->>Console: recover deployment
Playwright->>Console: verify existing session
Playwright->>ConsoleOperator: disable enabled ConsolePlugin
ConsoleOperator->>ConsoleDeployment: trigger rollout
Playwright->>Console: verify existing session after rollout
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/auth/sessions/combined_sessions_test.go`:
- Around line 288-296: Update the legacy-cookie test in
combined_sessions_test.go so it actually exercises legacy resolution through
GetSession: keep the existing byRefreshTokenID setup, add a LoginState entry to
byRefreshToken for "refresh-old", and change the assertion to expect that
session state instead of nil. Use the GetSession path and the existing
testCookies/refresh-old symbols to verify that a legacy cookie resolves
correctly during rolling upgrade.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: efcb9ee2-7afd-4bed-a0f5-e37fcad5a946
📒 Files selected for processing (9)
cmd/bridge/config/session/sessionoptions.gopkg/auth/oauth2/auth.gopkg/auth/oauth2/auth_oidc.gopkg/auth/oauth2/auth_oidc_test.gopkg/auth/oauth2/auth_openshift.gopkg/auth/sessions/combined_sessions.gopkg/auth/sessions/combined_sessions_test.gopkg/auth/sessions/loginstate.gopkg/auth/sessions/server_session.go
💤 Files with no reviewable changes (1)
- pkg/auth/sessions/server_session.go
| testCookies.WithRefreshToken("refresh-old").WithLegacyFormat() | ||
| req = testCookies.Complete(t, req) | ||
|
|
||
| testWriter := httptest.NewRecorder() | ||
| got, err := cs.GetSession(testWriter, req) | ||
| require.NoError(t, err) | ||
|
|
||
| // Legacy format should resolve through byRefreshTokenID map | ||
| require.Nil(t, got, "should not find session by refresh token alone without byRefreshToken mapping") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test a legacy cookie that resolves to a session.
The test only populates byRefreshTokenID. It does not populate byRefreshToken["refresh-old"]. GetSession therefore returns nil whether legacy ID resolution works or not.
Add a LoginState to byRefreshToken for "refresh-old". Assert that GetSession returns that state. This verifies the legacy-cookie compatibility required during a rolling upgrade.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/auth/sessions/combined_sessions_test.go` around lines 288 - 296, Update
the legacy-cookie test in combined_sessions_test.go so it actually exercises
legacy resolution through GetSession: keep the existing byRefreshTokenID setup,
add a LoginState entry to byRefreshToken for "refresh-old", and change the
assertion to expect that session state instead of nil. Use the GetSession path
and the existing testCookies/refresh-old symbols to verify that a legacy cookie
resolves correctly during rolling upgrade.
- Remove refresh token value from error log message to prevent credential leakage in pod logs - Revoke refresh token (OAuthAuthorizeToken) at the OAuth server on logout to prevent cookie replay attacks - Raise securecookie MaxLength to 8192 to accommodate large OIDC JWT refresh tokens that exceed the default 4096 limit Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/auth/oauth2/auth_openshift.go (1)
75-94: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winValidate configured cookie key lengths before constructing the session store. OpenShift validation checks that both keys are set or both are unset, but it does not validate file contents. Require an authentication key of at least 32 bytes and an encryption key of 16, 24, or 32 bytes; otherwise
NewCookieStorerecords the cipher error and cookie operations fail.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/auth/oauth2/auth_openshift.go` around lines 75 - 94, Before constructing the session store in the key-selection flow, validate configured cookie key lengths: require cookieAuthenticationKey to be at least 32 bytes and cookieEncryptionKey to be exactly 16, 24, or 32 bytes. Return a clear validation error for invalid configured values, while preserving random key generation when both keys are unset.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/auth/oauth2/auth_openshift.go`:
- Around line 216-221: Update the logout handler around
OAuthAuthorizeTokens().Delete so a refresh-token revocation error is returned as
a failure response instead of only being logged; stop the flow before deleting
the local session or returning 204. Preserve the existing successful revocation
behavior, and add a test covering the failed deletion path.
- Around line 216-221: Update logout’s access-token revocation failure path to
clear the session before returning: preserve the revocation error separately,
perform refresh-token cleanup and call sessions.DeleteSession, then return the
original error. Ensure cleanup also runs when OAuthAccessTokens().Delete fails
rather than exiting before the existing logout cleanup flow.
---
Outside diff comments:
In `@pkg/auth/oauth2/auth_openshift.go`:
- Around line 75-94: Before constructing the session store in the key-selection
flow, validate configured cookie key lengths: require cookieAuthenticationKey to
be at least 32 bytes and cookieEncryptionKey to be exactly 16, 24, or 32 bytes.
Return a clear validation error for invalid configured values, while preserving
random key generation when both keys are unset.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ace1f7b9-a182-4ba0-a71a-e452c7615b95
📒 Files selected for processing (2)
pkg/auth/oauth2/auth_openshift.gopkg/auth/sessions/combined_sessions.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/auth/sessions/combined_sessions.go
| if refreshToken := ls.RefreshToken(); refreshToken != "" { | ||
| if delErr := oauthClient.OAuthAuthorizeTokens().Delete(ctx, tokenToObjectName(refreshToken), metav1.DeleteOptions{}); delErr != nil { | ||
| klog.V(4).Infof("failed to revoke refresh token on logout: %v", delErr) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- relevant symbols ---'
rg -n -C 12 'func .*DeleteSession|DeleteSession\(|failed to revoke refresh token|OAuthAuthorizeTokens\(\)\.Delete|RefreshToken\(\)' pkg/auth/oauth2 pkg/auth/sessions
printf '%s\n' '--- bounded source slice ---'
sed -n '180,245p' pkg/auth/oauth2/auth_openshift.goRepository: openshift/console
Length of output: 35701
Broken Authentication (CWE-613): Insufficient Session Expiration
Reachability: External · Exploitability: Moderate
Return an error when refresh-token revocation fails.
OAuthAuthorizeTokens().Delete errors are logged, but the handler still deletes the local session and returns 204. A copied refresh-token cookie can remain valid. Return a failure response and add a test for this path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/auth/oauth2/auth_openshift.go` around lines 216 - 221, Update the logout
handler around OAuthAuthorizeTokens().Delete so a refresh-token revocation error
is returned as a failure response instead of only being logged; stop the flow
before deleting the local session or returning 204. Preserve the existing
successful revocation behavior, and add a test covering the failed deletion
path.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '180,235p' pkg/auth/oauth2/auth_openshift.go
printf '\n-- related symbols --\n'
rg -n -C 3 'DeleteSession|refreshSession|OAuthAccessTokens\(\)\.Delete|OAuthAuthorizeTokens\(\)\.Delete|RefreshToken\(\)' pkg/auth/oauth2 pkg/auth/sessionsRepository: openshift/console
Length of output: 20240
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path("pkg/auth/oauth2/auth_openshift.go")
lines = p.read_text().splitlines()
window = "\n".join(lines[177:224])
required = [
"err = oauthClient.OAuthAccessTokens().Delete",
'http.Error(w, "removing the session failed", http.StatusInternalServerError)',
"return",
'if refreshToken := ls.RefreshToken(); refreshToken != "" {',
"o.sessions.DeleteSession(w, r)",
]
positions = {text: window.find(text) for text in required}
print("logout control-flow positions:", positions)
print("access-delete failure returns before refresh cleanup:",
positions[required[1]] < positions[required[3]] and positions[required[2]] < positions[required[3]])
print("access-delete failure returns before local session deletion:",
positions[required[1]] < positions[required[4]] and positions[required[2]] < positions[required[4]])
PY
printf '\n-- session cookie deletion and refresh lookup --\n'
sed -n '88,108p' pkg/auth/sessions/combined_sessions.go
sed -n '219,250p' pkg/auth/sessions/combined_sessions.go
sed -n '252,274p' pkg/auth/oauth2/auth_openshift.goRepository: openshift/console
Length of output: 3274
Broken Authentication (CWE-613): Insufficient Session Expiration
Reachability: External
Clear the session when access-token revocation fails.
When OAuthAccessTokens().Delete returns an error, logout returns before refresh-token cleanup and o.sessions.DeleteSession. The refresh-token cookie and server-side session can then reach refreshSession. Run cleanup on this failure path and preserve the revocation error separately.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/auth/oauth2/auth_openshift.go` around lines 216 - 221, Update logout’s
access-token revocation failure path to clear the session before returning:
preserve the revocation error separately, perform refresh-token cleanup and call
sessions.DeleteSession, then return the original error. Ensure cleanup also runs
when OAuthAccessTokens().Delete fails rather than exiting before the existing
logout cleanup flow.
Two test cases: - Session survives console pod deletion and replacement - Session survives plugin toggle (operator-triggered rollout) Both tests log in (kubeadmin by default, htpasswd when env vars are set), trigger a console pod restart, and assert the user is still authenticated without a login redirect. Tagged @slow — pod rollout takes 30-60s per test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/e2e/tests/console/session-persistence.spec.ts (1)
33-33: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse Playwright
baseURL-resolved routes.Full URLs bypass Playwright route resolution and treat
WEB_CONSOLE_URLas a route prefix. Use leading-slash routes.
frontend/e2e/tests/console/session-persistence.spec.ts#L33-L33: usepage.goto('/dashboards').frontend/e2e/tests/console/session-persistence.spec.ts#L55-L58: usepage.goto('/k8s/cluster/nodes').frontend/e2e/tests/console/session-persistence.spec.ts#L128-L131: usepage.goto('/dashboards').Based on learnings: use
page.goto('/k8s/...')with a leading-slash absolute path in console E2E specs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/e2e/tests/console/session-persistence.spec.ts` at line 33, Update all three page.goto calls in frontend/e2e/tests/console/session-persistence.spec.ts at lines 33-33, 55-58, and 128-131 to use leading-slash Playwright routes: /dashboards, /k8s/cluster/nodes, and /dashboards respectively, removing baseURL interpolation.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/e2e/tests/console/session-persistence.spec.ts`:
- Line 14: Update the baseURL initialization in the session persistence test to
remove the page.url() fallback; use WEB_CONSOLE_URL when set, otherwise default
directly to http://localhost:9000 before passing it to performLogin.
- Around line 50-52: Update the console session-persistence test in
frontend/e2e/tests/console/session-persistence.spec.ts at lines 50-52 and
120-124: before deleting console pods, record their UIDs and require Ready
replacement pods with different UIDs; before applying the plugin patch, record
metadata.generation and wait for it to increase before calling
waitForDeploymentReady. Ensure readiness checks cannot succeed against stale
pods or an unchanged deployment.
---
Nitpick comments:
In `@frontend/e2e/tests/console/session-persistence.spec.ts`:
- Line 33: Update all three page.goto calls in
frontend/e2e/tests/console/session-persistence.spec.ts at lines 33-33, 55-58,
and 128-131 to use leading-slash Playwright routes: /dashboards,
/k8s/cluster/nodes, and /dashboards respectively, removing baseURL
interpolation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b35579f3-c0f2-4253-b387-ebc49baf1e8a
📒 Files selected for processing (1)
frontend/e2e/tests/console/session-persistence.spec.ts
| test.setTimeout(300_000); | ||
|
|
||
| test('session survives console pod deletion', async ({ page, k8sClient }) => { | ||
| const baseURL = process.env.WEB_CONSOLE_URL || page.url() || 'http://localhost:9000'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect whether the custom page fixture navigates before this test starts.
fd -a 'playwright.config.ts' frontend/e2e
rg -n -C 5 'test\.extend|page\s*:|page\.goto\(' frontend/e2eRepository: openshift/console
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- session-persistence.spec.ts ---'
cat -n frontend/e2e/tests/console/session-persistence.spec.ts | sed -n '1,75p'
printf '%s\n' '--- fixture definitions and configuration ---'
rg -n -C 8 'export const test|test\.extend|baseURL|storageState|page\.goto' frontend/e2e/fixtures frontend/e2e --glob '*fixture*' --glob 'playwright.config.ts' --glob '*.config.ts' | head -n 240Repository: openshift/console
Length of output: 3832
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact fixture/config files ---'
fd -a -t f . frontend/e2e | rg '(^|/)(fixtures|playwright\.config|.*fixture.*|.*config.*)\.(ts|js)$' | sortRepository: openshift/console
Length of output: 505
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- frontend/e2e/fixtures/index.ts ---'
cat -n frontend/e2e/fixtures/index.ts
printf '%s\n' '--- page fixture navigation and project baseURL ---'
rg -n -C 6 'baseURL|storageState|page\.goto|test\.extend|defineConfig' frontend/e2e package.json playwright.config.ts 2>/dev/null || trueRepository: openshift/console
Length of output: 50374
Remove page.url() from the baseURL fallback.
The default page fixture does not navigate before the test, so page.url() is about:blank. When WEB_CONSOLE_URL is unset, performLogin receives about:blank instead of the console URL. Use process.env.WEB_CONSOLE_URL || 'http://localhost:9000'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/tests/console/session-persistence.spec.ts` at line 14, Update
the baseURL initialization in the session persistence test to remove the
page.url() fallback; use WEB_CONSOLE_URL when set, otherwise default directly to
http://localhost:9000 before passing it to performLogin.
| await test.step('Wait for new console pods to be ready', async () => { | ||
| await k8sClient.waitForDeploymentReady(CONSOLE_DEPLOYMENT, CONSOLE_NAMESPACE, 180_000); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the readiness helper and determine whether it checks a target generation
# or newly created pods rather than only current Deployment readiness.
rg -n -C 8 'waitForDeploymentReady|waitFor.*Pod|metadata\.generation|updatedReplicas' frontend/e2eRepository: openshift/console
Length of output: 15116
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- session-persistence.spec.ts ---'
sed -n '1,145p' frontend/e2e/tests/console/session-persistence.spec.ts
printf '%s\n' '--- waitForDeploymentReady and related client methods ---'
sed -n '580,750p' frontend/e2e/clients/kubernetes-client.ts
printf '%s\n' '--- console pod discovery and deployment patch call sites ---'
rg -n -C 12 'CONSOLE_DEPLOYMENT|CONSOLE_NAMESPACE|list.*Pod|readNamespacedPod|patchNamespacedCustomObject|plugins' frontend/e2e/tests/console/session-persistence.spec.ts frontend/e2e/clients/kubernetes-client.tsRepository: openshift/console
Length of output: 30341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`
class DeploymentObservation:
desired: int
available: int
updated: int
available_condition: bool
generation: int
pod_uids: tuple[str, ...]
def wait_for_deployment_ready(d: DeploymentObservation) -> bool:
return (
d.available == d.desired
and d.updated == d.desired
and d.available_condition
)
old = DeploymentObservation(2, 2, 2, True, 7, ("old-a", "old-b"))
# A stale observation can pass immediately after deletion is accepted.
stale_after_delete = DeploymentObservation(2, 2, 2, True, 7, ("old-a", "old-b"))
assert wait_for_deployment_ready(stale_after_delete)
# A plugin change can leave the Deployment ready without a new generation.
no_rollout_after_plugin_patch = DeploymentObservation(2, 2, 2, True, 7, ("old-a", "old-b"))
assert wait_for_deployment_ready(no_rollout_after_plugin_patch)
assert no_rollout_after_plugin_patch.generation == old.generation
# The intended checks distinguish both cases.
replacement = DeploymentObservation(2, 2, 2, True, 7, ("new-a", "new-b"))
assert not set(replacement.pod_uids) & set(old.pod_uids)
rollout = DeploymentObservation(2, 2, 2, True, 8, ("new-a", "new-b"))
assert rollout.generation > old.generation
print("waitForDeploymentReady ignores generation and pod UIDs")
print("stale readiness and no-rollout readiness both satisfy its predicate")
PYRepository: openshift/console
Length of output: 278
Gate readiness on the requested disruption.
- Before deleting console pods, record their UIDs and wait for Ready replacement pods with different UIDs.
- Before the plugin patch, record
metadata.generationand wait for it to increase before callingwaitForDeploymentReady. Otherwise, stale readiness can pass without a rollout.
📍 Affects 1 file
frontend/e2e/tests/console/session-persistence.spec.ts#L50-L52(this comment)frontend/e2e/tests/console/session-persistence.spec.ts#L120-L124
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/tests/console/session-persistence.spec.ts` around lines 50 - 52,
Update the console session-persistence test in
frontend/e2e/tests/console/session-persistence.spec.ts at lines 50-52 and
120-124: before deleting console pods, record their UIDs and require Ready
replacement pods with different UIDs; before applying the plugin patch, record
metadata.generation and wait for it to increase before calling
waitForDeploymentReady. Ensure readiness checks cannot succeed against stale
pods or an unchanged deployment.
OpenShift's internal OAuth server does not support refresh tokens, so the refresh-token-in-cookie approach only works for OIDC auth. For OpenShift auth, store the encrypted access token + expiry in a separate recovery cookie (openshift-recovery-token). After pod restart, the backend reads the token from the cookie, validates expiry locally, and creates a new server-side session transparently. No OAuth server interaction or page redirect needed. - SetRecoveryCookie/GetRecoveryCookie/ClearRecoveryCookie methods - recoverSession() called in getLoginState() when session is nil - Recovery cookie cleared on logout and DeleteSession - MaxAge set to match token expiry - Unit tests for recovery cookie lifecycle Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/auth/sessions/combined_sessions_test.go`:
- Around line 310-356: Update the recovery-cookie tests around the request
constructions in the relevant subtests to capture the error returned by each
http.NewRequest call and immediately assert it with require.NoError(t, err).
Apply this to every affected request, including req and req2 in the set/get,
empty-request, clear-cookie, and expired-token cases, without changing the test
behavior.
In `@pkg/auth/sessions/combined_sessions.go`:
- Around line 263-285: Handle errors returned by CookieStore.Get in both
SetRecoveryCookie and GetRecoveryCookie. In SetRecoveryCookie, return a wrapped
decode error or initialize a fresh session before mutating and saving; in
GetRecoveryCookie, return the existing empty values with false when decoding
fails, before accessing session values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f56d416b-5576-4e6b-8775-88ccab4263b1
📒 Files selected for processing (4)
pkg/auth/oauth2/auth_openshift.gopkg/auth/sessions/combined_sessions.gopkg/auth/sessions/combined_sessions_test.gopkg/auth/sessions/server_session.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/auth/sessions/server_session.go
| req, _ := http.NewRequest(http.MethodGet, "/", nil) | ||
| w := httptest.NewRecorder() | ||
|
|
||
| err := cs.SetRecoveryCookie(w, req, accessToken, expiry) | ||
| require.NoError(t, err) | ||
|
|
||
| // Build a new request with the cookie from the response | ||
| req2, _ := http.NewRequest(http.MethodGet, "/", nil) | ||
| for _, c := range w.Result().Cookies() { | ||
| req2.AddCookie(c) | ||
| } | ||
|
|
||
| gotToken, gotExpiry, ok := cs.GetRecoveryCookie(req2) | ||
| require.True(t, ok) | ||
| require.Equal(t, accessToken, gotToken) | ||
| require.Equal(t, expiry.Unix(), gotExpiry.Unix()) | ||
| }) | ||
|
|
||
| t.Run("get recovery cookie from empty request", func(t *testing.T) { | ||
| req, _ := http.NewRequest(http.MethodGet, "/", nil) | ||
| _, _, ok := cs.GetRecoveryCookie(req) | ||
| require.False(t, ok) | ||
| }) | ||
|
|
||
| t.Run("clear recovery cookie", func(t *testing.T) { | ||
| req, _ := http.NewRequest(http.MethodGet, "/", nil) | ||
| w := httptest.NewRecorder() | ||
|
|
||
| cs.ClearRecoveryCookie(w, req) | ||
|
|
||
| cookies := w.Result().Cookies() | ||
| require.Len(t, cookies, 1) | ||
| require.Equal(t, openshiftRecoveryTokenCookieName, cookies[0].Name) | ||
| require.Equal(t, -1, cookies[0].MaxAge) | ||
| }) | ||
|
|
||
| t.Run("recovery cookie with expired token", func(t *testing.T) { | ||
| req, _ := http.NewRequest(http.MethodGet, "/", nil) | ||
| w := httptest.NewRecorder() | ||
|
|
||
| pastExpiry := time.Now().Add(-1 * time.Hour) | ||
| err := cs.SetRecoveryCookie(w, req, accessToken, pastExpiry) | ||
| require.NoError(t, err) | ||
|
|
||
| req2, _ := http.NewRequest(http.MethodGet, "/", nil) | ||
| for _, c := range w.Result().Cookies() { | ||
| req2.AddCookie(c) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 1 'http\.NewRequest' pkg/auth/sessions/combined_sessions_test.goRepository: openshift/console
Length of output: 1612
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '300,365p' pkg/auth/sessions/combined_sessions_test.go
printf '\nAll NewRequest call sites:\n'
rg -n -C 1 'http\.NewRequest' pkg/auth/sessions/combined_sessions_test.go
printf '\nIgnored NewRequest assignments in the reviewed range:\n'
python3 - <<'PY'
from pathlib import Path
import re
path = Path("pkg/auth/sessions/combined_sessions_test.go")
lines = path.read_text().splitlines()
for number, line in enumerate(lines, 1):
if 310 <= number <= 356 and re.search(r'\b\w+\s*,\s*_\s*:=\s*http\.NewRequest\b', line):
print(f"{number}: {line}")
PYRepository: openshift/console
Length of output: 4153
Handle http.NewRequest errors in the recovery-cookie tests.
Lines 310, 317, 329, 335, 347, and 354 discard the returned errors. Capture each error and call require.NoError(t, err) immediately after construction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/auth/sessions/combined_sessions_test.go` around lines 310 - 356, Update
the recovery-cookie tests around the request constructions in the relevant
subtests to capture the error returned by each http.NewRequest call and
immediately assert it with require.NoError(t, err). Apply this to every affected
request, including req and req2 in the set/get, empty-request, clear-cookie, and
expired-token cases, without changing the test behavior.
Source: Path instructions
| func (cs *CombinedSessionStore) SetRecoveryCookie(w http.ResponseWriter, r *http.Request, accessToken string, expiry time.Time) error { | ||
| s, _ := cs.clientStore.Get(r, openshiftRecoveryTokenCookieName) | ||
| s.Values["access-token"] = accessToken | ||
| s.Values["expiry"] = expiry.Unix() | ||
| maxAge := int(time.Until(expiry).Seconds()) | ||
| if maxAge > 0 { | ||
| s.Options.MaxAge = maxAge | ||
| } | ||
| return s.Save(r, w) | ||
| } | ||
|
|
||
| func (cs *CombinedSessionStore) GetRecoveryCookie(r *http.Request) (string, time.Time, bool) { | ||
| s, _ := cs.clientStore.Get(r, openshiftRecoveryTokenCookieName) | ||
| accessToken, ok := s.Values["access-token"].(string) | ||
| if !ok || accessToken == "" { | ||
| return "", time.Time{}, false | ||
| } | ||
| expiryUnix, ok := s.Values["expiry"].(int64) | ||
| if !ok { | ||
| return "", time.Time{}, false | ||
| } | ||
| return accessToken, time.Unix(expiryUnix, 0), true | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For github.com/gorilla/sessions v1.4.0, what does CookieStore.Get return when a cookie cannot be decoded or verified?
💡 Result:
For gorilla/sessions v1.4.0, when a cookie cannot be decoded or verified, CookieStore.Get returns a new session object along with a non-nil error [1][2]. Specifically: - The session object returned is a fresh session (i.e., its IsNew field will be true) [1][2]. - The error returned indicates the reason the existing cookie could not be successfully decoded or verified (such as a signature mismatch or expired/invalid data) [3][4]. Because Get always returns a session, even if it cannot decode the provided cookie, it is common practice in many applications to ignore the error returned by Get if you simply intend to proceed with a blank or new session when the existing one is invalid [5][6]. Calling session.Save on this returned session will effectively overwrite the invalid cookie with a new, valid one in the user's browser [6].
Citations:
- 1: https://github.com/gorilla/sessions/blob/v1.4.0/store.go
- 2: https://github.com/gorilla/sessions/blob/main/store.go
- 3: How to handle err from store.Get() gorilla/sessions#179
- 4: Changed auth/encryption key prevents the signed in user from accessing the webpage again gorilla/sessions#249
- 5: https://pkg.go.dev/github.com/gorilla/sessions
- 6: securecookie: the value is not valid gorilla/sessions#16
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
sed -n '230,310p' pkg/auth/sessions/combined_sessions.go
printf '%s\n' '--- call sites ---'
rg -n -C 3 'GetRecoveryCookie|SetRecoveryCookie' .
printf '%s\n' '--- dependency declarations ---'
rg -n -C 2 'gorilla/sessions|sessions v' go.mod go.sum vendor 2>/dev/null || true
printf '%s\n' '--- CookieStore.Get implementation references ---'
rg -n -C 8 'func \(.*CookieStore.*\) Get|type CookieStore|Decode' $(go env GOPATH 2>/dev/null)/pkg/mod/github.com/gorilla/sessions* 2>/dev/null || trueRepository: openshift/console
Length of output: 21836
🌐 Web query:
github.com/gorilla/sessions v1.4.0 CookieStore.Get source store.go securecookie.Decode error behavior
💡 Result:
In github.com/gorilla/sessions v1.4.0, the CookieStore.Get method behaves as a convenience wrapper around Registry.Get [1], which in turn calls CookieStore.New if a session does not already exist in the registry [1][2]. When CookieStore.New encounters an issue decoding an existing cookie (e.g., due to invalid keys, tampered data, or expired timestamps handled by the securecookie package), it returns a new, empty session along with the error [1]. Key aspects of this behavior include: 1. Always Returns a Session: CookieStore.Get (via New) is designed to always return a *Session object, even if an error occurs during the decoding process [1][3]. This ensures that application logic can proceed with a fresh session if the previous one is unreadable [4]. 2. Error Indication: The error returned indicates that while a cookie was present, it could not be successfully decoded using the configured codecs [1]. 3. Recommended Handling: The official gorilla/sessions documentation and community guidance suggest that while the error can be checked, Get() often returns a valid (though empty or new) session object regardless of the error [3][5]. Developers often choose to ignore the error returned by Get() if they intend to treat a failed decryption as a session reset (i.e., treating the user as unauthenticated) [5]. 4. Underlying Mechanism: The securecookie.Decode method (called within CookieStore.New) performs several validation steps, including checking the length, verifying the MAC (message authentication code), validating timestamps (if configured), and decrypting the payload [6]. Any failure in these steps results in an error [6], which CookieStore bubbles up [1]. Common scenarios where securecookie.Decode returns an error include key rotation mismatches, modified or malformed cookie values, and expired sessions [4][6][7]. Since CookieStore continues by returning a new *Session with IsNew set to true, the application effectively invalidates the unreadable session and starts a new one [1].
Citations:
- 1: https://github.com/gorilla/sessions/blob/v1.4.0/store.go
- 2: https://github.com/gorilla/sessions/blob/v1.4.0/sessions.go
- 3: https://github.com/gorilla/sessions/blob/v1.4.0/doc.go
- 4: Changed auth/encryption key prevents the signed in user from accessing the webpage again gorilla/sessions#249
- 5: https://github.com/gorilla/sessions/blob/main/README.md
- 6: https://github.com/gorilla/securecookie/blob/main/securecookie.go
- 7: How to handle err from store.Get() gorilla/sessions#179
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- session store construction and related error handling ---'
rg -n -C 5 'clientStore|NewCookieStore|CookieStore|CombinedSessionStore' pkg/auth/sessions pkg/auth/oauth2 | head -240
printf '%s\n' '--- recovery-cookie tests ---'
sed -n '280,375p' pkg/auth/sessions/combined_sessions_test.go
printf '%s\n' '--- vendor registry behavior ---'
sed -n '70,125p' vendor/github.com/gorilla/sessions/store.go
sed -n '1,115p' vendor/github.com/gorilla/sessions/registry.go 2>/dev/null || true
printf '%s\n' '--- standalone source-level behavioral verifier ---'
python3 - <<'PY'
from pathlib import Path
p = Path("vendor/github.com/gorilla/sessions/store.go")
text = p.read_text()
start = text.index("func (s *CookieStore) New")
end = text.index("\n}", start) + 2
snippet = text[start:end]
assert "return session, err" in snippet
assert "session.IsNew = true" in snippet
assert "if err == nil" in snippet and "session.IsNew = false" in snippet
print("CookieStore.New returns the allocated session with the decode error and keeps IsNew=true on decode failure.")
print("CookieStore.Get delegates to the registry, which returns that result.")
PYRepository: openshift/console
Length of output: 22748
Handle recovery-cookie decode errors.
CookieStore.Get returns a new session and a non-nil error when decoding fails. Handle this error at both call sites. Return a wrapped error from SetRecoveryCookie, or explicitly create a fresh session before saving. Return false from GetRecoveryCookie when decoding fails.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/auth/sessions/combined_sessions.go` around lines 263 - 285, Handle errors
returned by CookieStore.Get in both SetRecoveryCookie and GetRecoveryCookie. In
SetRecoveryCookie, return a wrapped decode error or initialize a fresh session
before mutating and saving; in GetRecoveryCookie, return the existing empty
values with false when decoding fails, before accessing session values.
Source: Path instructions
2 participants
Analysis / Root cause:
Console sessions are stored entirely in per-pod process memory. When a pod restarts (upgrades, scaling, OOM, operator reconciliation), all sessions are lost and users must re-login. The refresh token cookie only stores a 32-char reference ID that maps to the actual token in an in-memory map — after restart, the map is empty and the reference resolves to nothing.
Jira: https://redhat.atlassian.net/browse/OCPBUGS-71237
Related: https://redhat.atlassian.net/browse/OCPBUGS-58468, https://redhat.atlassian.net/browse/OCPBUGS-82365
Solution description:
Two coordinated changes (this PR + console-operator PR):
1. Shared encryption keys (console-operator side)
The operator now generates a
session-secretSecret with persistent encryption keys for all auth types (previously OIDC-only) and mounts them into console pods.2. Cookie-based session recovery (this PR)
auth_openshift.go)combined_sessions.go)getLoginState()already handles token refresh — it just needed the actual token from the cookierefreshTokenIDfield and accessor fromLoginStateRecovery flow after pod restart:
GetSession()→ nil (empty maps)getLoginState()callsGetCookieRefreshToken()which decrypts actual token from cookierefreshSession()exchanges token with OAuth server → new access tokenScreenshots / screen recording:
Test setup:
Test cases:
Browser conformance:
Additional info:
🤖 Generated with Claude Code
Summary by CodeRabbit