|
1
|
================================================================================================
|
|
2
|
PROPOSAL - [MINOR] functional - p2j.sso_reauth.js showSessionExpiredPage
|
|
3
|
Keystroke swallowing is not actually fixed: the keyboard listeners are never detached
|
|
4
|
================================================================================================
|
|
5
|
|
|
6
|
REVIEW FINDING (abridged)
|
|
7
|
"the keystroke-swallowing fix is incomplete - the symptom its own comment names ('the first Tab
|
|
8
|
towards the Go to Login link') still occurs. p2j.keyboard.init registers
|
|
9
|
onkeydown/onkeypress/onkeyup on document in capture phase and nothing on the teardown path detaches
|
|
10
|
them, so on the replaced page GUIKeyboardReader.onkeydown still runs: rejectKeyEvent is false because
|
|
11
|
excludedContainer (the cached #change-os-pass node) was detached by the document.body.innerHTML = ''
|
|
12
|
wipe, p2j.isDialogDisplayed() is false once cleanupReauthUi and the new disposeConnectionDialogs()
|
|
13
|
have emptied the dialogs set, and permittedKeyStrokes returns false for Tab (also F3 and every
|
|
14
|
Ctrl/Alt combination), so p2j.consumeEvent preventDefaults it - every Tab, not just the first,
|
|
15
|
leaving the link reachable only by mouse. CHUI mode behaves the same via sendKey. Note the disposal
|
|
16
|
is what re-enables the swallowing in the one case where it changes anything."
|
|
17
|
|
|
18
|
FILES
|
|
19
|
src/com/goldencode/p2j/ui/client/driver/web/res/p2j.keyboard.js (new detach API)
|
|
20
|
src/com/goldencode/p2j/ui/client/driver/web/res/p2j.sso_reauth.js (call it)
|
|
21
|
|
|
22
|
------------------------------------------------------------------------------------------------
|
|
23
|
1. ROOT CAUSE
|
|
24
|
------------------------------------------------------------------------------------------------
|
|
25
|
|
|
26
|
r16675 addressed the wrong owner of the problem. disposeConnectionDialogs() removes the *dialogs'*
|
|
27
|
capture-phase triggers, but the swallowing on the session-expired page comes from the driver's own
|
|
28
|
document-level listeners, which p2j.keyboard.init installed and which nothing ever removes:
|
|
29
|
|
|
30
|
p2j.keyboard.js:348-359 document.addEventListener('keydown', ..., true)
|
|
31
|
document.addEventListener('keypress', ..., true)
|
|
32
|
document.addEventListener('keyup', ..., true)
|
|
33
|
window.addEventListener('blur', ..., true)
|
|
34
|
(+ 'input', 'compositionstart', 'compositionend' in GUI mode)
|
|
35
|
|
|
36
|
The removal code for exactly these listeners already exists - p2j.keyboard.js:319-336 - but it is
|
|
37
|
inlined at the top of init() and only runs when init() is called again. There is no way for any other
|
|
38
|
module to ask for a teardown.
|
|
39
|
|
|
40
|
Consequently, after showSessionExpiredPage() wipes the body:
|
|
41
|
|
|
42
|
- rejectKeyEvent() returns false: excludedContainer is a node reference cached at init
|
|
43
|
(p2j.keyboard.js:348) and the wipe detached it, so contains(evt.target) is false for everything.
|
|
44
|
- p2j.isDialogDisplayed() is false, because cleanupReauthUi() and the new disposeConnectionDialogs()
|
|
45
|
between them emptied the dialogs set - the new disposal call is precisely what *re-enables* the
|
|
46
|
swallowing in the one case where the old code happened to escape it.
|
|
47
|
- permittedKeyStrokes() returns false for Tab (and F3, and every Ctrl/Alt combination), so
|
|
48
|
p2j.consumeEvent() preventDefaults it.
|
|
49
|
|
|
50
|
Result: every Tab is swallowed, not just the first, and the "Go to Login" link is mouse-only. CHUI mode
|
|
51
|
reaches the same place through sendKey.
|
|
52
|
|
|
53
|
------------------------------------------------------------------------------------------------
|
|
54
|
2. PROPOSED FIX
|
|
55
|
------------------------------------------------------------------------------------------------
|
|
56
|
|
|
57
|
2.1 Extract the existing removal block into a public detach API
|
|
58
|
----------------------------------------------------------------
|
|
59
|
|
|
60
|
p2j.keyboard.js - lift lines 319-336 out of init() into a named function and export it. Pure
|
|
61
|
refactoring plus one export; no behaviour change for the existing init() path.
|
|
62
|
|
|
63
|
+ /**
|
|
64
|
+ * Detach every document and window level key listener installed by init.
|
|
65
|
+ * <p>
|
|
66
|
+ * Called by init before it re-registers, and by any caller which is about to replace the page
|
|
67
|
+ * content: the listeners are registered on document in the capture phase, so they outlive the
|
|
68
|
+ * elements they were installed for and would go on consuming keystrokes on the replaced page -
|
|
69
|
+ * including Tab, which permittedKeyStrokes never permits.
|
|
70
|
+ */
|
|
71
|
+ me.detachListeners = function()
|
|
72
|
+ {
|
|
73
|
+ if (me.keyboardReader)
|
|
74
|
+ {
|
|
75
|
+ document.removeEventListener('keydown', me.keyboardReader.onkeydown, true);
|
|
76
|
+ document.removeEventListener('keypress', me.keyboardReader.onkeypress, true);
|
|
77
|
+ if (me.keyboardReader.oninput)
|
|
78
|
+ {
|
|
79
|
+ document.removeEventListener('input', me.keyboardReader.oninput, true);
|
|
80
|
+ }
|
|
81
|
+ if (me.keyboardReader.onCompositionStart)
|
|
82
|
+ {
|
|
83
|
+ document.removeEventListener('compositionstart',
|
|
84
|
+ me.keyboardReader.onCompositionStart,
|
|
85
|
+ true);
|
|
86
|
+ }
|
|
87
|
+ if (me.keyboardReader.onCompositionEnd)
|
|
88
|
+ {
|
|
89
|
+ document.removeEventListener('compositionend', me.keyboardReader.onCompositionEnd, true);
|
|
90
|
+ }
|
|
91
|
+ document.removeEventListener('keyup', me.keyboardReader.onkeyup, true);
|
|
92
|
+ window.removeEventListener('blur', me.keyboardReader.onfocuslost, true);
|
|
93
|
+
|
|
94
|
+ me.keyboardReader = null;
|
|
95
|
+ }
|
|
96
|
+
|
|
97
|
+ excludedContainer = null;
|
|
98
|
+ };
|
|
99
|
|
|
100
|
and init() becomes:
|
|
101
|
|
|
102
|
me.init = function(cfg)
|
|
103
|
{
|
|
104
|
// input element
|
|
105
|
inputText = document.getElementById(cfg.clipboard.input);
|
|
106
|
|
|
107
|
- if (me.keyboardReader)
|
|
108
|
- {
|
|
109
|
- ... 18 lines of removeEventListener ...
|
|
110
|
- }
|
|
111
|
+ me.detachListeners();
|
|
112
|
|
|
113
|
// init mapping
|
|
114
|
p2j.keymap.init(cfg);
|
|
115
|
...
|
|
116
|
|
|
117
|
Note on `me.keyboardReader = null` and `excludedContainer = null`: init() reassigns both immediately
|
|
118
|
afterwards, so the init path is unaffected. Clearing them matters for the teardown path - a stale
|
|
119
|
keyboardReader would let a *newly added* listener (from anywhere) still dispatch into a driver whose
|
|
120
|
DOM is gone, and a stale excludedContainer keeps a detached node alive.
|
|
121
|
|
|
122
|
2.2 Call it from showSessionExpiredPage
|
|
123
|
----------------------------------------
|
|
124
|
|
|
125
|
p2j.sso_reauth.js, in showSessionExpiredPage(), immediately before the body wipe:
|
|
126
|
|
|
127
|
document.body.innerHTML = '';
|
|
128
|
|
|
129
|
becomes
|
|
130
|
|
|
131
|
+ // The driver's key listeners are registered on document in the capture phase, so wiping the body
|
|
132
|
+ // does not remove them: GUIKeyboardReader.onkeydown would still run on this page, and since
|
|
133
|
+ // permittedKeyStrokes rejects Tab it would consume every Tab towards the "Go to Login" link.
|
|
134
|
+ // Disposing the connection dialogs above removes their triggers but not these.
|
|
135
|
+ try
|
|
136
|
+ {
|
|
137
|
+ p2j.keyboard.detachListeners();
|
|
138
|
+ }
|
|
139
|
+ catch (e)
|
|
140
|
+ {
|
|
141
|
+ console.error('SSO re-auth: detaching the keyboard listeners failed', e);
|
|
142
|
+ }
|
|
143
|
+
|
|
144
|
document.body.innerHTML = '';
|
|
145
|
|
|
146
|
The try/catch matches the surrounding style (the disposeConnectionDialogs() call directly above is
|
|
147
|
guarded the same way) and keeps a failure here from aborting the page replacement.
|
|
148
|
|
|
149
|
2.3 Correct the comment that overstates what the dialog disposal achieves
|
|
150
|
-------------------------------------------------------------------------
|
|
151
|
|
|
152
|
The comment introduced by r16675 above the disposeConnectionDialogs() call claims the dialog triggers
|
|
153
|
"would swallow the first keystroke on this page - including the first Tab towards the 'Go to Login'
|
|
154
|
link". That is not what happens: the driver listener swallows *every* Tab, and the dialog triggers were
|
|
155
|
in fact the only thing suppressing it (via isDialogDisplayed()). Rewrite it to state what the disposal
|
|
156
|
is actually for:
|
|
157
|
|
|
158
|
- // The connection dialogs are parented on document.body, so they must be disposed of before it is
|
|
159
|
- // wiped. Otherwise the wipe only detaches their nodes and leaves the widgets registered, each
|
|
160
|
- // still holding a capture-phase window keydown trigger that would swallow the first keystroke on
|
|
161
|
- // this page - including the first Tab towards the "Go to Login" link.
|
|
162
|
+ // The connection dialogs are parented on document.body, so they must be disposed of before it is
|
|
163
|
+ // wiped: the wipe only detaches their nodes and leaves the widgets registered, still holding a
|
|
164
|
+ // capture-phase window keydown trigger and still willing to run show/hide against nodes that are
|
|
165
|
+ // no longer in the document. Keystroke handling on the replaced page is a separate matter - see
|
|
166
|
+ // the detachListeners call below.
|
|
167
|
|
|
168
|
------------------------------------------------------------------------------------------------
|
|
169
|
3. ALTERNATIVES CONSIDERED
|
|
170
|
------------------------------------------------------------------------------------------------
|
|
171
|
|
|
172
|
(a) Make permittedKeyStrokes() permit Tab. Rejected: it is deliberately restrictive for the 4GL
|
|
173
|
client (p2j.keyboard.js:58 revision note "return true for all key strokes except TAB key"), and
|
|
174
|
changing it would affect every live session, not the expired page.
|
|
175
|
|
|
176
|
(b) Point excludedContainer at the new container. Rejected: it would rely on rejectKeyEvent() as a
|
|
177
|
whitelist mechanism it was not designed to be (its purpose is the #change-os-pass form), and it
|
|
178
|
would leave keypress/keyup/blur handlers running against a dead driver.
|
|
179
|
|
|
180
|
(c) Navigate away instead of replacing the body. That is what already happens on the normal path
|
|
181
|
(redirectToLogout() navigates within one fetch RTT) and is why this finding is MINOR: the expired
|
|
182
|
page is only interactive when p2j.embedded is set, or when the logout fetch rejects or hangs -
|
|
183
|
and unlike fetchAuthEndpoint() it has no timeout. Adding a timeout to that fetch is a worthwhile
|
|
184
|
separate change, but it does not make the page correct in embedded mode.
|
|
185
|
|
|
186
|
------------------------------------------------------------------------------------------------
|
|
187
|
4. RISK AND VERIFICATION
|
|
188
|
------------------------------------------------------------------------------------------------
|
|
189
|
|
|
190
|
Risk: low, and confined to the teardown path. The refactor in 2.1 is behaviour-preserving for init()
|
|
191
|
(the only current caller of the removal code). The only new exposure is that detachListeners() nulls
|
|
192
|
me.keyboardReader, so any code calling p2j.keyboard.keyboardReader after a detach must tolerate null -
|
|
193
|
after showSessionExpiredPage() the session is over and the socket is closed, but grep for
|
|
194
|
`keyboardReader` before landing to confirm no module dereferences it unguarded on that path.
|
|
195
|
|
|
196
|
Verification
|
|
197
|
1. Web GUI, force MSG_SSO_REAUTH and let the countdown expire with p2j.embedded set (so the page
|
|
198
|
stays interactive). Press Tab: focus must reach the "Go to Login" link. Before the fix: nothing
|
|
199
|
happens on any Tab press.
|
|
200
|
2. Same with the logout fetch blackholed (block /logout at the proxy) in non-embedded mode.
|
|
201
|
3. Web CHUI: same test; verify no sendKey traffic is attempted from the expired page.
|
|
202
|
4. Regression: a normal re-auth that *succeeds* must leave the keyboard fully working - that path
|
|
203
|
goes through cleanupReauthUi() and never calls showSessionExpiredPage(), so detachListeners()
|
|
204
|
must not be reached. Confirm by breakpoint or console trace.
|
|
205
|
5. Regression: a session that re-initialises the driver (reconnect) must still work, i.e. init() ->
|
|
206
|
detachListeners() -> re-register.
|