fix(login): honor SSO nosplash for unauthenticated visitors

[cweibel]

Problem

The /login page only auto-redirects to the identity provider when a valid session already exists. With SSO configured using the nosplash option, a first visit with no session stops on the login form showing a Sign In button, instead of redirecting straight through to the IdP.

The existing nosplash handling lives inside the "already logged in" branch of LoginPageComponent.ngOnInit (guarded by auth.loggedIn && sessionData.valid), so the unauthenticated first-visit case is never covered.

Change

Add an unauthenticated nosplash path in LoginPageComponent.ngOnInit that mirrors the existing logged-in redirect: when there is no valid session but sessionData.ssoOptions contains nosplash (and no SSO_Message error is present), it triggers the existing SSO login flow (doSSOLoginReactive()), sending the user straight to the IdP.

• Reuses existing imports/helpers only (filter, take, switchMap, queryParamMap, appReady$, doSSOLoginReactive) — no new dependencies.
• !queryParamMap().SSO_Message guard preserves error-message display and avoids redirect loops.
• take(1) + appReady$ match the sibling subscription's pattern.

Tests

Added unit tests in login-page.component.spec.ts:

• Positive: invalid session + ssoOptions: 'nosplash,logout' → doSSOLoginReactive called once.
• Negative: invalid session + no SSO → doSSOLoginReactive not called (login form shown).

Verified locally:

• vitest run --project=core on the login specs → passing.
• eslint clean on the changed files.

Verification transcript

$ eslint login-page.component.ts login-page.component.spec.ts
  (rc=0)

$ vitest run --project=core .../login-page.component.spec.ts .../logout-page.component.spec.ts
  Test Files  2 passed (2)
       Tests  4 passed (4)

Notes

• While validating, I hit a pre-existing, unrelated core suite failure in StratosBrandingService > resetTheme() (test localStorage mock returns '' vs. asserted null). Filed separately as Test failure: StratosBrandingService resetTheme() spec expects null but test localStorage mock returns '' #5677 — not touched by this PR.

---

Implemented with AI assistance (OpenCode); reviewed by a human contributor per Stratos AI contribution guidance. Commit is DCO signed-off.

[norman-abramovitz]

@cweibel

Can you fix the spec test or should I

https://github.com/cloudfoundry/stratos/actions/runs/29917476492/job/88914655817?pr=5678

[cweibel]

@cweibel

Can you fix the spec test or should I

https://github.com/cloudfoundry/stratos/actions/runs/29917476492/job/88914655817?pr=5678

I got it, pulling out the ngrx bits and refactoring

[norman-abramovitz]

Traced this through the redirect-loop protection — there's a gap worth closing before merge, plus a readability tidy on the new filter.

Redirect-loop guard. The new unauthenticated path fires doSSOLoginReactive() on the !loggedIn && !sessionData.valid state — exactly the state a browser lands in when it comes back from an SSO round-trip that didn't establish a session. That path has no loop protection:

• doSSOLoginReactive() does an unconditional hard redirect (window.open('/pp/v1/auth/sso_login…', '_self')) and never touches redirectAttempts.
• The counter guard (redirectAttempts++ / bail over MAX_REDIRECT_ATTEMPTS) only lives in handleSuccessfulLogin, which the SSO branch returns before reaching.
• take(1) doesn't help across page loads — each IdP round-trip is a full navigation, so we return to a fresh component instance and take(1) resets. The sessionStorage redirectAttempts counter is the only thing that survives, and this path neither reads nor bumps it.

Failure case: the IdP returns to /login with no Stratos session and no SSO_Message (silent auth failure, cookie/config issue, or the back button) → the filter matches again → redirect again, with nothing to break it. The !queryParamMap().SSO_Message guard only catches the error-message case. Given MAX_REDIRECT_ATTEMPTS = 2 already exists here, these round-trips evidently do fail to seat a session sometimes.

The existing logged-in nosplash branch is counter-free too, but it's inherently safe because it only fires on an already-valid session; the new path drops that precondition without replacing the guard.

