horde
Login-related changes - needed for TOTP
Proposed changes:
- Redesign code related to
$loginParmshandling in login.inc template - Move mode selector and most other hardcoded parameters and checks from the template to
login.php - Add support for extra properties via optional 'extra' parameter
- Add support for optional div wrapper via 'div' parameter
- Add
autocomplete=offfor OTP (second factor) input
Let's unbundle the login redesign from the fix. If I find time I will split it off myself. The fix part is OK and easy to merge.
The feature needs more review and I don't want to merge it right now.
- smartmobile mode login screen is ignored
- If display options are wrong in the inc file, they are wrong in the login.php file, too. Both variants cannot be unit tested easily.
- Language selection already reacts to a config file.
- Let's not mix the specifics of display with the availability of options on the feature level in new / refactored code.
- Templates directory can be outsourced to another dir via the registry. This allows admins to locally override defaults. Moving business logic from templates to code is mostly good but moving display logic to the non-customizable part calls for trouble. One can move building the widgets to a view_helper class though.
- Inline styling should move to css files as much as possible
Split off "fix" part into PR #22 and tested separately. Please rebase and let's think about this more before spending more work.
- What is the intended effect of the remaining feature on administrators or developers
- Do you need this for your own site?
- Do we need this before GA release?
Please rebase and let's think about this more before spending more work.
Done.
What is the intended effect of the remaining feature on administrators or developers
It should make things easier for administratos and developers.
Do you need this for your own site?
No. Note, autocomplete=off is needed for everyone.
Do we need this before GA release?
Absolutely.
It all started with the plan to simply add autocomplete=off for the second factor box. I looked inside of login.inc and realized that it is a collection of hardcoded checks, and the only way to add what i need without redesign would requre to add yet another hardcoded thing like <?php echo $key == 'horde_secondfactor' ? ' autocomplete="off"' : '' ?>, which makes it rely on exact naming coming from login.php, which is not good.
Instead we can now inject custom properties by using extra parameter, as simple as this:
'extra' => [ 'autocomplete' => 'off' ]
Not only this is cleaner, but also the code also applies htmlspecialchars [to the values].
The whole idea is to make login.inc unaware of exact names/parameters as much as possible by making sure all the custom things are kept in $loginParms. As I mentioned, the only hardcoded part that is left (which would be to trivially change too) is related caps-lock warning.
I would like to reiterate, that the proposed changes do not affect the existing logic or the resulted HTML output, just make things cleaner and easier to customize.
I tested it and compared original and new version output byte-by-byte with several scenarios (including type=hidden type). There are only minor white-space differences.
See #12 also. While this PR does not attempt to fix it, I believe it needs to be done before GA release.
This needs more testing. In my local instance the smartmobile login screen no longer worked after this patch.
This needs more testing. In my local instance the smartmobile login screen no longer worked after this patch.
I just re-tested - the smartmobile login screen worked just fine for me.
As far as I can see, it is not affected by this PR (it uses smartmobile.html.php, not login.inc).
There is also room for further enhancements:
- move caps-lock warning stuff to
$loginParms - move language selector to
$loginParms(it is already in$loginParmsfor smartmobile mode, so it is only logical)