Overriding JWTProvider about lexikjwtauthenticationbundle

[slashfan]

Hi,

You're right, those member variables should be set to protected to allow easier overwrite. I'll add interfaces too to have more flexibility.

[epicwhale]

Sounds good.. do let me know once done.

Are you implying that there will be a configuration to override the JWTProvider used?

While you're at this.. don't you think there should be a way to access the Request object within the JWTProvider class. The quickest way I deem this will be possible without breaking backwards compatibility is by adding the getRequest() function to the JWTManager class which already has the setRequest(..) function to inject the request. It only but makes sense that there should be a getter so the JWTProvider can access the Request context too..

For example, if I am building a multi-tenant API and I need to access the app id from the URL to ensure that the UserProvider returned belongs to the app id being requested, I will need access to the request object to access one or more attributes.

[slashfan]

That's a good idea, let's add the getRequest() method to the JWTManager.

I've started a branch if you want to have a look at it. Basically, it allows to completely override the JWTProvider used in the lexik_jwt section of the firewall configuration if needed.

For those who just don't want to authenticate users by username, I started moving out of the main methods the logic to extract user from token payload and to add user identity to payload. Overriding those methods in sub classes should be sufficient. Dispatching an event might also do the trick without having to create new classes.

What do you think ?

[epicwhale]

1. Yes, please add the getRequest() to the JWTManager, I've had to override the class and both the setRequest and getRequest to access the request object. (Even the $request variable is private and should ideally be protected).
2. The branch looks good. I feel the configuration name could be just authentication_provider instead of authentication_provider_service to keep it in line with how I suppose symfony2 names such config. (providers are implicitly service ids)
3. Regarding the third point. Looks neat. This way, I can easily modify the field names which are used for the user identity by overriding both these functions. Perhaps the array key name 'username' can be a protected variable too which can be overriden if someone doesn't want to change the logic of extracting user from token or adding it to token, but just changing the field name used.

protected $userIdentityField = 'username'

Also exposing this as a public getter public function getUserIdentityField. The use case for this could be if someone is building an endpoint to refresh the token, he would generally take the old taken as input and provide a refreshed token. This process requires extraction of the username field in the token (without checking if the token has expired) and then re-encoding it back into a new token. The new token can be easily generated using the AuthenticationSuccessHandler but the extraction of just the user identity will require the field name.

[slashfan]

OK for 1 and 2, it makes sense.

About 3, I think you're right. But in the meantime, it might be a lot for the JWTManager to handle.
It might be useful to add some sort of PayloadGenerator service (and the corresponding interface). Injected in the JWTManager like the encoder is, it would allow much more flexibility.

The default implementation would only do what's done in the manager : add username as identity and the expiration date (that way the ttl parameter should be removed from the manager).

The only downside I see to this is the overhead to maintain backward compatibility.

[epicwhale]

Excuse my delayed reply. Have you made any release progress on this?

I don't think anyone would find the need to remove the ttl parameter, and if they want to, removing it in the event listener seems fine.

I don't see any of the above changes breaking BC. Which particular implementation do you think will break backwards compatibility?

[slashfan]

Hi, I just updated the branch and added a configuration option for the userIdentityField. No BC break included, I'll do the big refactoring later. Tell me if it's ok for you. #40

[epicwhale]

@slashfan Looks positive. Will update my composer and try this out locally this week. Will let you know if any feedback.

[slashfan]

Ok, I'll tag it soon. Might be time for a 1.1.0.

[VanVan]

There is a new method setUserIdentityField() on JWTProvider