Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored WebAuthn with Windows Hello support #37675

Closed
wants to merge 27 commits into from
Closed

Refactored WebAuthn with Windows Hello support #37675

wants to merge 27 commits into from

Conversation

nikosdion
Copy link
Contributor

@nikosdion nikosdion commented Apr 27, 2022

Summary of Changes

  • Refactored as a native Joomla 4 plugin, using a service provider and implementing SubscriberInterface
  • Removed the ugly Joomla helper class, replacing it with native code
  • Now using the (previously undocumented) WebAuthn library's WebAuth\Server object. 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 [4.1] webauthn table accessibility #37464

The PR replaces #37673

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.

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).

Translation strings

The following language strings were added:

  • PLG_SYSTEM_WEBAUTHN_ERR_XHR_INITCREATE
  • PLG_SYSTEM_WEBAUTHN_FIELD_ATTESTATION_SUPPORT_DESC
  • PLG_SYSTEM_WEBAUTHN_FIELD_ATTESTATION_SUPPORT_LABEL
  • PLG_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:

  1. Your site must be able to access directly the URL https://mds.fidoalliance.org/.
  2. Your cache folder (administrator/cache) must be writeable by PHP.
  3. The system temporary directory must be writeable by PHP.
  4. 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.

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.2-dev labels Apr 27, 2022
@brianteeman
Copy link
Contributor

Working my way through the tests.
When I try to remove an authenticator it fails with the following js error in the console

ncaught TypeError: Cannot read properties of undefined (reading 'querySelectorAll')
    at Object.e.plgSystemWebauthnDelete (management.min.js?e42299c3645fe9dd5d2ac5c502af8bce:formatted:172:44)
    at HTMLButtonElement.e.plgSystemWebauthnDeleteOnClick (management.min.js?e42299c3645fe9dd5d2ac5c502af8bce:formatted:216:7)

@brianteeman
Copy link
Contributor

IMPORTANT! You cannot add the same authenticator twice (in the past you could; it was a bug that went unnoticed).

Is it really a bug if I want to use windows hello fingerprint and windows hello pin?

And if you insist it is can there be a more friendly error message please instead of
InvalidStateError: The user attempted to register an authenticator that contains one of the credentials already registered with the relying party.

@richard67
Copy link
Member

@nikosdion It seems drone doesn't like your PR due to the many emojis in the description text.

@nikosdion
Copy link
Contributor Author

@brianteeman I will look into the removal of the authenticator, I think it's the same issue as the edit button.

Regarding the authenticator, you are viewing this the wrong way. The authenticator is one and the same: Windows Hello, the platform authenticator provided by Windows. How it's unlocked — be it a PIN or a fingerprint or a face scan or a PIV card or... — is an implementation detail of the authenticator which is NOT communicated to the WebAuthn consumer (the browser). The browser asks the authenticator “yo, authenticators, gather round and tell me if you know any of these registered public keys the server sent me” and the Windows Hello authenticator replies “eeeyup, one of these were issued by me, I can unlock it with my private key, don't ask me to do that again”.

The same goes for macOS where the platform authenticator can be unlocked by TouchID or double-clicking the side button of my paired Apple Watch. The unlocking gesture I use is an implementation detail. The browsers only see “macOS platform authenticator”.

@nikosdion
Copy link
Contributor Author

@richard67 Hahahaha! I BROKE Drone, AGAIN! It's a special talent. No worries, I will remove the Emojis.

@brianteeman
Copy link
Contributor

Regarding the authenticator

I understand what you are saying just not sure i agree. If I have two computers one with fingerprint and one without then it would be useful to have both fingerprint and pin setup

@brianteeman
Copy link
Contributor

... or as you know the state of my laptop the fingerprint doesnt always work so could use the pin as an alternative.

thats exactly how i login to the computer

@nikosdion
Copy link
Contributor Author

@brianteeman This is really up to the implementation on the OS side.

Seriously, what we were doing before wouldn't change that behaviour at all. We were just not sending to the browser the list of registered public keys when asking for attestation (adding authenticators), therefore the browser couldn't ask the authenticators if they already knew about them. On assertion (login) we were sending this list. Only the first public key of each authenticator was taken into account by the authenticator itself, though. All additional keys? Ignored. You could see that if you dumped the information from the database table and checked the stored signature counter. It only ever increased in the first authenticator.

