Skip to content

[1.x] Retrieve user through provider #180

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

Closed
wants to merge 1 commit into from
Closed

Conversation

driesvints
Copy link
Member

This is a partially fix for #171. Instead of relying on Eloquent to retrieve the user we make use of the user provider from the stateful guard and thus decouple the Two Factor implementation from Eloquent.

@LeoniePhiline
Copy link

Hi @driesvints

... and thus decouple the Two Factor implementation from Eloquent.

The handling of recovery keys is still tightly coupled to Eloquent ($user->forceFill([...])->save()):

Maybe that's entirely okay (I have never not used Eloquent) - only the claim that 2fa handling was now User-Provider-agnostic appears false to me.

I'm unsure how this could be solved the most easily, since the Eloquent methods (forceFill/save) used on the user model are not defined in any interface.

The above-listed Actions would need a replaceable EloquentAdapter implementing a fill/fill-and-save interface. Any other ideas?

Leonie

@driesvints
Copy link
Member Author

@LeoniePhiline I'm with vacation as of just now. I'll try to look into this somewhere next week.

@LeoniePhiline
Copy link

@driesvints It's not urgent at all. Have a wonderful time! 🎄❄️✨

@taylorotwell
Copy link
Member

Personally I don't have any problem with this package requiring Eloquent. I don't really have the desire at this point to make it support anything else.

@taylorotwell taylorotwell deleted the update-user-provider branch December 30, 2020 16:11
@LeoniePhiline
Copy link

@taylorotwell

Clarification, as the discussion might have been misleading:

This PR was only marginally about independence from Eloquent.

It was about leaving credentials work up to the Provider instead of assuming (hard-coding) a specific way to retrieve users and verify credentials and duplicating this functionality's implementation.

The current implementation

  1. duplicates this already existing functionality and
  2. clashes with the ability to use of custom user providers.

Even if you don't care about 2.), you might still want to merge this PR to have 1.) fixed, to at least not re-implement the provider's functionality.

Developers might use a custom provider but still use Eloquent.

@driesvints driesvints restored the update-user-provider branch January 4, 2021 14:13
@driesvints
Copy link
Member Author

@LeoniePhiline I've resent it here: #189

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.

3 participants