Add functionality to limit MFA to specific user groups

scott-nz commented 3 years ago

https://github.com/silverstripe/silverstripe-mfa/issues/398

AC: CMS admin can set which groups will have MFA enabled. AC: If no groups are selected, MFA is enabled for all users AC: MFA workflow is enabled for Members who belong to at least one of the selected groups AC: MFA workflow is bypassed if the Member does not belong to any of the selected groups.

~Further discussion is required around the expected behaviour if a user was part of a selected group and has set up MFA but is no longer a member of any of the selected groups.~ It has been agreed that if a user has already registered for MFA, they should continue logging in with MFA even if the rules change to only enable MFA for a group they are not a part of. This would cover a situation where a users access is downgraded or an entire group is removed from the list of MFA enabled groups.

brynwhyman commented 3 years ago

Raised a little PR to fix the failing build: https://github.com/silverstripe/silverstripe-mfa/pull/410

brynwhyman commented 3 years ago

Further discussion is required around the expected behaviour if a user was part of a selected group and has set up MFA but is no longer a member of any of the selected groups.

Good point. There are scenarios where we prioritise a user's preference for MFA over global settings, an example being: having the minimum setting as being 'Optional' and allowing users to register before it's something that's enforced, letting them prioritise their user security before an administrator enforces it.

IMO the same could be said here - have the user keep the MFA credentials after they are moved from an assigned group. Assuming the user still has some form of access to their CMS profile section (or another custom area to manage their MFA), it would still be possible to manually remove their registered MFA devices as required.

ScopeyNZ commented 3 years ago

IMO the same could be said here - have the user keep the MFA credentials after they are moved from an assigned group.

Chiming in from the decisions we made building this; I completely agree with Bryn here. We allow admins to make suggestions/enforcements around requiring MFA, but whether a user has the autonomy to set up their own MFA, and keep using it once they've set it up ideally should not be affected by administrators - that's sort of inherently insecure.

MFA should always show if you've set it up. I think there is an edge case that we begrudgingly put in this module to globally disable - although we suggested the solution for that was to just uninstall the module IIRC.

dnsl48 commented 3 years ago

That's a really good PR, thanks for contributing that @scott-nz!

Plus one to what @ScopeyNZ said. Apart from that I'm posting the merge checklist below. The main couple of things from my POV would be adding some user-facing docs about the new feature and fixing the borked Behat test. I'll do some debugging and have a look at why it's borked some time this week. If you don't have time, I can put together some docs too. Optional, but if we need to do more commits, we ideally can squash that into a single one and give it prefix ENH (more about prefixes in the docs).

Here's the merge checklist before we merge:

[x] The target branch is correct
[x] All commits are relevant to the change (e.g. no debug statements or arbitrary linting)
[ ] The commit messages follow the contribution guidelines
[x] The patch follows the contribution guidelines
[x] New features are covered with tests (back-end with unit tests, front-end with Behat)
[ ] Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
[x] CI is green
[ ] At least one peer reviewer approved; no outstanding changes requested

dnsl48 commented 3 years ago

Just had a look at what's going on with the broken Behat test, and it appears to be something unrelated to this PR.

Scenario: I can set MFA to be required                               # tests/Behat/features/mfa-enabled.feature:11
    Given I go to "/admin/settings"                                    # SilverStripe\Framework\Tests\Behaviour\FeatureContext::visit()
    And I click the "Access" CMS tab                                   # SilverStripe\Framework\Tests\Behaviour\CmsUiContext::iClickTheCmsTab()
    Then I should see "Multi-factor authentication (MFA)"              # SilverStripe\Framework\Tests\Behaviour\FeatureContext::assertPageContainsText()
    When I select "MFA is required for everyone" from the MFA settings # SilverStripe\MFA\Tests\Behat\Context\LoginContext::iSelectFromTheMfaSettings()
    And I press "Save"                                                 # SilverStripe\Framework\Tests\Behaviour\FeatureContext::pressButton()
