FWD » Core Development » Runtime Infrastructure

================================================================================================

PROPOSAL - [MINOR] functional - p2j.sso_reauth.js handleSsoReauth

The expiry path leaves the IdP popup open as an orphan

================================================================================================

REVIEW FINDING (abridged)

   "The expiry branch no longer calls cleanupReauthUi, which the deleted showSessionExpiredPage did

   as its first statement (await cleanupReauthUi(true)), so the reauthPopup.close() it performs is

   skipped [...] popupCloseCheckInterval and reauthMessageListener die with the page but the popup

   window does not, and the reloaded page holds no reference to it."

FILES

   src/com/goldencode/p2j/ui/client/driver/web/res/p2j.sso_reauth.js

------------------------------------------------------------------------------------------------

1. ROOT CAUSE

------------------------------------------------------------------------------------------------

cleanupReauthUi (p2j.sso_reauth.js:436) is the only place that closes the popup:

   if (reauthPopup && !reauthPopup.closed)

   {

      reauthPopup.close();

   }

   reauthPopup = null;

The deleted showSessionExpiredPage called it as its first statement, so every pre-change expiry path

closed the popup. The replacement branch reaches window.location.reload() with no cleanup call of

any kind.

The grace period does not prevent this. The guard is

   if (reauthInFlight() && (Date.now() - reauthDeadline) < REAUTH_GRACE_MS)

whose second conjunct is time-bounded, so once 10 s have elapsed past the deadline the branch falls

through regardless of reauthInFlight() still being true from an open popup. The tick then reloads

with reauthPopup non-null and unclosed.

A popup opened by window.open is an independent browsing context; reloading the opener does not

close it. reauthPopup is module-private and referenced nowhere else, and the reloaded page starts

with reauthPopup = null, so nothing can ever close it afterwards. The user is left with a stray

window sitting on the IdP page.

Concrete trigger: the user clicks Re-login, the IdP popup opens, and they abandon it without

closing it - a realistic outcome, since the popup is where they would go if they could not remember

their credentials.

------------------------------------------------------------------------------------------------

2. PROPOSED FIX

------------------------------------------------------------------------------------------------

2.1 Close the popup before navigating

---------------------------------------

The full cleanupReauthUi is async and removes the overlay, which is not wanted here - the overlay

should stay visible until the page actually goes away, and awaiting a dialog close before a

navigation invites the same cancelled-navigation problem as

proposal-issue-minor-functional-3. A synchronous, targeted close is the right size:

            countdownDiv.textContent = 'Confirming sign-in...';

   +        // the popup is an independent browsing context and would survive this navigation with

   +        // nothing left holding a reference to it

   +        if (reauthPopup && !reauthPopup.closed)

   +        {

   +           reauthPopup.close();

   +        }

   +        reauthPopup = null;

            p2j.socket.beginInternalReload();

            window.location.reload();

Placed before beginInternalReload so that a cancelled navigation still leaves the popup closed - the

popup is dead either way at this point, since the deadline plus grace has passed.

2.2 Alternative: extract the popup teardown

---------------------------------------------

If the duplication with cleanupReauthUi is unwelcome, extract those five lines:

   +  /** Close the re-login popup if one is open.  Synchronous, unlike cleanupReauthUi. */

   +  function closeReauthPopup()

   +  {

   +     if (reauthPopup && !reauthPopup.closed)

   +     {

   +        reauthPopup.close();

   +     }

   +     reauthPopup = null;

   +  }

and call it from both the expiry branch and cleanupReauthUi. There are already two further inline

copies of the same three lines at :346 and :360 (the success and failure message handlers), so the

extraction pays for itself four times over. This is the better change if the file is being touched

anyway.

------------------------------------------------------------------------------------------------

3. RELATIONSHIP TO OTHER PROPOSALS

------------------------------------------------------------------------------------------------

   proposal-issue-minor-functional-2   introduces p2j.socket.beginInternalReload, shown in the hunk

                                       above.  If that proposal is not taken, drop that one line;

                                       this fix is independent of it.

   proposal-issue-minor-functional-3   reorders the same branch (reloadRequested guard).  If both

                                       are taken, the popup close belongs inside the

                                       "first time through" path, not on every tick.

------------------------------------------------------------------------------------------------

4. RISK AND VERIFICATION

------------------------------------------------------------------------------------------------

Risk: negligible. window.close() on an already-closed or never-opened window is guarded, and the

popup is unusable at this point by definition.

One caveat: a popup that navigated cross-origin to the IdP may not be closeable by the opener in

every browser (window.close() is honoured for script-opened windows, but a cross-origin document can

restrict what the opener may do). The call is best-effort; where it is refused the outcome is

today's behaviour, no worse.

Verification

   1. Trigger SSO re-auth, click Re-login, and leave the IdP popup open without signing in. Wait out

      the countdown plus REAUTH_GRACE_MS. Expected: the popup closes as the opener reloads. Pre-fix

      it remains open on the IdP page.

   2. Same, but sign in successfully - confirm the existing success path (which closes the popup at

      :346) is unchanged.

   3. Same, but close the popup manually before the deadline - confirm no error from closing an

      already-closed window.

   4. Confirm the Logout button path still closes the popup via cleanupReauthUi(true).