From 5eb2c2dc8d1f126844e52c1f91f42d6fbc2741b5 Mon Sep 17 00:00:00 2001 From: Ian Flores Siaca <18703558+ian-flores@users.noreply.github.com> Date: Tue, 8 Sep 2026 08:56:54 -0700 Subject: [PATCH 1/5] fix(workbench): exercise the real SSO login path in test_workbench_login test_workbench_login skipped whenever auth_provider != "password", so no lane ever verified a Workbench login under SSO/OIDC. Reuse the silent-SSO round-trip workbench_login already implements (and the sign-out scenario already relies on): thread auth_provider, interactive_auth, auth_mode, and workbench_auth_error through the login step, and only strip the pre-loaded IdP session for password auth so SSO deployments keep what the round-trip needs. Closes #602. --- src/vip_tests/workbench/test_auth.py | 50 +++++++++++++++++++--------- 1 file changed, 34 insertions(+), 16 deletions(-) diff --git a/src/vip_tests/workbench/test_auth.py b/src/vip_tests/workbench/test_auth.py index ad5327f2..c001a1fc 100644 --- a/src/vip_tests/workbench/test_auth.py +++ b/src/vip_tests/workbench/test_auth.py @@ -53,23 +53,32 @@ def _restore_session_after_signout(page: Page, workbench_url: str): @pytest.fixture -def page(request: pytest.FixtureRequest, browser: Browser, browser_context_args: dict): +def page( + request: pytest.FixtureRequest, + browser: Browser, + browser_context_args: dict, + auth_provider: str, +): """Override the default page fixture for the login-form test only. - The login scenario must genuinely exercise the password login form, so it - needs a *logged-out* context: storage_state (injected by --interactive-auth - / --headless-auth) is stripped. Every other test in this module — notably - the sign-out scenario — must stay *logged in* via that session, so they - keep storage_state. Stripping it for sign-out would leave the browser - anonymous and, under SSO, unable to re-authenticate (no password), so the - "I am logged in" precondition could never be met. + The login scenario must genuinely exercise the password login form, so + under password auth it needs a *logged-out* context: storage_state + (injected by --interactive-auth / --headless-auth) is stripped. Under + SSO/OIDC, storage_state instead carries the pre-loaded IdP session that + workbench_login's silent SSO round-trip depends on, so it must stay -- + stripping it would leave the browser with no IdP session to reuse. Every + other test in this module — notably the sign-out scenario — must stay + *logged in* via that session regardless of auth provider, so they keep + storage_state too. All other context args (TLS, CA bundle, etc.) are preserved so this page behaves consistently with the rest of the suite. The autouse _cleanup_sessions fixture in workbench/conftest.py uses this same page, keeping cleanup and execution in the same context. """ - strip_storage_state = request.node.name.startswith("test_workbench_login") + strip_storage_state = ( + request.node.name.startswith("test_workbench_login") and auth_provider == "password" + ) args = { k: v for k, v in browser_context_args.items() @@ -84,11 +93,7 @@ def page(request: pytest.FixtureRequest, browser: Browser, browser_context_args: @given("Workbench is accessible at the configured URL") -def workbench_accessible(workbench_client, auth_provider: str): - # This test only validates password-based login form flow - if auth_provider != "password": - pytest.skip(f"test_auth only supports password auth, not {auth_provider!r}") - +def workbench_accessible(workbench_client): assert workbench_client is not None, "Workbench client not configured" status = workbench_client.health() assert status < 400, f"Workbench health-check returned HTTP {status}" @@ -100,9 +105,22 @@ def navigate_and_login( workbench_url: str, test_username: str, test_password: str, + auth_provider: str, + interactive_auth: bool, + auth_mode: str, + workbench_auth_error: str | None, ): - """Log in using password auth form.""" - workbench_login(page, workbench_url, test_username, test_password) + """Log in using password auth form, or the real SSO round-trip under SSO/OIDC.""" + workbench_login( + page, + workbench_url, + test_username, + test_password, + auth_provider, + interactive_auth, + auth_mode=auth_mode, + workbench_auth_error=workbench_auth_error, + ) @then("the Workbench homepage is displayed") From 90c81554caea804c7b7ac146503b006faa6fc570 Mon Sep 17 00:00:00 2001 From: Ian Flores Siaca <18703558+ian-flores@users.noreply.github.com> Date: Tue, 8 Sep 2026 09:27:07 -0700 Subject: [PATCH 2/5] fix(workbench): recognize an active session as already logged in workbench_login's fast path only checked for the homepage's own chrome, so a valid session cookie that redirects straight into a running session's IDE view (as CI's mock-idp-e2e lane just proved happens under --headless-auth) was invisible to it. It fell through to the password-form retry loop and failed with "Login failed after 3 attempts" despite the user being authenticated the whole time. Handle it the same way test_sessions.py already does when navigating back from a session: detect "/s/" in the URL and go to /home. --- src/vip_tests/workbench/conftest.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/vip_tests/workbench/conftest.py b/src/vip_tests/workbench/conftest.py index 2e12373f..e8d112ce 100644 --- a/src/vip_tests/workbench/conftest.py +++ b/src/vip_tests/workbench/conftest.py @@ -800,6 +800,17 @@ def workbench_login( if homepage_logo.is_visible(): return + # A valid session cookie can redirect straight into a running session's IDE + # view instead of the homepage -- that view has none of Homepage's chrome, so + # the check above misses it and the login-page probe below also misses it + # (it's neither a login page nor the homepage). Same case test_sessions.py + # handles when navigating back from a session: go to /home explicitly. + if "/s/" in page.url: + page.goto(f"{workbench_url}/home") + page.wait_for_load_state("load") + if homepage_logo.is_visible(): + return + # Check if we landed on a login/IdP page if _on_login_page(page.url): # The sign-in page renders client-side after ``load``; wait once for From 5c8be8fcd0774fe490b15e534d0125b495f91311 Mon Sep 17 00:00:00 2001 From: Ian Flores Siaca <18703558+ian-flores@users.noreply.github.com> Date: Tue, 8 Sep 2026 09:39:17 -0700 Subject: [PATCH 3/5] debug(workbench): trace page state in the workbench_login SSO path Temporary instrumentation to see why the /s/-detection fix for #602 didn't resolve the mock-idp-e2e failure. Not for merge. --- src/vip_tests/workbench/conftest.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/vip_tests/workbench/conftest.py b/src/vip_tests/workbench/conftest.py index e8d112ce..210a7799 100644 --- a/src/vip_tests/workbench/conftest.py +++ b/src/vip_tests/workbench/conftest.py @@ -795,9 +795,11 @@ def workbench_login( page.goto(workbench_url) page.wait_for_load_state("load") + print(f"DEBUG_602: after initial goto, url={page.url!r}", flush=True) # Fast path: already logged in (common with interactive_auth)? if homepage_logo.is_visible(): + print("DEBUG_602: homepage_logo visible on initial goto, returning", flush=True) return # A valid session cookie can redirect straight into a running session's IDE @@ -806,8 +808,16 @@ def workbench_login( # (it's neither a login page nor the homepage). Same case test_sessions.py # handles when navigating back from a session: go to /home explicitly. if "/s/" in page.url: + print("DEBUG_602: '/s/' branch taken, navigating to /home", flush=True) page.goto(f"{workbench_url}/home") page.wait_for_load_state("load") + print( + f"DEBUG_602: after /home goto, url={page.url!r} " + f"homepage_logo.is_visible={homepage_logo.is_visible()} " + f"title={page.title()!r}", + flush=True, + ) + print(f"DEBUG_602: body snippet={page.locator('body').inner_text()[:500]!r}", flush=True) if homepage_logo.is_visible(): return From 691c28a07df4b823c077c18831cd474e8b6ef5fd Mon Sep 17 00:00:00 2001 From: Ian Flores Siaca <18703558+ian-flores@users.noreply.github.com> Date: Tue, 8 Sep 2026 10:30:23 -0700 Subject: [PATCH 4/5] debug(workbench): embed a page-state trail in the login failure message print() doesn't survive VIP's custom terminal reporter (no "Captured stdout" section appears even on failure). Accumulate the debug trail and put it directly in the AssertionError text instead, since that renders in full every time. --- src/vip_tests/workbench/conftest.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/vip_tests/workbench/conftest.py b/src/vip_tests/workbench/conftest.py index 210a7799..a9dab479 100644 --- a/src/vip_tests/workbench/conftest.py +++ b/src/vip_tests/workbench/conftest.py @@ -793,13 +793,14 @@ def workbench_login( "Pass --interactive-auth or --headless-auth to pre-load browser storage state." ) + _debug_602: list[str] = [] + page.goto(workbench_url) page.wait_for_load_state("load") - print(f"DEBUG_602: after initial goto, url={page.url!r}", flush=True) + _debug_602.append(f"after initial goto, url={page.url!r}") # Fast path: already logged in (common with interactive_auth)? if homepage_logo.is_visible(): - print("DEBUG_602: homepage_logo visible on initial goto, returning", flush=True) return # A valid session cookie can redirect straight into a running session's IDE @@ -808,16 +809,14 @@ def workbench_login( # (it's neither a login page nor the homepage). Same case test_sessions.py # handles when navigating back from a session: go to /home explicitly. if "/s/" in page.url: - print("DEBUG_602: '/s/' branch taken, navigating to /home", flush=True) page.goto(f"{workbench_url}/home") page.wait_for_load_state("load") - print( - f"DEBUG_602: after /home goto, url={page.url!r} " + _debug_602.append( + f"after /home goto, url={page.url!r} " f"homepage_logo.is_visible={homepage_logo.is_visible()} " - f"title={page.title()!r}", - flush=True, + f"title={page.title()!r} " + f"body={page.locator('body').inner_text()[:300]!r}" ) - print(f"DEBUG_602: body snippet={page.locator('body').inner_text()[:500]!r}", flush=True) if homepage_logo.is_visible(): return @@ -927,6 +926,10 @@ def workbench_login( try: login_form.wait_for(state="visible", timeout=TIMEOUT_QUICK) except Exception: + _debug_602.append( + f"attempt {attempt}: login_form not visible, url={page.url!r} " + f"title={page.title()!r} body={page.locator('body').inner_text()[:300]!r}" + ) continue # Fill and submit @@ -958,7 +961,7 @@ def workbench_login( raise AssertionError(f"Login failed: {error_text or 'Unknown error'}") # Transient error (e.g., rate limit) - retry - raise AssertionError(f"Login failed after {max_retries} attempts") + raise AssertionError(f"Login failed after {max_retries} attempts. DEBUG_602: {_debug_602}") # --------------------------------------------------------------------------- From d1a96b671c5534cc3dd39214b0669a6d74985db3 Mon Sep 17 00:00:00 2001 From: Ian Flores Siaca <18703558+ian-flores@users.noreply.github.com> Date: Tue, 8 Sep 2026 10:42:22 -0700 Subject: [PATCH 5/5] fix(ci): stop pinning mock-idp-e2e's release leg to the stale latest tag rstudio/rstudio-workbench:latest hasn't updated since 2022-08-19 (a pre-Posit-rebrand build with no #posit-logo element), while jammy rolls forward with every release and was current as of yesterday. Use jammy for the release leg instead, so it actually tracks newest without a manual bump. Also drops the DEBUG_602 diagnostic instrumentation added while investigating this -- its job is done. --- .github/workflows/mock-idp-e2e.yml | 8 +++++--- src/vip_tests/workbench/conftest.py | 15 +-------------- 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/.github/workflows/mock-idp-e2e.yml b/.github/workflows/mock-idp-e2e.yml index 351de9e8..2df3daed 100644 --- a/.github/workflows/mock-idp-e2e.yml +++ b/.github/workflows/mock-idp-e2e.yml @@ -126,13 +126,15 @@ jobs: # RSW_VERSION is the Workbench image tag baked into the compose build # (FROM rstudio/rstudio-workbench:${RSW_VERSION}). `release` maps to the - # `latest` tag — the stack's existing default — so the PR/newest leg is - # unchanged; pinned versions map to the `jammy-` tags. + # bare `jammy` tag, which Docker Hub rolls forward with every release, so + # the PR/newest leg always tracks current without a manual bump; `latest` + # is stale (last updated 2022-08-19, years behind `jammy`) and must not be + # used. Pinned versions map to the `jammy-` tags. - name: Start mock-IdP stack (Keycloak + Connect + Workbench) env: RSC_LICENSE: ${{ secrets.CONNECT_LICENSE }} RSW_LICENSE: ${{ secrets.WORKBENCH_LICENSE }} - RSW_VERSION: ${{ matrix.workbench-version == 'release' && 'latest' || format('jammy-{0}', matrix.workbench-version) }} + RSW_VERSION: ${{ matrix.workbench-version == 'release' && 'jammy' || format('jammy-{0}', matrix.workbench-version) }} run: docker compose -f compose.mock-idp.yml up -d --build --wait # The default `docker` driver can't export to the GHA cache backend diff --git a/src/vip_tests/workbench/conftest.py b/src/vip_tests/workbench/conftest.py index a9dab479..e8d112ce 100644 --- a/src/vip_tests/workbench/conftest.py +++ b/src/vip_tests/workbench/conftest.py @@ -793,11 +793,8 @@ def workbench_login( "Pass --interactive-auth or --headless-auth to pre-load browser storage state." ) - _debug_602: list[str] = [] - page.goto(workbench_url) page.wait_for_load_state("load") - _debug_602.append(f"after initial goto, url={page.url!r}") # Fast path: already logged in (common with interactive_auth)? if homepage_logo.is_visible(): @@ -811,12 +808,6 @@ def workbench_login( if "/s/" in page.url: page.goto(f"{workbench_url}/home") page.wait_for_load_state("load") - _debug_602.append( - f"after /home goto, url={page.url!r} " - f"homepage_logo.is_visible={homepage_logo.is_visible()} " - f"title={page.title()!r} " - f"body={page.locator('body').inner_text()[:300]!r}" - ) if homepage_logo.is_visible(): return @@ -926,10 +917,6 @@ def workbench_login( try: login_form.wait_for(state="visible", timeout=TIMEOUT_QUICK) except Exception: - _debug_602.append( - f"attempt {attempt}: login_form not visible, url={page.url!r} " - f"title={page.title()!r} body={page.locator('body').inner_text()[:300]!r}" - ) continue # Fill and submit @@ -961,7 +948,7 @@ def workbench_login( raise AssertionError(f"Login failed: {error_text or 'Unknown error'}") # Transient error (e.g., rate limit) - retry - raise AssertionError(f"Login failed after {max_retries} attempts. DEBUG_602: {_debug_602}") + raise AssertionError(f"Login failed after {max_retries} attempts") # ---------------------------------------------------------------------------