Anyway, this is a moot point not up to discussion as the WebAuthn specification mandates the corrected behaviour.

@brianteeman
Copy link
Contributor

brianteeman commented Apr 27, 2022

Only the first public key of each authenticator was taken into account by the authenticator itself, though. All additional keys? Ignored.

To avoid misunderstanding are you saying that on 4.1 if i setup windows hello fingerprint and then windows hello pin only the fingerprint will work as a login method? That is not my experience

You could see that if you dumped the information from the database table and checked the stored signature counter.

Which table has the counter?

@nikosdion
Copy link
Contributor Author

nikosdion commented Apr 27, 2022

As per https://developer.mozilla.org/en-US/docs/Web/API/CredentialsContainer/create

excludeCredentials (Optional): An Array of descriptors for existing credentials. This is provided by the relying party to avoid creating new public key credentials for an existing user who already have some.

We pass the existing credentials for the user when we ask for a new attestation. Also see https://www.w3.org/TR/webauthn-2/#dom-publickeycredentialcreationoptions-excludecredentials

What will happen next is between the BROWSER and the AUTHENTICATOR(S) neither of which is under our control. That's the entire point of WebAuthn! We the Relying Party (site) know exactly jack shit about the internal workings of the Authenticator and the Client. We don't have the Authenticator's private keys, we don't know how the Authenticator interacts with the user to unlock itself for cryptographic operations, we do not care how the Client communicates with the Authenticator, we don't care how the Client decides that an Authenticator has already been used to create credentials for our Relying Party ID (our site's URL). We are oblivious to all of that by design of the WebAuthn specification itself.

If Windows Hello or any other authenticator uses multiple PRIVATE keys depending on how you unlock it, therefore rejects asserting a credential if you use the “wrong” way to interact with this Authenticator, this is not our business. We cannot and must not anticipate this as per https://www.w3.org/TR/webauthn-2/#sctn-security-considerations-rp points 4 and 5:

  1. The Relying Party can automatically support multiple types of user verification - for example PIN, biometrics and/or future methods - with little or no code change, and can let each user decide which they prefer to use via their choice of authenticator.
  2. The Relying Party does not need to store additional secrets in order to gain the above benefits.

In the end of the day, our responsibility as a Relying Party (site) begins and ends by sending the excluded credentials to the Client (browser) and letting it figure out the gory details.

@micker
Copy link

micker commented Apr 27, 2022

very cool feature ! i will test ASAP

@brianteeman
Copy link
Contributor

You could see that if you dumped the information from the database table and checked the stored signature counter.

Which table has the counter?

@brianteeman I will look into the removal of the authenticator, I think it's the same issue as the edit button.

Confirmed its the same issue and the same fix

@nikosdion
Copy link
Contributor Author

@brianteeman The credentials are stored in #__webauthn_credentials. However this the base64 representation of an encrypted JSON document. You should set a breakpoint at the last line of \Joomla\Plugin\System\Webauthn\CredentialRepository::findAllForUserEntity to see what is loaded from the database. Good luck!

@brianteeman
Copy link
Contributor

You are missing the point I am making about windows hello. Unless I am completely misunderstanding everything then windows hello is tied to the physical computer. So if I want to use windows hello with webauthn on both my work computer AND my home computer I need to be able to set up two instances of the windows hello authenticator. Doesnt matter if one is pin and the other fingerprint or both are fingerprint they are different because the harddware is different on the different pcs.

@nikosdion
Copy link
Contributor Author

Brian, this is TOTALLY NOT the use case you were discussing.

Each physical device's Windows Hello is a different authenticator. Likewise, each Apple device's TouchID/FaceID is a different authenticator. You can of course register different authenticators, therefore you'd be able to register different devices.

I actually have a test site where I've registered Windows Hello, a Yubikey 4, a Yubikey 5C, a Security Key NFC, two different Security Key (old FIDO version), my iPhone's FaceID, my iPad's FaceID, and my MacBook Pro's TouchID.

