? Pending

User tests: Successful: Unsuccessful:

avatar nikosdion
nikosdion
16 May 2022

Improved Two Factor Authentication

ATTENTION TESTERS!!!

If you had tested this PR when it was still called Two Factor Authentication you will need to follow these steps:

  • Run composer install
  • Run npm ci
  • Delete the file administrator/cache/autoload_psr4.php
  • REINSTALL A NEW JOOMLA SITE, FROM SCRATCH, USING THE UPDATED VERSION OF THIS PR

The latter step is extremely important as all the plugin names have changed, including the internal Joomla extension names.

Executive Summary

This PR implements Multi-Factor Authentication (MFA) using a “Captive Login” approach.

Unlike the temporary Two Factor Authentication solution we've been using until today, it does NOT require you to enter your Security Code with your login information (e.g. username and password). In fact, there is no longer a Security Code field at all in the login module or page. Instead, you login using whichever login method you want to use. Then you enter the “captive login” mode in Joomla where you can only interact with a subset of com_users: you can proceed with MFA validation, choose a different MFA method or log out. Non-HTML views are forbidden.

Tagging @SniperSister @brianteeman @richard67 who were involved in the original discussion in the context of improving Joomla's security, @zero-24 @bembelimen @roland-d @HLeithner as the current and future release leads that need to be in the loop, @laoneo for his keen eye and good suggestions and @rdeutz because he needs to know about flagship features. I don't know if @nibra is interested in the architectural aspect — I tried to stay as true to our August 2015 decision on orthogonality of new features as plausible for this kind of feature (at least you get a Trait instead of Yet Another System Plugin). I hope I am not forgetting anyone. Feel free to tag anyone else who needs to be kept in the loop.

Draft PR

This is a Draft PR. The intention here is for all CMS stakeholders to go through the code AND the process (not to mention the language — Brian, I am counting on you, man), have a discussion about everything I didn't think to put here in writing, things like that. Basically, figure out how to shape this up for prime time hopefully before 4.2 betas start rolling out.

There are also a few less important missing bits and ends, mainly on the code polishing side of things, which I document at the bottom of this PR's description. They do not detract from testing the process of Two Factor Authentication so I decided to go through with the Draft PR anyway.

Extra features implemented and considerations made

I implemented a few extra features to the old TFA since I already had the code for them and it'd be a waste to remove it only for someone else to try and replicate them in the future. I also tried to be very considerate of the security of the solution, keeping it in a fine balance with practicality. Without further ado, here's what I did.

Multiple MFA methods per user. There is no good reason why a user shouldn't have multiple MFA methods. In fact, it is a better approach e.g. having two WebAuthn keys (main and backup) and a classic six-digit code for use in an older smartphone which doesn't support WebAuthn dongles. This makes MFA more usable and more resilient to accidents that lock people out of their site.

Default method. If you have multiple authentication methods you don't want to stop and think which method you will be using to authenticate today, especially since you're likely to use the same method 99% of the time. Therefore you can pick which method will be your default. You can NOT use emergency codes as the default method to reduce the probability of silly mistakes.

Methods batching. Let's say you set up three WebAuthn authenticators. When you log in you want to see a single page asking you for a WebAuthn login, not having to click to select which WebAuthn authenticator you will be using (reduces the PICNIC errors — Problem In Chair, Not In Computer). I call that method batching because you can batch process all instances of a MFA method using a single input. WebAuthn and YubiKey let you set up multiple instances of that method so, indeed, they support method batching. Regular Authentication Code and Authentication Code by Email only allow a single instance so they don't. As simple as that.

Automatic migration of legacy data. To support multiple MFA methods per user we moved the MFA data from the two #__users columns (otpKey and otep) to its own table (#__user_tfa). Any already set up legacy TFA method is automatically migrated upon first login. This means that MFA will migrate cleanly even on sites with hundreds of thousands of users, something which would NOT be the case had we performed the migration during update (we'd timeout as we have to load each record individually, decrypt it, convert it, re-encrypt it and save it). Important: Until a user goes through a login you will NOT see their TFA/MFA status or configuration settings in com_users in the backend.

Data is encrypted. As alluded above, the MFA configuration data is still encrypted using AES-256 using a key derived from the site's secret. Therefore a simple SQLi vuln in any extension won't divulge the secrets for MFA.

New MFA methods. You can now use Authentication Code by Email and WebAuth on top of the existing Authentication Code and YubiKey methods. The former sends you a 6-digit code to your email and can also be set up to be forcibly enabled for anyone (to provide a viable backup if your users are not very tech savvy). The latter supports Windows Hello and Android, bringing in the improvements from my other PR (#37675).

Forced MFA and Forbidden MFA groups. You can optionally set up certain user groups to have MFA forcibly enabled and other groups to have MFA forcibly denied. The former will have to set up MFA and use it to continue using the site. The latter will never be asked to go through MFA, even if they had previously enabled it, nor will they see the MFA configuration section in com_users.

Onboarding to drive adoption. You can optionally have users (who do not belong in the forbidden MFA groups!) be automatically shown an onboarding page if they log in and they have not yet enabled MFA. They can set up MFA, go to a different page or forever dismiss the onboarding page. You can also customise the onboarding URL, e.g. if you want to display your own article instead of showing the default MFA setup page. According to my experience this increases MFA adoption by a factor of ten. Note that onboarding is DISABLED by default; we don't want people coming at us with torches and pitchforks for ruining their site's login experience.

Super Users cannot edit other Super Users. As a Super User you cannot touch another Super User's MFA settings at all. Consequently, if you mess up your MFA as a Super User you need to do some database editing. There's only so much we can do.

Administrators can only remove TFA options for other users. As a privileged user you can only remove MFA options from other (non-Super Users) user accounts, or disable their MFA completely. You cannot edit their TFA configuration or enrol new MFA methods.

Single click disable. The MFA configuration interface includes a Turn Off button which disables MFA completely for your user account. This is useful if you lost access to all MFA methods, you have used an emergency code and you just want to turn the darned thing off until you sort out your other problems.

Post-installation message. The old PIM for MFA has been removed and a new one has taken its place.

Module positions customisation in the captive page. You can choose which module positions to display in the captive MFA page. This lets you display, for example, your site's header and footer to give users a better visual experience.

No MFA on silent logins. By default, MFA is disabled on silent logins. You can toggle that as well as customise which login methods are to be considered a "silent" log in. By default these are the Cookie Authentication (remember me) and Passwordless (WebAuthn). The latter needs some explaining. WebAuthn is phishing-resistant and insanely secure — we only ever store a public key in the database, even if it's stolen it cannot be used to log you in (a stolen password hash can be brute forced MUCH more easily, by several orders of magnitude). This means that WebAuthn is strong enough to NOT require Multi-factor Authentication. That's the whole point of WebAuthn; replace usernames and passwords and the need for multifactor authentication with One Authentication Method To Rule Them All And In Security Bind Them.

Implementation notes

This PR is based on my past work on Akeeba LoginGuard, the first practical Two Step Verification solution for Joomla, which had been in active development since 2016. That component had begun as a proof of concept for the Captive Login concept as I had been told repeatedly until that point in time it's not possible in Joomla. Not only is it possible, after proving it feasible Joomla 4 is now already using it in several places (“agree to terms” and “requires password reset”). Meanwhile, I had been actively developing that extension with an eye on contributing it back to Joomla since this is the Two Factor Authentication we had promised to implement nearly ten years ago. What we still have was meant to be a “temporary solution, for 6 to 12 months tops, until we release Joomla 4 with a REAL Two Factor Authentication with a captive login”. Well, better late than never, right?!

This being a core PR instead of an extension I no longer have to do weird bollocks to make the captive login work, using a system plugin, string and duct tape to hold it all together. I also didn't want to repeat the past mistakes of sprinkling magic code all over the darned place to integrate MFA to Joomla. So I used a Trait instead. Only the SiteApplication and AdministratorApplication use it. This is deliberate.

The CLI application does not have any authentication to speak of, nor it needs to. If you are in a position to either access a shell or define CRON jobs you have more control on the hosting environment than a Super User.

The Api application on the other hand is non-interactive. That's why it has a token authentication instead of relying on a username and password (see #27021 and the handy comic panel that drives the point home in #26925).

So, if you end up writing an interactive custom application you can use the Trait. If not, it's cool, DON'T integrate MFA — you actually MUST NOT.

Missing odds and ends

I have not (yet?) integrated this with Joomla's Action Logs. This is deliberate as there's currently an open PR (#37788) touching it and I don't want to create a Merge Conflict Hell.

Moreover, you will see that the MFA plugins are doing a couple of things which can be improved:

  1. They are using $db instead of the spanking new Joomla\Database\DatabaseAwareTrait. I will work on that before making the PR final. We are not using the database object directly in the plugins anymore.
  2. They are using $app instead of injecting the application. Also something to be worked on before the final PR. There is no ApplicationAware interface so I'm sticking with the CMSPlugin magic for now.
  3. There is a repeated setResult method in them. That's temporary, until #36578 is merged and I can use the event's setResult method as I originally intended. Until then, I needed something that works. Hence the method in question. Fixed now that the PR is merged; we are using concrete events which are results aware.
  4. Remember the WebAuthn claim for Windows Hello and Android compatibility? Yeah, I still need to pull that code in. Already addressed.

If you bump into one of this, yup, I know.

PS: Brave adventurer, beware! There be dragons! I crammed a month's work into four days. I'd be mighty surprised if there are no bugs.

avatar nikosdion nikosdion - open - 16 May 2022
avatar nikosdion nikosdion - change - 16 May 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 16 May 2022
Category SQL Administration com_admin Postgresql com_joomlaupdate com_users
avatar nikosdion nikosdion - change - 16 May 2022
Labels Added: ?
avatar Kostelano
Kostelano - comment - 16 May 2022

It probably makes sense to move 2 parameters regarding 2FA from the "User Options" tab to the new "Two Factor Authentication" tab in the user settings.

Screenshot_1

avatar nikosdion nikosdion - change - 16 May 2022
The description was changed
avatar nikosdion nikosdion - edited - 16 May 2022
avatar nikosdion
nikosdion - comment - 16 May 2022

@Kostelano Ah! These two options actually need to be removed completely.

The first one makes no sense. Having TFA only on one side of the application made sense ten years ago when the frontend was completely isolated (no longer the case; we can have joined sessions) and didn't have much in the way of affecting the site (no longer true; we can change global configuration from the frontend now). So off with his head I'll go.

The other option is already superseded by a similar option I added.

Thank you for the heads up!

avatar Kostelano
Kostelano - comment - 16 May 2022

Add a Two Factor Authentication method, Incorrect link built

Screenshot_1

avatar nikosdion
nikosdion - comment - 16 May 2022

@Kostelano Oops! I decided against having a link (the W3C page is not exactly enlightening) but I forgot to change the lang string. Thanks!

avatar Kostelano
Kostelano - comment - 16 May 2022

Different type of strings for the code (probably wrong for the email plugin).

Screenshot_1

avatar Kostelano
Kostelano - comment - 16 May 2022

Profile - Edit, 2FA tab, create for "Verification code":

An error has occurred.
0 There is no "plg_twofactorauth_totp.setup" asset of a "script" type in the registry. 

Screenshot_1

avatar nikosdion
nikosdion - comment - 16 May 2022

Different type of strings for the code (probably wrong for the email plugin).

Screenshot_1

These are two very different field types. The Fixed code, as you are told by the description, is free text. Essentially, a second password. It's just an example and definitely not a numeric code.

The other plugin only accepts a six digit code. Therefore it's a numeric field.

avatar brianteeman
brianteeman - comment - 16 May 2022

One thing I am confused about.

TFA, TSV, Two Factor Authentication, Two Step Authentication

Are they the same or different. You use all 4 seemingly interchangeably in both the comments and the text

avatar nikosdion
nikosdion - comment - 16 May 2022

@brianteeman There were two instances of "TSV" in the comments and several of "Two Step Verification" in the comments and text. I have fixed it to all read "Two Factor Authentication". Comments do abbreviate it as "TFA" in several places.

This code comes from LoginGuard, as I said in the PR description. Over there I was calling that Two Step Verification to distinguish that from Joomla's Two Factor Authentication — Joomla had trained the users to think that TFA is sent with your username and password. Since this PR restores TFA to what it was supposed to be all along, a captive login page, this PR calls it Two Factor Authentication.

As I said, I crammed a month's work in four days. Some things were not replaced as I didn't have the time to go through the slowly. Thanks for spotting that, now it's consistent (or at least I cannot find any other instances of TSV).

avatar nikosdion
nikosdion - comment - 16 May 2022

Two Two ;)

Where?

avatar nikosdion
nikosdion - comment - 16 May 2022

Profile - Edit, 2FA tab, create for "Verification code":

An error has occurred.
0 There is no "plg_twofactorauth_totp.setup" asset of a "script" type in the registry. 

Screenshot_1

You need to run npm ci. Otherwise neither the Authentication Code nor the WebAuthn plugins will work correctly.

avatar brianteeman
brianteeman - comment - 16 May 2022

@nikosdion will test the code tomorrow.

avatar richard67
richard67 - comment - 17 May 2022

@nikosdion We have javascript linter errors: https://ci.joomla.org/joomla/joomla-cms/54152/1/26

Other question. I am preparing a PR for your branch with a few code style and consistency fixes for the SQL scripts. Doing this, I have noticed that you use the "longtext" data type for the "options" column of your new table for MySQL. Does it really need to be that big? Up to now we have no other "longtext" columns in the core. For "params" and "options" we use all over "text", only at one place in some finder table we use "mediumtext". Would "mediumtext" of even "text" be sufficient here?

avatar richard67
richard67 - comment - 17 May 2022

@nikosdion Next question: Any reason why your new table class doesn't support null values e.g. for the dates? If not: Shall I also fix that with my upcoming PR for you?

avatar brianteeman
brianteeman - comment - 17 May 2022

task=method.add

Is there a specific reason that you have save and cancel as button and not as a toollbar. It would be more consistent in the UI if you used a toolbar

avatar brianteeman
brianteeman - comment - 17 May 2022

image

Please use either the accessible tooltips or the regular descriptions. This is not accessible

avatar brianteeman
brianteeman - comment - 17 May 2022

image

Please use either the accessible tooltips or the regular descriptions. This is not accessible. It's also introducing a new style to the UI. Personally I would use regular descriptions

avatar nikosdion
nikosdion - comment - 17 May 2022

@brianteeman Which accessible tooltips? Where is the documentation for that?

avatar nikosdion
nikosdion - comment - 17 May 2022

task=method.add

Is there a specific reason that you have save and cancel as button and not as a toollbar. It would be more consistent in the UI if you used a toolbar

Can't do.

WebAuthn or any method requiring a JS trigger / rendering its own interface needs a predictable button target in both the front- and the backend of the site. The only way to do that is with a button rendered inline the form.

Moreover, having the button inline allows browsers and password managers to autofill the authentication code (they detect it as a TFA form) and YubiKey to be registered by pressing on its disk without having to click the UI button (it sends ENTER at the end of the OTP).

Having a toolbar button in the backend would create a different UI for the front- and backend implementations of the same thing which would be disorienting for the users, it would be a lot of work AND it would remove creature comfort features. I don't see the reason to do that.

avatar brianteeman
brianteeman - comment - 17 May 2022

documentation - lol

image

<div role="tooltip" id="tip-publish<?php echo $i; ?>">
	<?php echo Text::_('COM_CATEGORY_COUNT_PUBLISHED_ITEMS'); ?>
</div>
avatar nikosdion
nikosdion - comment - 17 May 2022

@nikosdion We have javascript linter errors: https://ci.joomla.org/joomla/joomla-cms/54152/1/26

/src/administrator/components/com_media/resources/scripts/components/browser/items/image.vue

This is not from me.

25:22 error 'qrcode' is not defined no-undef

Can't do anything about it. The linter is unaware that joomla.asset.json has a dependency to qrcode, therefore it's defined.

The other ones I can can fix and they are really strange because they didn't appear on the local run. Does Joomla use a linter configuration different than the default and if so why is it not in the repo?

Other question. I am preparing a PR for your branch with a few code style and consistency fixes for the SQL scripts. Doing this, I have noticed that you use the "longtext" data type for the "options" column of your new table for MySQL. Does it really need to be that big? Up to now we have no other "longtext" columns in the core. For "params" and "options" we use all over "text", only at one place in some finder table we use "mediumtext". Would "mediumtext" of even "text" be sufficient here?

Yeah, mediumtext would be fine, on account of most live hosts' databases not even supporting packages over 1Mb big and the options I've seen being saved in the real world not being larger than a couple Kb big anyway. You can go ahead and change it to mediumtext.

avatar brianteeman
brianteeman - comment - 17 May 2022

image

I was confused when I saw this. I thought it was something like the messages in com_finder when there is a plugin not enabled.

then I saw
image

Now it is clearer what it actually meant.

As a first guess how about changing the text from Two Factor Authentication is disabled
to
There are no two factor authentication methods enabled

avatar nikosdion
nikosdion - comment - 17 May 2022

@nikosdion Next question: Any reason why your new table class doesn't support null values e.g. for the dates? If not: Shall I also fix that with my upcoming PR for you?

Please do fix that, I totally missed it.

avatar brianteeman
brianteeman - comment - 17 May 2022

image

Turn Off implies that it is non-destructive and is something I can turn back on. From what I can see it is a destructive change as it wipes all existing methods.

Probably just needs some different terminology other than turn off

avatar brianteeman
brianteeman - comment - 17 May 2022

Authenticator App link

image

When/where can this clickable link be used? I assume that its "in some cases, your browser." If so which browsers support this

avatar nikosdion
nikosdion - comment - 17 May 2022

image

Turn Off implies that it is non-destructive and is something I can turn back on. From what I can see it is a destructive change as it wipes all existing methods.

Probably just needs some different terminology other than turn off

I have used this terminology on my own live site for 5 ½ years. Thousands of users have seen it and made use of it without a problem. In fact, that's what users are looking for; mentally, they are looking for a way to “turn off” TFA on their account. Most users (just over 96% when I checked three months ago) only have one method anyway. For them clicking that button or clicking the delete buttons everywhere is functionally the same, with “Turn Off” complying to their mental model better than clicking the individual delete buttons.

Regarding the term, there was a lot of thought going into it. I thought about “Reset” but while technically correct it sounds like it's going to keep everything enabled but somehow reset it to an unspecified default value which is just plain confusing. “Wipe clean” is technically correct but has a very WTF sound on it. “Disable” would be the same as turn off with a stronger connotation that it is non-destructive. “Remove all” might do but I'm not sure people will understand it refers to the TFA methods when the message implies it applies to TFA itself.

In any case, I have no problem changing it as long as someone provides an alternative term and takes responsibility for this change as it will be replacing my real world user-tested term.

avatar brianteeman
brianteeman - comment - 17 May 2022

image

I assume its just a change in text here as there is no submit button etc

avatar nikosdion
nikosdion - comment - 17 May 2022

Authenticator App link

image

When/where can this clickable link be used? I assume that its "in some cases, your browser." If so which browsers support this

Definitely Safari supports this on macOS, iOS and iPadOS as of macOS 12 and iOS/iPadOS 15.

On Android it will open Google Authenticator if you have it installed. Generally speaking, on any browser and OS it will open your password manager or authentication application if it has registered itself as a protocol handler for that kind of URL.

avatar brianteeman
brianteeman - comment - 17 May 2022

The problem with the link is that on an unsupported platform eg my chrome on windows the link does nothing. you dont even get an error in the browser.

Just an error hidden in the console. Can that be displayed?

Failed to launch 'otpauth://totp/zzzzzzzzzzz@zzzzzzzzzzzzz?secret=AXDTTIO6NH7YSQXY' because the scheme does not have a registered handler.

avatar nikosdion
nikosdion - comment - 17 May 2022

@nikosdion We have javascript linter errors: https://ci.joomla.org/joomla/joomla-cms/54152/1/26

These are fixed now. I found out what happened. phpStorm did auto formatting on commit on JS files. I fixed that on my end and addressed the bad formatting inflicted by this.

avatar nikosdion
nikosdion - comment - 17 May 2022

The problem with the link is that on an unsupported platform eg my chrome on windows the link does nothing. you dont even get an error in the browser.

Just an error hidden in the console. Can that be displayed?

Failed to launch 'otpauth://totp/zzzzzzzzzzz@zzzzzzzzzzzzz?secret=AXDTTIO6NH7YSQXY' because the scheme does not have a registered handler.

It is impossible to know — either on the server- or the client-side — if it is a supported handler. On those platforms where it is supported it is definitely a time saved. I will keep this link. It works beautifully on Safari and various Keepass-based password managers.

avatar richard67
richard67 - comment - 17 May 2022

@nikosdion We have javascript linter errors: https://ci.joomla.org/joomla/joomla-cms/54152/1/26

These are fixed now. I found out what happened. phpStorm did auto formatting on commit on JS files. I fixed that on my end and addressed the bad formatting inflicted by this.

@nikosdion Well, among the remaining ones, those about the vue file are just warnings and so don't make it fail. But is there nothing we can do about the error 'qrcode' is not defined no-undef? This is the only error remaining.

avatar brianteeman
brianteeman - comment - 17 May 2022

The problem with the link is that on an unsupported platform eg my chrome on windows the link does nothing. you dont even get an error in the browser.
Just an error hidden in the console. Can that be displayed?
Failed to launch 'otpauth://totp/zzzzzzzzzzz@zzzzzzzzzzzzz?secret=AXDTTIO6NH7YSQXY' because the scheme does not have a registered handler.

It is impossible to know — either on the server- or the client-side — if it is a supported handler. On those platforms where it is supported it is definitely a time saved. I will keep this link. It works beautifully on Safari and various Keepass-based password managers.

Ah I see the security issue with that and why its no longer possible. Just needs a better description then. Perhaps as a start "ckick this link on supported browsers" at least then people might assume "oh it doesnt work because I dont have a supported browser" even if it doesnt solve the "what are supported browsers?"

avatar nikosdion
nikosdion - comment - 17 May 2022

Ah I see the security issue with that and why its no longer possible. Just needs a better description then. Perhaps as a start "ckick this link on supported browsers" at least then people might assume "oh it doesnt work because I dont have a supported browser" even if it doesnt solve the "what are supported browsers?"

Okay, I solved this in your proposed way.

avatar nikosdion
nikosdion - comment - 17 May 2022

@nikosdion Well, among the remaining ones, those about the vue file are just warnings and so don't make it fail. But is there nothing we can do about the error 'qrcode' is not defined no-undef? This is the only error remaining.

Yup, found a way to do it the same way we tell PHPCS to ignore non-applicable rules, using a comment :)

avatar nikosdion
nikosdion - comment - 17 May 2022

image

I assume its just a change in text here as there is no submit button etc

Okey dokey, fixed.

avatar nibra
nibra - comment - 17 May 2022

@nikosdion You should really rethink your commit message strategy 😉

avatar nikosdion
nikosdion - comment - 17 May 2022

@nibra It shouldn't make a difference if it's squash committed. For big feature PRs like that it doesn't make sense including hundreds of commits when merging. At least that's what I am doing in my repos.

avatar brianteeman
brianteeman - comment - 17 May 2022

its hard for reviewers

avatar Kostelano
Kostelano - comment - 17 May 2022

@nikosdion I see that there is also a post-message about the new functionality in the localization, but I was not able to get this message. I even installed a completely new build with this PR but didn't see that message either. Where did I go wrong?

COM_USERS_POSTINSTALL_TWOFACTORAUTH_TITLE="Improved Two Factor Authentication"
COM_USERS_POSTINSTALL_TWOFACTORAUTH_BODY="<p>Joomla! comes with a drastically improved <a href="https://en.wikipedia.org/wiki/Multi-factor_authentication" target="_blank" rel="noreferrer">Two Factor Authentication</a> experience to help you secure the logins of your users.</p><p>Unlike previous versions of Joomla, user <em>no longer have to enter a Security Code with their username and password</em>. The second factor authentication happens in a separate step, called a “Captive Login”, after logging into the site. Until they complete their Two Factor Authentication validation users cannot navigate to other pages or use the site — that's what “captive” refers to. This makes Two Factor Authentication <a href="https://en.wikipedia.org/wiki/Phishing" target="_blank" rel="noreferrer">phishing</a>&ndash;resistant. It also allows for interactive validation methods like WebAuthn (including integration with Windows Hello, Apple TouchID / FaceID and Android Biometric Screen Lock), or sending 6-digit authentication codes by email. Both of these interactive, convenient methods are now available as plugins shipped with Joomla! itself.</p>"
COM_USERS_POSTINSTALL_TWOFACTORAUTH_ACTION="Enable the new Two Factor Authentication plugins"
avatar nibra
nibra - comment - 17 May 2022

@nibra It shouldn't make a difference if it's squash committed. For big feature PRs like that it doesn't make sense including hundreds of commits when merging. At least that's what I am doing in my repos.

Well, commit messages should, among other things, help a reviewer to identify the significant changes in a commit.

avatar brianteeman
brianteeman - comment - 17 May 2022

submitted a pr to your branch

avatar nikosdion
nikosdion - comment - 17 May 2022

@Kostelano Try disabling all plugins in the twofactorauth folder. The message should show up.

@nibra @brianteeman I am not sure if you are looking at the commit messages or just their first line rendered by default on GitHub. All of my commit messages describe EXACTLY what I changed.

For example, c91d859 has the commit message:

Captive TFA

Remove variables from SQL when not necessary

If you click the three dots you can see it plain as day.

You will see that I have done that on every single one of my commits, not only on this PR but all my PRs to Joomla. I am also doing the same on my own commits on my own repos when I have a big feature which will span several commits.

When my PR consist of just a single commit I refrain from writing a cryptic commit message to the tune of “Fixed X”. In those PRs you will see that my commit message includes a heck of a lot of text describing the issue conditions, the technical explanation and WHY I had to change that particular bit of code. Something that nobody does so every time I use git blame I have to view the commit on GitHub to find which PR it came from to find which issue is refers to and read an endless conversation to find out why the heck that line of code was changed...

avatar nibra
nibra - comment - 17 May 2022

If you click the three dots you can see it plain as day.

Sorry, I hadn't seen it - I take it all back and say the opposite 😉

avatar Kostelano
Kostelano - comment - 17 May 2022

In the block with emergency passwords, a line is visible behind the message.

Screenshot_1

avatar Kostelano
Kostelano - comment - 17 May 2022

By clicking on the delete method button (red in the screenshot above), this happens without confirmation, instantly. I'm not sure, but it seems to me that it's better to do this through confirmation.

avatar Kostelano
Kostelano - comment - 17 May 2022

Email method. We receive an email:

Screenshot_1

avatar nikosdion
nikosdion - comment - 17 May 2022

If you click the three dots you can see it plain as day.

Sorry, I hadn't seen it - I take it all back and say the opposite 😉

No worries. I tried making a commit message that is more verbose but, dunno, it felt a lot like I'm filing papers for something bureaucratic. I think I will go back to my usual style, omitting the common summary to make it easier for you guys to see what I did in each commit.

avatar brianteeman
brianteeman - comment - 17 May 2022

If you click the three dots you can see it plain as day.

Sorry, I hadn't seen it - I take it all back and say the opposite 😉

No worries. I tried making a commit message that is more verbose but, dunno, it felt a lot like I'm filing papers for something bureaucratic. I think I will go back to my usual style, omitting the common summary to make it easier for you guys to see what I did in each commit.

Its stupid github which only emails you with the title and not the extra message

avatar nikosdion
nikosdion - comment - 17 May 2022

By clicking on the delete method button (red in the screenshot above), this happens without confirmation, instantly. I'm not sure, but it seems to me that it's better to do this through confirmation.

I was actually pondering on that yesterday.

Asking the user for confirmation is currently only possible with JavaScript's confirm() function. However, Chrome has removed them in cross-origin sources and is likely to remove them altogether.

Should I do use them anyway or not, this is the question. I am inclined to say I should on the basis that Joomla already uses them in Delete buttons elsewhere. If confirm() is banished from modern browsers I would rather hope Joomla (and literally everything on the web) would come with a solution pretty darned fast.

avatar brianteeman
brianteeman - comment - 17 May 2022

Thank you for changing the way you do the commit messages - the emails are so much more useful now.

image

avatar nikosdion
nikosdion - comment - 17 May 2022

@brianteeman A problem explained is a problem solved 😉

avatar HLeithner
HLeithner - comment - 18 May 2022

I'm busy with psr12 so didn't have time too look at this pr yet. only one question for now. What do we call it two factor, isn't it now possible to have more then 2 and shouldn't it be Multi factor now? (also or may more because of marketing)

avatar nikosdion
nikosdion - comment - 18 May 2022

@HLeithner Technically speaking, multi-factor is anything that requires more than one authentication factor, see https://www.nist.gov/itl/smallbusinesscyber/guidance-topic/multi-factor-authentication

What Joomla already had could already be called MFA. Password (what the user knows) was one factor. The OTP from an authenticator device or a YubiKey (what the user has) was a second factor.

A true MFA would require all three things: what the user knows (password), what the user has (security code, YubiKey OTP, code by text/email/push notification, WebAuthn with non-biometric device) and what the user is (biometric authentication, that would be through WebAuthn).

Now, why everyone is now calling it MFA instead of TFA? It's marketing, but it's also not wrong; just not the entire truth 😉 A password or PIN remains as one authentication factor. The second factor can be WebAuthn protected by biometrics or an authenticator app behind another PIN / password / biometrics. So there you have it, you have introduced biometrics through the back door and you can now plausibly call two factor authentication as multi-factor authentication without as much as a hint of a blush on your face.

So, yes, for marketing reasons we can say that Joomla support Multi-Factor Authentication through its "Two Factor Authentication" feature. We can put first and foremost the fact that Joomla supports WebAuthn with supports biometric authenticators and use Windows Hello, Apple's TouchID/FaceID, Android's biometric screen lock and third party FIDO2 dongles with biometrics (e.g. YubiKey Bio, what I currently use exclusively for authentication on my sites).

If you want to push how far ahead Joomla is from the competition you can tell people that Joomla supports the most secure, Multi-Factor Authentication method for authentication since version 4.0: WebAuthn. Even external FIDO2 dongles can be set up with a PIN. Therefore logging into your site requires something you have (dongle) and something you know (its PIN) or something you are (fingerprint to unlock the dongle).

There you have it. Joomla has had MFA for ten years, improved MFA for one and now gets the final missing puzzle piece to enforce MFA regardless of the authentication method used. Ta-da!

avatar HLeithner
HLeithner - comment - 18 May 2022

Yeah it's clear that we always head multifactor ;-) the question is should we rename the entire thing from TFA to MFA. I think it's better to ask marketing if they want this.

avatar nikosdion
nikosdion - comment - 18 May 2022

@HLeithner Better ask them and let me know in the next couple of days because it will be a major PITA changing everything to MFA / MultiFactor from TFA / TwoFactor.

avatar HLeithner
HLeithner - comment - 18 May 2022

@nikosdion already done, I asked in production and the Outreach DC @softforge to have a look, personally I would like MFA more because it fits better the technique but of course I can life with TFA too.

avatar softforge
softforge - comment - 18 May 2022

Multi-Factor does have a nice ring about it and I appreciate that there are different factors to take into account in a true multifactor, but the fact that you have more than one multifactor choice (security code, YubiKey OTP, code by text/email/push notification, WebAuthn with non-biometric device), will also be read as being multifactor even if not try multifactor in itself, so Multi is certainly more future proof than two factor from a naming point of view.

From marketing point of view we need to use the words others are searching for if we are to get a wider impact so I did a bit of research as there are 3 ways we can write it.

multifactor
About 140,000,000 results

multi factor
About 9,390,000,000 results

multi-factor
About 10,090,000,000 results

Words that are terms usually evolve so
log in, becomes
log-in and when acceptance is widespread the word morphs to
login and a new word is born.

We are at the hyphenated stage so
multi-factor is the most popular searched

But I have just read several articles by Microsoft where they mix and match multi-factor, multifactor, and multi factor, as if they have no policy.

For now, I think we should use and stick to multi-factor so that any articles that are written, docs, announcements and magazine tutorials will be indexed and picked up by search engines for the term that the public are searching for which is multi-factor

avatar brianteeman
brianteeman - comment - 18 May 2022

For now, I think we should use and stick to multi-factor so that any articles that are written, docs, announcements and magazine tutorials will be indexed and picked up by search engines for the term that the public are searching for which is multi-factor

Agreed and this should be added to the style guide

avatar nikosdion
nikosdion - comment - 18 May 2022

Tell me if it's official that this should be called Multi-factor Authentication (plugin group multifactorauth) so I can make the necessary changes.

The major change I need to do is make sure that the multifactorauth TOTP and YubiKey plugins would retain the publish state of their legacy twofactorauth counterparts. Otherwise we get to the point where upgrading your site would disable Two Factor Authentication. That'd be doubleplusungood.

avatar softforge
softforge - comment - 18 May 2022

I think it's inevitable to shift from twofactorauth to multifactorauth so sounds good to do it at the earliest point. Gets my vote and I will make sure marketing use multi-factor authentication and not two-factor authentication in documents

avatar nikosdion
nikosdion - comment - 18 May 2022

Okay. I will make the change.

avatar brianteeman
brianteeman - comment - 18 May 2022

Updated the styleguide - not sure who can merge it

joomla/user-interface-text#67

avatar nikosdion
nikosdion - comment - 18 May 2022

I have renamed the entire feature to Multi-factor Authentication.

ATTENTION TESTERS!!! If you had tested this PR when it was still called Two Factor Authentication you will need to follow these steps:

  • Run composer install
  • Run npm ci
  • Delete the file administrator/cache/autoload_psr4.php
  • REINSTALL A NEW JOOMLA SITE, FROM SCRATCH, USING THE UPDATED VERSION OF THIS PR

The latter step is extremely important as all the plugin names have changed, including the internal Joomla extension names.

avatar nikosdion nikosdion - change - 18 May 2022
Title
Improved Two Factor Authentication
Multi-Factor Authentication (replaces Two Factor Authentication)
avatar nikosdion nikosdion - edited - 18 May 2022
avatar nikosdion nikosdion - change - 18 May 2022
The description was changed
avatar nikosdion nikosdion - edited - 18 May 2022
avatar Kostelano
Kostelano - comment - 18 May 2022

Disable all MFA plugins, go to messages after installing Joomla. Under the topic about the functionality of the MFA there is a button to enable plugins. After clicking it, I am redirected to the HTTP Headers system plugin. 4 plug-ins are really included in this case.

Please check.

This is a new installation of Joomla with this PR after being renamed to MFA.

avatar Kostelano
Kostelano - comment - 18 May 2022

Can we put "Last used" next to the date? It seems to me that it will be better. Or as an option to place immediately below the date.

I have a small monitor, it's hard for me to imagine how such a "gap" will look on a really large monitor.

Screenshot_1

avatar Kostelano
Kostelano - comment - 18 May 2022

After adding the method (in my case, this is email), one-time passwords were created. Go to them and click "Regenerate Emergency One Time Passwords":

0 The Joomla\CMS\Event\MultiFactor\NotifyActionLog event class does not support the onComUsersControllerMethodAfterRegenerateBackupCodes event name.

avatar nikosdion
nikosdion - comment - 18 May 2022

Can we put "Last used" next to the date? It seems to me that it will be better. Or as an option to place immediately below the date.

I have a small monitor, it's hard for me to imagine how such a "gap" will look on a really large monitor.

Screenshot_1

You need some visual separation otherwise it all becomes run together and nobody understands anything anymore. I am not sure what the “best” approach is. I implemented the one I have used in my software used by thousands of users on my site alone for the last 6 years without any complaints.

avatar nikosdion
nikosdion - comment - 18 May 2022

@Kostelano I addressed your other two feedback items. I am still thinking about what to do about the Last Used date.

avatar nikosdion
nikosdion - comment - 18 May 2022

@Kostelano And then it dawned on me that I can have a flexbox row with a big enough gap between the cells. So, your pending feedback item about the Last Used formatting is addressed too.

avatar Kostelano
Kostelano - comment - 18 May 2022

Pay attention to the script file. There are TWOFACTOR files here (localization, scripts, etc), but no MFAs.

UPD
Thanks to @richard67 (below), I didn't pay attention. Sorry.

avatar Kostelano
Kostelano - comment - 18 May 2022

Please rename in the file https://github.com/nikosdion/joomla-cms/blob/feature/tfa/administrator/components/com_users/forms/user.xml#L138.

Excuse me for writing separately - I write upon discovery.

avatar richard67
richard67 - comment - 18 May 2022

Pay attention to the script file. There are TWOFACTOR files here (localization, scripts, etc), but no MFAs.

@Kostelano That array in script.php is the list of files which will be deleted on update. So it is pretty fine to have the old files there. They appear in the list because they are old language files with prefix "en-GB.", and those have to be deleted when updating from 3.10. In J4 we do not have this prefix on the file name anymore.

@nikosdion You don't need to care about deleted files and folders in script.php. In J4 where not everything is in the sources and so not clear for every contributor what has to be deleted, we once had agreed that it is on the release leads (or their willing and knowledgable helpers like me) to maintain the lists of files and folders to be deleted on update in script.php, so you can leave that to us (or me).

avatar Kostelano
Kostelano - comment - 18 May 2022

Renaming to MFA completed. Also renamed some constants that are not actually used.
If you have time, please also remove to get rid of the junk:

administrator/language/en-GB/com_users.ini

COM_USERS_ERROR_SECRET_CODE_WITHOUT_MFA="You have entered a Secret Code but Multi-factor Authentication is not enabled in your user account. If you want to use a secret code to secure your login please edit your user profile and enable Multi-factor Authentication."
COM_USERS_USER_FIELD_TWOFACTOR_LABEL="Authentication Method"
COM_USERS_USER_OTEPS_WAIT_DESC="There are no emergency one time passwords generated in your account. The passwords will be generated automatically and displayed here as soon as you activate Multi-factor Authentication."

administrator/language/en-GB/com_login.ini

COM_LOGIN_TWOFACTOR="For Two-Factor Authentication"

administrator/language/en-GB/joomla.ini (also from api/language/en-GB/joomla.ini)

JENFORCE_2FA_REDIRECT_MESSAGE="You were redirected because you are required to set up Multi-factor Authentication to continue."
JGLOBAL_AUTH_INVALID_SECRETKEY="The Multi-factor Authentication Secret Key is invalid."
JGLOBAL_OTPMETHOD_NONE="Disable Multi-factor Authentication"
JGLOBAL_SECRETKEY_HELP="If you have enabled Multi-factor Authentication in your user account please enter your secret key. If you do not know what this means, you can leave this field blank."

administrator/language/en-GB/plg_authentication_joomla.ini

PLG_AUTHENTICATION_JOOMLA_ERR_SECRET_CODE_WITHOUT_MFA="You need to enable Multi-factor Authentication in your user profile to use the secret code field."

language/en-GB/com_users.ini

COM_USERS_ERROR_SECRET_CODE_WITHOUT_MFA="You have entered a Secret Code but Multi-factor Authentication is not enabled in your user account. If you want to use a secret code to secure your login please edit your user profile and enable Multi-factor Authentication."
COM_USERS_PROFILE_TWOFACTOR_DESC="Select the Multi-factor Authentication method you want to use."
COM_USERS_PROFILE_TWOFACTOR_LABEL="Authentication Method"
COM_USERS_USER_OTEPS_WAIT_DESC="There are no emergency one time passwords generated in your account. The passwords will be generated automatically and displayed here as soon as you activate Multi-factor Authentication."

language/en-GB/joomla.ini

JENFORCE_2FA_REDIRECT_MESSAGE="You were redirected because you are required to set up Multi-factor Authentication to continue."
JGLOBAL_AUTH_INVALID_SECRETKEY="The Multi-factor Authentication Secret Key is invalid."
JGLOBAL_OTPMETHOD_NONE="Disable Multi-factor Authentication"
JGLOBAL_SECRETKEY_HELP="If you have enabled Multi-factor Authentication in your user account please enter your secret key. If you do not know what this means, you can leave this field blank."

UPD
Also, after renaming to the MFA, it is desirable to sort alphabetically.

avatar nikosdion
nikosdion - comment - 18 May 2022

@Kostelano They are intentionally left there. They are the legacy files which needs to be removed as @richard67 said :)

@richard67 I know, I know, but I am not your average contributor, am I? 🤣 I know that if I don't put that in the script file it will be forgotten or misplaced (see the two files from the 4.1.1 release which were placed in the folders section and I moved back where they belong).

avatar nikosdion
nikosdion - comment - 18 May 2022

@Kostelano Not all of the lang strings can be removed. The joomla.ini constants cannot be removed until the next major version (the implicit arrangement is that lang strings in there remain usable throughout the major version they were introduced in). I will remove the other ones.

avatar richard67
richard67 - comment - 18 May 2022

I know that if I don't put that in the script file it will be forgotten or misplaced (see the two files from the 4.1.1 release which were placed in the folders section and I moved back where they belong).

@nikosdion That's why we have a conflict now because I've meanwhile fixed that in 4.2-dev: #37826 . If you want I solve the conflict for you.

avatar nikosdion
nikosdion - comment - 18 May 2022

I know that if I don't put that in the script file it will be forgotten or misplaced (see the two files from the 4.1.1 release which were placed in the folders section and I moved back where they belong).

@nikosdion That's why we have a conflict now because I've meanwhile fixed that in 4.2-dev: #37826 . If you want I solve the conflict for you.

Please do. We have a lightning storm here which knocked out Internet for me and the power is flickering. I can't really work like that.

avatar brianteeman
brianteeman - comment - 18 May 2022

I submitted a pr to your branch

avatar Kostelano
Kostelano - comment - 18 May 2022

The description of the method is duplicated in the frontend - under the title and when hovering over the icon. In the admin panel, the description is only under the method heading.

Screenshot_1

avatar Kostelano
Kostelano - comment - 18 May 2022

I noticed that the tab uses a much larger indent than all other tabs. It jumped into the eyes.
Consider using an identical style.

Screenshot_1

Also in the methods a small indent. Yes, I'm boring and always notice such little things :)

Screenshot_2

avatar Kostelano
Kostelano - comment - 18 May 2022

Enable any of the methods, log out. Try to log in and switch to one-time passwords when you log in. The field does not have a title and (maybe) it makes sense to add some kind of description.

Screenshot_1

avatar Kostelano
Kostelano - comment - 18 May 2022

I see that there are parameters in the localization file of the email method, but they are not implemented at the moment. Is there any information on this?

PLG_MULTIFACTORAUTH_EMAIL_CONFIG_TIMESTEP_120="Two minutes (recommended)"
PLG_MULTIFACTORAUTH_EMAIL_CONFIG_TIMESTEP_180="Three minutes"
PLG_MULTIFACTORAUTH_EMAIL_CONFIG_TIMESTEP_300="Five minutes"
PLG_MULTIFACTORAUTH_EMAIL_CONFIG_TIMESTEP_30="Half a minute"
PLG_MULTIFACTORAUTH_EMAIL_CONFIG_TIMESTEP_60="One minute"
PLG_MULTIFACTORAUTH_EMAIL_CONFIG_TIMESTEP_DESC="A new code is generated every this many minutes. Do note that a generated code is valid for at least this much time and at most twice as much time. The higher this period is the more likely it is for the code to be brute forced, therefore the least secure your site is. A period of 2 minutes is a good trade-off between usability and security."
PLG_MULTIFACTORAUTH_EMAIL_CONFIG_TIMESTEP_LABEL="Code Generation Period"

etc...
avatar nikosdion
nikosdion - comment - 19 May 2022

@Kostelano I have addressed all your feedback items so far. Thank you!

avatar Kostelano
Kostelano - comment - 19 May 2022

Can anyone fix the distribution builder + PR? It doesn't work at the moment, but would like to continue testing after changes are made.

Screenshot_1

avatar HLeithner
HLeithner - comment - 19 May 2022

Can anyone fix the distribution builder + PR? It doesn't work at the moment, but would like to continue testing after changes are made.

Screenshot_1

I'm fixing it.

avatar Kostelano
Kostelano - comment - 20 May 2022

After deleting all previously used methods, one-time emergency passwords still remain in the profile, which are not needed without multi-factor authentication.

If I'm not mistaken, the behavior in Joomla 3 was different - after disabling two-factor authentication, one-time passwords "disappeared" as well.

I don't know if this is expected behavior, just in case I report it.

UPD
One-time passwords disappear when you turn off multi-factor authentication. If to DELETE methods - passwords remain.

avatar nikosdion
nikosdion - comment - 20 May 2022

@Kostelano If you delete each method individually the OTEPs remain but they will not be requested when you log in next time.

The idea is this. You only had an Authenticator Code method added to your profile but your phone was reset / stolen. You use an OTEP to log in. First order of business, disable Multi-factor Authentication. At this point it may be several hours or days until you have a new phone. When you do, you'll come back and add the Authenticator Code again to your user profile. You do not have to reset your OTEPs in this use case so they are not deleted.

On the other hand, if you click on Turn Off it completely removes all traces of Multi-factor Authentication from your user account: all authentication methods AND OTEPs. It resets your Multi-step Authentication to the default, blank state.

The idea for Turn Off is this. All the methods you had created are unusable and you have not noted down your backup codes. Either an administrator resets MFA on your account or you were lucky to still be logged in and want to prevent a catastrophe. In this use case you do want to reset your OTEPs, hence their removal.

I hope that makes more sense now that I explained it :)

avatar Kostelano
Kostelano - comment - 20 May 2022

Users - MFA, enable the "Onboard new users" parameter, but do not fill in the "Custom redirection URL" parameter. Log out of the admin panel using the logout button, try to log in again. After each login, I am first thrown to the error:

0 Call to a member function getBackupCodes() on null

If we still fill in the "Custom redirection URL" parameter, then everything seems to work. As soon as we disable the "Onboard new users" option, the error will stop reproducing.

avatar nikosdion nikosdion - change - 23 May 2022
The description was changed
avatar nikosdion nikosdion - edited - 23 May 2022
avatar nikosdion
nikosdion - comment - 23 May 2022

Alright, y'all! I think that NOW this PR is ready for final review, testing and hopefully merging into 4.2-dev.

Everyone who had reviewed this code, can you please make a formal test so this PR can be set RTC? Thank you ever so much for your hard work and collaboration!

avatar brianteeman
brianteeman - comment - 23 May 2022

@bembelimen
This PR removes useless and unused language strings which I fully support. However you have previously rejected this as a b/c break (ref #37519 (comment))

avatar nikosdion
nikosdion - comment - 23 May 2022

Well, if language strings are removed or changed within the same release it's a B/C break. No doubt about that. When going to a new release (even a minor one) it can't be.

If changing the language strings is a b/c break we can never update anything in Joomla, add features, or remove core plugins (as we did now) without bumping the major version. If that were the case I reckon we'd already be at Joomla 697.1 or something silly like that.

avatar Kostelano
Kostelano - comment - 23 May 2022

Rules must be applied, but not thoughtlessly.

Lines have been removed from a specific extension that no longer exists. Some of the lines are left for backward compatibility, as @nikosdion rightly noted. Yes, these lines, being in the "general" file, theoretically can be used in some third-party extensions.

But why drag trash lines for another 5-7 years, I don’t understand at all.

I independently translated Joomla 4 into Russian from scratch in order to do it efficiently. I think that not many people know, but there are some lines from Joomla 1.5 that have not been used for 10+ years. Precisely because there are strange rules for preserving strings before the major version, they are simply not deleted on time.

PS. I'll test again today.

avatar brianteeman
brianteeman - comment - 23 May 2022

Well, if language strings are removed or changed within the same release it's a B/C break. No doubt about that. When going to a new release (even a minor one) it can't be.

If changing the language strings is a b/c break we can never update anything in Joomla, add features, or remove core plugins (as we did now) without bumping the major version. If that were the case I reckon we'd already be at Joomla 697.1 or something silly like that.

Don't shoot the messenger - https://developer.joomla.org/news/586-joomla-development-strategy.html#backward_compatibility

full backward compatibility can be expected within a major series. Backward compatibility may only be broken when a new major series is started.

Changing or deleting a language key is considered a backwards compatibility break.

avatar nikosdion
nikosdion - comment - 23 May 2022

So... a feature like this PR (which removes TFA and introduces MFA) would not be allowed until Joomla 5.0. But the JSST and the production leadership explicitly told me to go ahead with it, the latter demanded that it's part of com_users instead of a standalone component even. So clearly something isn't quite right one way or another.

avatar brianteeman
brianteeman - comment - 23 May 2022

So clearly something isn't quite right one way or another

exactly

avatar bembelimen
bembelimen - comment - 23 May 2022

So... a feature like this PR (which removes TFA and introduces MFA) would not be allowed until Joomla 5.0. But the JSST and the production leadership explicitly told me to go ahead with it, the latter demanded that it's part of com_users instead of a standalone component even. So clearly something isn't quite right one way or another.

Thanks for bringing it up (and again thanks for this great PR). There are several things which should be considered here.

Language files
I see the point, that it's a bit stupid, that we have to keep the language strings (not files I think) when we have removed the whole extensions. But that's for now the rule, so yes we should mark them as deprecated and then we can remove them with 5.0.

New feature
All Minor versions are open for new features, so there is no problem introducing this for 4.2 (and now here is the "but":), but we should think about B/C. We can't just disable 2FA by uninstalling the old plugins and ship non-configurated new plugins, so sites are suddenly without any protection (which they assume, they have). So we should spend a bit of brainpower here, what we can do. So probably we can migrate the old stuff to the new plugins?

Production decision
That was the reason why we made the vote. In general we can't just throw away a function and replace it with something complete different in a Minor-Release, but if Production (and not a Release Lead or one Team like JSST but the whole production) agrees, then we can go for it. But as said in the last section, we should spend some time in thinking, how we can make the progress as smooth as possible, like implementing a "Mock"-layer, so old 3rd party 2FA plugins still working (will then be deprecated and removed with 5.0).
And if we gave our best to make it as B/C as possible we then can go for it, as it was a production decision (based on JSST recomendation).

Hope this clears a bit the cloud and does not raise more questions :)

So thanks again for all this contributions.

avatar nikosdion
nikosdion - comment - 23 May 2022

Language files

This is totally stupid but it can be addressed.

I guess this also means that we can't CHANGE the contents of strings, so I have to introduce new ones. Blimey. The translators will LOVE this!

I also guess we will idiotically have language files for plugins which no longer exist. Language files with language strings ONLY used in these plugins which no longer exist.

We can't just disable 2FA by uninstalling the old plugins and ship non-configurated new plugins, so sites are suddenly without any protection (which they assume, they have).

Joomla has historically not given a toss about migration. That's not how I roll. I always provide migrations for my extensions and I sure as hell do the same for my PRs.

If you look at the upgrade SQL files I am setting the publish state of the MFA totp and yubikey plugins (the two direct equivalents to the old TFA plugins) to the publish state of their old TFA plugins' counterparts.

Further to that and as I very explicitly stated at the very top of the PR about the automatic migration of data, their configuration is automatically ported over on first use.

So not only have I put A LOT of thought into this, I have very explicitly addressed and documented the exact use case you describe. All I ask you to do is READ what I wrote before telling me that I need to put more thought into it. What you do is insulting.

Production decision

Hold up! Before I started working on this I asked for an EXPLICIT permission to work on this PR because I said upfront we are REPLACING the legacy TFA with a new captive login MFA. This is of course a b/c break — an unimportant one as nothing third party is actually affected — and a lot of work. That's the reason for asking permission in advance.

@SniperSister emailed me that the production leadership gave me their explicit permission. I received this by email on May 10th, 2022 at 16:48 EEST and @brianteeman can confirm as he was CC'ed to it:

Hey Nic,

the Production Departement team leaders voted on our proposal to refactor the current TFA implementation in Joomla 4.x to a captive-page-based approach - and I’m happy to inform you that the proposal was accepted unanimously! :)

Cheers,
David

So I am wondering what is going on here. Are you talking out of your ass @bembelimen or is this the production leadership changing its story? I am of course worried and upset, having put 120+ hours into this, with my business suffering as a direct result.

At no point was I told that we need to maintain b/c with the old and WRONG TFA methods. It's not possible anyway:

  • The old TFA stores all information in the #__users table, in two fields. This is incompatible with having multiple methods which is mandatory for preventing lock-outs, making MFA usable across devices (VERY IMPORTANT!) and so on.
  • The old TFA can only support ONE configuration because of the way it renders the TFA configuration page in the Joomla user profile. This makes it impossible to have a configuration page which works for both MFA and legacy TFA plugins.
  • The old TFA can only be a code you enter WITH the username and password — exactly the reason why the old TFA is a security risk (it only works with login methods which require a password, only works with authentication methods which generate a typeable password before user authentication — forget about WebAuthn which is the future of authentication per Google, Apple and Microsoft i.e. the makers of the most common OS in existence today — and is susceptible to phishing). Retaining b/c with that means that we are NOT solving the security issues and we are NOT solving the usability issues.

If we are introducing MFA only to have it suffer from the same problems... why bother? What is the point of spending so much time only to reiterate the mistakes of a wrong, temporary solution introduced ten years ago as a stop-gap, something which was supposed to be replaced with captive MFA back by 2015 — but the leadership failed expically to deliver?

Now, plugins for the old TFA needing b/c... I should point out that there are exactly ZERO third party TFA plugins out there because of the aforementioned problems with the legacy TFA. The only third party plugins to have ever existed where mine and they were discontinued in 2016. As a result, the whole point of a b/c layer is pointless. B/c with bloody what exactly?!

Before I spend any more time on this I need official acknowledgement that this PR is acceptable.

If not, I will have to retract it along with my relicensing of my code under the GPLv2, delete my branch and that's the end of any possibility to fix TFA in Joomla forever. I am not doing that work again and you can't use my code without violating its license. You can deprecate TFA in 4.2, tell people it's unsafe to use, remove it in Joomla 5.0 and tell people to use Akeeba LoginGuard instead — it's no shame, WordPress does the same, asking its users to use a third party TFA solution.

If it's acceptable, awesome, I can continue working on it. But before I know that this is the case I am not touching that branch. I am done being lied to and wasting my time.

avatar bembelimen
bembelimen - comment - 23 May 2022

I don't know where you read all this out from my text, I'm in favour of this PR and just wanted to explain why it is a YES, although there are some B/C breaks.

So tl;tr: We should stick to the language string B/C promise and we should try to be as much as possible B/C with the changes (I did not evaluate/ if you did it or not, just a general statement). Nothing more....and as you answered: "yes this was considered" we're all good.

So yet shorter: Everything is good if we "fix" the language thing (which I also think is stupid...but yeah). peace?

avatar nikosdion
nikosdion - comment - 23 May 2022

@bembelimen It sounded like “yes, but no, we shouldn't”. Sorry. I have been burned before and I am defaulting to abject pessimism on my interactions with Joomla. Let's just say that 2012 to 2014 inclusive were not very happy times and leave it at that.

I WILL work on the lang strings. Just to make it clear. We cannot remove lang strings even if they are of a removed plugin; we cannot delete or radically change any other lang strings. That about right?

avatar bembelimen
bembelimen - comment - 23 May 2022

I WILL work on the lang strings. Just to make it clear. We cannot remove lang strings even if they are of a removed plugin; we cannot delete or radically change any other lang strings. That about right?

Sadly yes...

avatar nikosdion
nikosdion - comment - 23 May 2022

OK then 🙈 🙉 🙊

avatar rdeutz
rdeutz - comment - 23 May 2022

I have also worked on a reply but you both figured it already out, so here what is left from my writing ....

Hi Nick,

that's really a great PR with good documentation. Thanks for working on it in nearly light speed and bringing it to a state it can be tested.

avatar nikosdion
nikosdion - comment - 23 May 2022

@rdeutz Thank you, it means a lot to me! I will improve on the documentation. Currently, every MFA method I am adding is already documented as part of LoginGuard (https://github.com/akeeba/loginguard/wiki). It's just a matter of letting me know where to put that documentation in the Joomla docs wiki and I'll do it once the PR is merged. It'd be a waste not to reuse the documentation I have already written and I'm philosophically against waste 😉

avatar nikosdion
nikosdion - comment - 23 May 2022

And now the language strings are restored. I have marked them as obsolete, making it inifnitely easier for @HLeithner to find them and remove them for Joomla 5.0.

avatar brianteeman
brianteeman - comment - 24 May 2022

My brain is failing me. Just did a clean install and I cant find where I configure MFA

avatar nikosdion
nikosdion - comment - 24 May 2022

My brain is failing me. Just did a clean install and I cant find where I configure MFA

Make sure at least one of the MFA plugins is enabled (they are all disabled by default; that's why we have the post-installation message). Then go edit your user profile. There should be a Multi-factor Authentication tab.

avatar brianteeman
brianteeman - comment - 24 May 2022

That is what I thought.

Only the fixed code is disabled by default on a clean install.

This is what I have.

image

image

avatar nikosdion
nikosdion - comment - 24 May 2022

@brianteeman Stupid question. Did you use the prebuilt packages instead of building it yourself?

avatar brianteeman
brianteeman - comment - 24 May 2022

Not a stupid question. It was a direct checkout from your branch. I just forgot to recreate the autoload

avatar brianteeman
brianteeman - comment - 24 May 2022

@chmst This really really needs an a11y audit

avatar nikosdion
nikosdion - comment - 24 May 2022

@brianteeman A few observations as I DID test this PR using a screen reader (VoiceOver on my Mac) and I have been doing that for the past week.

Changing the icons to your more complicated and less readable HTML code made no change whatsoever. What I had before had the span with the icon be invisible to screen readers and the description invisible to visual display. As a result screen readers announced the text label and visual browsers displayed the icon (and the tooltip on hover). I also think that having the icon span claim to be labeled by the label span is technically incorrect. It's not the icon that is being labeled, it's the button / link.

Omitting heading levels made no difference in non-prose pages. Having continuity in heading levels is important in prose. This is not prose, it's a management level. If you omit, for example, H3 it's not the end of the world. Screen readers still work.

Going back to a higher heading level does make a difference. What is more important and you did miss in your review is going back to a higher heading level in the middle of the text. Where you had me change H4 to H3 there was another DIV (the backup codes) going back to H4. This should've been an H5. I did address that in my last commit.

Keyboard navigation is important. I am not sure if anyone is testing this? I did and it seems to work, at least on Safari using VoiceOver.

avatar brianteeman
brianteeman - comment - 24 May 2022

accessibility is not just about screenreaders or navigating with keyboards.

Just because you didnt observe an error does not mean that one does not exist. Skipping a heading level for example is a big no no due to the way people and tools navigate a page. (yes I know we have errors with this elsewhere but it doesnt mean we should allow new ones to be created)

avatar brianteeman
brianteeman - comment - 24 May 2022

I notice that you are putting two spaces after an echo. Is this a code style change?

example
<?php echo $this->title ?> <small> &ndash;

avatar brianteeman
brianteeman - comment - 24 May 2022

Unable to test the webauthentication methods as I am getting the following error

An error has occurred.

0 There is no "plg_multifactorauth_webauthn.webauthn" asset of a "script" type in the registry.

Call stack

| Function | Location

1 | () | JROOT/libraries/src/WebAsset/WebAssetRegistry.php:132
2 | Joomla\CMS\WebAsset\WebAssetRegistry->get() | JROOT/libraries/src/WebAsset/WebAssetManager.php:277
3 | Joomla\CMS\WebAsset\WebAssetManager->useAsset() | JROOT/libraries/src/WebAsset/WebAssetManager.php:201
4 | Joomla\CMS\WebAsset\WebAssetManager->__call() | JROOT/plugins/multifactorauth/webauthn/tmpl/register.php:18
5 | include() | JROOT/plugins/multifactorauth/webauthn/src/Extension/Webauthn.php:153
6 | Joomla\Plugin\Multifactorauth\Webauthn\Extension\Webauthn->onUserMultifactorGetSetup() | JROOT/libraries/vendor/joomla/event/src/Dispatcher.php:486
7 | Joomla\Event\Dispatcher->dispatch() | JROOT/administrator/components/com_users/src/Model/MethodModel.php:102
8 | Joomla\Component\Users\Administrator\Model\MethodModel->getRenderOptions() | JROOT/administrator/components/com_users/src/View/Method/HtmlView.php:114
9 | Joomla\Component\Users\Administrator\View\Method\HtmlView->display() | JROOT/administrator/components/com_users/src/Controller/MethodController.php:120
10 | Joomla\Component\Users\Administrator\Controller\MethodController->add() | JROOT/libraries/src/MVC/Controller/BaseController.php:735
11 | Joomla\CMS\MVC\Controller\BaseController->execute() | JROOT/administrator/components/com_users/src/Controller/MethodController.php:75
12 | Joomla\Component\Users\Administrator\Controller\MethodController->execute() | JROOT/libraries/src/Dispatcher/ComponentDispatcher.php:146
13 | Joomla\CMS\Dispatcher\ComponentDispatcher->dispatch() | JROOT/libraries/src/Component/ComponentHelper.php:389
14 | Joomla\CMS\Component\ComponentHelper::renderComponent() | JROOT/libraries/src/Application/AdministratorApplication.php:145
15 | Joomla\CMS\Application\AdministratorApplication->dispatch() | JROOT/libraries/src/Application/AdministratorApplication.php:188
16 | Joomla\CMS\Application\AdministratorApplication->doExecute() | JROOT/libraries/src/Application/CMSApplication.php:296
17 | Joomla\CMS\Application\CMSApplication->execute() | JROOT/administrator/includes/app.php:63
18 | require_once() | JROOT/administrator/index.php:32

avatar nikosdion
nikosdion - comment - 24 May 2022

@brianteeman Oh, come on, you know very well who you are talking to. I am not your average ignoramus armed with a keyboard, a code editor and little sense. I've been trying to improve accessibility in my own software ever since all the way back in 2011 I received a complaint that a user's blind wife could not use my software. BTW, the recommendation to use a screen reader was yours and you last made it a few months ago to me.

Accessibility is about how people with any form of disability (motor, cognitive, visual, hearing etc), regardless of its degree, can navigate the site. I have ADHD; I know that as someone affected by these issues on sites (in my case, excessive motion and noise, unrelated eye-catching asides within the main text and so on).

So I am asking you what EXACTLY is the problem encountered by people if a heading level is skipped? I am asking you because for very obvious reasons you cannot always guarantee the heading level in all generated pages of the site. For example, you have conditionals. Think about something like this pattern which is very common in Joomla:

<h3>Something</h3>
<p>Some text</p>
<?php if ($headingFlag): ?>
<h4>Another heading</h4>
<?php endif; ?>
<?php if ($hasDragons): ?>
<div class="alert alert-warning">
  <h5 class="alert-heading">Beware of the dragons</h4>
  <p>Brave traveller, beware! Dragons lie ahead. Equip with your fireproof armour or face a fiery death.</p>
</div>
<?php endif; ?>
<p>Even more text</p>

In this case the heading level inside the alert would nominally have to change between h4 and h5 depending on the $headingFlag. This is a simple example with a very easy fix. In most cases the fix is nowhere as simple and fixing it could even lead to code that's outright unmaintainable or prevent reuse (think about Layout files).

So I am asking you again. What are the REAL WORLD consequences of the missed heading level? Telling there are some definite problems must mean that you can answer this simple question by listing the definite problems. I am not being contrarian, I just want to know, dammit! If I understand it I can address it. If I don't and nobody bothers to explain it I will write it off as a pseudoreligious axiom.

I notice that you are putting two spaces after an echo. Is this a code style change?

No, that would be a typo.

My code originally had <?= which as of PHP 5.4.0 is always allowed (regardless of the PHP short open tags directive) and equivalent to <?php echo . Joomla's code style is stuck in the Dark Ages, before 5.4.0 was a thing (so, more than 12 years ago) and disallows short echo. As a result I mass searched and replaced short echo with the more verbose equivalent. I also tried to search for <?php echo (note the trailing two spaces) and replace it with <?php echo (note the single trailing space). It's possible I missed something. I will go through this again.

avatar nikosdion
nikosdion - comment - 24 May 2022

@brianteeman

Unable to test the webauthentication methods as I am getting the following error

I accidentally misspelled the extension registry file name after renaming from TFA to MFA. GIve me 5' to fix it.

avatar nikosdion
nikosdion - comment - 24 May 2022

@brianteeman I cannot help but notice that the first two articles DO NOT mention AT ALL that heading levels should not be skipped. They also did not tell me anything that I did not already know — I am familiar with WebAIM, thank you. A missing level in the structure would at worst be rendered as such and that's all there is to it.

The third link, the W3C specification, actually says:

Skipping heading ranks can be confusing and should be avoided where possible: Make sure that a <h2> is not followed directly by an <h4>, for example. It is ok to skip ranks when closing subsections, for instance, a <h2> beginning a new section, can follow an <h4> as it closes the previous section.

Let me stress that:

avoided where possible

So, yes, if we CAN avoid it then we SHOULD avoid it, not that it's the end of the world or cause a major issue if we don't.

Remember that I told you that in some cases it may NOT be possible to do this, at least not in a way which does not make the software practically unmaintainable. Therefore, unless it's causing a show-stopper issue, it makes sense to prioritise maintainability over dogmatic obsession to a mere recommendation of “avoid where possible”. It's a yellow traffic light (stop unless it's not safe to do so), not a red light (MUST stop no matter what). You are treating it as a red light. I am asking you why.

Again: what is the REAL problem which would occur? You have not answered my question; you have avoided answering it and I'm not the kind of person which lets you off the hook with that. (Yeah, my professors hated me too, please join the queue).

What is the tool used by people with any form of disability which would break in such a spectacular fashion as to prevent them from navigating the site correctly and what exactly is that spectacular failure?

To be clear, what you pointed out was not a big deal to fix BUT there are cases where it's actually a massive deal to fix. I understand the risks of changing the code to an unmaintainable level (cannot customise leading to market loss, convoluted code results in introduction of bugs every time it is touched, non-reusable code makes it all the more likely that a bug fixed in one place persists in another and so on and so forth). I do NOT understand the risk of skipping a heading level and the articles you linked me to tend towards “it may be a bit confusing but it's not a deal breaker”. Yet, you say it's a deal breaker. WHY? What do you know that is not in your linked resources?

avatar brianteeman
brianteeman - comment - 24 May 2022
  1. Almost every accessibility checker that exists will flag up skipped headlines as at least a warning
  2. skipto (whose inclusion has been massiveley applauded in the a11y world) is set to only include h1,h2,h3
  3. when using most screenreaders you can use the keyboard to navigate through the headings. This is either linear or based on the heading level
  4. https://www.w3.org/TR/WCAG20-TECHS/G141.html

To facilitate navigation and understanding of overall document structure, authors should use headings that are properly nested (e.g., h1 followed by h2, h2 followed by h2 or h3, h3 followed by h3 or h4, etc.).

The dealbreaker part is
At the end of the day it doesnt matter at all what either of us think about this. What matters is that someone evaluating joomla for accessibility before adopting it does believe it to be accessible. Most evaluators will staart their evaluation by using an automated tool so if we are able to remove an issue from a bug report then we should do.

ARC toolkit

image

Wave

image

Jooa11y, Sa11y, editoria11y

image

avatar brianteeman
brianteeman - comment - 24 May 2022

registry fix now lets me in ;)

IS this intentional to have the two buttons in different rows etc

image

image

avatar brianteeman
brianteeman - comment - 24 May 2022

Email by code

The input field is of type number which means it displays a spinner and a screen reader will do things like announcing the min and max values and the step (even if you dont explicitly set them)

I would probably change that to
<input type="text" pattern="{0,9}" maxlength="6" inputmode="numeric">

note the inputmode is for mobile devices

avatar brianteeman
brianteeman - comment - 24 May 2022

Backup Codes

How about changing it to

Before

image

After

image

and then putting the information to print out the codes on the next gae

Before

image

After

image

avatar nikosdion
nikosdion - comment - 24 May 2022

IS this intentional to have the two buttons in different rows etc

I would probably change that to

Please submit a PR with the code which makes these changes. If not, this is the best I can offer. Take it or leave it.

avatar nikosdion
nikosdion - comment - 24 May 2022

Backup Codes
How about changing it to

Please submit a PR with the code which makes these changes. If not, this is the best I can offer. Take it or leave it.

avatar brianteeman
brianteeman - comment - 24 May 2022

wanted to see if it would be approved first. will happily submit a pr

avatar nikosdion
nikosdion - comment - 24 May 2022

wanted to see if it would be approved first. will happily submit a pr

I REALLY want to see your PR for the first two issues. I know what it takes. You will quickly realise that there are four alternatives:

  1. Go to hardcoded HTML layouts for every MFA method plugin. We have already learned from the legacy TFA that this solution does not scale, does not extend, does not help when making any change to the CMS or the CSS framework.
  2. Rewrite the abstraction, its implementation and a good third of each plugin's code from scratch. Massive amount of work, nobody will do it, least of all me (I already sunk 100+ hours into this; at some point I have to let this PR be and start doing some paid work to make me a living).
  3. Remove input field types (mobile no longer autofills the code), remove the logout button (users are trapped in MFA without any way to exit bar clearing cookies). The problems introduced are worse than those solved.
  4. Leave it as it is.

For the Backup Codes, sure, you can submit a PR but I will reject it because it's a UX nightmare and reintroduces a problem I solved years ago. Did you REALLY think I added an alert out of the blue for no reason? Tsk, tsk, tsk...

Real problem in real world sites: users do not know what the backup codes are. Worse, yet, they do not notice that area when the backup codes are automatically generated upon registering their first MFA method. As a result they get locked out and have to email me to reset their MFA because they don't have backup codes at hand. I was replying to 2 emails like that every day. Ever since I added the alert on that page I am replying to 2 emails like that A MONTH (85% improvement). Of these emails I still have to field the majority are users who lost everything, including their backup codes. It happens. The others are users who had set up their MFA before I added the notice. Therefore the actual reduction of problems by this simple change is closer to 95%. Conversely, removing it now would increase the problems with users getting locked out after losing their MFA methods 20-fold.

avatar brianteeman
brianteeman - comment - 24 May 2022

two pr submitted

avatar brianteeman
brianteeman - comment - 24 May 2022

Authenticator app - debug?

Please check your authenticator app setup, and make sure that the time and time zone on your device is set correctly.

Tried multiple authenticator apps on my phone and each time the verifcation code is rejected. The time and time zone on my phone are set correctly (its done automatically)

Any suggestions how to debug this further

avatar nikosdion
nikosdion - comment - 25 May 2022

@brianteeman One of your PRs is rejected with prejudice nikosdion#7 Please refer to the explanation I gave there. What you are proposing is an evidently BAD user experience. Your mental model of what users understand best is diametrically opposite with what dozens of thousands of real world users have demonstrated they understand best over the past 6 years. A user study with real world users on a real world sites is infinitely more important than what one person THINKS it's best. UX is about serving the users, not our preconceptions and prejudices.

I am still evaluating the other PR (nikosdion#8). I do not like this approach. There must be a better way.

avatar brianteeman
brianteeman - comment - 25 May 2022

lol -if only i was so arrogant to think my code was perfect and could never be improved. You are correct. The ui/ux on your own site for mfa is so bad i can believe that people never see that there are backup codes.

But it is awesome to hear thAt you have tens of thousands of users that are using mfa on your site.

avatar nikosdion
nikosdion - comment - 25 May 2022

@brianteeman Are you for real? The sum total of your experience is doing training for a few hundred people. So let's talk numbers.

From 2006 to the present day I have had 100,806 users registering a user account on the site out of which 36,786 have purchased at least one subscription. There are currently 17,172 active users (people who have interacted with the site at least once in the past year). Between 2006 and today I have replied to 37,186 support tickets and an approximately equal number of forum posts, contact requests and other means real world people have contacted me asking for my help.

At the time of this writing there are 1,752 users on akeeba.com which use an MFA method. Make that 1,753 because I had to reset someone's MFA three hours ago because they lost their YubiKey and the printout of their backup codes. Over the past 6 years the total number of people on akeeba.com who have used an MFA method is well over 10,000.

Your “improvement” was a regression to what the MFA methods management page looked in Akeeba LoginGuard between 2016 and 2017. It was causing a real problem with real world users. Ever since I added this info box the problem disappeared. You are STUBBORNLY insisting that your suggestion, proven wrong in the real world, is right whereas my design, proven better in the real world, is wrong.

Who's arrogant now?

I am here on a good will gesture, trying to contribute the code I have written and been maintaining for the last 6 years. I have put thousands of hours into it. What I contributed is real world tested.

I am not obliged to sit here and take this kind of abuse and attack on my experience and ability to deliver software. Even more so when your PROVEN CATASTROPHIC changes to Joomla are committed without as much as a second look and despite my clear warning that there is a problem. Meanwhile my contributions are met not just with excruciating scrutiny but with outright insults like this.

What the heck is your problem with me, huh?

Every single time I have warned you that something is wrong and will cause real world problems some arrogant clown who has zero clue of the subject matter tells me I don't know what I am doing, you people know better and so on. EVERY. SINGLE. TIME. I have been proven right!!! EVERY. SINGLE. TIME!!! Do you want me to remind you about the disastrous problems in Joomla Update I warned you about FOUR TIMES between 2020 and August 2021? The problems I was spot on and I even filed a PR to fix your most blatant mistakes which is STILL open since January 2nd, 2022 because you're all too afraid to accept you were wrong?

In any case, I have no reason to be here and take this abuse. I am retracting my PR, deleting my branch and revoking my permission to relicense my code from GPLv3 to GPLv2. You are FORBIDDEN from using it until you relicense your own CMS to GPLv3 (which you can't, as you can't get the permission of all code authors, LOL!). If you dare do as much as copy anything from my code I'll sue you to oblivion. I've had enough!

avatar nikosdion nikosdion - close - 25 May 2022
avatar nikosdion nikosdion - change - 25 May 2022
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2022-05-25 16:17:51
Closed_By nikosdion
avatar richard67
richard67 - comment - 25 May 2022

@nikosdion Please don't do that. The Joomla community is more than one single person.

avatar richard67
richard67 - comment - 25 May 2022

And please guys no swear words.

avatar nikosdion
nikosdion - comment - 25 May 2022

I am done with this. Joomla should deprecate the bad TFA in Joomla 4 because it's a massive security issue and remove it in Joomla 5. It should also suggest that its users make use of WebAuthn (if you people can figure out how to improve it without copying my code) or a third party TFA/MFA solution.

PS: Sorry about the swear word. I will edit and remove it.

avatar brianteeman
brianteeman - comment - 25 May 2022

Enable the option to onbpoard new users.
Log in as a manager and you get a 403 on the captive login

The reason is that the default permissions for the users component do not allow a manager to access the administrator interface

avatar brianteeman
brianteeman - comment - 25 May 2022

We all have one aim - to make joomla the best it can be. Thats we work together to try and improve it. Sure we dont always agree on how it can be improved or in which direction those improvements should take. But no one demands anything. I only made suggestions. Some of those suggestions you have accepted. Others not and have demanded empirical evidence such as the skipped headings.

That's fine - I am not crying - I just dont agree with you.

avatar richard67
richard67 - comment - 26 May 2022

@nikosdion Please think it over and restore your branch and reopen this PR. It would be sad to throw away all your work.

avatar nikosdion
nikosdion - comment - 26 May 2022

My work is far from being thrown away. It's been part of a product called Akeeba LoginGuard which is free of charge and has been around since September 2016. The idea was that I was going to contribute that to Joomla itself.

As I have shown in this PR I do not claim that I never make mistakes or know everything. I am open to suggestions which have good, stated reasoning. I will not accept suggestions for what has already been tried and found to be an abject UX failure, no matter what the person making the suggestion believes. Field testing trumps personal belief (not to mention that my PERSONAL belief as to what would have been a better user interface coincides with said suggestion; unfortunately it was proven wrong).

My problem is that rejecting a suggestion with an explanation of how field testing already proved it wrong should NEVER be an excuse to be passively-aggressively called a liar and my ability to write software called into question. If anything, I have a track record of brutal honesty and maintaining the top rated extension in JED for 12 years straight.

It is very disheartening to see that this kind of toxic behaviour is considered acceptable, if not expected. It's not the first time I have encountered this nor am I the only one. I have talked to other 3PDs who have given up contributing to Joomla and they all had the same horror story to tell. Normalising a toxic contribution environment is the very reason Joomla does not get any new contributors and losing the few it already has.

In any case, nobody seems to want to apologise to me for the abuse I received and I'm pretty much told to deal with it. So I am dealing with it by not contributing. I have better things to do with my time.

avatar brianteeman
brianteeman - comment - 26 May 2022

Nik - I apologize if you were offended by my comment and that you understood my comment in a different way to what was intended. I have to say I felt the same about your comments to me about the headings but I deleted my reply and carried on trying to test this PR because its the best thing for everyone.

avatar nikosdion
nikosdion - comment - 26 May 2022

@brianteeman Apology accepted and I also apologise for my comments about the headings.

I was honestly curious as to what problem occurs and in which tool. I am using VoiceOver. It has the WebRotor which finds and lets you navigate to various places on the page. One of its features is navigating by headings. It announces that the first level is, for example, heading level 3. It's very clear. I even tried using Screen Curtain (which blacks out the screen) and navigate the page only with audio — a feat for me as ADHD and rapid spoken text are naturally born enemies. I had no problem doing that, even on site pages other than MFA. That's why I was perplexed more than anything else about your remarks that a screen reader is not what I should focus about and something would have a problem.

I actually went through the entire accessibility documentation for macOS and the only navigation options I found are:

  • VoiceOver using speech
  • VoiceOver using a refreshable Braille display — I could only test with the virtual Braille display, obviously, which also shows text legends underneath it and found it to be the same as VoiceOver with speech.
  • Button / joystick / trackpad controls. They use the same rotor as VoiceOver.

I even tried Safari, Edge and Chrome just in case something renders differently. Nope, they all work the same.

I also observed that Joomla itself, both Cassiopeia and Atum, don't have heading continuity. Cassiopeia, for example, has the site top area in a BANNER and the main content area in a MAIN which are announced differently. The sidebar is an ASIDE but not announced differently, it's announced as H3.

That was with using structural navigation for WebRotor. When I switched to context navigation which takes into account the rendered layout it was even easier to navigate.

I fully understand how the accessibility checkers work, however I am the person who always says that the role of tools is to help us solve real world issues. Being a slave to the tool doesn't improve much.

For MFA, the heading levels are going to be arbitrary anyway. Some templates will render the site name inside a BANNER, others will have is as H1. Some templates may even have an H2. Which is the top-level heading we should use in MFA? It should be H1 or H2 in the frontend but which one... I can't know.

It's worse for any other frontend component as its top level heading would have to take into account how the HTML before the main content area is structured. The top level heading could be H1, H2, H3 or even H4. You can't really solve that in any way that makes sense and doesn't make the code impossible to read and the component impossible to configure.

Based on that, I wanted to know: if we skip a level will the person with disabilities still be able to navigate the site or not? The answers seems to be that yes, they can, there's just a bit of confusion because the first heading level announced is not H1.

I hope that now I expanded on it you understand what I was trying to figure out and why I was so insistent on asking what accessibility software other than a screen reader (and the similarly working “joystick control” family of control software, typically for people with motor disabilities) may exist that I don't know of.

Anyway, I will reopen this PR when I have some time. Right now I need to do some paid work first as it's been two weeks since I last did that and it's catching up with me :(

avatar brianteeman
brianteeman - comment - 26 May 2022

I fully understand how the accessibility checkers work, however I am the person who always says that the role of tools is to help us solve real world issues. Being a slave to the tool doesn't improve much.

I am not a fan of devloping based on test results either but the reality is that this is something that is immediately flagged as a warning by all testing tools.

I actually went through the entire accessibility documentation for macOS and the only navigation options I found are:

This is why the accessibility experts say that we shouldnt try and test screen readers unless we are regular users. For example there are a gazillion hotkeys available to you if you have a screenreader that let you navigate a page in ways that those of us who can see would never imagine. You have to see it in action by an experienced user to understand. For example I can hit a hotkey to list all the links on a page or all the headings. Or even jump to the next heading level etc etc. now that I know about them I really miss not having access to them from my keyboard when I am not running a screen reader which was how I found the skipto script

This is one of the reasons thata the generaal advice is to always use headings and to use them in order without skipping a level. As this is something that we can do programatically in many places then we should do it if we can.

For example we have already started to modify the code in the article views to change the heading level of the item title programatically depending on if the page title is being displayed or not.

avatar nikosdion
nikosdion - comment - 26 May 2022

For example there are a gazillion hotkeys available to you if you have a screenreader that let you navigate a page in ways that those of us who can see would never imagine.

You underestimate me. I read the documentation and used nothing but the shortcuts.

For example I can hit a hotkey to list all the links on a page or all the headings. Or even jump to the next heading level etc etc.

The first is called WebRotor (VO-U) the latter are the VoiceOver navigation hotkeys (VO-RIGHT, VO-LEFT, VO-SHIFT-RIGHT, VO-SHIFT-LEFT).

I also told you, I used the Screen Curtain hotkey to hide the screen. I was only using my ears and my keyboard. I don't do things by half. I am crazy like that.

avatar brianteeman
brianteeman - comment - 26 May 2022

The link would have helped https://webaim.org/resources/evalquickref/

But also see this from the webaim 2022 (please trust me - if they didnt think it was imporytant then they wouldnt be emphasising itn https://webaim.org/projects/million/#headings

avatar nikosdion
nikosdion - comment - 26 May 2022

There ARE many more keys. I gave you an example, not a documentation. The documentation can be found at https://support.apple.com/guide/voiceover/welcome/mac For navigating web pages the starting point is https://support.apple.com/en-gb/guide/voiceover/vo27974/mac I briefly talked about the Web Rotor https://support.apple.com/en-gb/guide/voiceover/mchlp2719/10/mac/12.0 and how I navigated using landmarks https://support.apple.com/en-gb/guide/voiceover/vo35709/10/mac/12.0 There is of course Quick Nav which is what you are trying to talk about https://support.apple.com/en-gb/guide/voiceover/vo27943/10/mac/12.0 And that's just one screen reader I happen to be using.

In any case, let me tell you something about this:

For example we have already started to modify the code in the article views to change the heading level of the item title programatically depending on if the page title is being displayed or not.

Congratulations, you are addressing a subcase of a niche case without addressing the problem itself.

As I said, one template has the site name / logo inside a banner, the other uses an H1. The top-level heading level on your article would be H1 or H2 respectively. This should ALSO inform the starting heading level in the content which let me remind you is hardcoded HTML in the database.

Now let's take into account that the same URL with tmpl=component does not have the site chrome. In the first template there's no change in the headings ranks. In the second template you know need to promote the content's top level rank from H2 to H1. You also need to change the content accordingly.

But your content is hardcoded HTML in the database. You cannot change it because it'd break existing sites. Such a breaking change would lead people back to third party content management extensions with all the problems we've encountered.

You think that's unlikely to happen? It will happen just using the core: the Agree to ToS plugin would do that, for example.

Therefore any automated way to handle this would lead to the same problem if you use a different template than Cassiopeia or tmpl=component. The former is the typical use case of Joomla, the latter is common in the CMS even in core.

So this change doesn't actually address any real world problem. It's only meant to have a default Joomla installation satisfy the checklist of an accessibility checking tool so that Joomla can claim it's accessible by design. However, I posit it creates an ever bigger issue.

The reality is that the only way to address this problem is having a site integrator who understands how core Joomla and all of its extensions work, curating the content and the template overrides to explicitly avoid this kind of problem. This kind of site wouldn't benefit from your change. However, the average site integrator will now put less effort into accessibility checking because you told them jOoMlA iS aCcEsSiBlE bY dEsIgN.

The real solution? Sure, yes, do have the default installation accessible. But also educate the site integrators, explaining that once they start using third party software they need to do accessibility assessments and template overrides.

In any case, back to this PR.

The reason I was asking all these question is that I know FOR A FACT that I cannot choose the right top-level heading rank for my code as I do NOT know which template it's going to be used in and what is in the module positions the site owner has explicitly allowed in com_users' configuration.

If I were to make an accessibility checker happy for the default Joomla installation with Cassiopeia I should be using H1 and H2. However, if the user has installed a different template or used modules which appear in the HTML structure BEFORE my MFA feature's output I can't use H1 and H2, I'd be causing the exact wrong structure problem the article you linked me to is talking about! That's why I chose H3.

However, by choosing H3 I now know that the default Joomla installation with Cassiopeia has two skipped heading ranks (H1 and H2). So what I was trying to establish is whether my compromise is actually causing a problem I cannot detect by using a screen reader. This, you have still not answered and it's the only thing I don't know. Everything you shared with me, sure, I know. But that knowledge doesn't answer my question!

From my point of view, you seem to only be taking into account the unrepresentative use case of a default Joomla installation or at least a site using Cassiopeia. I am torturing myself with "how I do make something that makes sense for BOTH the default installation / Cassiopeia nobody (except few of us, like my wife and myself) will use in the real world AND the way most people actually use Joomla". It's easy to address an issue for the one use case nobody cares about. It's really effin' hard trying to find a good compromise which works reasonably well across the majority of our users.

As you said, we ARE on the same side, we ARE both trying to make it better for everyone. Can you please help me with that objective? That's all I am asking. It's okay to say that you don't know; that's an acceptable answer too. If this is the case we note that as pending a user test from someone who has a visual impairment and uses screen readers as their only way to interact with a site. Fair?

avatar brianteeman
brianteeman - comment - 26 May 2022

If you remember the point I raised in this pr regarding the skipped heading was that you were skipping a heading in your own code. I agree that its impossible to know what the top heading is etc etc but we cn at least avoid it here.

All my comments have been about use within atum. I have not tested anythig at all with this pr and cassiopeia.

I am not going to change my opinion about skipped headdings. They are part of the standard tests. People put great value on those test results without looking any further. So if we can fix it (and here it is two seconds to fix) we should.

avatar nikosdion
nikosdion - comment - 26 May 2022

@brianteeman Yup, I know what you reported and I fixed that skipped heading level immediately upon reading your feedback. That was not put into question. Sorry if there was some misunderstanding about that.

Your feedback, however, made me think about whether my compromise about the top level heading would cause a problem or not. That's why I kept nagging you.

The way I see it, there are two and a half options about the top level heading:

  • Use a top heading rank which ensures continuity of heading ranks in the default Joomla installation (Cassiopeia in the frontend, Atum in the backend).
  • Use a top heading rank which is unlikely to cause problems with third party templates.
  • Use a top heading rank which ensures continuity of heading ranks in the backend (people are unlikely to use anything other than Atum) and a top heading rank which is unlikely to cause problems with third party templates in the frontend.

I personally think that the third (combined) option is the best compromise even though the default frontend with Cassiopeia will be skipping H1 and H2. If you have another practical idea I want to hear it.

Does me phrasing it like that make more sense to you?

avatar brianteeman
brianteeman - comment - 26 May 2022

@brianteeman Yup, I know what you reported and I fixed that skipped heading level immediately upon reading your feedback. That was not put into question. Sorry if there was some misunderstanding about that.

What you said at the time suggested you objected to the request to fix the skipped heading. You had already rejected the request for a top level heading on the page in the admin.

Omitting heading levels made no difference in non-prose pages. Having continuity in heading levels is important in prose. This is not prose, it's a management level. If you omit, for example, H3 it's not the end of the world. Screen readers still work.

With my understanding of what you are clarifying now - it is completely off topic for this PR as you are referring to generic scenarios.

Can I suggest that when you are ready to re-open the pr you actually do it as a new pr so it has a cleaner and on-topic comment history

avatar nikosdion
nikosdion - comment - 26 May 2022

@brianteeman I would appreciate it if you did not lie profusely about what has taken place on the public record.

First a screenshot.

Screenshot 2022-05-26 at 20 10 01

Now the links painting this sequence of events which is diametrically opposite to what you allege:

  • You ask me to change the heading level in #37811 (review)
  • I do it at the first opportunity I have in front of a code editor with 7227c6c
  • AFTER THESE EVENTS I ask you about heading continuity #37811 (comment)

How can you even allege I was objecting to the change of heading order since I had already committed your suggestion — and even carried it forward to the frontend which you had not yet reviewed — before starting the discussion on heading levels?!

You had already rejected the request for a top level heading on the page in the admin.

Nobody had ever made this kind of suggestion, nor would have it made sense — the front- and backend pages actually use the same templates with very minor changes between them. Same headings are used in both places.

What I did reject is adding TOOLBAR BUTTONS instead of inline buttons and that was seven days before the discussion on headings.

At no point had you asked to add a heading in any page. You asked about a toolbar button and I explained why it can't be done.

At no point did I reject your suggestion to reorder the headings. In fact, I committed it straight away in both front- and backend even though you only suggested one.

This is exactly the toxic environment I was talking about. I don't care if you do it on purpose (unlikely) or just because you can't be bothered to test your blatantly wrong recollection against the public record. The fact remains that you are dragging my integrity through the mud when I have NEVER, EVER given anybody an excuse to do so in the 18 years of my career so far. I am deeply insulted by these unfounded allegations.

Brian, you're a long-time friend, I appreciate you, but I've had enough of this. If you can't figure out how to communicate without being toxic — regardless of your intention — please stay out of my PRs. Thank you, mate.

Add a Comment

Login with GitHub to post a comment