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

[Feature] Usage for isolated services (no user DB access) #277

Closed
scaytrase opened this issue Dec 1, 2016 · 17 comments
Closed

[Feature] Usage for isolated services (no user DB access) #277

scaytrase opened this issue Dec 1, 2016 · 17 comments
Labels

Comments

@scaytrase
Copy link

Is possible to use this bundle as an authentication system for isolated services, which do not have the access to the user DB to reload the user data.

We can sign all the needed data (username, roles, som other payload) to the JWT, so on the isolited side we only need a public key to check the signature and once it is OK we can use the token as a trusted store of user information for UserInteface

As far as I can understand, currently the bundle uses the UserProvider via Guard and there is no documented way to bypass it?

@chalasr
Copy link
Collaborator

chalasr commented Dec 1, 2016

Hi @scaytrase,

Fortunately, the fact we use Guard should help here, compared to our v1 were we used a classic listener. The Guard authenticator which makes use of the configured user provider is extensible.

What I don't get here is that for being authenticated, you need an implementation of UserInterface to pass to the authenticated security token. Would you rebuild this object manually from the token informations?

Anyway, if I understand your need correctly, here is the way I would do proceed:

Note that this line must appear in your getUser() to get the authenticator working.

Another alternative could be to just write a custom user provider, declare it in configuration and specify it in the firewall mapping. The provider loadUserByUsername() would just return a new instance of UserInterface

I have to mention that in an authentication context, we encourage to favor hitting a datastore rather than the storing sensible data in the jwt, such as user roles which can be changed at any time, the jwt would keep the old roles until expiration thus potentially give access to resources that it shouldn't.

Please keep us informed if you think such should be easier to achieve or if you're fine with the proposed alternatives.

@scaytrase
Copy link
Author

scaytrase commented Dec 1, 2016

As far as I see the implementation of UserProviderInterface is not an alternative, but a mandatory, as soon as security component requires it to be configured for the firewall.

As for hitting the datastore, the my understanding exactly the same, that mentioned on the official jwt.io website:

Self-contained: The payload contains all the required information about the user, avoiding the need to query the database more than once.

So the only time we need to hit a DB is a generation moment. All other times we can use the "cached" information. Yes, as the other caches we need to understand the ways of invalidation of data, but this is another side of question.

If we hit the Auth DB each time we get a request - there is really no need with such complex things as JWT as soon as we can regenerate the whole JWT content from the DB hits. And the token itself can be replaced with any kind of bearer token (i.e. signed timestamp)

@scaytrase
Copy link
Author

So the general idea, as a feature request, is to do the following:

  • Define a kind of MutableRoleUserInterface with setRole(array $roles) method
  • Define a abstract stub for FactoryUserProvider (or just document it), returning the instance of interface above
  • Define the getUserRolesField manager method (like identity one), allowing to extract the role and put it into the user instance
  • Create the authenticator you described above, which puts the roles from the token into the user instance

As a bonus, allowing bootstrap with just configuration of bundle

  • put a stub final class just with username and roles properties, implementing needed interfaces

The everything else looks like already working, like having only pubkey configured

@chalasr
Copy link
Collaborator

chalasr commented Dec 1, 2016

Yeah, it's required by the symfony security component to have a configured user provider for securing resources, and this bundle uses the security component.
It's mandatory to have a UserInterface object for being able to create a security token (TokenInterface object) for authenticating the corresponding user during the Request, hence the advices given previously.

About the jwt.io statement, sure, we kind of lose the federation benefit since we load the user each time the token is used. That's a security net, preventing some weird behaviors such as the one of the ROLE_ADMIN granted user which is ungranted later and just unexpectedly keeps the same privileges as before.
Though, I tend to agree with you that we could allow bypassing this net without having to do weird things such as configuring an useless user-provider. I'll look at it, the first thing coming toi me is to provide an UserProvider out of the box, which doesn't load any user but create a new one from the token informations.

@chalasr
Copy link
Collaborator

chalasr commented Dec 1, 2016

Thank you for the hints. I'm more thinking about something like:

  • Provides an UserInterface implementation (e.g. JWTUser) with just the minimal fields (i.e. username, roles)
  • Add a JWTUser::createFromPayload($username, $payload): JWTUser method allowing to extend the JWTUser class and set fields such as roles as needed, retrieved from the payload.
  • Provides an UserProviderInterface implementation as you mentioned, but something like JWTUserProviderInterface, its constructor would take the user class so you can pass it when configuring the provider in your security.yml
  • Add a JWTUserProvider::createUserFromPayload($username, payload): JWTUser method takes the JWT payload and return something like $this->userClass::createFromPayload($username, $payload): JWTUser
  • Last, add a check in JWTTokenAuthenticator::getUser() such as $user = $provider instanceof JWTUserProvider ? $provider->createUserFromPayload($username, $payload) : $provider->loadUserByUsername($username);

So you would just have to make your User class extend the JWTUser class and configure the provider like:

security:
    providers:
        jwt_user_provider:
            lexik_jwt:
                class: AppBundle\YourJWTUser

Using it in your jwt-protected firewall and create the YourJWTUser class and populate it from createFromPayload().
What do you think?

@chalasr chalasr added the Feature label Dec 1, 2016
@scaytrase
Copy link
Author

Sounds good for the brief look.

Can you provide the sample implementation of createUserFromPayload method? Without external configuration for roles source it should be a kind of hardcode to extract it from payload, that's why I suggest to put the role field name to the configuration in the same way it has already been done for username field.

@chalasr
Copy link
Collaborator

chalasr commented Dec 2, 2016

See #278

@chalasr
Copy link
Collaborator

chalasr commented Dec 3, 2016

I suggest to put the role field name to the configuration in the same way it has already been done for username field.

I don't see much value in making the roles field name configurable. The username should be the unique identifier and is used to retrieve the key that is put in the payload at token creatin (i.e. form_login) so it's useful to have it configurable e.g. for email logins. Having roles hardcoded should be enough imho, but maybe I miss a need.

@scaytrase
Copy link
Author

If we speak about using the single algorythm alongside the coupled system - yes, there is no need to configure the roles field name. Moreover there is no need to configure the username field name (both could be hardcoded and thus fetched back with the same codebase). If we speak about having a distributed system that uses JWT as the bearer token authenticattion standard, we should give either a configuration option or the possibility to override the place, which configures the roles field name.

I agree, that hardcoded roles should cover the most of the user requirements, but one day someone want to override it.

@chalasr
Copy link
Collaborator

chalasr commented Dec 5, 2016

@scaytrase The user_identity_field doesn't only reflect the name of the key added to the payload, but the property name to use for getting the value from your UserInterface object and put it in the payload under the same key name.
Here we are talking about roles, we don't need to be aware of this property and its value must be accessible through getRoles() for having symfony giving the proper authorizations, while what is returned from getUsername() doesn't matter.

or the possibility to override the place, which configures the roles field name.

It can be achieved by hooking on the JWT_CREATED event, which allows you to customize the payload. Considering this and the fact I'm all for keeping configuration as simple as possible, I think we can hardcode this key as roles and let the user change it if needed. If someone asks/proposes to make it configurable with a concrete use case we could still reconsider it.

@chalasr
Copy link
Collaborator

chalasr commented Dec 5, 2016

The UserInterface::getUsername() method should be used for it, isn't it? What if I really have no real username field for object? maybe relation or virtual property i.e

It should be, but user providers do not always use username for loading users thus no, it is not.

Can you provide me some use case, when you are using the coupled system having only your bundle in action, but need to override the username field name?

Let's take FOSUserBundle (which is most of the times used with this bundle), the EmailUserProvider::loadUserByUsername() method loads users through the email field name, and it's far from being the only one. With such provider, doing $provider->loadUserByUsername($user->getUsername()) would just fail, it needs to be $provider->loadUserByUsername($user->getEmail()).
It is the only motivation behind the user_identity_field option, otherwise I would be really glad to remove it and let developers customize their payload by themselves.

having hardcoded roles field name as constant is some just bad code smell and covers some architertural approach problem, so what's why I'm concerning here.

Either I misunderstand or I totally don't agree here.
We are talking about a payload key, I can't see in what putting ['roles' => UserInterface::getRoles()] is bad code in a bundle which is indeed totally coupled to the symfony security (this is a symfony bundle, not a cross-framework library).

Why should every user-related key in the payload be configurable? And why should hardcoding the name of a payload key be prohibited? I really don't see any issue, things should be configurable only if there is a real interest, otherwise let's just be consistent with the UserInterface we are dealing with, imposed by the framework for which this bundle has been designed.

Again, If someone needs to change that key then he can just listen on the good event and change it, as any other key of the payload.

@scaytrase
Copy link
Author

scaytrase commented Dec 6, 2016

It should be, but user providers do not always use username for loading users thus no, it is not.

So, you are breaking the single responsibility principle here. Your code is neither responsible for fetching username from user nor from putting it there.

You are missing the some methods on UserProviderInterface which can help you to understand the flow better.

With such provider, doing $provider->loadUserByUsername($user->getUsername()) would just fail, it needs to be $provider->loadUserByUsername($user->getEmail()).
It is the only motivation behind the user_identity_field option, otherwise I would be really glad to remove it and let developers customize their payload by themselves.

For this purpose there is a method called UserProviderInterface::refreshUser(UserInterface $user): UserInterface. There are also an implementation of entity-based UserProviderInterface, named EntityUserProvider, which mostly do what you have implemented but in a more consistent manner.

All the logic working with how to fetch user by username (there is no matter email, login, etc, but username from any kind of login form or api), how to relfresh this user is incapsulated in UserProviderInterface. The login of backwards convertation (how to fetch a username from user) is incapsulated in UserInterface.

So you really do not need to know, which field is used to store the username. As I already mentioned above - there can be a situation when such field event does not exist. But you can still write a UserProviderInterface implementation for it.

So if you decouple your processing flows and re-use the build-in mechanics, the username field name config becames optional.

@chalasr
Copy link
Collaborator

chalasr commented Dec 6, 2016

You are missing the some methods on UserProviderInterface which can help you to understand the flow better.

@scaytrase I know the security component and what it provides, you don't get my point.
By loadUserByUsername($user->getUsername()) I mean:

// form_login authentication succeeded, let's create and return the token
$token = $jwtManager->create($user);
// Assuming we don't have the user_identity_field, the manager would do:
// payload = ['username' => $user->getUsername()];

// Now the user hits the api firewall with the token, let's authenticate it
$payload = $jwtManager->decode($token);

// we don't have to (and can't) use refreshUser here, we don't have access to 
// an user instance since the authentication is stateless in all cases, 
// but only the value of UserInterface::getUsername() previously put in the token
$user = $userProvider->loadUserByUsername($payload['username']);

Here, if the configured user provider is the EntityUserProvider (we use it in our sandbox btw) with property: email, then the call of $userProvider->loadUserByUsername($payload['username']); would fail since user.email !== user.username.

This bundle doesn't provide user management but only authentication, until here we don't know which user provider will be used and the user_identity_field is here to don't fail if the configured user provider uses a different property in loadUserByUsername() which we must call. The JWTUserProvider added is optional, any other user provider can be used and we can't lose it.

I made an example where the option must be set to make it work, using this bundle in combination with the EntityUserProvider: https://github.com/chalasr/lexik-jwt-authentication-sandbox/tree/without_user_identity_field , the readme gives everything to reproduce and fix it by setting the user_identity_field option.

So yes, this part of the code base is not sexy, but we need it actually. Of course, If you find a way to make it work without the option, I would be happy to see how it look in a PR.

@scaytrase
Copy link
Author

By loadUserByUsername($user->getUsername()) I mean:

No offence, I've just responded what I've seen :)
But thank for clarifying, now I understand you better.

I've just perform some digging about using emails with FOSUB and seems that the idea you trying to overcome is initially broken

See some links:

It's broken by design. The UserProvider you've linked has a significant drawback - it can login BOTH by username and by email, so there is not difference, whether to place a email or username there (so you can do payload['email'] = $user->getUsername(); and it will work, unfortunately).

It looks like the architectural drawback of the FOSUB which they cannot handle now (as far as I've understood the comments).

If you really want to support this, the better solution is to add some kind of compatibility (bridge?) layer for FOSUB only.

I made an example where the option must be set to make it work, using this bundle in combination with the EntityUserProvider: https://github.com/chalasr/lexik-jwt-authentication-sandbox/tree/without_user_identity_field , the readme gives everything to reproduce and fix it by setting the user_identity_field option.

But still, regarding FOSUB, leaving processing incapsulated should work
I'll try to patch your sandbox to make it work.

The thing I cannot understand, where the answer papi comes from? search gives nothing

@scaytrase
Copy link
Author

Well, actually the given example breaks the UserInterface here.

https://github.com/symfony/security/blob/master/Core/User/UserInterface.php#L74

Says that getUsername should return the username, used to authenticate the user.

fixing this making work without any additional configurations.

    public function getUsername()
    {
        return $this->getEmail();
    }

    public function getEmail()
    {
        return $this->email;
    }

    public function getNickname()
    {
        return $this->username;
    }

So everyhing is about conforming the contract

@chalasr
Copy link
Collaborator

chalasr commented Dec 6, 2016

No offence, I've just responded what I've seen :)

Of course and thanks a lot for the time spent here at trying to make this better, it's much appreciated.

It's broken by design

I already realized it but Indeed, the links you given confirm it. That's why I finally took the EntityUserProvider as example for showing you the issue that the user_identity_field is trying to solve.

I'll try to patch your sandbox to make it work.

To be honest I don't expect much since I tried to dig into it already, but at worst it'll make you understand why our code contains such weird things, at best you'll find what we are missing and help avoiding that stuff in our code, so thanks for looking at it.

The thing I cannot understand, where the answer papi comes from? search gives nothing

Oops, looks like a leftover from my tests. papi should be [email protected].
What happens is quite trivial:

  • Hitting the /login_check with success creates the token with username => $user->getUsername() in the payload (here)
  • Hitting /api with the token make this trying $userProvider->loadUserByUsername($payload['username']); (here), but since the configured provider is an EntityUserProvider with property: email instead of the default property: username, it just throws an UsernameNotFoundException that we catch in order to return a message that eases finding what goes wrong i.e. the user_identity_field which needs to be set to email, so that the token is created with ['email' => $user->getEmail()] in the first step.

Getting back to the original discussion, I finally think it's legit to make the roles field name configurable (while keeping $user->getRoles() called, no need for trying to access the field name as a property of the user object), I'll implement it in #278. My motivations for hardcoding it was only to don't add complexity in the configuration e.g. "If you're using the JWTUserProvider for don't reloading users on each request, then you can use the user_roles_field". But finally, the roles key will be added even if not using the JWTUserProvider so there is not any conditional configuration to add, it's fine.

@scaytrase
Copy link
Author

chalasr added a commit that referenced this issue Dec 30, 2016
From jwt.io:
> Self-contained: The payload contains all the required information about the user, avoiding the need to query the database more than once.

This is a working draft at the moment, allowing to preserve the federation benefit of JWT by avoiding fetching the user from the datastore more than once, so each token-authenticated request trusts in the JWT data rather than reloading the user to authenticate it.

I'm not sure how this should be introduced, at first I think it should not be the recommended way to use this bundle, but I do think we should have it since we are signing tokens and verifying them at each request, it makes sense to don't reverify the user.

Closes #277 once finished.
/cc @scaytrase
gzim324 pushed a commit to gzim324/LexikJWTAuthenticationBundle that referenced this issue Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants