NMS-20180: Fix various menu-related issues after NMS-20174 Review - #8792
Conversation
synqotik
changed the title
…s/** The unpacked dist-menu artifact also ships an index.html (the Vite build input) at /ui-components/, which nothing links to and which should not be anonymously reachable; only assets/** needs the ROLE_ANONYMOUS rule from NMS-20174. Document the deliberate login-page preload in bootstrap.jsp, and pin the NMS-20174 access/quiet fixes with WebappIT smoke tests: anonymous asset access with correct content types, index.html requiring authentication, and login.jsp never executing the menu bundle.
… locale Under a Turkish default locale, locale-sensitive toLowerCase() turns "I" into a dotless ı, so a saved "/API" request would slip past the "/api" prefix check added in NMS-20174. Use Locale.ROOT and cover it with a test that runs the matcher under tr-TR.
The "does not cancel navigation for regular items" test only checked that performLogout was not called, so an unconditional preventDefault() (breaking navigation for every self-service item) would still pass. Assert event.defaultPrevented is false so the test covers what its name claims.
6fabacc to
75fe937
Compare
marshallmassengill
left a comment
marshallmassengill
left a comment
There was a problem hiding this comment.
Couple blockers:
OpenNMSAuthSuccessHandler.java:76 does savedRequest.getServletPath().toLowerCase() (default locale) and passes that into isInvalidSavedRequestUrl, which then lowercases again with Locale.ROOT. Under a Turkish default locale /API/v2/nodes is already /apı/v2/nodes before the fixed method sees it, so it still fails the /api prefix check, still becomes the post-login redirect, and the browser still downloads JSON: exactly the NMS-20174 symptom. The new unit test passes only because it calls the utility directly. Fix is one line: delete the servletPathLower local and pass savedRequest.getServletPath(). (PasswordGateActionServlet:75 passes the raw path, so the fix does work there.)
Same locale defect two methods up in the file being edited: LoginModuleUtils.java:126 lowercases ROLE_ADMIN with the default locale before comparing to "admin", which locks admins out of the Karaf console under tr. Identical at AuthorityPrincipal.java:38.
verifyMenuIndexHtmlRequiresAuthentication asserts the status code only, so it passes on a 302 to anywhere. Add header("Location", containsString("login.jsp")).
verifyMenuBundleAssetsAreAnonymouslyAccessible checks js and css in one method, so a js failure hides the css result.
…ertions - Pass the raw servlet path to isInvalidSavedRequestUrl instead of pre-lowercasing with the default locale, which defeated the utility's Locale.ROOT handling under e.g. a Turkish locale - Use Locale.ROOT for the admin-role comparisons in LoginModuleUtils and AuthorityPrincipal - Assert the menu index.html auth redirect targets login.jsp - Split the menu asset smoke test so a JS failure cannot hide the CSS result
@marshallmassengill fixed in latest commit. |
2 participants
Fix various issues found in PR review for NMS-20174 (PR #8758) ("Fix Menubar/SideMenu issues in Firefox and Safari on JSP pages").
There were 9 different items. 5 are fixed here. 4 have been deferred as they represent more work or tradeoffs.
Fixed Items
Fix 1 — narrowed anonymous rule:
applicationContext-spring-security.xmlnow grants/ui-components/assets/**instead of/ui-components/**, with a comment explaining why (due to the strayindex.htmlin thedist-menuartifact, NMS-20180)./ui-components/index.htmlnow falls through to the/** → ROLE_USERcatch-all, so it 302s to login like any other page.Fix 3 — comment only: extended the
bootstrap.jsphead comment to mark the login-page stylesheet + modulepreload as deliberate cache-warming, cross-referencing the anonymous rule it depends on.Fix 4 — test gap: added
expect(event.defaultPrevented).toBe(false)to the "does not cancel navigation for regular items" test.FIx 7 —
WebappITsmoke tests (three new tests): anonymous GET of the menu JS/CSS returns 200 with the right content types;ui-components/index.htmlrequires auth (302, pinning fix 1's narrowing); andlogin.jspcontains no<script>tag for the menu bundle — with a positive control asserting the preload link is present so the negative assertion stays meaningful.Fix 9 — locale-safe URL matching (
LoginModuleUtils.java:163):toLowerCase()→toLowerCase(Locale.ROOT), with a comment explaining the Turkishdotless-ıproblem and added tests.Deferred Items
These four items were identified while reviewing the NMS-20174 fixes (PR #8758) but deliberately not fixed in NMS-20180. I will open separate tickets to follow up.
1. Vue menu disappeared from nine popup JSPs (documentation / QA spot-check)
What happened. NMS-20174 fix #2 made
bootstrap.jsphonor both quietmechanisms (the
param.quietinclude parameter and theBootstrap.flags("quiet")request attribute). The PR description only mentions
login.jsp, but nine other JSPsalso set the quiet flag and therefore silently lost the Vue menu with that change:
admin/discovery/add-er.jspadmin/discovery/add-ex-url.jspadmin/discovery/add-ir.jspadmin/discovery/add-specific.jspadmin/discovery/add-url.jspadmin/userGroupView/users/newPassword.jspgraph/forecast.jspgraph/nrtg.jspincludes/legendInfo-box.jspWhy it is probably fine. Every one of these is opened via
window.openas apopup/utility window. Losing the menu there is almost certainly the intended
rendering — and the pre-fix behavior (menu app executing in a popup that had asked
for quiet mode) is a plausible source of the original "menus overlapping content"
symptom.
2. Cache-buster stats the menu bundle files on every legacy page render
What happens today.
includes/bootstrap.jspcomputes the?v=cache-buster forthe menu bundle by calling
application.getRealPath(...)+File.lastModified()for both
index.jsandindex.csson every JSP page render (two filesystemstats per request). The
load-assets.jspprecedent it mirrors reads a cachedAssetLocator.lastModified()instead.Suggested fix. Memoize the computed version in a
ServletContextattribute,keyed on (or invalidated by) the files' mtimes, or re-checked on a short TTL.
Tradeoffs / why it was deferred.
There is no observed performance problem.
workflow (
build-and-copy.sh, ui/CLAUDE.md instructions): newly copied bundleswould keep serving the stale
?v=until a webapp restart. A correct fix needsmtime-keyed or TTL-based invalidation — more code for marginal benefit.
deleted anyway. Fixing this separately is only worthwhile if content-hashing is
rejected or slips.
Action: will consider folding it into the content-hashing work if/when we proceed with that./
3. Content-hashed filenames for the menu bundle (replace
?v=<mtime>)What happens today. The menu bundle is built with fixed filenames
(
ui/vite.config.menu.ts→assets/index.js,assets/index.css) becausebootstrap.jsphardcodes the URLs. Cache correctness relies on a?v=<deployed file mtime>query parameter appended bybootstrap.jsp(added in NMS-20174 fix #6).Why
?v=is not durable.Suggested fix.
Leaving out details, but adding a content hash to all filenames.
Costs / risks.
re-verification
or unreadable manifest must fail loudly with a sane fallback, or the menu breaks.
directory (already true for the main SPA — ui/CLAUDE.md documents the occasional
rm assets/*.*), and the manifest must be copied along with the assets.filenames.
Action: Will open a separate ticket to do the full content-hash + manifest change, targeted at the next
major release (Horizon 37).
4. Root fix for REST/API requests polluting the Spring Security request cache
Omitting details, but this is a larger effort as part of fixing
OpenNMSAuthSuccessHandlerandHttpSessionRequestCacheto not saved assets in the cache, and to more properly handle redirects.Action: Will open a separate ticket to do a comprehensive fix in Horizon 37.
External References