What you were saying before is that on the same device Windows Hello behaves differently when unlocked by PIN and fingerprint which sounds implausible but I can't test because of lack of hardware. Regardless of how you unlock it, Windows Hello will interface the TPM (hardware or software) which has a singular, device-specific private key. I don't see why the unlock method would make it work differently; if it does, that's a problem with Microsoft's implementation. Considering that they were among the parties to help define the standard I find it implausible.

Have you actually tried with different Windows computers or are you just speculating?

@brianteeman
Copy link
Contributor

ok remotely logged in to a second computer and that works fine with its own windows hello - so all good there. and I get it now.

it was the error message that really confused me and led me into the path of brainfailure and the way that windows presents its options

The user attempted to register an authenticator that contains one of the credentials already registered with the relying party.

@brianteeman
Copy link
Contributor

so other than the js error everything seems to be working fine - now i have my brain in gear - and as soon as you fix that one line of js I can mark it as a successful test

@nikosdion
Copy link
Contributor Author

Were you able to use a platform authenticator over RDP?! Windows 11 wouldn't let me do it, I had to be using a physical interface when I tried it yesterday. If that's the case I might resurrect Windows 10 on my old laptop to give this a go.

@brianteeman
Copy link
Contributor

I used nomachine to connect to my remote windows 10 computer and setup windows hello pin

would it be possible instead of giving the name as "authenticator added on date" to include the name of the actual authenticator eg "windows hello authenticator added on date"

@nikosdion
Copy link
Contributor Author

@brianteeman We do not get a human readable authenticator name. All you get is the AAGUID but we really don't want to have to trawl the Internet for all possible AAGUIDs and map them to human readable names (there is currently no registry; you have to check with each authenticator manufacturer).

@nikosdion
Copy link
Contributor Author

That should do the trick.

@brianteeman
Copy link
Contributor

That did fix it but to be honest with you I would drop the image. Doesnt look great (not your fault and you're not a designer). The inline styles will trigger some people and I'm pretty sure we dont need an ALT, Title and Aria

Otherwise everything in my tests worked well. Note that I have not tested what would happen when upgrading on a site that already has authenticators

@nikosdion
Copy link
Contributor Author

The images are those provided by the vendors 🤷🏽‍♂️

I think it's an okay solution to displaying the authenticator type when you have full attestation, maybe not so much if you're using multiples of the same authenticator. If people really hate it we can remove it and replace it with a small piece of text under the authenticator name.

@brianteeman
Copy link
Contributor

f people really hate it we can remove it and replace it with a small piece of text under the authenticator name.

I really hate it ;) But maybe the authenticators you have are all the same size image
What would the small piece of text say - can't imagine anything of value

image

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 71815ff


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37675.

@richard67
Copy link
Member

Appveyor (which does the ci checks on Windows) fails. It reports problems with the composer.lock file. See https://ci.appveyor.com/project/release-joomla/joomla-cms/builds/43381337 . Any idea what the problem could be?

@brianteeman
Copy link
Contributor

looks like ext-sodium is missing from the version of php on appveyor

@richard67
Copy link
Member

looks like ext-sodium is missing from the version of php on appveyor

Yes, that’s one of the reported problems.

@brianteeman
Copy link
Contributor

Yes, that’s one of the reported problems.

I think they are all related

@richard67
Copy link
Member

I‘ll try to get someone who knows more about the test set up than I do.

@nikosdion
Copy link
Contributor Author

nikosdion commented Apr 28, 2022 via email

@brianteeman
Copy link
Contributor

Brian is right.

@nikosdion
Copy link
Contributor Author

@richard67 There you go! I followed the suggestion in AppVeyor's output and it's working now. Basically, I tell it to ignore the fact that a dependency requires Sodium which isn't enabled and install it anyway. This is safe as we don't actually use the code that uses Sodium in AppVeyor.

@richard67
Copy link
Member

@richard67 There you go! I followed the suggestion in AppVeyor's output and it's working now. Basically, I tell it to ignore the fact that a dependency requires Sodium which isn't enabled and install it anyway. This is safe as we don't actually use the code that uses Sodium in AppVeyor.

@nikosdion I see, and I see it works. Thanks.

@richard67
Copy link
Member

@brianteeman I've restored your successful test result in the issue tracker since the commit which invalidated the test count has not changed anything on the tested code but only changed the test setup, see commit d2e20ce .

@nikosdion
Copy link
Contributor Author

