Migrate configurator/upgrade wizard from Apache Click to Jakarta Servlets + FreeMarker

[maximthomas]

Summary

Replaces the vendored Apache Click fork powering the OpenAM configurator/upgrade wizard
with plain Jakarta Servlets + FreeMarker templates, removing ~36k LOC of dead/forked
framework code while preserving all existing URLs, AJAX validation behavior, and backend
coupling (AMSetupServlet, UpgradeServices, OpenDJ).

Why

• Apache Click is an abandoned upstream dependency; OpenAM was carrying a large vendored
  fork of it just for this 13-page wizard.
• The wizard runs pre-configuration (before the CREST/CHF/Guice runtime is up), so the
  replacement had to be lightweight and runtime-independent — FreeMarker is already a
  managed dependency (used by openam-oauth2) and pairs cleanly with plain Jakarta servlets.
• Goal was debt removal, not a rewrite: keep the same server-rendered + AJAX-validation
  architecture and byte-for-byte URLs, just swap the framework underneath.

What changed

• New ConfiguratorServlet takes over the wizard's *.htm routing via a migrated-page
  registry, so migrating a page is a Java-only change (no per-page web.xml edits).
• All wizard/upgrade pages (step1–step7, wizard shell, options, defaultSummary, upgrade)
  ported from Click templates to FreeMarker .ftl templates under
  WEB-INF/templates/config/....
• UpgradePageProvider / Upgrade.java updated for the new routing.
• Click artifacts removed: click.xml, click-page.properties, error.htm,
  not-found.htm, legacy step7.htm.
• pom.xml updated to drop the Click dependency.
• New test coverage: WizardTest, ConfiguratorServletTest,
  ServletContextTemplateLoaderTest, UpgradeTest, ConfiguratorServletUpgradeRoutingTest.
• Delivered incrementally (Increment 0 → 8): each increment migrated one page with Click
  and FreeMarker running side-by-side (unmigrated pages delegated to the old Click fork via
  RequestDispatcher), so the reactor stayed green throughout. Click is fully removed only
  in the final increment.

Testing

• New/updated unit tests listed above cover servlet routing, template loading, and the
  upgrade path.
• Manual wizard walkthrough recommended pre-merge (fresh install + upgrade flow) since this
  touches the pre-configuration bootstrap path.

[vharseko]

Review summary

Solid, carefully-executed migration — retires the abandoned vendored Click fork (~36k LOC) for a small servlet + FreeMarker stack while preserving URLs, the AJAX-validation contract, and the AMSetupServlet/UpgradeServices/OpenDJ coupling. I checked parity against the deleted AjaxPage/ProtectedPage/TemplatedPage and read Step2/3/4/7 closely — no correctness regressions and no new security exposure. Nice work.

Security — clear

• No request parameter is reflected into any template; ${…} stays unescaped exactly as Velocity did (no regression). Dynamic values are server-controlled (${context}/${path}), i18n strings, echoed config values (self-XSS at most), and the server-generated ${changelist}.
• @ConfiguratorAction allow-list is an improvement over reflective public-method exposure.
• Access control preserved: onSecurityCheck() faithfully ports ProtectedPage; Options/Upgrade intentionally stay reachable post-config (documented).

Findings

1. Render smoke test — inline comment on ConfiguratorServlet.render(). Strict-undefined-variable 500 risk is the migration's soft spot and nothing renders a real .ftl in tests.
2. onSecurityCheck() copy-pasted into 7 classes — inline comment on Step1.onSecurityCheck(). A ProtectedSetupPage base would restore the single source of truth.
3. FreeMarker exception handler — inline comment on getFreemarkerConfig(). Default DEBUG_HANDLER writes stack traces into the response; RETHROW_HANDLER is the recommended web setting. Minor.
4. Pre-existing issues now in fresh files (not regressions, easy to fix here):
   • SetupUtils.jsonResponse() builds JSON via replaceFirst — a $/\ in the body throws/misbehaves and a " breaks the JSON; consider Matcher.quoteReplacement(...) or real escaping.
   • SetupPage.checkPasswords() — type.equals("agent") NPEs when type is absent; "agent".equals(type) closes it.

Nicely done

• Routing parity (all 11 config .htm pages registered; *.htm preserved).
• Defensive template migration — <#if x??> / ${x!""} used precisely where FreeMarker's strict-undefined behavior would otherwise 500 (the preserved embedded/isEmbedded and Step4 ODSEE quirks can't become errors).
• Clean ServiceLoader cross-module registration (avoids the Maven cycle).
• Excellent inline documentation of every behavioral nuance and preserved bug.