Saving screenshot into /silverstripe-mfa/artifacts/screenshots/mfa-enabled.feature_17.png
    Then I should see "Saved"                                          # SilverStripe\Framework\Tests\Behaviour\FeatureContext::assertPageContainsText()
      The text "Saved" was not found anywhere in the text of the current page. (Behat\Mink\Exception\ResponseTextException)

The error message is:

The text "Saved" was not found anywhere in the text of the current page. (Behat\Mink\Exception\ResponseTextException)

mfa-enabled feature_17

ScopeyNZ commented 3 years ago

Is it related to Maxime's work to Reactify the toast notifications? I think things like this should be fixed here. If it is related, maybe ask Max for a solution?

dnsl48 commented 3 years ago

cc @maxime-rainville

scott-nz commented 3 years ago

The Save button functionality will be taking slightly longer now as there is now a many many relation that needs processing. This will be resulting in it taking longer for the notification to appear. I never like doing it, but i suspect that just adding in a wait of 1 second would resolve the issue.

dnsl48 commented 3 years ago

i suspect that just adding in a wait of 1 second would resolve the issue

~I'm not sure that would be the case. Specifically for that we wait for every backend request to finish before proceeding to a next step. Also, the "Saved" popup is clearly seen on the screenshot. My guess is that we may have reimplemented that popup in react or something like that~

Ok, I've just tested it and "wait" for 2 seconds fixed the issue on my localhost. Interesting...

dnsl48 commented 3 years ago

Lol, I think I've figured it out. Indeed, testsession makes sure it doesn't go on to the next step in case there are any pending requests to the server. However, looks like that check happens before the Save button actually triggers the request to the server. At this time it moves on to the next step too quick and before Save can do anything.

ScopeyNZ commented 3 years ago

I believe that you can use Then I wait until I see "Saved" or something.

maxime-rainville commented 3 years ago

Could be a side effect of the toast notification.

I don't think Then I wait until I see is the proper way to test for toast. Use I should see a "Saved" success toast instead

dnsl48 commented 3 years ago

Use I should see a "Saved" success toast instead

That seems to work. I'm not sure it addresses the problem of async button, but I guess if it works, we could just go with it for now.

@scott-nz would you like to update the test? It's here https://github.com/silverstripe/silverstripe-mfa/blob/4/tests/Behat/features/mfa-enabled.feature#L17 We could just swap the line with I should see a "Saved" success toast

scott-nz commented 3 years ago

PR change requests made Tests fixed Made a modification so that if a user has already set up MFA but is no longer in one of the selected groups, MFA will not be bypassed

brynwhyman commented 3 years ago

cc @silverstripeux - a little enhancement that you'll be interested in :)

sachajudd commented 3 years ago

Will check this out when @dnsl48 can demo it for PUX locally.

dnsl48 commented 3 years ago

Some UX feedback from @sachajudd

Currently we have the field implemented like that (the bottom field is the new one from this PR):

Screen Shot 2020-11-09 at 12 45 49

However, it's inconsistent with the other fields we have on the page. E.g. let's consider the field "Who can edit pages on this site?". There are two radios and the field with list of groups shows up only when we choose the second radio. For consistency with the other settings we may need to refactor the UI a bit to follow the same flow. Here's the screenshot:

Screen Shot 2020-11-09 at 12 53 39

The radio options may look like "MFA Required for all groups" and "MFA Required for Only these groups (choose from list)"

scott-nz commented 3 years ago

@sachajudd Would the following be more consistent with the existing fields?

MFA is optional for everyone and MFA is required for everyone changed to MFA is optional and MFA is required

new field positioned after the MFA will be required from (optional) field: Who do these MFA settings apply to with options Everyone and Only these groups selecting Only these groups will reveal the MFA groups field

Do you think that a note should be added stating that changing these settings will have no effect on users who have already successfully set up MFA?

sachajudd commented 3 years ago

Hey @scott-nz, PUX has gone away and had another chat about this. We think it would be better to implement 3 radio options. Here's a quick mock up we are thinking:

Let us know what you think.

Noting here PUX will also raise a separate issue regarding an enhancement for the date field to be required. More information to come about this. Issue: #

scott-nz commented 3 years ago

Hi @sachajudd, the suggested implementation will prevent a client from being able to enable MFA for specific groups but only make it optional, this seems like a feature regression.

sachajudd commented 3 years ago

I've set up a meeting to discuss this early next week together, let's follow up back here once we've talked over the designs 🙂

maxime-rainville commented 3 years ago

I'm thinking maybe the proper solution here would be to provide a sensible default that work for most people and a flexible API along with some solid doc for people who have special requirements.

Otherwise we'll end up with 20 different options.

clarkepaul commented 3 years ago

I've just had a catchup with @scott-nz and seems the provided mockup will work fine (misunderstanding of the mockup but sorted now). Seems the direction will work fine with that layout, interactivity will make it a lot clearer with the fields showing /disappearing when the different options are selected.

Next steps:

Scott will piece it together with the functionality as per mockup.
We will swap the order of the date and group fields so that group comes first.
We will do another pass of the wording as we think it could still be slightly clearer.
We want to make the date field (under the second and third options) required fields, this might be opened as a separate issue—otherwise if the date field is empty it is not actually mandatory to anyone.

brynwhyman commented 3 years ago

It's my understanding that this new functionality of limiting MFA to certain groups has the main intention of supporting this user story:

"As a Site Owner with a website that acts as a customer portal, I want MFA to only apply to the CMS administrators or 'my other favoured member group', so that my customers using the same login form are not also challenged for MFA."

I'm not sure if the suggested designs totally support that? The aim should be, whether MFA is optional or mandatory, the CMS user can choose what member groups see the MFA registration screen after they log in.

clarkepaul commented 3 years ago

@brynwhyman might be easier to chat in person... with the proposed design MFA would be optional for non-specified groups. Are you thinking there should be no option to even use MFA for non-specified groups?

brynwhyman commented 3 years ago

Are you thinking there should be no option to even use MFA for non-specified groups?

Yes, that's right. In line with the AC in the description: "AC: MFA workflow is bypassed if the Member does not belong to any of the selected groups."

ScopeyNZ commented 3 years ago

I hope you're talking about registration and not login here. Otherwise it seems go against the original ethos in this module - setting up MFA can be required or optional, but once it's set up, you should always have to use it to login.

If you are talking about logging in, I think what @clarkepaul is saying is correct.

clarkepaul commented 3 years ago

Just incase it isn't obvious... If we are wanting to distinguish the website login screen verses the admin login screen and control MFA for them independently, then we would need different set of controls than what has been mocked up.

brynwhyman commented 3 years ago

Sorry for the late response here. We've had some conversations outside this issue to discuss a couple different scenarios that we've seen in projects and how they might relate to the original intent of this PR (I worked with @scott-nz to request this from a bespoke project in the first place).

I think discussions in this issue have conflated two different scenarios:

Scenario 1: Only certain user groups can register MFA methods

As a Site Owner of a project with a number of different user groups with different permission levels (perhaps without CMS access), I want the option of registering MFA to only be presented to user groups who I deem access private data, so that all other user groups cannot set up MFA - something which if led to support requests I could not support.

Example: A site has the following groups: 'CMS users', 'Merchants' who update website data objects through a custom front-end form (so have no access to the CMS), and 'Customers' who have a login to review their website purchase history. In this case only the 'CMS users' and 'Merchants' could be given the option to register MFA methods.

Scenario 2: MFA should be mandatory for certain groups but available to all

As a Site Owner, I want the option of MFA being mandatory to only apply to specific CMS user groups, so that groups like Administrators have extra login controls, where groups like Content Editors can secure their accounts by registering MFA methods at their own whim.

How can we solve these scenarios in this module?

Scenario 1: Something like what this PR had originally proposed (and what it looks like this PR still provides), see screenshot and the notable ACs:

AC: MFA workflow is enabled for Members who belong to at least one of the selected groups
AC: MFA workflow is bypassed if the Member does not belong to any of the selected groups.

Scenario 2: New functionality built according to the feedback outlined here: https://github.com/silverstripe/silverstripe-mfa/pull/409#issuecomment-725774762. Probably makes sense as a separate pull request(?)