@richard67 You may want to restore Brian's test again. I fixed a bug in the (old) WebAuthn library we are using and invalidated his test.

Also, I am continuing development on this as a third party plugin since it takes a very long time to get anything included in the core 😉 Some cool things I have implemented and will be able to contribute after this PR is merged:

  • Support for Apple Passkeys / synchronised authenticators. The WebAuthn specification says that clients may choose to synchronise authenticator credentials among several devices. Google, Microsoft and Apple have already come up with the plumbing and, shockingly, decided on a common standard. macOS 12.3 and iOS/iPadOS 15.4 have added preliminary support for this standard and call it Apple Passkeys (marketing name) / Syncing Platform Authenticator (developer-facing documentation). Enabling it is currently restricted to those with an Apple Developer account which I have, so I can tell you I figured out how to enabled support for it. Now, get this. This feature also allows a Mac computer to use an Android device as the authenticator, even in Safari! Reports of flying pigs are unconfirmed but deemed plausible in the light of this development.
  • Support for iOS/iPadOS 15.4 and later. Apple changed the attestation statement of its mobile devices to use the apple format instead of the packed format. Our very old copy of the WebAuthn library (2.x) does not support it but I just finished backporting it from version 4.x — and make it PHP 7.2 compatible. It wasn't that hard and solves the problem of WebAuthn not working properly on mobile Apple devices.
  • Support for resident authentication credentials (passkeys). Right now, our WebAuthn plugin only supports simple authentication credentials. These do not carry a user identification in them which is why you need to enter your username for password less authentication. Resident authentication credentials (marketing name: Passkeys) are different. They are stored in the authenticator device and contain the user identifier. Therefore you can log in without a password and without a username. It's magical! Caveat: hardware authenticators like FIDO2 USB-attached keys have limited memory and can only store 5 to 20 credentials. Before late 2021 you couldn't remove them; not the case anymore. Platform Authenticators, through, such as Windows Hello, Apple's TouchID/FaceID and Android biometrics have unlimited storage capacity. This means that you can reduce logging in to all of your Joomla sites to a biometric, PIN or device password check. No usernames.

All of these are features I implemented today in my copy of the plugin. I don't want to put them in this PR here as it'll get into massive scope creep, testability problems and we'll miss yet another Joomla version to ship improvements to WebAuthn. Once this PR is merged I'll do two PRs for these features. One for Apple stuff, one for the resident credentials.

I believe that this will put Joomla well ahead of any other CMS in terms of secure and easy authentication options. THE END (of passwords as means of authentication) IS NIGH!

* @return void
* @since __DEPLOY_VERSION__
*/
private function returnFromEvent(Event $event, $value = null): void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in the Event PR, I would say that should be moved to the (immutable?) Event class because this needs to be cause by all event listeners which needs/adds a result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I wrote this PR a month ago the Events PR wasn't merged :) I will circle back to this PR tomorrow and update it to remove the now no longer necessary trait. I suppose this gives us enough time to merge it in time for 4.2, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, after looking at this, I cannot remove this trait.

Right now the Joomla core code (and the third party event code) is trying to create a GenericEvent which does not implement ResultAware for the various generic events we handle (onUserAfterDelete, onUserLoginButtons, onContentPrepareForm, onContentPrepareData). We need to create concrete events for these core events before I can refactor this code.

What I can do as part of this PR is create concrete events for the various onAjaxWebauthn* events. I will do it now.

I suggest that the rest of the events are migrated in a different PR. We don't want to expend the scope of this PR to encompass completely unrelated features (scope creep is bad).

@nikosdion
Copy link
Contributor Author

I am no longer interested in contributing to a project which allows toxic people to call into question my integrity as a business owner and my ability as a software developer to write working software. Good luck figuring this out by yourself.

WARNING: DO NOT EVEN TRY TO COPY MY OWN CODE (WHICH IS LICENSED UNDER GPLv3).

@nikosdion nikosdion closed this May 25, 2022
@nikosdion nikosdion deleted the feature/webauthn-refactor-2 branch May 25, 2022 16:39
@richard67
Copy link
Member

