Conversation
49f1d39 to
0343e75
Compare
wborn
left a comment
There was a problem hiding this comment.
This review was AI-assisted. The implementation and relevant Keycloak/OpenRemote templates were compared to identify behavioral and compatibility differences.
Thanks for the work on this migration. The overall approach looks promising: using the OpenRemote Vaadin design system, keeping the page implementations typed and relatively small, retrieving branding from the manager configuration, and falling back to Keycloak's parent theme for pages OpenRemote does not customize should make the theme considerably easier to maintain.
The implementation was also compared with the old OpenRemote FreeMarker theme and the Keycloak 26.7 templates. The OTP/TOTP implementation in particular looks good and preserves the important behavior from the old theme.
There are, however, a few compatibility gaps that should be addressed before merging:
- The login page no longer handles Keycloak's hidden/preselected username state and loses the selected credential state.
- Some shared authentication controls are missing, including the locale selector and "try another way".
- Registration uses the Keycloak user profile, but currently treats essentially every custom attribute as a text field and doesn't handle terms acceptance or the full reCAPTCHA behavior.
- Global message suppression only knows about a small hard-coded set of fields and can therefore duplicate validation errors.
- The old login page's self-registration link has intentionally been removed. If that is a product decision, that is fine, but it does mean the new theme is not fully feature-equivalent to the old one.
These compatibility issues should therefore either be addressed or explicitly documented as intended changes before merging.
| and free text for anything a realm has added; advancedMsgStr handles both, | ||
| and falls back to the attribute name when there is neither. */ | ||
| label: advancedMsgStr(attribute.displayName ?? attribute.name), | ||
| type: attribute.name === "email" ? "email" : "text", |
There was a problem hiding this comment.
Since this now drives registration from profile.attributesByName, the profile field metadata should also be honored rather than treating every custom attribute as text. Keycloak supports selects/multiselects, radio/checkbox options, textareas, multivalued/read-only fields and HTML5 annotations here.
There are also registration controls outside the profile, notably termsAcceptanceRequired and the visible/invisible reCAPTCHA variants. With terms required, for example, this form currently has no termsAccepted control at all, so the user cannot complete registration.
Could we either implement the supported Keycloak registration semantics or deliberately fall back to the parent registration page for configurations we don't support?
There was a problem hiding this comment.
reCAPTCHA is not implemented in this theme, and the docs no longer suggest otherwise.
Keycloak's registration reCAPTCHA authenticator delivers Google's api.js through kcContext.scripts, which template.ftl renders as <script> tags (Keycloakify's React template does the same thing through useInsertScriptTags). This theme renders with Lit and never loads that list, so the widget is never initialised, no g-recaptcha-response is posted, and the server rejects the registration. Adding the field markup on its own would not have helped.
Implementing it means two things:
- Loading
kcContext.scripts, and Keycloak'sauthChecker.jsalongside it, the way Keycloakify's own template does. - Wiring the invisible variant, which is what Keycloak's
register.ftluses: ag-recaptchasubmit button carryingdata-sitekey,data-actionanddata-callback, plus the callback itself as a real global. A<script>written into a Lit template never runs, because script elements inserted as parsed HTML are inert, so the callback has to be installed from the module.
What is keeping it out of this PR is verification rather than effort. The invisible flow needs a real site key registered with Google: the well known public test keys cover the v2 checkbox widget only and always pass, so they never exercise the callback path that Keycloak's page depends on. No OpenRemote realm enables reCAPTCHA today, so this would be shipping something untested for a configuration nobody runs.
7ebd3a5 to
668c6d7
Compare
Vaadin's InputControlMixin manages the slotted <input>: it replaces the id and silently drops name, value, required and autocomplete, re-delegating its own from the host element. Setting them on the input rendered a correct-looking but unnamed field, so every form posted nothing but its submit button (login=). Move them onto the component in field() and on the two hand-written checkboxes, and hide Lumo's required indicator, which the design does not show.
The or-booting rule survived the Keycloakify migration but nothing set the class any more, so stock OpenRemote branding painted and then visibly flipped to the realm's own. index.html now adds it before first paint; applyBranding removes it on success, on failure, and from a 400ms timeout so an unreachable manager can never leave a blank page. Branding is cached per realm in localStorage and applied at once, with the fetch revalidating in the background, and the page is only revealed once the logo has decoded. The autofocus field is focused again on reveal, since browsers refuse focus inside visibility: hidden.
Resolved against the manager like the logo, and set on the one <link rel="icon"> the theme ships rather than appending a second, which browsers would choose between by their own rules. Bumps the branding cache version, because the cached shape gains a field.
Keycloak draws the code on a fixed 246px canvas, so its quiet zone varies with the otpauth URL (10.2% to 15.4% measured on 26.7). The image is cropped a fixed 6% per side and a quiet zone we control is added back. If Keycloak ever renders less white border, the crop cuts into the code and it may stop scanning.
- info.ftl - login-page-expired.ftl - login-update-profile.ftl - idp-review-user-profile.ftl - login-idp-link-confirm.ftl - login-idp-link-email.ftl
668c6d7 to
02fa5f9
Compare
Description
Restyles login, register, forgot-password, 2FA setup and 2FA login to the new design using
@openremote/or-vaadin-components. Logo, app title and brand colour come frommanager_config.jsonat runtime, so a custom project needs no theme changes.Also covered though not in the designs: identity providers, all ~30 Keycloak locales, and Keycloak's
infopage.Two implementations
9b213df<or-vaadin-*>, consuming prebuilt UMD bundles. No JS toolchain in this repo.HEADHEADreplaces the UMD version; the commit is kept so the trade-off stays reviewable.Build
UMD (
git checkout 9b213df) - needs a localopenremotecheckout, because@openremote/or-vaadin-components@1.29.0and@openremote/theme@1.29.0never published theirdist/artifacts, so the npm download 404s:Keycloakify (
HEAD) - needs Node 20+, Maven and a JDK on the PATH:./gradlew installDist # -> build/image/openremote-theme-provider.jarOptionally run
docker build -t openremote/keycloak:vaadin .Copy to a remote instance
/opt/keycloak/themesis a symlink into/deployment, which the deployment bind-mounts from the host — so write to the host path, notdocker cp. That also survives container re-creation.UMD - the theme is already a plain directory:
Keycloakify - extract it from the jar first (this is the only copy containing both the
login and email themes):
./gradlew :theme:jar rm -rf /tmp/or-theme unzip -q theme/build/libs/openremote-theme-provider.jar 'theme/*' -d /tmp/or-theme tar czf /tmp/or-theme.tgz -C /tmp/or-theme/theme openremoteThen, for either:
Notes:
openremoteis safe: a folder theme overrides the same-named theme inside theprovider jar, so realms already pointing at
openremotepick up the new one with no settings change. Roll back by deleting the directory and restarting.chmodmatters: Keycloak runs as uid 1000 and a root-owned copy with a tight umaskserves 500s.
openremotealready exists in the jar, so it is in the theme cache and serves stale until restart.
Checklist