Do these scenarios warrant new default functionality though?

@clarkepaul has previously raised that CMS user groups are often not well maintained within a project's CMS. If we're not comfortable having all sites rely on user groups as a way to manage MFA then maybe neither scenario should be met with features that are available by default with the MFA module? I don't have data to back this up now but it's believable.

If that's the case, this pull request could be updated to move the functionality behind an API/ feature flag that Developers could enable if required. We could then follow a similar pattern if functionality for Scenario 2 is ever built.

I understand there's been a bit of back and forth here (sorry @scott-nz) so an alternative to investing more time into this pull request could be closing this and instead document the implementation in the module docs for others to follow as required.

TL;DR

I think conversation in this thread has conflated two different scenarios
I think the original intent of this PR should be retained, but there's the question of whether it should be a default feature
- If this should not be a default feature, should this PR be updated to be put behind a feature flag; or should the implementation be covered in module documentation and this PR closed?

ScopeyNZ commented 3 years ago

I think we still need to be clear around the original statement:

Further discussion is required around the expected behaviour if a user was part of a selected group and has set up MFA but is no longer a member of any of the selected groups.

IMO, you still use MFA. Having it removed from your account just feels weird. I guess if you lose access to the admin panel during that process (ie. the site has a custom "manage self" screen) then it makes sense to remove MFA - but I think we added in controls for that?

I think that scenario 2 that you've mentioned @brynwhyman is definitely outside the scope of this issue. At the moment, MFA is available to all, or nobody, as I remember it. I don't think this PR should include changes to that. It should make it mandatory for a subset of users, but should still be available to all. Then you don't have the issue of losing MFA if you're removed from a group, and the logic around whether you use MFA during login is kept simple - as it's "have you set it up".

From reading the code, it appears to satisfy how I believe it should work. You still use MFA if it's set up, otherwise you never get redirected to set it up. Feels like it's just a :shipit: moment.

sunnysideup commented 2 years ago

@here - bump - we are keen to get this across the line. What would that take?

ScopeyNZ commented 2 years ago

Regardless of the discussion above that I'm out of the loop on, I'd be happy enough to merge this as is. Some of the wording could maybe use some tweaking as the options read like "enable MFA for everyone" but then it's immediately followed by a control that lets you choose specific groups - but that's a very minor point.

Summary of the current changes in the PR:

Adds a user group selector below the existing "required/optional" & "grace period" options
Users that do not belong to any of the groups selected will never be redirected to MFA unless they have already configured MFA (and is therefore required for them to log in)

brynwhyman commented 2 years ago

For what it's worth I'm in support of this getting merged too.

My only gripe was updating this PR to implement the later designs (which would have regressed what this PR was originally providing). From what I can see this didn't happen.

In regards to wording improvements. If they were to happen, I think Scott had a good suggestion in this comment which could be followed:

MFA is optional for everyone and MFA is required for everyone changed to MFA is optional and MFA is required

new field positioned after the MFA will be required from (optional) field: Who do these MFA settings apply to with options Everyone and Only these groups selecting Only these groups will reveal the MFA groups field

Do you think that a note should be added stating that changing these settings will have no effect on users who have already successfully set up MFA?

ScopeyNZ commented 2 years ago

Sweet. I'm happy to merge and accept follow-ups, unless someone wants to volunteer to implement the changes 😉

michalkleiner commented 1 year ago

@scott-nz is there any appetite to pick this up again and rebase etc. with the code as is? I'd also be happy to merge it in the last state after all the lengthy discussions — appreciate your patience btw.!

scott-nz commented 1 year ago

@scott-nz is there any appetite to pick this up again and rebase etc. with the code as is? I'd also be happy to merge it in the last state after all the lengthy discussions — appreciate your patience btw.!

@michalkleiner I'm keen to revisit this, I will try and find the time to rebase this soon.

dhensby commented 1 year ago

@michalkleiner contributors can (usually) rebase and push PRs if that's something holding up merging.

