Skip to content

OCPBUGS-71237: Persist console sessions across pod restarts #16911

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.

Sign up for GitHub

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

Open
jhadvig wants to merge 9 commits into
base: main
Choose a base branch
from
Open

OCPBUGS-71237: Persist console sessions across pod restarts #16911

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8c83365
OCPBUGS-71237: Persist console sessions across pod restarts
jhadvig Aug 5, 2026
9d72995
OCPBUGS-71237: Address security review findings
jhadvig Aug 5, 2026
e200791
OCPBUGS-71237: Add e2e test for session persistence
jhadvig Aug 5, 2026
8afcc78
OCPBUGS-71237: Add access token recovery cookie for OpenShift auth
jhadvig Aug 7, 2026
ae77e5e
OCPBUGS-71237: Fix CI lint failures
jhadvig Aug 10, 2026
5d9e366
OCPBUGS-71237: Handle missing session key files gracefully
jhadvig Aug 11, 2026
07b0832
OCPBUGS-71237: Address CodeRabbit review findings
jhadvig Aug 12, 2026
60c662a
OCPBUGS-71237: Support graceful cookie encryption key rotation
jhadvig Aug 12, 2026
aac1845
Fix formatting issues causing build failure
rhamilto Aug 21, 2026
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
12 changes: 9 additions & 3 deletions cmd/bridge/config/session/sessionoptions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ func NewSessionOptions() *SessionOptions {
}

func (opts *SessionOptions) AddFlags(fs *flag.FlagSet) {
fs.StringVar(&opts.CookieEncryptionKeyPath, "cookie-encryption-key-file", "", "Encryption key used to encrypt cookies. Must be set when --user-auth is 'oidc'.")
fs.StringVar(&opts.CookieAuthenticationKeyPath, "cookie-authentication-key-file", "", "Authentication key used to sign cookies. Must be set when --user-auth is 'oidc'.")
fs.StringVar(&opts.CookieEncryptionKeyPath, "cookie-encryption-key-file", "", "Encryption key used to encrypt cookies. Required when --user-auth is 'oidc', optional when 'openshift'.")
fs.StringVar(&opts.CookieAuthenticationKeyPath, "cookie-authentication-key-file", "", "Authentication key used to sign cookies. Required when --user-auth is 'oidc', optional when 'openshift'.")
}

