@andyjdavis opened this Pull Request on March 28th 2021 Contributor

Description:

Trying to avoid a situation where admin turns on require 2fa without having it set up themselves. https://github.com/matomo-org/matomo/issues/17352

This is just a text change in the admin UI. I will ask my questions about how to determine whether the current user has 2fa enabled in the orginal issue (#17352 ).
2fa

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@andyjdavis commented on April 6th 2021 Contributor

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!"

@diosmosis commented on April 6th 2021 Member

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.

@andyjdavis commented on April 7th 2021 Contributor

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.

@andyjdavis commented on April 8th 2021 Contributor

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

@andyjdavis commented on April 12th 2021 Contributor

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.

@diosmosis commented on April 12th 2021 Member

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

@andyjdavis commented on April 12th 2021 Contributor

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

https://github.com/matomo-org/matomo/pull/17400/commits/4e6af4893795e69de78dbbf5f1a24c257a1ef304

@diosmosis commented on April 12th 2021 Member

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 :+1:. 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.)

@andyjdavis commented on April 12th 2021 Contributor

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.

@diosmosis commented on April 12th 2021 Member

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'];
}
@andyjdavis commented on April 13th 2021 Contributor

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

@diosmosis commented on April 13th 2021 Member

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

@andyjdavis commented on April 13th 2021 Contributor

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

This Pull Request was closed on April 13th 2021
Powered by GitHub Issue Mirror