|
1
|
================================================================================================
|
|
2
|
PROPOSAL - [MINOR] functional - p2j.sso_reauth.js handleSsoReauth
|
|
3
|
The expiry reload raises the leave-confirmation, and "Stay" freezes the overlay
|
|
4
|
================================================================================================
|
|
5
|
|
|
6
|
REVIEW FINDING (abridged)
|
|
7
|
"The expiry path calls window.location.reload() while p2j.socket.js's window.onbeforeunload is
|
|
8
|
still armed [...] returns the 'ANY PENDING CHANGES WILL BE LOST!' confirmation whenever
|
|
9
|
p2j.isGui && !exitTheApplication [...] If the user picks 'Stay', the reload is aborted after
|
|
10
|
clearInterval(countdownInterval); countdownInterval = null; has already run, leaving the overlay
|
|
11
|
frozen at 'Confirming sign-in...' with no further ticks."
|
|
12
|
|
|
13
|
FILES
|
|
14
|
src/com/goldencode/p2j/ui/client/driver/web/res/p2j.sso_reauth.js
|
|
15
|
src/com/goldencode/p2j/ui/client/driver/web/res/p2j.socket.js
|
|
16
|
|
|
17
|
------------------------------------------------------------------------------------------------
|
|
18
|
1. ROOT CAUSE
|
|
19
|
------------------------------------------------------------------------------------------------
|
|
20
|
|
|
21
|
Two independent mistakes on the same three lines.
|
|
22
|
|
|
23
|
(a) The prompt. me.init() installs window.onbeforeunload (p2j.socket.js:6896) unconditionally at
|
|
24
|
page load, and it returns the confirmation string whenever p2j.isGui && !exitTheApplication.
|
|
25
|
At re-auth expiry no logout has run, so exitTheApplication is still false - the only
|
|
26
|
assignments setting it true are doRedirectToLogoutPage() and onpagehide, and onpagehide fires
|
|
27
|
*after* beforeunload. Nothing ever clears the handler. Sticky user activation is satisfied
|
|
28
|
(a session that reached SSO token expiry has had canvas input; clicking Re-login alone sets
|
|
29
|
it), so the browser prompts.
|
|
30
|
|
|
31
|
This prompt is newly introduced by this diff: the deleted showSessionExpiredPage() rewrote
|
|
32
|
document.body and never unloaded, so the old code could not hit it.
|
|
33
|
|
|
34
|
Note the codebase's own convention for a deliberate scripted navigation is to set
|
|
35
|
exitTheApplication = true first, which is exactly what the overlay's own Logout button does
|
|
36
|
via doRedirectToLogoutPage. This reload is the one navigation that skips it.
|
|
37
|
|
|
38
|
(b) The teardown ordering. The branch runs
|
|
39
|
|
|
40
|
clearInterval(countdownInterval);
|
|
41
|
countdownInterval = null;
|
|
42
|
countdownDiv.textContent = 'Confirming sign-in...';
|
|
43
|
window.location.reload();
|
|
44
|
|
|
45
|
reload() only *requests* a navigation; it returns, and the navigation can be cancelled. By
|
|
46
|
then the interval is already gone, so on "Stay" nothing ticks again: the overlay is stuck on
|
|
47
|
"Confirming sign-in...", cleanupReauthUi never runs, the modal keydown trap stays armed and
|
|
48
|
the socket keeps echoing the heartbeat. The Logout button still works as a manual escape, so
|
|
49
|
the user is not trapped, but the automatic recovery is silently defeated.
|
|
50
|
|
|
51
|
------------------------------------------------------------------------------------------------
|
|
52
|
2. PROPOSED FIX
|
|
53
|
------------------------------------------------------------------------------------------------
|
|
54
|
|
|
55
|
2.1 Suppress the prompt for this reload
|
|
56
|
-----------------------------------------
|
|
57
|
|
|
58
|
Reuse the internalReload flag introduced in proposal-issue-minor-functional-2 section 2.1 - the same
|
|
59
|
signal that stops onpagehide arming quit-on-reload should stop beforeunload prompting, since both
|
|
60
|
ask the same question ("is the user leaving?") and the answer is the same "no":
|
|
61
|
|
|
62
|
window.onbeforeunload = function(evt)
|
|
63
|
{
|
|
64
|
- if (p2j.isGui && !exitTheApplication)
|
|
65
|
+ if (p2j.isGui && !exitTheApplication && !internalReload)
|
|
66
|
{
|
|
67
|
var confirmationMessage = "ANY PENDING CHANGES WILL BE LOST!\n" + ...
|
|
68
|
|
|
69
|
If minor-functional-2 is not taken, this proposal must introduce the flag itself; the two are
|
|
70
|
better landed together.
|
|
71
|
|
|
72
|
2.2 Keep the countdown alive across a cancelled navigation
|
|
73
|
------------------------------------------------------------
|
|
74
|
|
|
75
|
Do not tear down before requesting the navigation. Let the interval keep running and make the branch
|
|
76
|
idempotent instead:
|
|
77
|
|
|
78
|
+ if (reloadRequested)
|
|
79
|
+ {
|
|
80
|
+ // the navigation was requested and is either in flight or was cancelled by the user;
|
|
81
|
+ // do not request it again on every tick
|
|
82
|
+ return;
|
|
83
|
+ }
|
|
84
|
+
|
|
85
|
+ reloadRequested = true;
|
|
86
|
- clearInterval(countdownInterval);
|
|
87
|
- countdownInterval = null;
|
|
88
|
...
|
|
89
|
countdownDiv.textContent = 'Confirming sign-in...';
|
|
90
|
+ p2j.socket.beginInternalReload();
|
|
91
|
window.location.reload();
|
|
92
|
return;
|
|
93
|
|
|
94
|
with a module variable beside the others:
|
|
95
|
|
|
96
|
+ /** True once the expiry reload has been requested; keeps the tick from re-requesting it. */
|
|
97
|
+ var reloadRequested = false;
|
|
98
|
|
|
99
|
The interval is left running deliberately. On a successful navigation it dies with the page; on
|
|
100
|
"Stay" it keeps ticking, which is what lets 2.3 offer the user a way forward.
|
|
101
|
|
|
102
|
2.3 Give the user something to do if they decline
|
|
103
|
---------------------------------------------------
|
|
104
|
|
|
105
|
With the interval still alive, a declined navigation should say so rather than sit on "Confirming
|
|
106
|
sign-in...":
|
|
107
|
|
|
108
|
+ countdownDiv.textContent = 'Sign-in could not be confirmed. Use Logout to continue.';
|
|
109
|
|
|
110
|
set on the tick *after* reloadRequested is already true (i.e. in the early-return branch above),
|
|
111
|
so it only appears once the navigation has demonstrably not happened. The Logout button is already
|
|
112
|
enabled and already works.
|
|
113
|
|
|
114
|
An alternative worth considering is to leave the overlay alone and simply not offer a reload at all
|
|
115
|
on this path - see proposal-issue-minor-functional-2 section 2.2, where the server reports the
|
|
116
|
outcome over the existing socket and no navigation is needed.
|
|
117
|
|
|
118
|
------------------------------------------------------------------------------------------------
|
|
119
|
3. RELATIONSHIP TO OTHER PROPOSALS
|
|
120
|
------------------------------------------------------------------------------------------------
|
|
121
|
|
|
122
|
proposal-issue-minor-functional-2 introduces internalReload and owns the sessionStorage half of
|
|
123
|
the same reload. Prerequisite for 2.1.
|
|
124
|
proposal-issue-minor-functional-4 closes the orphaned popup on the same branch.
|
|
125
|
|
|
126
|
All three edit the same expiry branch; apply as one change.
|
|
127
|
|
|
128
|
------------------------------------------------------------------------------------------------
|
|
129
|
4. RISK AND VERIFICATION
|
|
130
|
------------------------------------------------------------------------------------------------
|
|
131
|
|
|
132
|
Risk: low. 2.1 narrows a prompt that should never have fired on this path. 2.2 removes a teardown
|
|
133
|
that was premature; the new reloadRequested guard is what prevents the reload being re-requested
|
|
134
|
once per second, so do not take 2.2 without it.
|
|
135
|
|
|
136
|
Verification
|
|
137
|
1. Trigger SSO re-auth in the GUI client, interact with the canvas first so sticky activation is
|
|
138
|
set, and let the countdown expire. Expected: no leave-confirmation dialog, page reloads
|
|
139
|
straight away. Pre-fix the dialog appears.
|
|
140
|
2. Pre-fix build, same test, choose "Stay": confirm the frozen "Confirming sign-in..." overlay.
|
|
141
|
Post-fix, force the same state (e.g. temporarily re-enable the prompt) and confirm the tick
|
|
142
|
still runs and the message changes to the actionable text from 2.3.
|
|
143
|
3. Confirm the Logout button still tears down cleanly from the post-reload-request state.
|
|
144
|
4. Confirm the prompt still appears for an ordinary user-initiated close or F5 with unsaved work,
|
|
145
|
i.e. internalReload has not disabled it globally.
|