func (opts *SessionOptions) ApplyConfig(config *serverconfig.Session) {
Expand All @@ -50,9 +50,15 @@ func (opts *SessionOptions) Validate(userAuthType flagvalues.AuthType) []error {
if opts.CookieEncryptionKeyPath == "" || opts.CookieAuthenticationKeyPath == "" {
errs = append(errs, fmt.Errorf("cookie-encryption-key-file and cookie-authentication-key-file must be set when --user-auth is 'oidc'"))
}
case flagvalues.AuthTypeOpenShift:
bothSet := opts.CookieEncryptionKeyPath != "" && opts.CookieAuthenticationKeyPath != ""
neitherSet := opts.CookieEncryptionKeyPath == "" && opts.CookieAuthenticationKeyPath == ""
if !bothSet && !neitherSet {
errs = append(errs, fmt.Errorf("cookie-encryption-key-file and cookie-authentication-key-file must both be set or both be unset when --user-auth is 'openshift'"))
}
default:
if opts.CookieEncryptionKeyPath != "" || opts.CookieAuthenticationKeyPath != "" {
errs = append(errs, fmt.Errorf("cookie-encryption-key-file and cookie-authentication-key-file must not be set when --user-auth is not 'oidc'"))
errs = append(errs, fmt.Errorf("cookie-encryption-key-file and cookie-authentication-key-file must not be set when --user-auth is not 'oidc' or 'openshift'"))
}
}

Expand Down
159 changes: 159 additions & 0 deletions frontend/e2e/tests/console/session-persistence.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
import { test, expect } from '../../fixtures';
import { performLogin } from '../../setup/login-helper';

const CONSOLE_NAMESPACE = 'openshift-console';
const CONSOLE_DEPLOYMENT = 'console';

test.describe(
'Session persistence across pod restarts',
{ tag: ['@admin', '@slow'] },
() => {
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';
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

await test.step('Log in to the console', async () => {
const htpasswdUser = process.env.BRIDGE_HTPASSWD_USERNAME;
const htpasswdPass = process.env.BRIDGE_HTPASSWD_PASSWORD;
const htpasswdIdp = process.env.BRIDGE_HTPASSWD_IDP;

if (htpasswdUser && htpasswdPass) {
await performLogin(page, baseURL, htpasswdUser, htpasswdPass, htpasswdIdp);
} else {
const kubeadminPassword = process.env.BRIDGE_KUBEADMIN_PASSWORD;
test.skip(!kubeadminPassword, 'No credentials configured');
await performLogin(page, baseURL, 'kubeadmin', kubeadminPassword!, 'kube:admin');
}

await expect(page.getByTestId('user-dropdown-toggle')).toBeVisible({ timeout: 60_000 });
});

await test.step('Verify dashboard loads', async () => {
await page.goto(`${baseURL}/dashboards`, { waitUntil: 'domcontentloaded' });
await expect(page).toHaveTitle(/Overview/);
});

await test.step('Delete all console pods', async () => {
const pods = await k8sClient.getPods(CONSOLE_NAMESPACE);
const consolePods = pods.filter(
(p) => p.metadata?.labels?.['component'] === 'ui',
);

expect(consolePods.length).toBeGreaterThan(0);

for (const pod of consolePods) {
await k8sClient.deletePod(pod.metadata!.name!, CONSOLE_NAMESPACE);
}
});

await test.step('Wait for new console pods to be ready', async () => {
await k8sClient.waitForDeploymentReady(CONSOLE_DEPLOYMENT, CONSOLE_NAMESPACE, 180_000);
});
Comment on lines +50 to +52

@coderabbitai coderabbitai Bot Aug 5, 2026

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.

🎯 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/e2e

Repository: 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.ts

Repository: 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")
PY

Repository: 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.generation and wait for it to increase before calling waitForDeploymentReady. 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.


await test.step('Verify session persisted — no login redirect', async () => {
await page.goto(`${baseURL}/k8s/cluster/nodes`, {
waitUntil: 'domcontentloaded',
timeout: 60_000,
});

await expect(page.getByTestId('user-dropdown-toggle')).toBeVisible({ timeout: 30_000 });
await expect(page).toHaveTitle(/Nodes/);
expect(page.url()).not.toContain('oauth-openshift');
expect(page.url()).not.toContain('/login');
});
});

test('session survives console plugin toggle', async ({ page, k8sClient }) => {
const baseURL = process.env.WEB_CONSOLE_URL || page.url() || 'http://localhost:9000';

await test.step('Log in to the console', async () => {
const htpasswdUser = process.env.BRIDGE_HTPASSWD_USERNAME;
const htpasswdPass = process.env.BRIDGE_HTPASSWD_PASSWORD;
const htpasswdIdp = process.env.BRIDGE_HTPASSWD_IDP;

if (htpasswdUser && htpasswdPass) {
await performLogin(page, baseURL, htpasswdUser, htpasswdPass, htpasswdIdp);
} else {
const kubeadminPassword = process.env.BRIDGE_KUBEADMIN_PASSWORD;
test.skip(!kubeadminPassword, 'No credentials configured');
await performLogin(page, baseURL, 'kubeadmin', kubeadminPassword!, 'kube:admin');
}

await expect(page.getByTestId('user-dropdown-toggle')).toBeVisible({ timeout: 60_000 });
});

let pluginName: string | undefined;

await test.step('Find an enabled ConsolePlugin to toggle', async () => {
const consoleOperator = await k8sClient.customObjectsApi.getClusterCustomObject({
group: 'operator.openshift.io',
version: 'v1',
plural: 'consoles',
name: 'cluster',
});

const plugins: string[] =
(consoleOperator.body as any)?.spec?.plugins ?? [];
pluginName = plugins[0];
test.skip(!pluginName, 'No enabled ConsolePlugins found on this cluster');
});

await test.step('Disable the plugin via operator config', async () => {
const consoleOperator = await k8sClient.customObjectsApi.getClusterCustomObject({
group: 'operator.openshift.io',
version: 'v1',
plural: 'consoles',
name: 'cluster',
});

const currentPlugins: string[] =
(consoleOperator.body as any)?.spec?.plugins ?? [];
const updatedPlugins = currentPlugins.filter((p: string) => p !== pluginName);

await k8sClient.mergePatchResource(
'/apis/operator.openshift.io/v1/consoles/cluster',
{ spec: { plugins: updatedPlugins } },
);
});

await test.step('Wait for console rollout', async () => {
// The operator triggers a new rollout when plugin config changes
// Wait briefly for the rollout to start, then wait for it to complete
await page.waitForTimeout(10_000);
await k8sClient.waitForDeploymentReady(CONSOLE_DEPLOYMENT, CONSOLE_NAMESPACE, 180_000);
});

await test.step('Verify session persisted after plugin toggle', async () => {
await page.goto(`${baseURL}/dashboards`, {
waitUntil: 'domcontentloaded',
timeout: 60_000,
});

await expect(page.getByTestId('user-dropdown-toggle')).toBeVisible({ timeout: 30_000 });
await expect(page).toHaveTitle(/Overview/);
expect(page.url()).not.toContain('oauth-openshift');
expect(page.url()).not.toContain('/login');
});

await test.step('Re-enable the plugin', async () => {
const consoleOperator = await k8sClient.customObjectsApi.getClusterCustomObject({
group: 'operator.openshift.io',
version: 'v1',
plural: 'consoles',
name: 'cluster',
});

const currentPlugins: string[] =
(consoleOperator.body as any)?.spec?.plugins ?? [];
if (!currentPlugins.includes(pluginName!)) {
currentPlugins.push(pluginName!);
await k8sClient.mergePatchResource(
'/apis/operator.openshift.io/v1/consoles/cluster',
{ spec: { plugins: currentPlugins } },
);
}
});
});
},
);
2 changes: 2 additions & 0 deletions pkg/auth/oauth2/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,8 @@ func NewOAuth2Authenticator(ctx context.Context, config *Config) (*OAuth2Authent
consoleBaseAddress: c.ConsoleBaseAddress,
cookiePath: c.CookiePath,
secureCookies: c.SecureCookies,
cookieAuthenticationKey: c.CookieAuthenticationKey,
cookieEncryptionKey: c.CookieEncryptionKey,
constructOAuth2Config: a.oauth2ConfigConstructor,
}