Recommend the render smoke tests before merge given this is the install/upgrade bootstrap path; #2–#4 are nice-to-haves. Manual fresh-install + upgrade walkthrough still worth doing.

[vharseko]

Review — Click → Jakarta Servlets + FreeMarker migration

I focused on the real review surface (the new servlet infrastructure, the ported wizard pages and templates), checking behavior against the originals on master rather than the ~36k lines of deleted vendored Click fork.

Verdict: no blocking bugs or regressions found. This is a careful, behavior-preserving migration with good test coverage. A few cosmetic leftovers and hardening opportunities below; none are merge blockers.

What holds up well

• Reflection dispatch is safely gated. ConfiguratorServlet.findAction only resolves methods annotated with @ConfiguratorAction, so a request cannot invoke an arbitrary public method. Unknown actionLink → 404, unregistered path → 404.
• FreeMarker's null-strictness is handled systematically. Velocity rendered a missing $foo as a literal; FreeMarker ${foo} with RETHROW_HANDLER throws. Pages either always populate values (getAttribute(..., default)) or templates guard with ${x!""} / <#if x??>. The trickiest page, step3.ftl (${type}, <#if store.password??>), is covered because both come from LDAPStoreWizardPage.onInit() (type="config", and store is never null via ensureConfig()). This is backstopped by ConfiguratorServletRenderSmokeTest, which renders all 10 pages and asserts no exception.
• Security posture is faithfully ported. ProtectedSetupPage.onSecurityCheck() reproduces the old ProtectedPage re-entry guard (AMSetupServlet.isConfigured()); Options deliberately extends SetupPage without a guard, matching the old Options extends TemplatedPage.
• Routing / downstream registration are clean. web.xml swaps click-servlet → configurator-servlet on *.htm; Upgrade (in openam-upgrade) self-registers via ServiceLoader/META-INF/services (same idiom as SetupListener), avoiding a Maven cycle. No dangling references to the removed openidentityplatform.openam.click package remain in live code.
• Prior review rounds already fixed real issues (NPE in checkPasswords, JSON escaping via JSONObject, TemplateExceptionHandler).

Minor cleanup (worth doing in this PR)

1. Stale WAR manifests. fam-console.list#L1239 and fam-noconsole.list#L1334 still list WEB-INF/click.xml and WEB-INF/classes/click-page.properties, which this PR deletes. The build doesn't break (only the legacy Ant src/main/previous_scripts/build.xml reads these), but the lists are now inaccurate.
2. Stale docs. chap-endpoints.adoc / chap-endpoints.xml still describe the click-servlet endpoint.
3. copyPublicFields precedence. ConfiguratorServlet.copyPublicFields runs after the model is seeded from getModel(), so a public field silently overrides an addModel entry of the same name. Harmless today (values coincide), but a latent footgun — a comment would help.

Observations — pre-existing, not introduced by this PR (no action required here)

