[#37910] - Refactored WebAuthn with Windows Hello support
- Fixed in Code Base
- 27 Jun 2022
- Medium
- Build: 4.2-dev
- # 37910
- Diff
- nikosdion:feature/webauthn-refactor-2
User tests: Successful: 2 Kostelano, brianteeman Unsuccessful: 0
Summary of Changes
- Refactored as a native Joomla 4 plugin, using a service provider and implementing SubscriberInterface
- Removed the ugly
Joomlahelper class, replacing it with native code - Now using the (previously undocumented) WebAuthn library's
WebAuth\Serverobject. This adds Windows Hello support without having to update to a new major version of the third party WebAuthn library - We no longer pass URLs as data arguments; we can figure them out using
Joomla.getOptions - Fixed the breakage introduced in #37464
The PR replaces #37673 and #37675
Testing Instructions
Please remember to run npm ci after applying the PR — the JavaScript has changed.
Please remember to use HTTPS with a certificate trusted by your computer; WebAuthn doesn't work on plain HTTP.
Please use a relatively recent (2019 onwards) build of Chrome, Edge, Firefox etc.
Go to your user profile in the backend of the site.
Click on the ‘W3C Web Authentication (WebAuthn)’ tab.
On a Windows computer without any hardware authenticator attached click on Add New Authenticator.
Actual result BEFORE applying this Pull Request
The browser asks you to plug in an authenticator.
Expected result AFTER applying this Pull Request
You can enter your PIN / show your face / use a fingerprint scanner to register Windows Hello as an authenticator.
Further testing
Delete the authenticator and try adding it again in the user profile page in the frontend of the site. It should still work.
Make sure that in the frontend you can delete an authenticator you added in the backend.
Make sure that in the backend you can delete an authenticator you added in the frontend.
Please make sure you can add more than one authenticators. IMPORTANT! You cannot add the same authenticator twice (in the past you could; it was a bug that went unnoticed). You can only test this if you have more than one authenticators, e.g. Windows Hello, a FIDO or FIDO2 hardware authenticator, an Android phone and so on.
Please make sure that you can edit the name of the authenticator. This was broken in #37464 and was still broken at the time I made this PR.
Please make sure you can log into the front- and backend of the site.
Please test on as many platforms as you have: Android (works on Android 9 and later if you have a fingerprint scanner but only on Chrome as far as I know), iOS/iPadOS (both TouchID and FaceID), macOS (TouchID, if you have a MacBook Air/Pro or an iMac/Mac Studio with Apple Silicon and the Apple keyboard with a TouchID sensor) as well as various FIDO and FIDO2 authenticators. I have tested all of these and Linux EXCEPT for Android due to lack of hardware running Android (my Android phone's battery bloated, I had to decommission it before it spontaneously turned into an incendiary grenade). Edit: I now have a Samsung Galaxy A21 device for testing with face recognition and fingerprint scanner which I used to confirm it works with Android.
Translation strings
The following language strings were added:
PLG_SYSTEM_WEBAUTHN_ERR_XHR_INITCREATEPLG_SYSTEM_WEBAUTHN_FIELD_ATTESTATION_SUPPORT_DESCPLG_SYSTEM_WEBAUTHN_FIELD_ATTESTATION_SUPPORT_LABELPLG_SYSTEM_WEBAUTHN_LBL_DEFAULT_AUTHENTICATOR
The following language strings were changed:
PLG_SYSTEM_WEBAUTHN_LBL_DEFAULT_AUTHENTICATOR_LABEL
Documentation Changes Required
As of Joomla! DEPLOY_VERSION the WebAuthn plugin has attestation enabled by default. This means that only authenticators with publicly verifiable cryptographic signatures can be registered with WebAuthn starting with this version of Joomla.
The publicly verifiable certification authorities for authenticators are retrieved from the FIDO Alliance site, namely the URL https://mds.fidoalliance.org/.
This default setting will prevent some cheaper authenticators which are not FIDO-certified from being used with WebAuthn. Moreover, some sites may be unable to download and/or cache the root certificates from FIDO Alliance, or it might take so long that the plugin aborts the operation to prevent your site from timing out. If you encounter any problems with registering authenticators with WebAuthn please edit the plugin settings and disable the Attestation Support option.
The Attestation Support feature requires the following prerequisites to work:
- Your site must be able to access directly the URL
https://mds.fidoalliance.org/. - Your cache folder (
administrator/cache) must be writeable by PHP. - The system temporary directory must be writeable by PHP.
- The OpenSSL extension must be installed and enabled on your site — this is a requirement for WebAuthn as a whole.
If these prerequisites are not met the WebAuthn plugin will proceed without verifying the cryptographic signatures of the authenticators against the publicly verifiable certification authorities published by the FIDO Alliance. This is still secure — in fact far more secure than using a password and Two Factor Authentication. The only downside is that you may experience a short delay, up to 5 seconds, once a month when the plugin attempts to download the root certification authority information from the FIDO Alliance.
If your site meets all of the prerequisites except the first one you may download the information from https://mds.fidoalliance.org/ and place them in the file administrator/cache/fido.jwt. In this case the WebAuthn plugin can operate with attestation support. This is very useful if your site is behind a firewall or disconnected from the Internet (e.g. on a high security intranet handling sensitive material). You need to remember to update this file once every month to avoid any problems.
Enabling the Attestation Support feature also allows Joomla to identify the maker and model of most FIDO2 certified authenticators. If you register an authenticator after enabling this option you will see an icon of the maker's logo next to the Authenticator Name when viewing the list of authenticators. Furthermore, registering a new authenticator will have a more user-friendly default name, e.g. “Yubikey 5Ci added on 28 April 2022, 18:00” instead of “Authenticator added on 28 April 2022, 18:00”.
If you disable the Attestation Support option the logo and the authenticator type will be hidden.
Finally, do note that authenticators added with previous versions of Joomla or while the Attestation Support feature is disabled or while the Attestation Support feature is enabled but its prerequisites not met will always be displayed as “Generic Authenticator” as the necessary information to determine the make and model of the authenticator will have not been relayed to Joomla when you registered your authenticator.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Unit Tests Administration Language & Strings Repository NPM Change JavaScript External Library Composer Change Layout Libraries Front End Plugins |
| Labels |
Added:
?
Language Change
Composer Dependency Changed
NPM Resource Changed
?
|
||
@brianteeman Windows, Android or something else? And which browser? It's been a while since I wrote the code, it'll be easier for me to test myself with a similar environment to the one you are using :)
Are you accessing the local server as localhost or using a (locally resolved to 127.0.0.1) domain name? In the former case I expect it to fail.
| Category | Unit Tests Administration Language & Strings Repository NPM Change JavaScript External Library Composer Change Layout Libraries Front End Plugins | ⇒ | Unit Tests Administration com_admin com_associations com_banners com_cache com_categories com_config com_contact com_content com_fields com_finder com_installer com_joomlaupdate com_languages com_media com_menus com_modules com_newsfeeds com_postinstall |
@brianteeman Fixed. Try again now, please.
That problem is fixed.
Next problem is
I kind of expected the error message (for reasons stated above) but not the deprecated messages
Can't do anything about the deprecated warnings. As you can see, they come from third party libraries and I was explicitly told that Joomla's sever promise extends to third party libraries included via Composer even if they are only consumed internally.
The invalid certificate chain error happens because you do not have the PHP OpenSSL extension enabled and you do not have the openssl command line programme on your server either. Again, nothing I can do about.
@brianteeman Also, FWIW, both the deprecated notice and the invalid certificate message will happen with the WebAuthn plugin as included in Joomla 4.0. They were NOT introduced with this PR. They come from third party code which as I already explained I'm not allowed to update...
@brianteeman I also received an email about two suggestions you made... but they are nowhere to be found on GitHub?!
Can't do anything about the deprecated warnings. As you can see, they come from third party libraries and I was explicitly told that Joomla's sever promise extends to third party libraries included via Composer even if they are only consumed internally.
ok
The invalid certificate chain error happens because you do not have the PHP OpenSSL extension enabled and you do not have the openssl command line programme on your server either. Again, nothing I can do about.
What I expect - was just reporting. Disabling atestation resolved that
@brianteeman I also received an email about two suggestions you made... but they are nowhere to be found on GitHub?!
sorry - deleted them and replaced with a new suggestion
| Labels |
Removed:
Language Change
Composer Dependency Changed
NPM Resource Changed
|
||
@nikosdion We have 2 conflicting files now after the last upmerge from 4.1-dev to 4.2-dev, both caused by PR #37676 . Do you think you can fix them? Let me know if you need some help with that (but I'm sure you won't need some).
| Category | Unit Tests Administration com_admin com_associations com_banners com_cache com_categories com_config com_contact com_content com_fields com_finder com_installer com_joomlaupdate com_languages com_media com_menus com_modules com_newsfeeds com_postinstall | ⇒ | Unit Tests Administration Language & Strings Repository NPM Change JavaScript External Library Composer Change Layout Libraries Front End Plugins |
@richard67 It should be all good now
When editing an authenticator:
And, returning to the issue of confirming the deletion (as it was eventually implemented in the MFA), we delete the authenticator without confirmation. I would still prefer to have a second "intentional" click before deleting.
roland-d
- | Labels |
Added:
Language Change
Composer Dependency Changed
NPM Resource Changed
|
||
@Kostelano Both points addressed :) If you're happy with it I think it's ready to merge.
I think it needs again 2 human tests.
@richard67 Maybe @Kostelano and @brianteeman would be able to provide their successful tests again. The bulk of the code which actually introduces the new feature is unchanged the past couple of months, really.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37910.
@nikosdion Hmm, drone reports a javascript code style error here https://ci.joomla.org/joomla/joomla-cms/55438/1/26 :
/********/src/build/media_source/plg_system_webauthn/js/management.es6.js
376:5 error Arrow function expected no return value consistent-return
Could you fix that?
@richard67 There you go
I've restored the previous human test result because the change after that was not really functional but code style.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37910.
my branch was not 100% up to date
@Kostelano Could you test this PR again? I know, you did already a few times, but there have been some changes which were not code style only. Thanks in advance.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37910.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37910.
/me doing a happy dance!
Guys, this is awesome. Between this and MFA we can market Joomla 4.2 as the security and privacy enhancement update. Yay! Joomla rocks!
| Labels |
Added:
?
|
||
Quy
- | Status | Ready to Commit | ⇒ | Pending |
Also I tried the PR injecting the Application by hand and wasn't able to use fido and/or windows hello.
After downgrading to beta2 I was able to login with fido again.
The reason maybe is because I also have the yubi key activated as 2fa method too. (fido and MFA)
Completely unrelated to this login issue when I rename the second key in the webauthn list and press enter it changes the first key to an input field and doesn't save the the key I try to rename.
| Labels |
Removed:
?
|
||
That pr breaks quite a bit.
Well, #38060 has broken plenty of things as it looks like.
The MFA is currently broken as well. As a result I cannot test my blind fix which I just committed.
based on the other pr you shouldn't extend the constructor instead you should implement the ApplicationAwareInterface and inject the Application in the service provider.
Disabling the MFA plugins by editing the database I was able to confirm that my fix worked.
The real problem is that #38060 should have never been merged and I am looking at you @roland-d . Architectural work MUST ALWAYS be split in separate PRs: one PR to do the architectural work with 100% backwards compatibility and adding deprecations for what is being replaced. Then a number of PRs making changes in the affected areas with thorough testing.
For example, I see that in plg_multifactor_auth @laoneo just removed $this->app and added the application by injection. But he DID NOT EVEN CHECK that $this->app is no longer present in the plugins/multifactorauth/webauthn tree! Had he done this basic check he'd have seen that there's tmpl/default.php which is still using it. This is what was causing a problem for me trying to fix the problem @HLeithner reported.
Whenever I see something AS BASIC AS THAT not being done in a PR I only have one recommendation to make: revert the PR as soon as possible. It was not ready to get merged.
When I submit a PR you nitpick on it and I have to spend 10x the time I spent writing code just to address your pointless nitpicking. Yet when one of your “inner circle” submits a PR which breaks userland what do you do? Oh, right, zero discussion, just merge it and break userland! Then you get upset when I call you hypocrites and I am labelled "difficult to work with". To be clear: what I want is that EVERYONE's Pull Requests undergo the SAME amount of scrutiny and that amount of scrutiny has to be close to or equal to the scrutiny you put mine. Otherwise you ARE hypocrites and you are not doing right by the community who trusted you with commit rights.
Now you are telling me off for extending the constructor. You know what? Fuck it.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-06-23 15:08:20 |
| Closed_By | ⇒ | nikosdion |
I did try and flag that it was way too late in the release cycle for the type of changes in that pr #38103 (comment)
Now you are telling me off for extending the constructor. You know what? Fuck it.
I was not involved in the other pr, I only looked how he did it and suggested it. Aside this yes the PR is really ugly and wrong. Reverting it doesn't sound like a bad idea problem at it is that beta2 has released and we should fix it in a way that we keep the interface and the trait. The rest should be done as you said.
I did try and flag that it was way too late in the release cycle for the type of changes in that pr #38103 (comment)
Actually, the PR #38103 is fine as it is. It is kind of bug fix for the ugly code, it does not break anything and could be merged, even for patch release. But It is up to maintainer to decide.
It was a general flag and not specific. I dont have the skills to comment on the specific code but I do have 15 years of watching that type of pr being merged without tests just before releases and watching them blow up.
@nikosdion Don't be childish. Please re-open this PR.
Dude, this is now how you de-escalate :)
When my ISP decides to fix the problem which causes no service to the entire area I will re-open the PR. Can’t do it on mobile as I can’t push the branch again.
On another note, once I do that please do take over any necessary changes. I hate having to do that to you but it’s for medical reasons.
I believe in full transparency and I have no problem explaining this further. I’m on ADHD medication which helps me deal with memory issues and impulsiveness. It does, however, affect the autonomic nervous system. As a result I got a dangerously high resting heart rate the last two weeks. We tried lowering my dose, that only made me irritable (well, as you know) so now I have to go through a whole lot of tests to see if I can also get beta blockers. Until then I am to reduce my stress levels which unfortunately means I can’t contribute to Joomla, among other things I like but stress me out that I have to forego temporarily.
| Status | Closed | ⇒ | New |
| Closed_Date | 2022-06-23 15:08:20 | ⇒ | |
| Closed_By | nikosdion | ⇒ |
| Status | New | ⇒ | Pending |
@richard67 There you go. Good luck, folks.
Thanks, and all the best for you.
@HLeithner For doing the changes you recommended, I would need advice. Or you make a PR. If necessary I can make a branch in my repo and you can do a PR against that so we don’t bother Nik. I really want this PR to be finished. I would have tested it myself but I am not really familiar with all these authenticators.
@nikosdion thanks for sharing your private situation. Now I can put your words in the right context. If you want we can have a private talk and I can explain you the current situation why all this architecture work is happening. And I hope you understand that it was never my intention to add BC breaks. But at the end of the day we are all humans and do make mistakes, important is that we solve them. And final words from me, don't blame or shitstorm Roland, I was the person how made that stuff and put pressure on him for merges.
@richard67 I created the nikosdion#9 pr which (also blindly) should bring this PR in sync with the request of the cms plugins.
That didn't fix the 2fa problem yet. For this we have to find out why it doesn't get redirect to the correct page
@richard67 I created the nikosdion#9 pr which (also blindly) should bring this PR in sync with the request of the cms plugins.
@HLeithner I can't merge in @nikosdion repo, so either he merges it, or I have to take over this PR here by making a new one (using a branch in my repo based on Nik's branch here so commit history would not get lost).
I can merge PR in my repo. Just at-me so I know when I have to.
@HLeithner Is there something more to do after your PR for this branch has been merged by Nik? If not and this PR is ok now: Could you dismiss or resolve your review so that GitHub doesn't show anymore "Changes requested" in the list of PRs? It's something GitHub does when there are unresolved reviews, not to be mixed up with the "Updates Requested" label which we set.
I only die the low hanging stuff but didn't hat the time to check why the redirection to the MFA didn't work when fido is used
That didn't fix the 2fa problem yet. For this we have to find out why it doesn't get redirect to the correct page
@HLeithner Ah, I just see there is still something to be done. Or is that the thing solved with PR #38132 ? I don't think it is, but I'm asking to be sure.
@brianteeman would you be so kind and test if the error exists for you too? maybe it's my installation
Wont get a chance until tomorrow am
I updated Joomla with this PR. Deleted / Created a fingerprint key, everything works. I couldn't get any errors.
If you need to check a certain list of actions, please tell me which one - I haven't caught it yet.
I updated Joomla with this PR. Deleted / Created a fingerprint key, everything works. I couldn't get any errors.
not only fingerprint, please add a 2fa authentication like google authenticator
testing now
Completely new install directly from nikosdion:feature/webauthn-refactor-2
logged into the admin and went to my user account and the W3C Web Authentication (WebAuthn) Login tab
successfully added a windows hello pin
went to the Multi-factor Authentication tab and clicked on the Add a new web authentication
error has occurred
An error has occurred.
0 Call to a member function getDocument() on null
Call stack
| Function | Location
1 | () | JROOT/plugins/multifactorauth/webauthn/tmpl/default.php:36
2 | include() | JROOT/plugins/multifactorauth/webauthn/src/Extension/Webauthn.php:146
3 | Joomla\Plugin\Multifactorauth\Webauthn\Extension\Webauthn->onUserMultifactorGetSetup() | JROOT/libraries/vendor/joomla/event/src/Dispatcher.php:486
4 | Joomla\Event\Dispatcher->dispatch() | JROOT/administrator/components/com_users/src/Model/MethodModel.php:102
5 | Joomla\Component\Users\Administrator\Model\MethodModel->getRenderOptions() | JROOT/administrator/components/com_users/src/View/Method/HtmlView.php:118
6 | Joomla\Component\Users\Administrator\View\Method\HtmlView->display() | JROOT/administrator/components/com_users/src/Controller/MethodController.php:120
7 | Joomla\Component\Users\Administrator\Controller\MethodController->add() | JROOT/libraries/src/MVC/Controller/BaseController.php:746
8 | Joomla\CMS\MVC\Controller\BaseController->execute() | JROOT/administrator/components/com_users/src/Controller/MethodController.php:75
9 | Joomla\Component\Users\Administrator\Controller\MethodController->execute() | JROOT/libraries/src/Dispatcher/ComponentDispatcher.php:146
10 | Joomla\CMS\Dispatcher\ComponentDispatcher->dispatch() | JROOT/libraries/src/Component/ComponentHelper.php:389
11 | Joomla\CMS\Component\ComponentHelper::renderComponent() | JROOT/libraries/src/Application/AdministratorApplication.php:144
12 | Joomla\CMS\Application\AdministratorApplication->dispatch() | JROOT/libraries/src/Application/AdministratorApplication.php:187
13 | Joomla\CMS\Application\AdministratorApplication->doExecute() | JROOT/libraries/src/Application/CMSApplication.php:296
14 | Joomla\CMS\Application\CMSApplication->execute() | JROOT/administrator/includes/app.php:63
15 | require_once() | JROOT/administrator/index.php:32
Oh that's a new issue^^
@brianteeman Could you test again? I think your issue has been fixed with PR #38132 , and that just has been merged and now I have update the branch of this PR here so it has that change, too.
@richard67 that resolved that issue. Now what was the issue @HLeithner has - i cant see it here
@richard67 that resolved that issue. Now what was the issue @HLeithner has - i cant see it here
That's a good step forward.
I also installed the current version and I have the same result. Additional I got an error message when trying to save the user with 2 x webauthn + 1x 2fa methods which seems to be related to the privacy action log:
Save failed with the following error: Field 'extensions' doesn't have a default value
After disabling the user action log plugin I was able to save the user again.... maybe unrelated? didn't checked the code yet if it creates an action log entry.
The "the user action log plugin" problem is a php 8.1 problem.
So, guys, good news, I am medically cleared for action. Well, at least coding action. Do you mind if I jump right back in?
So, guys, good news, I am medically cleared for action. Well, at least coding action. Do you mind if I jump right back in?
sure I just want to merge this "now" because I would like to start the PSR-12 merges and since the PSR convert script for PRs adapts the complete file it's hard so see the changes afterwards especially for such complex prs.
The plan was to start PSR-12 at 16:00 UTC (now)
Give me half an hour or so to check all events creations. I think I have an idea about what happened.
Give me half an hour or so to check all events creations. I think I have an idea about what happened.
sounds good.
@HLeithner Done.
When moving the plugin to concrete events I made a mistake in the way I was creating the events. I have updated the code to work similar to how I had updated triggerEvent to work so when we create concrete events for user login / login failure nothing will break.
@HLeithner Done.
When moving the plugin to concrete events I made a mistake in the way I was creating the events. I have updated the code to work similar to how I had updated triggerEvent to work so when we create concrete events for user login / login failure nothing will break.
ok will test it and merge it afterwards
@nikosdion can you change the 3 files having a license issue because you copy-paste it from upstream, they have to stay MIT with the correct reference to at least that's my understanding.
Beside the license issue, I tested this successful.
When you are combining GPL code with any GPL-compatible (e.g. MIT licensed) code the entire combined product becomes GPL. So even if we note these files as MIT licensed since they are part of our GPLv2 product they are effectively licensed under GPL when the software is distributed to the users. See https://www.gnu.org/licenses/gpl-faq.html#WhatDoesCompatMean
Sure, I can change the license of these files but it's pointless to the effect that the mere act of putting them on GitHub as part of Joomla constitutes distribution therefore they are effectively licensed as GPLv2 (even though they were forked off MIT code).
License notes changed per your request
License notes changed per your request
Thanks, better give proper contribution then nothing even if it's maybe legal but I'm not a layer so I hope that fits our needs.
| Status | Pending | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-06-27 17:16:09 |
| Closed_By | ⇒ | HLeithner |
So I merge this for now and if we find problems we can fix them after the psr-12 merge.
thanks
Sorry to be a pain but this doesnt satisfy the terms of the licence
The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.
When you are combining GPL code with any GPL-compatible (e.g. MIT licensed) code the entire combined product becomes GPL. So even if we note these files as MIT licensed since they are part of our GPLv2 product they are effectively licensed under GPL when the software is distributed to the users. See https://www.gnu.org/licenses/gpl-faq.html#WhatDoesCompatMean
Sure, I can change the license of these files but it's pointless to the effect that the mere act of putting them on GitHub as part of Joomla constitutes distribution therefore they are effectively licensed as GPLv2 (even though they were forked off MIT code).
The combined product is GPLv2 but the original copyright on the individual files does not change and the terms of that licence must still be adhered to within those files.
Can you create a pr to fix this? I think you have the best knowledge on this topic?
@brianteeman I linked to the file with the notice just like they do in the library itself. If you think you need to copy the entire MIT license text in every file please do so in a new PR.
@nikosdion yoiu missed the copyright.
I did a pr #38159
Trying to test this and I keep getting the error message "Cannot get the authenticator registration information from your site."
This could be because its a local server with a self signed cert in which case I will just move along but thought I should check with you first