search

silverstripe / silverstripe-mfa

MultiFactor Authentication for Silverstripe CMS
BSD 3-Clause "New" or "Revised" License
11 stars 25 forks source link

FIX Check whether user is eligible for MFA in canSkipMFA method #379

Closed Cheddam closed 4 years ago

Cheddam commented 4 years ago

Users with no CMS access are not eligible to use MFA by default. This was respected in the shouldRedirectToMFA method, but not in canSkipMFA, which resulted in these users being booted back to the login screen whenever they attempted to log in.

This commit also tidies some of the docblocks in EnforcementManager, and adjusts the hasAdminAccess method to always act as the provided Member.

Will resolve #376 for the 4.0 branch; requires backporting to 3.0 alongside #369.

Cheddam commented 4 years ago

Pulled this one out of the oven a little early 😅

Working on expanding the unit test coverage for canSkipMFA to include more states.

Cheddam commented 4 years ago

Tests are green, and I've successfully manually tested the fix. The change is not hitting the coverage goal for the diff, despite bumping it for the overall project. I've added additional test coverage to get across as many code paths as possible, but...

Coverage has been 'lost' on the loop that checks LeftAndMain menu items for access. I'm actually fairly confident this code never runs - LeftAndMain::MainMenu() calls CMSMenu::get_viewable_menu_items(), which has its own internal canView checks and seems to return an empty ArrayList for members with no admin access. So.. we might be able to drop the loop and just check if it's empty? cc/ @robbieaverill

Basically, the PR is green, I've bumped overall test coverage slightly in the process, and codecov is being unreasonable. Ready for a peer review! 👍