4. No FreeMarker HTML auto-escaping. Form values (serverURL, configStoreHost, rootSuffix, LDAP hosts, step3.ftl password into value=, and the step7 summary) render unescaped — exactly as the old Velocity $foo did. Not a regression, and these pages run pre-configuration. But since this is a rewrite it's a natural place to consider HTMLOutputFormat. Caveat: it can't be a blind flip — some templates intentionally inject HTML built in Java (e.g. Step2's initialCheck), which would need ?no_esc. Reasonable to defer to a follow-up.
5. validateInput sets an arbitrary session attribute from request key/value — a faithful port of the old AjaxPage. See SetupPage.validateInput. Low risk within the setup wizard's trust model.
6. Dead pushConfig / testNewInstanceUrl action paths. wizard.ftl#L157 still calls actionLink=pushConfig, but no handler exists → 404. The old Wizard.java only had ActionLink fields pointing at methods that were never defined (only createConfig() had a body), so this path was already broken (Click would have errored). Carried over as-is; a 404 now instead of a 500.
7. step7 summary embedded check. step7.ftl#L24 <#if embedded??> is never true because Step7.java#L57 adds isEmbedded, not embedded — but the old step7.htm had the identical #if($embedded) vs add("isEmbedded",...) mismatch. Faithful port of a latent bug.

[maximthomas]

Review feedback addressed — review 4656516610 @vharseko

The review found no blocking bugs or regressions and flagged 3 minor cleanups plus 4 pre-existing observations it considered optional. All seven are now resolved on this branch (4334332 → 2fd719c):

Review item	Resolution	Commit
Minor 1 — stale WAR manifests still list click.xml / click-page.properties	Removed both entries from fam-console.list and fam-noconsole.list	4334332
Minor 2 — stale docs describe the removed click-servlet endpoint	Updated chap-endpoints.adoc / .xml; /ccversion/* is now documented against VersionServlet	ff2ce13
Minor 3 — copyPublicFields precedence is a latent footgun	Expanded the Javadoc to document that a public field overrides a same-named addModel entry	0338588
Obs 4 — no FreeMarker HTML auto-escaping	Enabled HTMLOutputFormat so every ${...} is escaped; the few entries that legitimately carry markup (Step2.initialCheck, Upgrade changelist, Step3/Step4 checked fragments) are marked ?no_esc at their use site; added a render smoke test	3d2b5b6
Obs 5 — validateInput could store an arbitrary session attribute	Restricted it to an 8-key allow-list (every key the wizard's own JS sends); any other key is refused; added tests	69c3a55
Obs 6 — dead pushConfig / testNewInstanceUrl action paths (404)	Removed the dead action links from wizard.ftl	da119a8
Obs 7 — step7 summary embedded?? never true (embedded vs isEmbedded)	step7.ftl now reads isEmbedded, matching Step7.java; added a render smoke test	67f0a86

Observations #4–#7 were pre-existing (faithful ports of the old Click/Velocity behavior), not regressions introduced by this PR — they were fixed here anyway. Also included: 2fd719c fixes a setup-IT race (waits for the config link to be enabled before clicking) in IT_Setup / IT_SetupWithOpenDJ.

Ready for review after successful checks

[vharseko]

Review: Migrate configurator/upgrade wizard from Apache Click to Jakarta Servlets + FreeMarker

Overview

The PR replaces the abandoned, vendored Apache Click fork (~36k LOC) that powered the 13-page configurator/upgrade wizard with a small purpose-built stack: ConfiguratorServlet (explicit path→page registry + @ConfiguratorAction reflection dispatch), a SetupPage/ProtectedSetupPage base-class pair ported from AjaxPage/ProtectedPage, a Jakarta-compatible FreeMarker TemplateLoader, and .ftl ports of every wizard template. Downstream modules (openam-upgrade) self-register pages via ServiceLoader, avoiding a Maven cycle. URLs, form field names, and actionLink parameter names are preserved byte-for-byte.

Verdict: high-quality migration — approve after fixing the copyright-header issues and deciding on fam-noconsole.list. Every migrated page class was cross-checked against its merge-base original, every .ftl against its .htm, all 28 ?no_esc sites, all actionLink names against @ConfiguratorAction methods, and the FreeMarker missing-variable surface — no correctness regressions found in reachable code. The code is unusually well-commented about why parity decisions were made.

Issues to fix before merge

1. Copyright/provenance — dropped upstream notices (CDDL compliance).

• SetupPage.java is presented as a new file with only Copyright 2026 3A Systems LLC., but it ports substantial verbatim logic (validateInput, checkPasswords, setLocale, getBaseDir, getLocalizedString, …) from AjaxPage.java, whose header read Copyright (c) 2007 Sun Microsystems Inc. + Portions Copyrighted 2011-2016 ForgeRock AS. under an explicit "DO NOT ALTER OR REMOVE COPYRIGHT NOTICES" banner. The Sun/ForgeRock lines must be carried over.
• ProtectedSetupPage.java is a rename of ProtectedPage.java (git similarity 53%) that replaced Copyright 2014 ForgeRock AS. with the 3A Systems line instead of adding a Portions line.
• Minor same-family items: step7.ftl is a new file with no license header at all; upgrade.ftl was substantially modified but gained no 3A Systems Portions line; DefaultSummary.java header year not extended (2025 → should be 2025-2026).

2. fam-noconsole.list is now internally inconsistent (medium, with a fossil caveat). It still lists the deleted ./config/wizard/step*.htm, ./config/options.htm, ./config/defaultSummary.htm, adds no WEB-INF/templates/config/*.ftl entries and no freemarker jar — while its web.xml was updated to map *.htm → ConfiguratorServlet. A createwar noconsole WAR would 500 on every configurator page (TemplateNotFound). Caveat: the whole list is OpenSSO-era fossil content (opensso.jar, velocity-1.7.jar), so the pipeline is likely already dead — either make the list consistent or leave the file untouched as a fossil, but don't half-update it.

3. Dangling references. Shipped javadoc/comments point at docs/migration/click-to-freemarker/03-migration-plan.md / 04-implementation-notes.md (e.g. ConfiguratorServlet.java:55, Step3.java:163, Step6.java:63, several tests) — no docs/ directory exists on the branch. Also legal/THIRDPARTYREADME.txt still attributes the removed click-nodeps-2.3.0.jar/click-extras-2.3.0.jar.

Security — net improvement

• Closes real stored-XSS sinks. HTMLOutputFormat auto-escaping replaces Click/Velocity's raw output; merge-base templates echoed session-writable values (configStoreHost, rootSuffix, encryptionKey, serverURL, store.password, step7 summary) unescaped into attributes. All 28 ?no_esc sites were audited: every one is server-generated markup or a bundle string that legitimately contains HTML — none carry request-controlled data. Localized strings in JS contexts correctly use ?js_string?no_esc. No injection path found.
• validateInput is now allowlisted to the 8 session keys the wizard JS actually sends. The old AjaxPage let a hand-crafted ?actionLink=validateInput&key=...&value=... write any session attribute — including ADMIN_PWD, bypassing checkPasswords validation. Verified complete against every template caller; no shipped functionality blocked.
• @ConfiguratorAction narrows the dispatch surface — only annotated methods are reachable, vs Click's public-field ActionLinks.
• Nit: overriding service() means every HTTP method (PUT, DELETE, TRACE…) runs the full lifecycle including action dispatch; old ClickServlet exposed only GET/POST. Consider rejecting non-GET/POST/HEAD.

Correctness / parity (verified, not just sampled)

• Every old ProtectedPage subclass now extends ProtectedSetupPage; Options and Upgrade correctly stay unprotected (required for the upgrade path on configured installs). Guard behavior (empty 200) is identical.
• All action bodies are byte-equivalent apart from the setPath(null) → skipRender() translation, which is semantically identical; lifecycle order (onSecurityCheck → onInit → action/onGet) matches Click, including Wizard's eager port-scan initializers correctly moved to onInit().
• FreeMarker RETHROW_HANDLER + missing-variable audit: every unconditional ${var} is unconditionally populated; every conditional value is ??/!""-guarded (including the ported selectLDAPv3opends ODSEE quirk) — no render-time 500 risk found.
• Two deliberate, tested behavior deltas worth noting in the PR description: step7 now actually renders the embedded-store Admin/JMX port rows (the old template guarded on a never-set $embedded key — a fix, covered both directions by the render smoke test), and the dead pushConfig dialog (whose backing methods never existed at merge base) was dropped from wizard.ftl.
• Only reachable-in-theory regression: step4.htm?actionLink=clearStore (via wizard.ftl's disableCustomConfig, which has zero callers) now 404s instead of harmlessly rendering. Dead code today.

Build & tests

• org.freemarker:freemarker is version-managed in the root pom (2.3.31, pre-existing) and already shipped in the WAR via openam-server-only; no pom still references Click; click.version and the javadoc package excludes are cleaned up.
• Tests fit the modules' conventions (TestNG + Mockito + AssertJ, all versions already managed; no new deps). Coverage is substantive, not mock-theater: ConfiguratorServletTest drives real service() against the real registry (including the allowlist refusals and 404 paths); ConfiguratorServletRenderSmokeTest renders the actual .ftl files and asserts the XSS-escaping, ?no_esc, ?js_string, and step7 toggle behavior; ConfiguratorServletUpgradeRoutingTest is the only genuine end-to-end proof of the ServiceLoader wiring. Weak spots are honest and documented (OptionsTest/DefaultSummaryTest near-tautological due to AMSetupServlet coupling).
• Test nits: Step7Test:100 comment contradicts the shipped template (the port did fix the $embedded mismatch); ConfiguratorServletUpgradeRoutingTest:77 still mocks a click-servlet named dispatcher nothing uses.

Suggested follow-ups (non-blocking)

• Fix the stale SetupPage comment claiming DefaultSummary is "not yet migrated".
• checkPasswords with a missing type param now silently stores the admin password where old code NPE'd — consider rejecting instead (hand-crafted requests only).
• The PR's "manual wizard walkthrough recommended pre-merge" note stands: fresh-install + upgrade flow, since this is the pre-configuration bootstrap path and the render smoke test can't cover the live AMSetupServlet.processRequest handoff.