Modernize login#5500
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes the login flow by migrating the legacy Zend-based login view/controller/form to ipl/compat equivalents and consolidating the login page markup into a reusable widget, with related CSS/JS adjustments.
Changes:
- Replace the Zend
LoginFormwith an iplCompatFormimplementation and adapt login handling to the compat lifecycle. - Replace the legacy
login.phtmlview script with a newLoginPagewidget used by aCompatController-basedAuthenticationController. - Update login-related CSS/JS selectors and styles, including spacing for additional login buttons and a typo fix in orb IDs.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| public/js/icinga/history.js | Adjusts the login selector used during onload redirect handling for the new nesting. |
| public/css/icinga/login.less | Updates layout and spacing for the new login markup and added login buttons container. |
| public/css/icinga/login-orbs.less | Fixes the orb ID typo to match the updated markup (orb-notifications). |
| library/Icinga/Web/Widget/LoginPage.php | Introduces a widget encapsulating the full login page structure (form, footer, social links, orbs). |
| application/views/scripts/authentication/login.phtml | Removes the legacy view script in favor of the widget-based rendering. |
| application/forms/Authentication/LoginForm.php | Migrates the login form to CompatForm, adds CSRF/UID, and updates success/error handling. |
| application/controllers/AuthenticationController.php | Migrates controller to CompatController and renders LoginPage instead of a view script. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lippserd
left a comment
lippserd
left a comment
There was a problem hiding this comment.
Replace LoginButtonForm::ON_SUCCESS with ON_SUBMIT
(AuthenticationController.php:131)
ON_SUCCESS is a deprecated alias for ON_SUBMIT in ipl-html. It still
works, but it is inconsistent with how the main login form's event is wired
on the lines just above it.
Verify or remove the $this->controls = new HtmlDocument() workaround
(AuthenticationController.php:144-145)
The comment says this suppresses the tab bar, but setTitle() does not add
any tabs to the controls object. Controls::isEmpty() would therefore
already return true without this replacement. If runtime testing confirms
the tab bar does not appear without it, remove the assignment. If it is
needed, fix the PHPStan type violation (Controls property assigned a
HtmlDocument) and update the comment to name the framework path that
populates controls on the login page. The type mismatch is a new PHPStan
error not yet in the baseline.
Add an Errors decorator to the username element
(LoginForm.php, assemble())
The password and rememberme elements both include an Errors decorator
so their validation messages render. username has only RenderElement and
ControlGroup, so a server-side required-field error on that field is
silently discarded. HTML5 validation can be disabled by the browser or
bypassed entirely with curl or similar tools, which means the server must
handle and display validation errors for every field. Add an Errors
decorator to username to match the other fields.
Add rel="noopener noreferrer" to the social link anchors
(LoginPage.php, assembleSocialLinks())
Both the Facebook and GitHub links open in a new tab (target="_blank")
without this attribute. Without it, the opened page can access
window.opener and navigate the login page away (tabnabbing). Add
'rel' => 'noopener noreferrer' to the attributes of both anchors.
Add a summary line to the LoginPage constructor docblock
(LoginPage.php:49)
The constructor docblock contains only @param tags. The PHPDoc convention
requires a summary line as the first line; for constructors it reads
Create a new LoginPage.
Add docblocks to onSuccess() and onRequest()
(LoginForm.php:135,197)
Both methods are non-trivial override points with side effects: onSuccess()
calls setRerenderLayout() and sets the redirect URL; onRequest() detects
external-only authentication and surfaces a warning. Neither has a docblock.
Simplify the error margin shorthand
(login.less:59)
margin: 1em 0 1em 0; has identical top and bottom values and identical
left and right values. Write it as margin: 1em 0;.
jrauh01
force-pushed
the
modernize-login
branch
2 times, most recently
from
164fde5 to
4897909
Compare
|
@lippserd I have fully implemented your requested list. Additionally I rebased on LoginForm.php:
AuthenticationController.php:
LoginPage.php:
|
jrauh01
force-pushed
the
modernize-login
branch
2 times, most recently
from
b3bb326 to
28e5e85
Compare
|
Rebased on |
Replace the legacy Zend-based `LoginForm` with an ipl `CompatForm`-based form:
- Add `CsrfCounterMeasure` and `FormUid` traits
- Replace `init()`/`createElements()` with `__construct()´/`assemble()` using
inline ipl decorator objects instead of Zend decorator arrays
- Replace `getRedirectUrl()` with `createRedirectUrl()` because
`getRedirectUrl()` would override `ipl\Html\Form::getRedirectUrl()`
- use `hasBeenAssembled()` instead of `$this->created`
- use `str_contains()` instead of `strpos()`
- Rewrite `onSuccess()` for `CompatForm`:
- change visibility from `public` to `protected`
- void return type
- use `Icinga::app()->getResponse()` instead of `$this->getResponse()`
- use `addMessage()` instead of `addError()`
- call `setRedirectUrl()` explicitly
Extract the login page structure (logo, footer, social links, decorative orbs) from `login.phtml` into a reusable ipl-html widget. Extending `HtmlDocument` rather than `BaseHtmlElement` lets it emit `#login` and the seven `.orb` sibling divs without a wrapping root tag. Both the login action and the upcoming 2FA challenge action can now share the same visual scaffolding without duplicating markup and without using view scripts anymore.
Convert `AuthenticationController` from the legacy Zend Controller to `CompatController`, dropping `login.phtml` in favour of the new `LoginPage` widget and `addContent()`. The view variable assignments are replaced by `setTitle()` and `addContent(new LoginPage(...))`. Improve `httpBadRequest()` message for external redirect attempts. In `CompatController`, `$this->controls` is the tab bar area rendered above the page content. When no tabs are added it still emits an empty `<div class="controls">` wrapper. Setting `$this->view->compact = true` suppresses that wrapper entirely, keeping the login page markup clean. Handle the redirect on success in an `ON_SUBMIT` event handler. Delegate the external-backend-only check to `LoginForm::onRequest()` via `ON_REQUEST`. Use `$this->getServerRequest()` instead of `ServerRequest::fromGlobals()`. Two structural fixes required by the changed DOM nesting: - `login.less`: height `100%` -> `100vh` (`#login` is now inside `.content` which has no explicit height, so percentage inheritance breaks) - `history.js`: `#layout > #login` -> `#layout #login` (direct-child selector breaks because `#login` is now a grandchild of `#layout` through `.content`)
Adjust login.less to match the HTML structure emitted by CompatForm: - Add width: unset and margin on `.icinga-form` inside `.login-form` so the ipl form does not overflow its container and control groups have spacing - Restyle `.errors` at the li level rather than the container level, giving each error message its own red box with margin between them - Remove the padding from `.errors`/`.form-errors` that was set at the container level (now on each li) - Change `.remember-me-box` align-items from `flex-start` to `center` so the toggle switch and label sit at the same baseline Also fix a pre-existing typo in `login-orbs.less`: `#orb-notifactions` -> `#orb-notifications`
Ensure `.errors` inside `.login-buttons` has no extra padding and that the last error item retains a bottom margin, matching the layout used for the primary login form errors.
Prevents the linked page from accessing `window.opener` and stops the referring URL from being sent in the `Referer` header.
|
@lippserd Do you have a releasedate for icingaweb2 2.14.0, I think this might brake my oidc implementation |
This PR replaces the legacy Zend-based login form and authentication controller with their ipl equivalents, and extracts the login page view script markup into a reusable widget.
Convert
LoginFormtoCompatFormLoginFormnow extendsCompatForm, replacinginit()/createElements()with__construct()/assemble()and Zend decorator arrays with inline ipl decorator objects. TheCsrfCounterMeasureandFormUidtraits are added.getRedirectUrl()is renamed tocreateRedirectUrl()to avoid overridingCompatForm::getRedirectUrl(), usinghasBeenAssembledinstead of$this->createdandstr_contains()instead ofstrpos().onSuccess()is rewritten for the CompatForm lifecycle: void return, explicitsetRedirectUrl()call,Icinga::app()->getResponse()instead of$this->getResponse(), andaddMessage()instead ofaddError().New
LoginPagewidget to replace view scriptExtracts the login page structure (logo, footer, social links, decorative orbs) from
login.phtmlinto a newLoginPagewidget extendingHtmlDocument, so the markup is defined in one place and no longer lives in a view script. Additional login buttons provided byLoginButtonHookare now grouped in adiv.login-buttonsflex container, rendered only when buttons are actually present.Convert
AuthenticationControllertoCompatControllerAuthenticationControllernow extendsCompatController, droppinglogin.phtmlin favour ofLoginPageandaddContent(). The form is built with anON_SUBMIThandler for the redirect and anON_REQUESThandler delegating toLoginForm::onRequest(). Uses$this->getServerRequest()instead ofServerRequest::fromGlobals().CSS / JS fixes
login.less:height: 100%->100vhon#login(now a grandchild of#layoutvia.content).login-buttonsstyled as a flex column with a row gap and top marginlierror styling, including spacing inside.login-buttons.icinga-formwidth and control-group spacingalign-items: centeron.remember-me-boxmargin-rightreset for the disabled-state descriptionhistory.js:#layout > #login->#layout #login(direct-child selector broke with the new nesting)login-orbs.less: fixes long-standing typo#orb-notifactions->#orb-notificationsresolve #5495