[FIX] CMSSecurity doesn't have Authenticators assigned. #7007
Conversation
|
Agreed, good point, didn't think of it that way, gimme a few :) |
Firesphere
force-pushed
the
pulls/getApplicableAuthenticators
branch
from
831620a
to
3fe837d
Compare
|
Travis is green again, so, here ya go. This should be more robust, more logical with each Controller having it's own Authenticator(s) assigned where possible. |
| @@ -31,5 +31,8 @@ SilverStripe\Core\Injector\Injector: | |||
| properties: | |||
| Authenticators: | |||
| default: %$SilverStripe\Security\MemberAuthenticator\MemberAuthenticator | |||
| SilverStripe\Security\CMSSecurity: | |||
There was a problem hiding this comment.
Do we want to go ahead and just have one-authenticator per Security class?
There was a problem hiding this comment.
For the defaults, I think so. Because we don't want the standard Security to have the CMS login option, as it is unneeded in this case.
We can have multiple per Security-class easily, I can confirm this works (though my tests are failing because it's actual two step now, instead of "try in a single step")
| { | ||
| // Disable shortcut | ||
| if (!static::config()->get('reauth_enabled')) { | ||
| return false; | ||
| } | ||
|
|
||
| return count(Security::singleton()->getApplicableAuthenticators(Authenticator::CMS_LOGIN)) > 0; | ||
| return count($this->getApplicableAuthenticators(Authenticator::CMS_LOGIN)) > 0; |
There was a problem hiding this comment.
Might need to talk about this on Monday, but I think the CMS_LOGIN key is probably redundant. We probably over-engineered the authenticator assignment system in the initial implementation.
|
Will merge for now since it fixes the bug. We can discuss API later. :) |
Switching to Security::singleton()->getApplicableAuthenticators() and Security::singleton()->getAuthenticators() fixes this and makes it more robust.