@nikosdion Please think it over and restore your branch and reopen this PR. If it is really like you say, do you really think one person can damage your reputation? I don’t think so. Yes, in a community driven open source project there are all kinds of people, and some are not so easy to deal with. But as long as they don’t violate CoC, what shall we do? Let me know if there is anything I can do to make you change your mind. If necessary you can contact me via the contact form on my website and we can have a talk. I want peace and I do not want to lose your expertise for the project. Am saying this as a normal contributor, not speaking for anybody else, but I am sure many others fell like I do.

@nikosdion
Copy link
Contributor Author

@richard67 I am sorry, what?! Putting someone's integrity into question (i.e. calling someone a liar), even passively-aggressively, and attacking their experience is not a CoC violation? It's an ad hominem attack!

The same thing happened to me ten years ago when Andrew Eddie personally attacked me. The end result is that he got a bit less than a slap on the wrist and allowed to continue contributing (and abusing people) just fine while I was handed a six month ban for defending myself against an uncalled for personal attack. Unfortunately, nothing has changed in the last ten years. Victim blaming abound, toxic behaviour left unchecked, normalised and encouraged.

Let me put it this way. Would you like to work in a company where your co-workers call you a fraud and a liar? Then why do that without being paid and have this unfounded accusations levelled against you in public?! If contributions are punished then I shall not contribute.

@nikosdion
Copy link
Contributor Author

I redid this PR in #37910

roland-d pushed a commit that referenced this pull request Jun 4, 2022
)

* Captive TFA

Import YubiKey plugin

* Captive TFA

Prepare SQL for new plugins

* Captive TFA

Import Fixed plugin (EXAMPLE)

* Captive TFA

System plugin

* Captive TFA

Replace the two factor authentication integration in the core

* Captive TFA

Fix wrong SQL / table name

* Captive TFA

Use correct prefix in the TFA helper when getting config UI

* Captive TFA

Fix a whoopsie or four

* Captive TFA

Coffee has long stopped working

* Captive TFA

Format the Methods page

* Captive TFA

Fix wrong TFA method internal name

* Captive TFA

Make sure we get the right view in the controllers

* Captive TFA

Remove yet another integration of the legacy TFA

* Captive TFA

Automatic migration from old TFA upon first login

* Captive TFA

Frontend MVC

* Captive TFA

Frontend routing

* Captive TFA

Style the method select page

* Captive TFA

Missed a legacy integration which needs removal

* Captive TFA

Better format of the configuration UI in the profile page

* Captive TFA

Use language strings when migrating data from legacy TFA

* Captive TFA

Only show the prompt to add a TFA method if none is already added

* Captive TFA

YubiKey should allow entry batching

This means that you can authenticate with any registered
YubiKey in your user profile.

* Captive TFA

Replace Tfa::triggerEvent

* Captive TFA

Import WebAuthn plugin

* Captive TFA

Improve TFA behavior on non-HTML pages. Basically, block
them!

* Captive TFA

Replace alerts with Joomla messages

* Captive TFA

Move onUserAfterDelete code to the `joomla` user plugin

* Captive TFA

Remove the System - Two Factor Authentication plugin

Use a trait for the application and fold the rest of
the code into Joomla's core user plugin.

* Captive TFA

Remove accidental leftover references to loginguard

* Captive TFA

Import Code by Email plugin

* Captive TFA

Post-installation messages

* Captive TFA

Enable the TFA plugins on NEW installations

* Captive TFA

XML formatting

* Captive TFA

Language and grammar in comments

* Captive TFA

Rearrange XML attributes

* Captive TFA

Fix typo

* Captive TFA

Fix wrong language key name

* Captive TFA

Remove leftover legacy TFA options

* Captive TFA

Fix wrong CSS class

* Captive TFA

Merge the padding classes

* Captive TFA

This lang string should never have had a link

* Captive TFA

Hide the Key emoji from screen readers

* Captive TFA

Accessibility improvements

* Captive TFA

Accessibility improvements

* Captive TFA

Accessibility improvements

* Captive TFA

Accessibility improvements

* Captive TFA

Accessibility improvements

* Captive TFA

Accessibility improvements

* Captive TFA

Use “Two Factor Authentication” / TFA consistently

* Captive TFA

Tytytytypo

* Captive TFA

Fixed PHPCS issue unrelated to PR but reported by Drone nonetheless