One wrinkle if you add the counter check: a pure-SSO success never runs clearRedirectAttempts() (that's inside handleSuccessfulLogin, which the SSO branch skips), so the counter wouldn't reset on a good login and a later genuine visit could inherit a stale count. Worth deciding where the reset lives — probably clear it once the session comes back valid. This is the part that really wants a test in a live nosplash SSO setup, since the round-trip can't be reproduced here.

Rest looks correct — the new filter is mutually exclusive with the logged-in branch (no double-fire), queryParamMap() at emit time is fine for a load-time param, and the tests assert real behavior. See the inline suggestion for the guard plus a readability pass on the predicate.

[norman-abramovitz]

The predicate is hard to scan as a flat && chain. Naming each condition makes the rule read at a glance, and it's the natural place to fold in the loop guard (underRedirectCap in the filter + redirectAttempts++ before the redirect). This also swaps .indexOf('nosplash') >= 0 for .includes(...) and uses ?. to drop the !!sessionData noise:

Suggested change

	this.auth$.pipe(
	filter(auth =>
	!auth.loggingIn &&
	!auth.verifying &&
	!auth.loggedIn &&
	!!auth.sessionData &&
	!auth.sessionData.valid &&
	!!auth.sessionData.ssoOptions &&
	auth.sessionData.ssoOptions.indexOf('nosplash') >= 0 &&
	!queryParamMap().SSO_Message
	),
	take(1), // Only trigger once on init
	switchMap(() => this.appReady$) // Wait for app to be stable before navigating
	).subscribe(() => {
	this.doSSOLoginReactive().subscribe();
	});
	this.auth$.pipe(
	filter(auth => {
	const idle = !auth.loggingIn && !auth.verifying;
	const noValidSession = !auth.loggedIn && !!auth.sessionData && !auth.sessionData.valid;
	const nosplash = !!auth.sessionData?.ssoOptions?.includes('nosplash');
	const showingError = !!queryParamMap().SSO_Message;
	const underRedirectCap = this.redirectAttempts <= this.MAX_REDIRECT_ATTEMPTS;
	return idle && noValidSession && nosplash && !showingError && underRedirectCap;
	}),
	take(1), // Only trigger once per component instance
	switchMap(() => this.appReady$) // Wait for app to be stable before navigating
	).subscribe(() => {
	this.redirectAttempts++; // survives the IdP round-trip via sessionStorage; breaks a silent redirect loop
	this.doSSOLoginReactive().subscribe();
	});

The guard values still want a test against a real nosplash IdP (loop break + counter reset on success).

If you'd rather, the whole predicate could move into a named method — reads even cleaner, and it lets you dedupe against the existing logged-in nosplash check above (lines 240–242 run the same nosplash + !SSO_Message test):

      filter(auth => this.shouldAutoRedirectNosplash(auth)),

  private shouldAutoRedirectNosplash(auth: AuthState): boolean {
    const idle           = !auth.loggingIn && !auth.verifying;
    const noValidSession = !auth.loggedIn && !!auth.sessionData && !auth.sessionData.valid;
    const nosplash       = !!auth.sessionData?.ssoOptions?.includes('nosplash');
    return idle && noValidSession && nosplash
      && !queryParamMap().SSO_Message
      && this.redirectAttempts <= this.MAX_REDIRECT_ATTEMPTS;
  }

That version touches the working logged-in branch if you dedupe, so it's a judgment call — the inline suggestion above keeps the change scoped to your new code.

[cweibel]

@norman-abramovitz thanks for the careful trace — you're right that the unauthenticated path dropped the loop protection the logged-in branch got for free. Pushed a fix (42775c9) addressing each point:

Redirect-loop guard. The new path now gates on the persisted redirectAttempts counter and bumps it before redirecting:

this.auth$.pipe(
  filter(auth => this.shouldAutoRedirectNosplash(auth)),
  take(1),
  switchMap(() => this.appReady$)
).subscribe(() => {
  this.redirectAttempts++;  // survives the IdP round-trip via sessionStorage; breaks a silent loop
  this.doSSOLoginReactive().subscribe();
});

Since redirectAttempts is backed by sessionStorage, it survives the full-navigation IdP round-trip that take(1) cannot. After MAX_REDIRECT_ATTEMPTS the filter stops matching, so a silent auth failure (cookie/config issue, back button) no longer loops.

Counter reset. Good catch on the pure-SSO success skipping handleSuccessfulLogin (where the reset lived). I put the reset on the valid-session transition in the logged-in branch, right before the normal redirect, so a genuine login resets the count and a later visit won't inherit a stale one:

// A valid session was established... clear the nosplash redirect counter
this.clearRedirectAttempts();
await this.handleSuccessfulLogin(auth);

Readability. Moved the predicate into a named method as you suggested, with ?.includes('nosplash') and dropping the !!sessionData noise:

private shouldAutoRedirectNosplash(auth: AuthState): boolean {
  const idle           = !auth.loggingIn && !auth.verifying;
  const noValidSession = !auth.loggedIn && !!auth.sessionData && !auth.sessionData.valid;
  const nosplash       = !!auth.sessionData?.ssoOptions?.includes('nosplash');
  return idle && noValidSession && nosplash
    && !queryParamMap().SSO_Message
    && this.redirectAttempts <= this.MAX_REDIRECT_ATTEMPTS;
}

I kept it scoped to the new code and did not dedupe against the logged-in nosplash check (240–242) — per your note that touching the working branch is a judgment call, I left it untouched to keep the blast radius small. Happy to dedupe in a follow-up if you'd prefer.

Tests. Added specs for the guard: SSO_Message present → no redirect, redirect-cap exceeded → no redirect, and the happy path now asserts the counter bumps to 1. Full core login spec is green (11 passed) and eslint is clean.

Live validation. Agreed the loop-break + counter-reset really wants a real nosplash SSO IdP — the full-navigation round-trip can't be reproduced in unit tests. I'll validate against our dev cloud.gov deployment and report back here.

[cweibel]

Heads-up on an out-of-scope build fix I folded into this branch to unblock CI/dev deployment (happy to split into its own PR if you'd prefer):

fix(build): checksums script fails on CF-only release (a77c6d1)

build/create-checksums.sh globbed *.tar.gz *.zip unconditionally. A CF-only release (make release cf, which our downstream deploy pipeline runs) stages only a .zip into dist/release/ — the .tar.gz archives are GitHub-release artifacts. Under set -euo pipefail the unmatched *.tar.gz glob stayed literal, sha256sum returned non-zero, and pipefail failed the _release.checksums target:

[checksums] Computing checksums for 1 archives...
make: *** [Makefile:797: _release.checksums] Error 1

Fix uses nullglob + an explicit archive array so only existing archives are checksummed. Verified a CF-only dist/release/ now generates + verifies SHA256SUMS and exits 0.

It's unrelated to the login change, but it blocked building this branch for our dev validation, so it's included here. Let me know if you'd like it pulled out into a separate PR.

[norman-abramovitz]

LGTM - I created a follow on ticket #5687 for the remaining work to close off this work

@cweibel Thanks Chris!