-
Notifications
You must be signed in to change notification settings - Fork 7
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
NEW Log out all devices for members with no admin access #56
NEW Log out all devices for members with no admin access #56
Conversation
d13d81d
to
8d88367
Compare
* @param Member $member | ||
* @return bool | ||
*/ | ||
private function hasAdminAccess(Member $member): bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function was copy pasted from https://github.com/silverstripe/silverstripe-mfa/blob/4/src/Service/EnforcementManager.php#L221
It's a little odd, though it works
It's a private
function so easy to remove in the future if we add a function like this to core
8d88367
to
07a90af
Compare
07a90af
to
1535bc2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me that we should cater to users who do not have access to the CMS.
Most sites who allow users to register have very specific workflows in mind and will spent quite a bit of time thinking those through. If devs want to give allow users to log out from all devices, they can implement that feature in their front end.
In any case, this seems to be a completely different problem than what was initially raised in #46.
Yeah agree it's pretty different and since it's adding new functionality so should probably split Makes more sense to have team discussion on the parent issue rather than this pull-request - would you be able to move your comments to there? |
I've split the parent issue to a new issue #58 |
Closing this PR for now as it's very unlikely to be merged. It can be reopened if there's a case for including it. |
Issue #58