* Captive TFA

Lang improvement

Co-authored-by: Brian Teeman <brian@teeman.net>

* Captive TFA

Lang improvement

Co-authored-by: Brian Teeman <brian@teeman.net>

* Captive TFA

Remove no longer valid plugin options

* Captive TFA

Typo in plugin path

* Captive TFA

Move TFA options in com_users config next to the
password options

* Captive TFA

Add Show Inline Help button to com_users' options page

* Captive TFA

Move loading static assets to the view template

See #37356 for
the reasoning. This should REALLY have been documented
somewhere...

* Captive TFA

Fixed wrong plugin path

* Captive TFA

Language style guide

Co-authored-by: Brian Teeman <brian@teeman.net>

* Captive TFA

Language style guide

* SQL code style and consistency fixes

* Add "CAN FAIL" installer hint

* Change longtext to mediumtext

* Change longtext to mediumtext in update script

* No default value for method

* Use real null values for last_used

* Captive TFA

Fix JS linter errors

* Captive TFA

Fix PHPCS issues after merging @richard67 's PR

* Captive TFA

Update formatRelative to use JNEVER, simplifying the
code in the view templates.

* Captive TFA

Fix typo

* Captive TFA

Fix transcription error

* Captive TFA

Show correct TFA column in the backend Users page

* Captive TFA

Fix PHPCS errors in UsersModel unrelated to this PR

* Captive TFA

Add note about supported browsers in TOTP's link

* Captive TFA

Remove bogus ESLint notice about qrcode

* Captive TFA

Fix confusing prompt

* Captive TFA

Consistently change ->qn to ->quoteName

* Captive TFA

Strict equality check

* Captive TFA

Move setSiteTemplateStyle to the views

* Captive TFA

Rename regbackupcodes to regenerateBackupCodes

* Captive TFA

Rename dontshowthisagain to doNotShowThisAgain

* Captive TFA

Throw deprecated notices from deprecated methods

* Captive TFA

Strict comparison

* Captive TFA

Typo in comment

* Captive TFA

Rename TwoFactorAuthenticationAware to TwoFactorAuthenticationHandler

* Captive TFA

Fix comment typo

* Captive TFA

Remove variables from SQL when not necessary

* codestyle changes

* Renamed SiteTemplateAware to SiteTemplateTrait

Change made against feedback item #37811 (review) for pull request #37811 with title "Improved Two Factor Authentication".

Feedback item marked the SiteTemplateAware trait name and had the following content:

> Still ...Aware is not a good name for a trait, since it usually denotes interfaces

* Remove more instances of "2SV"

Per feedback item #37811 (comment)

* s/Two Step Verification/Two Step Validation/

Per feedback item #37811 (comment)

* Language style

Per feedback item #37811 (comment)

* Remove unnecessary language string

* Remove redundant paragraph tags from PLG_TWOFACTORAUTH_EMAIL_XML_DESCRIPTION

Per feedback item #37811 (comment)

* Remove redundant paragraph tags from PLG_TWOFACTORAUTH_EMAIL_XML_DESCRIPTION

Per feedback item #37811 (comment)

The other file with the same language string I forgot to put in the previous commit.

* Remove the info tooltip in the methods list

Addresses feedback in #37811 (comment)

* Simplify the TFA enabled / disabled message

Per feedback item #37811 (comment)

* Fix layout of backup codes in methods list

Per feedback item #37811 (comment)

* Fix mail message

Per #37811 (comment)

* Confirm TFA method deletion

Per feedback item #37811 (comment)

* Simplify code label in Email plugin

Per feedback #37811 (comment)

We show short instructions above the field and the field label is simplified. Applied the same change to the Fixed plugin for consistency.

* Remove more dead code referencing the legacy TFA

* Use concrete events

This was the plan all along. Now that #36578 is merged we can FINALLY do it!

* WebAuthn support for some Android devices and FIDO keys

Backported from #37675

* Rename Tfa to Mfa

Ongoing process

* Move Joomla\CMS\Event\TwoFactor to Joomla\CMS\Event\MultiFactor

Ongoing process

* Two Factor Authentication => Multi-factor Authentication

Ongoing process

* `#__user_tfa` => `#__user_mfa`

Ongoing process

