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

Delegate user credential checking to PasswordChecker #584

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

natelenart
Copy link

Storage component expects user/password to come from the database. I have a situation in which I would like to forward the credentials along to an external processor (LDAP, in my case). My first thought was to use a custom encoder with an LDAP service embedded, but that won't work because the encoder factory generates the encoder. Second approach is to extend the storage class or delegate to it. In either case, this leads to a lot of boilerplate (either messy constructor in the first case, or a lot of unnecessarily overridden methods to fulfill the interfaces in the latter), when I really only needed one line changed.

This approach allows the storage service to delegate the actual password checking, which defaults to using the encoder as before (no BC break). With this change, it's easy to override the password checker to implement whatever functionality is needed to check the password. I also made the password checker not nullable and swapped the order of the arguments to keep the nullable argument at the end. It didn't make sense to me that prior logic had the encoder factory as nullable, but called getEncoder on it directly without checking for null anyway.

This PR represents the simplest approach I could think of that provides extensibility without a BC break. Please let me know if there's a more Symfony-esque way of accomplishing this - I'm open to placing the functionality somewhere else, renaming files/methods, etc.

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.

1 participant