fix(auth): add dynamic auth providers - #151
Merged
Merged
Conversation
JessicaJHee
force-pushed
the
add-auth-provider
branch
from
August 21, 2026 16:41
b06a9b6 to
2d1660c
Compare
JessicaJHee
force-pushed
the
add-auth-provider
branch
from
August 21, 2026 19:38
2d1660c to
f178244
Compare
Signed-off-by: Jessica He <jhe@redhat.com>
JessicaJHee
force-pushed
the
add-auth-provider
branch
from
August 21, 2026 22:52
f178244 to
5c8efd7
Compare
jrichter1
approved these changes
Aug 22, 2026
gustavolira
added a commit
that referenced
this pull request
Aug 24, 2026
#151 merged today and claimed 2.1.10, so the renumber in the previous commit collided with main and the Version Bump Check would have refused it. Same number as before, different reason: this is no longer about #149, which now bumps nothing at all. Worth noting separately: main says 2.1.10 but the publish workflow for it failed, so npm latest is still 2.1.9. If that is not resolved before this merges, 2.1.11 publishes over a 2.1.10 that never existed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gustavolira
added a commit
that referenced
this pull request
Aug 24, 2026
The PR was DIRTY, which is why GitHub had stopped running checks on it — a pull request that cannot be merged gets no merge commit to test, so the last two pushes produced zero check runs and looked like CI hanging. Both conflicts were version bookkeeping from #151: package.json keeps 2.1.11, and its 2.1.10 section moves below ours in the changelog rather than competing for "Current". #151 also touched deployment.ts, which this branch changes, but git merged those cleanly and the suite is green. 141 tests pass, yarn check clean, docs build clean.
subhashkhileri
pushed a commit
that referenced
this pull request
Aug 26, 2026
…oting legacy (#152) * fix(nfs): make a lane that is not actually running NFS fail instead of pass Three defects found while triaging the overlay migration, all with the same shape: the harness cannot tell a working NFS lane from one that quietly fell back to the legacy shell. RHIDP-16457 — a workspace could disable the NFS it asked for. The NFS secret layer merges before the workspace's own tests/config/rhdh-secrets.yaml, so a workspace setting APP_CONFIG_app_packageName or ENABLE_STANDARD_MODULE_FEDERATION for its own reasons overrode it. Nothing then failed: the legacy suite re-ran and passed, and the only thing the lane existed to prove was never exercised. deploy() now checks the merged secret still carries both markers and throws naming the key, the value found, and the file responsible. The merge order is untouched — a workspace has to be able to override defaults, which is exactly why the outcome needs checking rather than assuming. Also from RHIDP-16457, and from RHIDP-16461: configure() now logs which of the three mechanisms decided the shell, because none of them is visible from a single file and the answer currently requires reading a project name, an environment variable and a configure() call together. A namespace ending in -app-next that was explicitly configured with useNewFrontendSystem: false warns, since it deploys legacy under a name that reads as NFS everywhere. Only that direction is checked: enabling NFS without the suffix is legitimate and is what github and homepage do. RHIDP-16459 — the GitHub session file was a bare relative authState_<user>.json, resolved against process.cwd(), which the worker fixture sets to the same workspace directory for every project. A workspace's lanes shared one file per user, unlocked: one lane could inject another's storage state, and a reader landing mid-write died on truncated JSON as a flake that looked nothing like the plugin under test. Every lane added is another writer. The path is now absolute and keyed by project as well as user, the write goes through a temp file and a rename, and an unusable session falls through to a full login rather than throwing. RHIDP-16458 — SidebarTabs hardcoded "Self-service", which is the legacy shell's scaffolder title; under app-next the page is "Create", so a spec running there could not type-check without a cast. Both are members now. Deliberately only that: which label a lane should use is RHIDP-16462, three workarounds are in flight for it, and picking one here would be a fourth. All four new behaviours are mutation-tested — reverting each turns the matching tests red and nothing else. 136 tests pass; typecheck, eslint and prettier are clean; the docs build validates its own links. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(nfs): the session fix would have caused the failure it prevents Review found two of the three changes wrong in ways the tests did not reach. Keying the GitHub session file per project removed cross-lane sharing, which is what the ticket asked for — and would have made every lane of a workspace log in to the same GitHub account concurrently. logintoGithub derives its 2FA code from one shared TOTP secret, so lanes starting inside the same 30-second window submit the identical code and GitHub rejects the second. This file already carries retry handling for exactly that error, which is how it is known to happen; bulk-import would have gone from one login to three simultaneous ones. So the file is one per user again, and what was actually missing is added instead: a lock spanning read-or-login-and-write, not just the write, because two lanes that both decide there is no session go on to log in anyway. RHDH cookies from another lane were never the hazard — each lane's RHDH lives on its own namespace hostname. The [nfs] line was logged from configure(), but the worker fixture calls configure() with no arguments for every project before any spec runs. For github and homepage — the two lanes the change was written for, which opt in through configure({ useNewFrontendSystem: true }) — the first line said "off, from nothing", and the first line is the one a reader greps. Reported from deploy() now, once, after configuration is final. Two smaller ones. The guard accepted an unquoted YAML `true` by coercing in the comparison only, so it green-lit a payload the API server rejects with "cannot unmarshal bool into Go struct field ... of type string" — an opaque k8s failure in place of the guard's own message. It now rewrites the value as well. And the temp file survived a failed storageState write, accumulating untracked authState*.tmp files in a directory nothing gitignores them from. New tests for the lock, both write outcomes, and the boolean rewrite; the serialisation test waits for the first holder rather than racing two callers from the same tick, which was flaky. Mutation-tested: removing the lock, the temp-file cleanup, or the in-place coercion each turns exactly one test red. 141 pass, five consecutive runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: take 2.1.10, now that #149 claims no version #149 was reduced to a documentation change, so it no longer bumps the version and 2.1.10 is free. The note about the two PRs colliding goes with it — there is only one version-bumping PR open now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: back to 2.1.11 — main took 2.1.10 #151 merged today and claimed 2.1.10, so the renumber in the previous commit collided with main and the Version Bump Check would have refused it. Same number as before, different reason: this is no longer about #149, which now bumps nothing at all. Worth noting separately: main says 2.1.10 but the publish workflow for it failed, so npm latest is still 2.1.9. If that is not resolved before this merges, 2.1.11 publishes over a 2.1.10 that never existed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(nfs): the deployment guide said the workspace secret always wins It does not any more, and the guide said it in as many words: "your rhdh-secrets.yaml still wins on conflicts". True for every key except the two NFS markers, where overriding now throws instead of quietly deploying the legacy shell — so the sentence this PR contradicts is corrected in the same change rather than left to be discovered from a stack trace. Also documented, because a new hard failure with no docs costs someone an hour: the `[nfs]` line and what to grep for when a lane behaves like the wrong shell, why it comes from deploy() rather than configure(), the exact throw and the fact that it is a post-condition on the caller's own config, and the -app-next namespace warning together with why the reverse is not checked. The dynamic-plugins layer is named as the unguarded one. Disabling app-auth or app-integrations from a workspace's own dynamic-plugins.yaml degrades the lane the same silent way, and nothing catches it. No workspace does that today — 14 ship their own dynamic-plugins.yaml and none names either package — so a second guard would be insurance against something nobody does, which is the argument that just closed rhdh-plugin-export-overlays#3375. Documented instead, and it is the first place to look when an NFS lane behaves like the legacy shell. deploy()'s step list gains both the report and the throw. 141 tests pass, yarn check clean, docs build clean. * fix(nfs): fail the lane on an intent conflict instead of warning about it The guard found the conflict and then let the run continue. A lane named <ws>-app-next configured with useNewFrontendSystem: false deployed the legacy shell, re-ran the legacy suite, passed, and left one console.warn behind on a run that exited 0 — which is indistinguishable from a run that had nothing to say. That is the exact failure this module exists to stop, and the PR's own title claims it does. assertNfsMarkersSurvived already throws for the other path, where a workspace's own rhdh-secrets.yaml overwrites the markers. The two paths now behave the same. describeNfsIntentConflict stays a pure describer with its existing tests; assertNfsIntentMatches is a thin wrapper over it so the throwing behaviour is directly testable rather than reachable only through a private method on RHDHDeployment. The [nfs] line is logged before the check so the resolved state is on record even when the next line ends the run. Mutation-verified: turning the throw back into a return fails "fails the deploy on the conflict, rather than only naming it" and nothing else. Closes the library half of RHIDP-16457 — bulk-import's local expect(rhdh.deploymentConfig.useNewFrontendSystem).toBe(true) was stronger than the library guard until now, which is the inverse of what that ticket asks for. * fix(github-session): lock only session creation, and scope this PR to that Two changes from review. Lock scope. The lock spanned read-or-login-and-write, so the reuse path — addCookies, goto, Sign In, wait for nav or popup — ran inside it too. Reuse needs no exclusivity: the lanes are different namespace hosts and only creation submits a TOTP. Every loginAsGithubUser therefore queued behind a sign-in it did not need, and a waiter could exhaust Playwright's default test timeout before proper-lockfile's own retries ran out, because test.setTimeout(260_000) is raised inside logintoGithub — precisely the path a waiter is not on. ensureGithubSession now checks for a session outside the lock and takes it only to create one, re-reading inside so a caller that queued behind the lane that created it reuses that session rather than logging in again with the same TOTP code. loginAsGithubUser splits into _reuseGithubSession and _createGithubSession; creation already leaves the page signed in, so the reuse path is not replayed after it. Mutation-verified, one test each: dropping the outer check fails "reuses an existing session without taking the lock"; dropping the inner re-read fails "creates once when two callers find no session at the same time". The first version of that first test was wrong and passed under the mutation — it started ensureGithubSession in the same tick as the lock holder, so it raced the scheduler rather than the lock and the result depended on how loaded the run was. It now waits until the lock is demonstrably held, and is bounded by a timer so reintroducing the bug fails instead of deadlocking CI. Scope. The NFS work is removed — nfs-guard, the deploy-time report and marker check, the SidebarTabs label, and the docs for them. With the old frontend coming out there is no legacy shell to silently fall back into, so guarding against it earns little and would be deleted in the cleanup anyway. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Summary
auth:would boot without a sign-in provider.dynamic-plugins.yaml; the Keycloak profile addsoidc-provider(sign-in stays stock OIDC against the Keycloak IdP).Next Steps plan