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