Expand Down
2 changes: 2 additions & 0 deletions pkg/auth/oauth2/auth_oidc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ type oidcConfig struct {
consoleBaseAddress string
cookiePath string
secureCookies bool
cookieAuthenticationKey []byte
cookieEncryptionKey []byte
constructOAuth2Config oauth2ConfigConstructor
}

Expand Down
27 changes: 9 additions & 18 deletions pkg/auth/oauth2/auth_oidc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -965,14 +965,13 @@ func testOAuth2ConfigConstructor(endpointConfig oauth2.Endpoint) *oauth2.Config
}

type testCookieFactory struct {
cookieCodecs []securecookie.Codec
sessionToken *string
refreshToken *string
refreshTokenID *string
customCookies map[string]map[interface{}]interface{}
serverStore *sessions.SessionStore // needed to set up refresh token ID mapping
tokenVerifier sessions.IDTokenVerifier
signPayload func(string) string // function to sign an ID token payload
cookieCodecs []securecookie.Codec
sessionToken *string
refreshToken *string
customCookies map[string]map[interface{}]interface{}
serverStore *sessions.SessionStore
tokenVerifier sessions.IDTokenVerifier
signPayload func(string) string
}

func (f *testCookieFactory) WithSessionToken(sessionToken string) *testCookieFactory {
Expand Down Expand Up @@ -1002,26 +1001,18 @@ func (f *testCookieFactory) Complete(t testing.TB, req *http.Request) *http.Requ
f.cookieCodecs)
}
if f.refreshToken != nil {
var id string
if f.serverStore != nil && f.tokenVerifier != nil && f.signPayload != nil {
// Use AddSession to properly set up the state
token := addIDToken(
&oauth2.Token{RefreshToken: *f.refreshToken},
f.signPayload(`{"sub":"testuser","exp":`+strconv.FormatInt(time.Now().Add(5*time.Minute).Unix(), 10)+`}`),
)
loginState, err := f.serverStore.AddSession(f.tokenVerifier, token)
_, err := f.serverStore.AddSession(f.tokenVerifier, token)
require.NoError(t, err)
id = loginState.RefreshTokenID()
} else {
// Fallback to generating a random ID
id = randomString(32)
}
f.refreshTokenID = &id

// Store only the ID in the cookie
attachCookieOrDie(t, req, "openshift-refresh-token",
map[interface{}]interface{}{
"refresh-token-id": id,
"refresh-token": *f.refreshToken,
},
f.cookieCodecs)
}
Expand Down
36 changes: 24 additions & 12 deletions pkg/auth/oauth2/auth_openshift.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,19 +72,26 @@ func newOpenShiftAuth(ctx context.Context, k8sClient *http.Client, c *oidcConfig
}
o.oauthEndpointCache.Run(ctx)

