test(auth): add a SAML lane to the mock-IdP stack to reproduce #263 - #595
Draft
ian-flores wants to merge 5 commits into
Draft
test(auth): add a SAML lane to the mock-IdP stack to reproduce #263#595ian-flores wants to merge 5 commits into
ian-flores wants to merge 5 commits into
Conversation
The mock-IdP stack runs Connect and Workbench on separate hostnames behind a real Keycloak and is green daily, which shows the cross-hostname piggyback works for OIDC. The one variable #263 has that the stack does not is SAML. Add a second Workbench lane configured for SAML against the same Keycloak realm: a SAML client in the realm import, a Workbench image whose entrypoint sets auth-saml, a compose service behind a "saml" profile on its own hostname and port, and a certificate for it. SAML cannot run alongside PAM or OIDC on one instance, so it needs its own container rather than a toggle. The OIDC lane is untouched and remains the control. Add a workflow_dispatch-only diagnostic job that runs vip verify against the SAML lane, both Workbench-only and alongside Connect, and uploads both logs. The job is expected to fail, since it reproduces a bug, so it is continue-on-error and excluded from the status gate. Promote it to a normal lane once the fix makes it green. Refs #263
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an opt-in SAML “lane” to the existing mock-IdP E2E stack so VIP can reproduce and diagnose issue #263 (suspected Workbench auth sharing failure under SAML/Okta with separate hostnames), while keeping the existing OIDC lane unchanged as a control.
Changes:
- Adds a second Workbench container configured for SAML against the same Keycloak realm (new Keycloak SAML client + Workbench SAML entrypoint/Dockerfile + new compose service behind a
samlprofile). - Extends the TLS cert generation to include
workbench-saml.vip.test, plus a newjust mock-idp-saml-uphelper target. - Adds a
workflow_dispatch-only GitHub Actions diagnostic job that runs VIP against the SAML lane (Workbench-only, then Connect+Workbench) and uploads logs/artifacts regardless of failures.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
justfile |
Adds mock-idp-saml-up to start the stack with the SAML Workbench profile enabled. |
docker/workbench/entrypoint-saml.sh |
New entrypoint to configure Workbench SAML settings and trust the stack CA before starting supervisord. |
docker/workbench/Dockerfile.saml |
New thin wrapper image that installs the SAML entrypoint. |
docker/tls/gen-certs.sh |
Adds generation of workbench-saml leaf cert/key for the new hostname. |
docker/keycloak/realm-vip.json |
Adds a SAML client + mapper for the Workbench-SAML lane in the mock realm import. |
compose.mock-idp.yml |
Adds workbench-saml service under a saml profile and its volume/hostname wiring. |
.github/workflows/mock-idp-e2e.yml |
Adds a dispatch-only diagnostic job that runs VIP against the SAML lane twice and uploads artifacts/logs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+15
to
+17
| # A fourth hostname, workbench-saml.vip.test, is a second Workbench lane | ||
| # configured for SAML instead of OIDC against the same Keycloak realm. It | ||
| # exists to reproduce issue #263 (Workbench behind SAML on a separate |
…anly A SAML diagnostic run timed out after the IdP accepted credentials and MFA, and the error said only that login "did not complete" — not where the browser had ended up. That single missing fact is the difference between the IdP rejecting our request, the product rejecting the assertion, and a misconfigured test fixture, which are three different problems with three different fixes. Report the browser's final URL, the page title and the expected product origin at all three authentication timeout sites, guarding those reads so a crashed page cannot turn a timeout into a different error. Name the protocol actually in use rather than hardcoding OIDC, since the message appeared verbatim during a SAML flow. Derive the stated duration from the real deadline, which is scaled: under VIP_TIMEOUT_SCALE=2 the tool waited ten minutes and reported five. Raise AuthTimeoutError, a subclass of AuthConfigError, instead of a bare RuntimeError, and wrap the interactive-auth call site the way the headless one already was. Both modes previously escaped as a pytest INTERNALERROR with a traceback; the interactive path is the one issue #263 was reported on. Use one timeout constant for the IdP round trip. Workbench's wait was 2 minutes against the primary path's 5, which made it skip spuriously (#596). Refs #263
Contributor
|
Preview Links
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The lane could not support its own conclusion. Workbench had no local Unix account for the realm's test user, so rserver refused every sign-in before any SAML-specific path ran; the run reported "OIDC login did not complete" because cli.py hardcodes oidc whenever --idp is set with no vip.toml; and a genuine auth failure became a pytest skip, so the step reporting the #263 topology exited 0 while the login test never ran. Provision the account from entrypoint-saml.sh, add a --provider flag so the lane can declare saml, thread provider into _authenticate_workbench so its timeout names the protocol that actually ran, and assert in the workflow that the login tests executed rather than trusting the exit code. The workflow guard is temporary and cites #596, which fixes the exit-code contract properly. Also fixes gen-certs.sh regenerating only when the CA is missing, which left a volume warmed before workbench-saml was added without its leaf cert.
The stock Workbench image already ships /usr/local/bin/startup.sh, and its supervisord "rstudio-workbench" program runs it to create the rstudio user and exec rserver. Copying VIP's startup.sh over that path meant supervisord ran the user-creation script instead, which exits 0, so rserver never started and the container died after three retries.
This was referenced Aug 18, 2026
Collaborator
Author
|
Dispatching the mock-IdP lane on #601 turned up something that affects this PR's premise directly.
Separately, the missing local Unix account is confirmed as the cause of the SSO timeout, and #601 fixes it: in run 32172732884 The skip-guard added in this PR behaves correctly and would have caught the above immediately — it fails the step when the named login test does not execute. It will now fire every run until #602 is resolved, which is the guard doing its job rather than a fault in it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This does not fix #263. It builds the thing that lets us find out what #263 actually is.
Why
#263 says interactive-auth storage state captured for Connect does not authenticate Workbench on a separate ALB under SAML/Okta. But the
mock-idp-e2estack already runs Connect onconnect.vip.test:3939and Workbench onworkbench.vip.test:8787— separate hostnames — and it is green every day. So separate hostnames alone do not break us, and the issue's framing ("storage state does not carry over") is a customer's inference, not an observation we have reproduced.The one variable #263 has that the green stack does not is SAML. That matches samcofer's own note on the call that the problem may be SAML rather than OIDC handling.
What this adds
A second Workbench lane speaking SAML against the same Keycloak realm, changing exactly one variable:
samlclient in the realm import, SP entity IDhttps://workbench-saml.vip.test:8788/saml/metadata, ACS/saml/acs, and aUsernameattribute mapperentrypoint-saml.sh/Dockerfile.samlsettingauth-saml,auth-saml-metadata-urlandauth-saml-sp-attribute-usernameworkbench-samlcompose service behind asamlprofile, on its own hostname and port 8788, with a cert fromcert-initjust mock-idp-saml-upThe endpoints and the
Usernameattribute name are taken from the SAML SSO admin guide, not inferred. SP request signing is left off, per that guide: "In most situations, SAML authentication request signing is not required or even supported."SAML is exclusive of PAM/OIDC on a single instance, which is why this is a separate container rather than a toggle. The OIDC lane is untouched — it is the control in this experiment.
The diagnostic job
mock-idp-e2e-saml-diagnosticrunsvip verifyagainst the SAML lane twice: Workbench-only, then Connect + Workbench (the actual #263 topology). Both logs are uploaded regardless of where it breaks.This job is expected to be red. That is the point — it reproduces a bug. So it is
workflow_dispatch-only,continue-on-error: true, and excluded from thestatusgate, so it cannot block a merge. It should be promoted to a normal gated lane once the fix makes it green.Known limit
This probes
--headless-authonly. #263 was reported under--interactive-auth, which CI cannot run (it opens a headed browser and blocks). Both modes share_authenticate_workbench, where the leading hypothesis sits, so a red result here is real evidence — but a green result would not exonerate the interactive-only prompt path.Next
Dispatch the job. If the OIDC lane stays green and the SAML lane fails, #263 is cornered and the fix gets written against the trace. If both pass, the SAML hypothesis is wrong and we go back to the customer for a verbose trace rather than inventing a fix.
Refs #263