LonelyVikingMichael / litestar-users

Authentication and user management for the Litestar framework
https://lonelyvikingmichael.github.io/litestar-users/
MIT License
42 stars 12 forks source link

Add option to select PasswordManager's crypt scheme #27

Closed dialvarezs closed 1 year ago

dialvarezs commented 1 year ago

It would be useful to select the crypt scheme without redeclaring or extendind the PasswordManager class. I think bcrypt is a good default, but Argon2 is supposed to be a more solid option (and I prefer it tbh).

This PR adds and argument to the init of that class, and also adds argon2 as extra, to install the argon2 implementation.

LonelyVikingMichael commented 1 year ago

This is valid, but the implementation needs some tweaking.

I think what's misleading here is that an instance of PasswordManager is created explicitly in the examples - but that is only because we're seeding the initial user directly into the db via Starlite's on_startup hook. This PR would allow you to customize such an instance, however - starlite_users.service.BaseUserService creates its own PasswordManager instance and doesn't cater to passing params at present.

Basically, your seeded user's password will be hashed with argon (or whatever scheme you decide to pass in) while users created via your BaseUserService subclass would still be hashed via bcrypt (assuming you're following the provided examples). Perhaps this is poor design on my part.

I think StarliteUsersConfig should accept a hash_scheme param which we pass down to the user service. Additionally, it might be better to create a public hash method on BaseUserService itself to abstract PasswordManager entirely. This should ensure that the same CryptContext instance is always used to hash passwords.

dialvarezs commented 1 year ago

Hi, sorry for the long time without updates for this.

I think StarliteUsersConfig should accept a hash_scheme param which we pass down to the user service

You mean viaget_service_dependency(), right? That should require to schemes in the service object, unless we pass the schemes to the methods literally (way less cleaner if you ask me). But, if we need to define the UserService class anyways, why not adding the hash scheme there directly and use it for the password manager attribute?

Additionally, it might be better to create a public hash method on BaseUserService itself to abstract PasswordManager entirely

So, the user won't have to create a PasswordManager instance to create initial users, right? I'm thinking in something like this:

@classmethod
def hash_password(cls, password: SecretStr):
    password_manager = PasswordManager(hash_schemes=cls.hash_schemes)
    return password_manager.get_hash(password.get_secret_value())

the user could use

UserService.hash_password(SecretStr("iamsuperadmin"))

Is something like this what you have in mind?

LonelyVikingMichael commented 1 year ago

Sorry for having kept this up as long as I did.

32 changes things up a bit - there's no longer any need to configure BaseUserService with class variables anymore, everything will be passed down from StarliteUsersConfig.

I've decided not to go with a hash method on the user service, as it defeats the purpose of the password manager in the first place and instantiating it manually for the 1st user's creation is a once-off deal (if required at all).

I need to make a few more small tweaks before wrapping this up in a release, thanks for your input.