Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add functionality to limit MFA to specific user groups #409

Draft
wants to merge 2 commits into
base: 5
Choose a base branch
from

Conversation

scott-nz
Copy link

@scott-nz scott-nz commented Oct 29, 2020

#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
Copy link

Raised a little PR to fix the failing build: #410

@brynwhyman
Copy link

brynwhyman commented Oct 30, 2020

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.

Copy link
Contributor

@ScopeyNZ ScopeyNZ left a comment

Choose a reason for hiding this comment

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

Looking good. Can you rebase please? Thanks for writing tests and well done choosing a great name with your mock data 😊

src/Extension/SiteConfigExtension.php Outdated Show resolved Hide resolved
src/Extension/SiteConfigExtension.php Outdated Show resolved Hide resolved
src/Service/EnforcementManager.php Outdated Show resolved Hide resolved
@ScopeyNZ
Copy link
Contributor

ScopeyNZ commented Oct 30, 2020

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
Copy link
Contributor

dnsl48 commented Nov 1, 2020

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:

  • The target branch is correct
  • All commits are relevant to the change (e.g. no debug statements or arbitrary linting)
  • The commit messages follow the contribution guidelines
  • The patch follows the contribution guidelines
  • 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
  • CI is green
  • At least one peer reviewer approved; no outstanding changes requested

@dnsl48
Copy link
Contributor

dnsl48 commented Nov 3, 2020

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
Copy link
Contributor

ScopeyNZ commented Nov 3, 2020

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
Copy link
Contributor

dnsl48 commented Nov 3, 2020

cc @maxime-rainville

@scott-nz
Copy link
Author

scott-nz commented Nov 3, 2020

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
Copy link
Contributor

dnsl48 commented Nov 3, 2020

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
Copy link
Contributor

dnsl48 commented Nov 3, 2020

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
Copy link
Contributor

ScopeyNZ commented Nov 3, 2020

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

@maxime-rainville
Copy link

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
Copy link
Contributor

dnsl48 commented Nov 4, 2020

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 scott-nz force-pushed the feature/restrict-to-group branch 3 times, most recently from aec7e0d to 5671026 Compare November 4, 2020 20:56
@scott-nz
Copy link
Author

scott-nz commented Nov 4, 2020

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
Copy link

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

@sachajudd
Copy link
Contributor

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

@dnsl48
Copy link
Contributor

dnsl48 commented Nov 9, 2020

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
Copy link
Author

scott-nz commented Nov 9, 2020

@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
Copy link
Contributor

sachajudd commented Nov 12, 2020

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:
image

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
Copy link
Author

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
Copy link
Contributor

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 🙂

@GuySartorelli
Copy link
Member

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
Copy link
Member

GuySartorelli commented Oct 2, 2023

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
Copy link
Member

GuySartorelli commented Oct 2, 2023

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
Copy link
Member

GuySartorelli commented Oct 2, 2023

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
Copy link
Member

GuySartorelli commented Oct 2, 2023

@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
Copy link
Author

scott-nz commented Oct 2, 2023

@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
Copy link

vinstah commented Oct 2, 2023

@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
Copy link
Member

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.

@GuySartorelli GuySartorelli force-pushed the feature/restrict-to-group branch 2 times, most recently from 4d7c5a2 to 73531c0 Compare October 2, 2023 22:35
@GuySartorelli GuySartorelli marked this pull request as ready for review October 2, 2023 22:35
@emteknetnz
Copy link
Member

@GuySartorelli Merge conflict

scott-nz and others added 2 commits October 4, 2023 10:25
If a user has already registered for MFA, enforce use of it even if they are not in an MFA group

Co-authored-by: Guy Marriott <guy.the.person@gmail.com>
@GuySartorelli
Copy link
Member

Conflict resolved

@emteknetnz
Copy link
Member

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

@GuySartorelli Could you confirm what the user experience is for people not in a required group?

  • If people are not in the required group are still prompted to sign up for MFA the first time they log in?
  • If not, are people not in the required group still able to sign up for MFA by editing their profile?

@GuySartorelli
Copy link
Member

GuySartorelli commented Oct 3, 2023

Could you confirm what the user experience is for people not in a required group?

  • If people are not in the required group are still prompted to sign up for MFA the first time they log in?

If a group (or groups) is selected and the user is not in that group, the user will not be prompted to use MFA at all unless they had already previously registered an MFA method for their account.
Users who have registered MFA for their account will always be prompted to use it.
The unit tests reflect these scenarios.

  • If not, are people not in the required group still able to sign up for MFA by editing their profile?

No, I don't think so.

This is as discussed in the comments above - there was a lot of discussion about how this should behave, and the consensus ended up being that if a group is selected, only members in that group should have any interaction with MFA (unless they already registered an MFA method for their account).

@emteknetnz
Copy link
Member

Sorry slightly I meant a slightly different thing (although may this is how you interpreted it) - the FIRST time a new user logs in BEFORE they had a chance to sign up for MFA they get this prompt:

image

I suppose if you're saying that they CANNOT signup for MFA in their profile then logically they won't get this prompt

I'm guess I'm OK with them not being prompted to sign-up for MFA (though I'd still prefer it) though I really dislike them NOT being able to enable MFA via their profile at all - you should at least have the option to secure your own account. If I understand this correctly and this is how this work in this PR, given the discussion/consensus was 3 years ago I think at very least we should pop this one in the backlog for refinement and have a discussion to confirm this is the behavior we want

@GuySartorelli
Copy link
Member

I suppose if you're saying that they CANNOT signup for MFA in their profile then logically they won't get this prompt

That's correct, yes.

I'm guess I'm OK with them not being prompted to sign-up for MFA (though I'd still prefer it) though I really dislike them NOT being able to enable MFA via their profile at all - you should at least have the option to secure your own account.

I'm working based off the decisions and consensus that was made on this issue already, which is that if a group is added, members who are not in that group cannot sign up for MFA at all.

If they can sign up by modifying their profile, then I am strongly of the opinion that they should be given the (optional, if they're not in the group) prompt to register MFA when they log in. And in that case, we go back to the design that was suggested in #409 (comment) and which was later rejected.

If I understand this correctly and this is how this work in this PR, given the discussion/consensus was 3 years ago I think at very least we should pop this one in the backlog for refinement and have a discussion to confirm this is the behavior we want

I guess so. I'll put it back in the backlog. I was kinda hoping to get this one over the line but sounds like people will have to wait a while yet for this functionality to be added in any capacity.

@GuySartorelli
Copy link
Member

I'm marking this as DRAFT in the meantime, pending yet another discussion about how this should behave.

@GuySartorelli GuySartorelli marked this pull request as draft October 3, 2023 22:20
@ScopeyNZ
Copy link
Contributor

ScopeyNZ commented Oct 4, 2023

the consensus ended up being that if a group is selected, only members in that group should have any interaction with MFA (unless they already registered an MFA method for their account).

FWIW I felt like the consensus is that individuals should not have the ability to register MFA removed. This PR should only affect who is required to have MFA on their account, and not affect who can even set it up at all.

To me, anything but this make very little sense to me as the default behaviour of this module. If you want to have MFA, but not make it available to some users, you can extend the module for your weird business logic yourself.

However, I don't agree that we should force the "do you want MFA" prompt on users as default behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.