[4.0] Correcting WebAuthn display in login menu item by infograf768 · Pull Request #32196 · joomla/joomla-cms

infograf768 · 2021-01-30T09:36:21Z

Pull Request for Issue #32182

Summary of Changes

#31545 forgot to modify com_users login default_login tmpl and login modules

Testing Instructions

Set webautn to display when login.
Create a login menu item.

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Note

This PR does not solve the tooltip display in the menu as well as in the login module
PLG_SYSTEM_WEBAUTHN_LOGIN_DESC="Login without a password using the W3C Web Authentication (WebAuthn) standard in compatible browsers. You need to have already set up WebAuthn authentication in your user profile."

infograf768 · 2021-01-30T10:00:14Z

@nikosdion
Does this break your former PR?

nikosdion

This PR breaks an important feature and needs to be changed before being considered for merging.

nikosdion · 2021-01-30T10:08:41Z

components/com_users/tmpl/login/default_login.php

-								<?php echo HTMLHelper::_('image', $button['image'], Text::_($button['tooltip'] ?? ''), [
-									'class' => 'icon',
-								], true) ?>
+								<?php echo $button['image']; ?>


This change is wrong and I consider it a blocker to merging this PR.

The image parameter returned by additional login button plugins is NOT meant to be an absolute URL. It's supposed to provide a URL relative to the media folder which can be overridden in the template. That's why it goes through HTMLHelper::image.

This is an important feature to preserve. By going through HTMLHelper the site integrator can drop in an image in media/templates/site/YOUR_TEMPLATE/... to override the image used by the additional login button plugin. With your change it's no longer possible. The site integrator would have to core hack the plugin itself(!!!) to change the image or do other unholy things, like hiding the image with CSS and using some sort of ::before magic to render a different image. Not exactly what I'd call as easy or practical or even feasible as dropping a file in a folder.

If you have a problem with the image for WebAuthn not being displayed the solution would be to edit the WebAuthn plugin to send the correct relative path.

I was sure there could be an issue. This was why I called you.
The problem lies in a double use of the htmlhelper.
Any proposal to help solving the issue in the plugin itself as it uses an inline SVG?

The solution is practically screaming at you. The plugin (plugins/system/webauthn/src/PluginTraits/AdditionalLoginButtons.php) needs to be corrected in line 165. Instead of 'image' => $image, you need to do 'svg' => $image, Then revert the change you made in your PR in the components/com_users/tmpl/login/default_login.php file regarding the image parameter.

As you can see, if you pass an svg parameter it's output verbatim. @brianteeman added that on January 2nd exactly so that you can use inline SVG files. The same code needs to be copied to modules/mod_login/tmpl/default.php and administrator/modules/mod_login/tmpl/default.php.

If you look at what's going on, using Git blame, you will see that my original contribution only had a provision for going through HTMLHelper::image. The inline SVG came later and, frankly, the lack of direction or understanding of where that code comes from (nobody heard of Git blame?) led to some code being partially there, some code being modified without ever working (AND MERGED! WTF!) and the three files I had carefully modified to implement additional login buttons are now all out of sync, each one working in a different and mutually incompatible way. Little wonder shit's broken all over the place, don't you bloody think?!!!

The PRs which made these changes should have never been merged. If anyone had looked at them they'd have figured out why. Instead, they got two ostensibly successful tests — even when the first test in this PR clearly demonstrated why these tests mean sod all in practice, as I've been saying for years — and someone with commit rights mashed that green GitHub merge button without thinking and of course without being held accountable. What are you doing @wilsonge @HLeithner ? Are you still running this project or is your role confined to applying major changes after beta 6? This is a serious question. If you think that Joomla's problem is whether it ships with BS4 or BS5 but not whether it actually, dunno, bloody works consistently or even at all then I think that your priorities are misaligned with the priorities of the people who actually try to use the darned product.

what have I done wrong this time. afaict my only involvement was to fix where a pr had been applied in one place but not another

@brianteeman Your PR was the only thing that wasn't broken. Another commit removed the code that was applied elsewhere. Yet another one broke the plugin to begin with. Your code is the last remaining example of the correct approach (use a different key for inline SVG) and should be copied over in the other two places.

I had tested already changing image to svg in the plugin and not modifying the default_login.php
In that case. it was working OK for the menu item but I lost the image in the login module.
I did not think about adding the svg conditional in the modules. :)
Modifying PR now.
Thanks for the hint.

nikosdion · 2021-01-30T10:10:36Z

components/com_users/tmpl/login/default_login.php

 					<div class="controls">
 						<button type="button"
-								class="btn btn-secondary <?php echo $button['class'] ?? '' ?>"
+								class="btn btn-secondary w-100 <?php echo $button['class'] ?? '' ?>"


This change is OK with me. If a third party plugin needs to override the width it can do so with CSS.

ceford · 2021-01-30T11:14:39Z

I have tested this item ✅ successfully on 6cd62f7

I don't have a secret key but the form looks fine after applying the patch.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32196.}

nikosdion · 2021-01-30T11:45:41Z

@ceford Please withdraw your successful test because it's misleading. The PR as is fixes the form but breaks important functionality as I've written in my review of the PR in #32196

Please do not limit your source of information to the Joomla issue tracker site. It doesn't convey the entire picture (and really has no reason of existence, but that's another discussion for another day).

ceford · 2021-01-30T12:53:08Z

I have not tested this item.

I looked at the diff and it looked simple! But, untested as requested.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32196.}

infograf768 · 2021-01-30T16:55:46Z

@N6REJ
I am afraid my solution here is no good and the plugin has to be corrected.

able to use svg

infograf768 · 2021-01-31T09:49:56Z

@nikosdion @ceford
Modified PR as discussed above.
(tooltip is still not taken care of and should be the matter of another patch)

Please test again

ceford · 2021-01-31T11:18:16Z

It works as before for me - and I did read the discussion this time. However, being of a nervous disposition, I will wait for @nikosdion approval before pressing the pass button.

richard67 · 2021-01-31T20:30:33Z

It works as before for me - and I did read the discussion this time. However, being of a nervous disposition, I will wait for @nikosdion approval before pressing the pass button.

@ceford No need to be nervous. If your test was successful, press the button. If it later turns out it was a mistake, that can happen. It happens to me, too sometimes. But that should not make us scared from further testing. I'm not sure if Nik will report back when all is ok now.

ceford · 2021-01-31T21:31:22Z

I have tested this item ✅ successfully on 85c9df0

Looked at the places changed in the diff - all seem OK.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32196.}

nikosdion

Perfect! The code is consistent, elegant and (mostly) self-documenting. The original intent is preserved. The inline SVG implementation intent is restored. Things work. Well done!

nikosdion · 2021-02-01T06:16:59Z

@richard67 I always comment when I ask someone to fix something and they do. Just not on Sundays, man.

alikon · 2021-02-01T07:21:57Z

I have tested this item ✅ successfully on 85c9df0

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32196.}

alikon · 2021-02-01T07:22:12Z

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32196.}

richard67 · 2021-02-01T07:53:24Z

@richard67 I always comment when I ask someone to fix something and they do. Just not on Sundays, man.

@nikosdion Yes, and thanks for that. I haven't said you won't comment for sure, I just have said that it might not happen. I had the feeling @ceford was discouraged from marking his test result, and I wanted to encourage him again.

richard67 · 2021-02-01T07:54:16Z

Thanks!

infograf768 added 2 commits January 30, 2021 10:19

[4.0] Correcting WebAuthn display in login menu item

3520837

keeping svg in case of

6cd62f7

joomla-cms-bot added the PR-4.0-dev label Jan 30, 2021

infograf768 changed the title ~~4.0 webauthn login menu~~ [4.0] Correcting WebAuthn display in login menu item Jan 30, 2021

infograf768 mentioned this pull request Jan 30, 2021

[4] Login: WebAuthn-Img shows alt #32182

Closed

nikosdion suggested changes Jan 30, 2021

View reviewed changes

infograf768 marked this pull request as draft January 30, 2021 17:38

reverting changes in default_login.php correcting login modules to be

85c9df0

able to use svg

infograf768 marked this pull request as ready for review January 31, 2021 09:48

nikosdion approved these changes Feb 1, 2021

View reviewed changes

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 1, 2021

richard67 merged commit 900d311 into joomla:4.0-dev Feb 1, 2021

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 1, 2021

richard67 added this to the Joomla 4.0 milestone Feb 1, 2021

infograf768 deleted the 4.0_webauthn_login_menu branch February 1, 2021 09:13

Uh oh!

Comments

Conversation

infograf768 commented Jan 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Note

Uh oh!

infograf768 commented Jan 30, 2021

Uh oh!

nikosdion left a comment

Choose a reason for hiding this comment

Uh oh!

nikosdion Jan 30, 2021

Choose a reason for hiding this comment

Uh oh!

infograf768 Jan 30, 2021

Choose a reason for hiding this comment

Uh oh!

nikosdion Jan 30, 2021

Choose a reason for hiding this comment

Uh oh!

brianteeman Jan 30, 2021

Choose a reason for hiding this comment

Uh oh!

nikosdion Jan 30, 2021

Choose a reason for hiding this comment

Uh oh!

infograf768 Jan 31, 2021

Choose a reason for hiding this comment

Uh oh!

nikosdion Jan 30, 2021

Choose a reason for hiding this comment

Uh oh!

ceford commented Jan 30, 2021

Uh oh!

nikosdion commented Jan 30, 2021

Uh oh!

ceford commented Jan 30, 2021

Uh oh!

infograf768 commented Jan 30, 2021

Uh oh!

infograf768 commented Jan 31, 2021

Uh oh!

ceford commented Jan 31, 2021

Uh oh!

richard67 commented Jan 31, 2021

Uh oh!

ceford commented Jan 31, 2021

Uh oh!

nikosdion left a comment

Choose a reason for hiding this comment

Uh oh!

nikosdion commented Feb 1, 2021

Uh oh!

alikon commented Feb 1, 2021

Uh oh!

alikon commented Feb 1, 2021

Uh oh!

richard67 commented Feb 1, 2021

Uh oh!

richard67 commented Feb 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

infograf768 commented Jan 30, 2021 •

edited

Loading