Skip to content

Conversation

@nikosdion
Copy link
Contributor

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 🩹

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

Documentation Changes Required

None! It actually make the plugin conform to the lang string which currently claims it works with Windows Hello (even though it actually doesn't on Joomla 4.0 and 4.1).

@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 26, 2022
@brianteeman
Copy link
Contributor

I see several * @since 7.0.0 is that a copy paste error?

* @package Joomla.Plugin
* @subpackage System.Webauthn
*
* @copyright (C) 2020 Open Source Matters, Inc. <https://www.joomla.org>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @copyright (C) 2020 Open Source Matters, Inc. <https://www.joomla.org>
* @copyright (C) 2022 Open Source Matters, Inc. <https://www.joomla.org>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is renamed and moved (it was in Helper/CredentialsCreation.php), it is not created from scratch. Therefore it needs to retain its original copyright from when it was initially created. At least that's how I understand the rule that the copyright of the file must match its first appearance in the codebase.

* @package Joomla.Plugin
* @subpackage System.Webauthn
*
* @copyright (C) 2020 Open Source Matters, Inc. <https://www.joomla.org>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @copyright (C) 2020 Open Source Matters, Inc. <https://www.joomla.org>
* @copyright (C) 2022 Open Source Matters, Inc. <https://www.joomla.org>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Likewise, this file was in the plugin root folder as webauthn.php. It was moved into src/Extension therefore its copyright shouldn't change based on what I've been told.

@brianteeman
Copy link
Contributor

Anyone know why there are no prebuilt packages for this pr?

@nikosdion
Copy link
Contributor Author

@brianteeman There was only one wrong @since tag in the service provider, that was a copy-paste error. Where did you see the other ones?

Regarding the copyright dates, I left them alone in files which were moved and renamed since there's Git history placing their first appearance in 2020. If this needs to be changed to 2020-2022 please tell me and do let me know exactly what is the rule in this case because I might need to change the copyright on all other files touched by this PR as well.

Nicholas K. Dionysopoulos added 2 commits April 27, 2022 10:00
@nikosdion
Copy link
Contributor Author

Ugh... I'll have to close this PR and redo it again because rebasing to the 4.2-dev branch made Git have a stroke, again, showing two file I have not changed as modified.

@brianteeman
Copy link
Contributor

yes sorry I didnt spot they were moved.

i swear i saw multipl since 7.0.0 last night but my eyes must have deceived me

@nikosdion
Copy link
Contributor Author

It's OK, diff views can be disorienting :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

4 participants