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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

NEW Only get an authenticator if it's an object #8961

Merged
merged 1 commit into from
May 6, 2019
Merged

NEW Only get an authenticator if it's an object #8961

merged 1 commit into from
May 6, 2019

Conversation

indygriffiths
Copy link
Contributor

Use-case: if a module is defining its own authenticator and you want to disable it, as it seems we don't have Authenticator::unregister_authenticator() anymore in SS4 and I can't spot how to remove YAML-based injected properties, then this lets you mark it as null or false to stop it from being used. Without this code fix it errors out when it attempts to call supportedServices()

Example YAML config

---
Name: removemoduleauthenticator
After:
  - 'moduleauthenticator'
---
SilverStripe\Core\Injector\Injector:
  SilverStripe\Security\Security:
    properties:
      Authenticators:
        SomeAuthenticatorProvidedByAModule: false

If you can un-define a property in YAML, I humbly withdraw this PR if it's not needed 馃槃

@robbieaverill
Copy link
Contributor

So the config layer lets you unset YAML values like this already, but I think because this is specifically pushing values into an injected property that it doesn't filter it out. I originally thought that we could filter falsy values out in Injector itself, but that would be wrong because you might actually want to have them.

One suggestion I might have is to use array_filter in getAuthenticators(), use that in hasAuthenticator() instead of the prop directly, then you could probably get away with a one line change instead

@kinglozzer
Copy link
Member

Yeah I鈥檝e come across this issue before when trying to replace the core authenticator instead of adding a second one. We worked around it with YAML: https://github.com/bigfork/silverstripe-oauth-login#replacing-the-default-authenticator

Adding array_filter to either getAuthenticators()/setAuthenticators() sounds good to me

Use-case: if a module is defining its own authenticator and you want to disable it, as it seems we don't have `unregister_authenticator()` anymore and I can't spot how to remove YAML-based injected properties, then this lets you mark it as null or false to prevent it from erroring out when it attempts to call `supportedServices()`
@ScopeyNZ ScopeyNZ merged commit 371588e into silverstripe:4 May 6, 2019
Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

Retrospective approval - nice :)

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

Successfully merging this pull request may close these issues.

None yet

4 participants