authnKey, err := utils.RandomString(64)
if err != nil {
return nil, err
}

encryptionKey, err := utils.RandomString(32)
if err != nil {
return nil, err
var authnKey, encryptionKey []byte
if len(c.cookieAuthenticationKey) > 0 && len(c.cookieEncryptionKey) > 0 {
authnKey = c.cookieAuthenticationKey
encryptionKey = c.cookieEncryptionKey
} else {
authnKeyStr, err := utils.RandomString(64)
if err != nil {
return nil, err
}
encryptionKeyStr, err := utils.RandomString(32)
if err != nil {
return nil, err
}
authnKey = []byte(authnKeyStr)
encryptionKey = []byte(encryptionKeyStr)
}

o.sessions = sessions.NewSessionStore(
[]byte(authnKey),
[]byte(encryptionKey),
authnKey,
encryptionKey,
c.secureCookies,
c.cookiePath,
)
Expand Down Expand Up @@ -206,7 +213,12 @@ func (o *openShiftAuth) logout(w http.ResponseWriter, r *http.Request) {
return
}

// Delete the session
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)
}
}

Comment on lines +235 to +240

@coderabbitai coderabbitai Bot Aug 5, 2026 •
edited
Loading

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.

🔒 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.go

Repository: 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/sessions

Repository: 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.go

Repository: 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.

@TheRealJon TheRealJon Aug 11, 2026

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.

These seem like valid concerns, have you looked into them yet?

@coderabbitai coderabbitai Bot Aug 11, 2026

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.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- auth OpenShift structure and logout-related tests ---'
ast-grep outline pkg/auth/oauth2/auth_openshift.go --items all --type function --match 'logout|DeleteSession|getLoginState|refreshSession'
ast-grep outline pkg/auth/oauth2/auth_openshift_test.go --items all --type function --match 'logout|Logout|DeleteSession|refresh'

printf '%s\n' '--- session store structure ---'
ast-grep outline pkg/auth/sessions/combined_sessions.go --items all --type function --match 'DeleteSession|GetSession|GetCookieRefreshToken|ClearRecoveryCookie'

printf '%s\n' '--- logout and deletion references with context ---'
rg -n -C 8 -P '\b(?:logout|DeleteSession|OAuthAccessTokens\(\)\.Delete|OAuthAuthorizeTokens\(\)\.Delete|GetCookieRefreshToken)\s*\(' \
  pkg/auth/oauth2 pkg/auth/sessions \
  -g '*.go'