* twofactorauth => multifactorauth

Ongoing process

* Change the post-install message

Ongoing process

* Remove references to “second factor”

Ongoing process

* Remove the legacy TFA plugins

* I missed a few things

* I missed a few more things

* Wrong redirection from post-installation messages

Addresses #37811 (comment)

* Fix NotifyActionLog expected event names

Addresses feedback item #37811 (comment)

* Improve display of Last Used date

Addresses feedback item #37811 (comment)

* MFA extension helper

moves the group to the correct alpha order in the array now that it doesnt begin with T

* Remove unused field

* Remove no longer used language strings

* Undo changes in old SQL scripts

* Improve layout and accessibility of the methods list page

Based on VoiceOver testing on macOS 12.4 and the feedback from #37811 (comment) and #37811 (comment)

* Add missing options to plg_multifactorauth_email

* Sort lines alphabetically

Why not confuse the translators with out of order labels providing zero context to what they are translating? It's the One True Joomla Way...

* Add label to the One Time Emergency Password input

Per feedback item #37811 (comment)

* Sort lines

* Fix PHPCS complaint

* Formatting of XML files

* Forgot to remove extra CSS class

* Apply suggestions from code review

Formatting, wrong copyright and version information

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Commit suggestions from code review

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Commit formatting suggestions from code review

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Commit formatting suggestions from code review

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Commit formatting suggestions from code review

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Commit formatting suggestions from code review

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Commit formatting suggestions from code review

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Commit formatting suggestions from code review

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Commit formatting suggestions from code review

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Commit formatting suggestions from code review

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Commit formatting suggestions from code review

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Update build/media_source/plg_multifactorauth_webauthn/js/webauthn.es6.js

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Fix update SQL

Feedback item #37811 (review)

* Onboarding would result in a PHP exception

Feedback item #37811 (comment)

* Make MFA plugins' publish state consistent between MySQL and PostgreSQL

Feedback item #37811 (review)

* Update administrator/components/com_users/src/Controller/MethodsController.php

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Update administrator/components/com_admin/sql/updates/mysql/4.2.0-2022-05-15.sql

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Update administrator/components/com_admin/sql/updates/postgresql/4.2.0-2022-05-15.sql

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Update administrator/components/com_admin/sql/updates/mysql/4.2.0-2022-05-15.sql

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Update administrator/components/com_admin/sql/updates/postgresql/4.2.0-2022-05-15.sql

Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>

* Update administrator/components/com_admin/sql/updates/postgresql/4.2.0-2022-05-15.sql

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>

* Restore obsolete language strings

Per discussion with @bembelimen

I had to rename One Time Emergency Passwords to Backup Codes so as not to make major changes to the obsolete language strings. Having them named One Time Emergency Passwords (OTEPs) was both misleading (they are not passwords, they are second factor authentication codes) and would collide with the `_OTEP_` component of language existing strings. Backup Codes is a good compromise, one that is also field tested for nearly seven years. So, there you go!

* Re-add the obsolete plugins' language files

Per discussion with @bembelimen

Yes, it's pointless, it looks wrong, it is what it is. At least I've put a header that this file needs to be removed.

* Remove no longer used twofactor field

* Rename CSS class to com-users-profile__multifactor

* Update administrator/language/en-GB/plg_multifactorauth_email.sys.ini

Co-authored-by: Brian Teeman <brian@teeman.net>

* Update administrator/language/en-GB/plg_multifactorauth_email.ini

Co-authored-by: Brian Teeman <brian@teeman.net>

* Update administrator/language/en-GB/plg_multifactorauth_email.ini

Co-authored-by: Brian Teeman <brian@teeman.net>

* Update administrator/language/en-GB/com_users.ini

Co-authored-by: Brian Teeman <brian@teeman.net>

* Update administrator/language/en-GB/com_users.ini

Co-authored-by: Brian Teeman <brian@teeman.net>

* Update administrator/language/en-GB/com_users.ini

Co-authored-by: Brian Teeman <brian@teeman.net>

* Update administrator/language/en-GB/com_users.ini

Co-authored-by: Brian Teeman <brian@teeman.net>

* Update administrator/language/en-GB/com_users.ini

Co-authored-by: Brian Teeman <brian@teeman.net>

* Accessibility improvement

