Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Make AuthUser trait more flexible #51

Closed
teenjuna opened this issue Feb 2, 2023 · 1 comment
Closed

Make AuthUser trait more flexible #51

teenjuna opened this issue Feb 2, 2023 · 1 comment

Comments

@teenjuna
Copy link

teenjuna commented Feb 2, 2023

Hi! I was evaluating this crate for use in my project and found it a little bit opinionated. I will describe my issue and would love to hear your feedback!

AuthUser trait is synchronous

In my project, User struct has an id, role and optional foreign key profile_id. There are 3 roles: guest, normal and admin. If user is a guest, then there's no related profile and profile_id is set to None. Profile struct has an email (which is the primary key), password_hash and some other data.

So, now I'm implementing the AuthUser trait. Getting a role is easy. But getting password_hash is not: for a guest user I can just return an empty Vec, but for normal and admin user I need to fetch related Profile, which would be an async operation inside a sync function. Surely, I can try to implement UserStore and fetch the profile eagerly. But this will complicate my type signature. Or I can move email and password_hash from Profile to User, but it won't make sense for a guest user.

Making the trait async would be a far better option. Surely, it is a breaking change, but the migration is very straightforward: just add #[async_trait] to the trait impl and async modifier before methods.

AuthUser expects password hash

But what if I want to use a passwordless authentication? For example, a magic-link. In this case, for every signed user I would store some kind of token, and use it instead of the password hash. Maybe this particular example is not ideal, but you probably get the idea.

How to deal with it? Just rename get_password_hash to get_secret and specify password hash as an example for such a secret in the docs.

@maxcountryman
Copy link
Owner

AuthUser trait is synchronous

The thinking here is that you've already gone through whatever process is required to retrieve your user. (That might be async or it might not be, but we don't have an opinion about that only that the user type implement this trait.)

More specifically, as you note, these are meant to be fields on the user type (or whatever type you're implementing the trait for).

There's a non-trivial cost to making this trait async. It might be worth it, but I'd like to make sure there's a good case for it.

But what if I want to use a passwordless authentication?

The method can return anything you like. We intentionally provide full flexibility here anticipating exactly this use case.

I think get_secret could be confusing in its own way, since within the crate "secret" is already used to mean something else.

I'm not opposed to renaming it, but it's a breaking change. It's also worth pointing out this could be addressed with better documentation.

Repository owner locked and limited conversation to collaborators Feb 2, 2023
@maxcountryman maxcountryman converted this issue into discussion #52 Feb 2, 2023

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants