I believe I currently have two independent problems in this PR. Quite impressive for such a small amount of code :| I'm new to this repo so likely making simple mistakes. Hopefully those mistakes are immediately apparent to someone who knows Matomo better.

1. This error is displayed.
   Maximum function nesting level of '500' reached, aborting!

The 500 comes from my PHP settings.
xdebug.max_nesting_level=500

The error is caused by something in these 3 lines as it doesn't occur if I comment them out. The function to set up the settings appears to be being called over and over.

$twoFa = StaticContainer::get(TwoFactorAuthentication::class);
$isWritable = $twoFa->isUserUsingTwoFactorAuthentication(Piwik::getCurrentUserLogin());
$setting->setIsWritableByCurrentUser($isWritable);

Interestingly the error is displayed even when first trying to access matomo before even reaching a log in page.

2. As mentioned in another comment thread once I include a call to Piwik::translate() the general settings page only partially loads. This assumes I have the 3 lines of code above commented out to let the page load at all. Other than adding the strings to the plugin's en.json I'm unsure if there are other steps i need to perform.

Having Matomo in development mode doesn't seem to result in anything else being output. The error log just contains the message "Error in Matomo: Maximum function nesting level of '500' reached, aborting!"

Hi @andyjdavis looks like there's some infinite recursion. From a quick look, it seems like it's caused by accessing TwoFactorAuthentication in the SystemSettings class. TwoFactorAuthentication requires SystemSettings to be injected in the constructor, and SystemSettings tries to access TwoFactorAuthentication during construction creating a cyclic dependency. You will probably have to refactor and move the logic in TwoFactorAuthentication::isUserUsingTwoFactorAuthentication() to a static method and access that in SystemSettings.

Hi @diosmosis I've had a go at avoiding the circular dependency and I think I've got it all working. It was fairly straightforward to make TwoFactorAuthentication::isUserUsingTwoFactorAuthentication() a static method. It calls a few private member functions which I have also made static. Luckily it didn't turn out to require anything that made having an instance of TwoFactorAuthentication necessary.

I believe I have found and updated every location calling TwoFactorAuthentication::isUserUsingTwoFactorAuthentication()

Let me know if the way I have done this isn't ok. As an alternative I could leave the existing method as non-static then add a new static method. The non-static method could just call the static method. That would reduce the size of the diff and mean that any other third party code (plugins etc) not in this repo that use the TwoFactorAuthentication class wouldn't need to change. I'm not sure if that is a risk here.

Looks like there are some problems I need to fix. I will get onto them as soon as I am able.

Update. There is a failing test for which I don't yet have a solution that I am happy with plus the last commit in this PR definitely shouldn't be merged. I am setting this PR to draft until I get it into a state where i believe it can be merged (pending review of course).

This problem is that the superuser created for system tests does not have 2fa set up. That means that don't have the ability to turn on the system setting requiring all users to use 2fa. One of the 2fa related system tests toggles on the require 2fa setting but this now results in an error.

@andyjdavis I took a quick look and looks like the latest build is passing (except for BlobReportLimitingTest which is a random failure). Unless I'm missing something, I'm happy to review again tomorrow.

@dio The last commit currently in this PR makes the test pass but I'm pretty sure it largely defeats the point of the 2fa check. Apologies for leaving that commit up. I'm at work right now so can't remove it.

4e6af48

Ok @andyjdavis, taking a look at that code, a quick fix for the problem would be to replace Piwik::hasSuperUserAccess() with defined('PIWIK_TEST_MODE'). No need to go through too much trouble for this PR, though, we know it's not your job 👍. You're free to finish it up in your own time or if you don't have the time to spare one of us can put the finishing touches in. (If you want to finish the PR yourself, then you're right about it being possible to simplify the static method change. If it would make the PR effectively smaller it would be a good thing to do.)

defined('PIWIK_TEST_MODE')

@diosmosis this makes things so much simpler than what I was trying to do :) The tests should now pass.

When not in test mode $setting->setIsWritableByCurrentUser() relies on whether the current user is using 2fa.

Hi @andyjdavis, thanks for making the change! The code looks good. I tested the change locally and I noticed the removal of the field could be a problem, since a user may never know the option exists. Instead of using setIsWritableByTheCurrentUser, can you instead use uiControlAttributes to set a disabled="disabled" property on the form field? Something like:

if (!$isUsing2FA) {
    $field->uiControlAttributes = ['disabled' => 'disabled'];
}

@diosmosis I had the same usability concern but wasn't sure how to go about it. I've now pushed a commit that disables the setting without removing it. I have also updated the string to give the user some information on what they need to do to get the setting to enable.

Thanks @andyjdavis, the change looks good to merge now! Thanks again for the contribution and for following through with the requested changes.

@diosmosis thank you for your patience while I worked through this :)