printf '%s\n' '--- bounded implementation slices ---'
sed -n '180,245p' pkg/auth/oauth2/auth_openshift.go
sed -n '200,300p' pkg/auth/sessions/combined_sessions.go

Length of output: 31672

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- candidate OpenShift authentication test files ---'
fd -t f -e go . pkg/auth/oauth2 | sort

printf '%s\n' '--- OpenShift auth construction and logout test references ---'
rg -n -C 10 -P '\b(?:newOpenShiftAuth|openShiftAuth|LogoutFunc|func\s+Test.*(?:OpenShift|Logout)|OAuthAuthorizeTokens|OAuthAccessTokens)\b' \
  pkg/auth/oauth2 -g '*_test.go'

printf '%s\n' '--- test helpers that provide HTTP/API-client behavior ---'
rg -n -C 8 -P '\b(?:httptest\.NewServer|RoundTripper|Transport:|http\.Client|NewForConfig|issuerURL|testValidRefreshToken)\b' \
  pkg/auth/oauth2 -g '*_test.go'

printf '%s\n' '--- complete local deletion and refresh-token lookup behavior ---'
sed -n '120,175p' pkg/auth/sessions/combined_sessions.go
sed -n '220,270p' pkg/auth/sessions/combined_sessions.go

Length of output: 1403

🏁 Script executed:

#!/bin/bash
set -u

printf '%s\n' '--- auth test file structure ---'
ast-grep outline pkg/auth/oauth2/auth_test.go --items all --type function
printf '%s\n' '--- bounded auth test source ---'
sed -n '1,260p' pkg/auth/oauth2/auth_test.go

printf '%s\n' '--- repository-wide OpenShift logout and API client test references ---'
rg -n -C 8 -P '\b(?:newOpenShiftAuth|openShiftAuth|OAuthAuthorizeTokens|OAuthAccessTokens|LogoutFunc|logout)\b' \
  -g '*_test.go' . || true

printf '%s\n' '--- HTTP transport fakes in authentication tests ---'
rg -n -C 8 -P '\b(?:RoundTripper|Transport|httptest\.NewServer|http\.Client|NewForConfig)\b' \
  pkg/auth -g '*_test.go' || true

printf '%s\n' '--- relevant auth construction ---'
sed -n '240,340p' pkg/auth/oauth2/auth.go

Length of output: 37782

@TheRealJon, yes. I reviewed the implementation, the session-store behavior, and the existing tests.

Both findings identify valid defects:

  1. If OAuthAccessTokens().Delete fails, logout returns before it calls DeleteSession and ClearRecoveryCookie. The encrypted refresh-token cookie remains available. getLoginState can use it to refresh or restore the session.

  2. If OAuthAuthorizeTokens().Delete fails, the remote refresh token can remain valid. The current handler logs the error and returns 204.

The first finding needs one correction. On a refresh-token revocation failure, the handler must not return before local cleanup. It must expire the local session, refresh-token cookie, and recovery cookie first. It should then return an error response to report that remote revocation failed.

The robust flow is:

  • Attempt both remote revocations.
  • Record any revocation error.
  • Always clear local session state and cookies.
  • Return 204 only if both remote revocations succeed.
  • Return 500 after local cleanup if either remote revocation fails.

There is no existing OpenShift logout test. pkg/auth/oauth2/auth_test.go only covers authenticator construction. The change should add failure-path tests that assert both the error response and local cookie/session cleanup.

You are interacting with an AI system.

o.sessions.DeleteSession(w, r)
w.WriteHeader(http.StatusNoContent)
}
Expand Down Expand Up @@ -234,7 +246,7 @@ func (o *openShiftAuth) refreshSession(ctx context.Context, w http.ResponseWrite
).Token()

if err != nil {
return nil, fmt.Errorf("failed to refresh a token %s: %w", cookieRefreshToken, err)
return nil, fmt.Errorf("failed to refresh a token: %w", err)
}

ls, err := o.sessions.UpdateTokens(w, r, nil, newTokens)
Expand Down
Loading