In this case, there are no conflicts so I'm not sure a rebase is required? What are you hoping to achieve?

dhensby commented 1 year ago

@michalkleiner rebased for you

michalkleiner commented 1 year ago

@dhensby the CI config, for example, was still using Travis and therefore the tests were broken. Having the PR created off a recent branch/rebased helped with that. That was the main reason, secondary reason was that the PR itself was over a year old so just to make sure it still works as expected/is up to date, in general, as a good practice.

Thanks for the rebase, will try it myself first next time.

vinstah commented 12 months ago

Hi, it would be good to know status of this PR.

thanks

GuySartorelli commented 11 months ago

@vinstah The status of this PR is that it is stale, and nobody has reviewed it for almost a year, unfortunately. It's on my radar, but there's a lot of PRs to review and only so much review time available.

GuySartorelli commented 11 months ago

I've changed the target branch to 5, as this is the branch for new features now. Also rebased on the 5 branch to double check that CI is still happy, since it's been so long.

GuySartorelli commented 11 months ago

I would have preferred that when MFA is "required" for specific groups, it would still be optional for everyone else so that they can opt-in to securing their account - this is what was suggested in this comment. That said, it seems like the consensus is that the behaviour the PR currently provides is the preferred behaviour, so I will defer to that decision.

There is still some outstanding action required as mentioned in this comment - I will action these changes so that the PR is fully ready for review, and then add it to our peer review column to be reviewed by another member of the team.

GuySartorelli commented 11 months ago

I was having a problem where if I tried to sign in as a user who is not in the assigned group, I could not sign in at all. It would just keep redirecting me to the login form saying I must be logged in, and to enter my credentials. This is because EnforcementManager::isMFARequired() doesn't (and can't because it would require changing the method signature) take the new groups config into account.

I've made a change that fixes this specific scenario by amending the logic in EnforcementManager::canSkipMFA() to account for this scenario - but it's worth noting that anyone using any custom logic in their projects or modules which relies on EnforcementManager::isMFARequired() will not be compatible with this change.

I think that because of this, this change should be done in a new major release, where EnforcementManager::isMFARequired() takes a member argument, and does the group check there. That said, I have added a note about this to the PHPDoc for the method. We could merge this as is for the next minor release and accept that some projects may need to adjust their logic. I'll also create a PR for the changelog calling this change out.

I'm just fleshing out the test coverage a bit more so I've set it to draft until I've finished with that.

GuySartorelli commented 11 months ago

In addition to the above, I have:

Replaced ListboxField with TreeMultiselectField since groups are hierarchical, and this matches other similar fields in the CMS
Made the new group field hidden until we set the rule to "Only these groups"
Updated unit tests to cover pretty much all scenarios that would be affected by this PR
- Existing unit tests were refactored so there's a lot less repetition - should be clearer what the expected outcome is for any given scenario now
Updated behat tests to include playing with the new fields in a variety of combinations

GuySartorelli commented 11 months ago

@scott-nz I hope you don't mind me taking over the PR by the way - if you'd like to continue working on it I'll be happy to let you do so, I just figured this was the quickest way to get this over the line so it can finally get merged.

scott-nz commented 11 months ago

@GuySartorelli Happy with you taking over. It's good to see it progressing, I don't have the capacity / availability to get finish this off at the moment.

vinstah commented 11 months ago

@GuySartorelli thanks for the updates I was thinking that this PR may have been superseded by CMS 5 enhancement around user access.

One thing that might be OK to include is extension points so new logic can be added when developers extend this functionality

GuySartorelli commented 11 months ago

I was thinking that this PR may have been superseded by CMS 5 enhancement around user access.

User access is very different from who can register MFA for their account, so it's still a valuable enhancement.

One thing that might be OK to include is extension points so new logic can be added when developers extend this functionality

Extension points sound like a great enhancement for a separate PR. :p I'm trying to get the existing feature enhancement over the line, so any added scope beyond what was already requested in existing comments in this PR is explicitly out of scope.

emteknetnz commented 11 months ago

@GuySartorelli Merge conflict

silverstripe / silverstripe-mfa