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

[12.x] Model::hashUsing() #52324

Closed
wants to merge 2 commits into from
Closed

Conversation

gdebrauwer
Copy link
Contributor

@gdebrauwer gdebrauwer commented Jul 30, 2024

The Eloquent model class has an encryptUsing() method that allows you to customize the encrypter used by the encrypted cast (#35080). This PR adds a similar method called hashUsing that allows you to customize the hasher used by the hashed cast.

I targeted the master branch because I had to update the Hasher interface. I needed to add the isHashed method as that method currently only existed on the HashManager class. I did not add the verifyConfiguration method to the interface because that method is marked as @internal on the existing hashers so I thought that may be an optional method on a hasher class.

@driesvints
Copy link
Member

cc @valorin

@valorin
Copy link
Contributor

valorin commented Jul 31, 2024

Oh nice, I like this idea. Adds a bit of flexibility while keeping it consistent with the encrypter. 👍

One concern though, how would this interact with automatic rehashing on login?

@gdebrauwer
Copy link
Contributor Author

gdebrauwer commented Aug 1, 2024

@valorin I updated the EloquentUserProvider class to use the hasher set on the Eloquent model. That way, the correct hasher is used in the rehashPasswordIfRequired method. That method is used by the 'automatic rehashing on login' feature.

@taylorotwell
Copy link
Member

I feel like this would be really rare to need to customize? What's the use case?

@taylorotwell taylorotwell marked this pull request as draft August 1, 2024 14:52
@gdebrauwer
Copy link
Contributor Author

When I added the "hashed" cast, I saw that the "encryptUsing" method existed, so I thought that it might make sense to have the same method for setting the hasher, just for the sake of consistency. It will probably be used a lot less then the method for the encrypter I assume.

@driesvints driesvints marked this pull request as ready for review August 2, 2024 08:41
@driesvints
Copy link
Member

@gdebrauwer always mark a PR as ready when you need an answer.

@taylorotwell
Copy link
Member

I don't think it makes as much sense as encryptUsing - which is used if you need to encrypt a specific models records using a different key that is local to them. Hashing doesn't use keys so wouldn't need such customization.

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.

4 participants