* Improve language

* Change the heading level

* Fix case of extension registry file

Regression after renaming TFA to MFA

* Remove accidental double space after echo

* Remove BS3 leftovers

* Remove BS3 leftovers

* Remove BS3 leftovers

* Update administrator/components/com_users/tmpl/methods/list.php

Co-authored-by: Brian Teeman <brian@teeman.net>

* Update components/com_users/tmpl/methods/list.php

Co-authored-by: Brian Teeman <brian@teeman.net>

* PHP warnings when there are no MFA plugins enabled

* MFA onboarding was shown with no MFA plugins enabled

* Backup codes alert is narrower than page on super-wide screens

* Backup codes alert heading font size fix in backend

* Revert wording for JENFORCE_2FA_REDIRECT_MESSAGE

* Backend users without `core.manage` on com_users were blocked

They were blocked from setting up / manage their on MFA,
blocked from the onboarding page and blocked from the
captive login page.

* Onboarding in backend shouldn't have a Back button

* Improve layout of method add/edit page

* Remove unnecessary H5 tag from TOTP setup table

* Kill that bloody Back button with fire

* MFA WebAuthn: use Joomla.Text instead of Joomla.JText

* MFA WebAuthn: show meaningful error on HTTP

* MFA Email: more sensible email body

* MFA WebAuthn: must be able to edit the title

* MFA add/edit: remove placeholders, replace with help text

* Heading levels

We assume an H1 will already be output on the page. This is always true on Atum and never true on Cassiopeia — but very likely on real world sites's frontend templates. So it's a compromise which is at least better than the previous case of starting at h3 or h4.

* Editing a user would show the wrong interface

When editing a user other than ourselves we need to show the MFA editing interface for the user being edited, not the MFA editing interface for our own user.

* Refactor security checks

Now they are conforming to the original intention

* Add missing Group By to the SQL query

* Show MFA enabled when a legacy method is enabled

* Users: filter by MFA status

* Language clarification

* Move the frontend onboarding page header to the top

* User Options language clarification

* PostgreSQL installation SQL wasn't updated

* Adding periods to the end of lines of error messages you will never, ever see

* Remove a tab

* Remove another tab from a comment

* Typo removing junk

* Remove useless imports

* Busywork

* Typo in the INI file

* Align comment

* Remove redundant SQL for PostgreSQL

* Typo in labels' `for` attribute

* Move backup codes to the top of the page

* Mandatory and forbidden MFA was not taken into account

If only one group matched, due to typo.

* Show information when MFA is mandatory

* Make the buttons smaller

* The secondary button looks horrid in the frontend

* Redirect users to login page in the frontend

When they try to access a captive or methods / method page.

* MFA Email: fallback to standard mailer when the mail template isn't installed

* Delete backup codes when the last MFA method is deleted

* Use text inputs for TOTP

With the correct input box attributes

* Fix the buttons for WebAuthn

* Clarify language strings

* Use toolbar buttons in the backend

Except for screen size small and extra small. Over there we ALSO display the inline content buttons because the toolbar buttons are hidden behind an unintuitive gears icon.

JUST BECAUSE THE DEFAULT JOOMLA WAY IS TO USE A TOOLBAR IT DOES NOT MEAN THAT IT MAKES SENSE ALWAYS, EVERYWHERE. THE USER IS KING. WE SERVE THE USER, NOT OURSELVES!

* Change the icon classes

* Forgot to copy over the changes to the frontend

* Regression: configure existing authenticators

We used to set field_type to custom to make the code entry disappear. After the changes to the field type handling we need to instead set input_type to hidden.

* Backup codes should never become the default method automatically

* Improve methods list layout

Now it is more clear which methods are enabled and which are available.

* Use toolbar buttons in backend pages

Except when the screen size is extra small which is the point where the toolbar is hidden and the interface becomes unintuitive.

* Fix return URLs for backend MFA edit pages

* Edit / Delete buttons mention the auth method name in the respective button's visually hidden text

* RTL aware back buttons

* Consistent use of the term Fixed Code

* Fix typo

Co-authored-by: Brian Teeman <brian@teeman.net>
Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
Co-authored-by: heelc29 <66922325+heelc29@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Composer Dependency Changed Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester Unit/System Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants