Lumen 5.2 Issues!

[irazasyed]

So I've been testing this package in Lumen 5.2 and i came across the following issues:

1. Class cache.store does not exist
Path: Tymon\JWTAuth\Providers\Storage\Illuminate

So the issue with this is that, with Illuminate\Contracts\Cache\Repository as CacheContract in constructor, it fails for some reason but if that is replaced with simple app('cache'), it works fine. Weird and not sure what's happening.

2. Fatal error: Call to undefined method Illuminate\Auth\RequestGuard::onceUsingId()
Path: Tymon\JWTAuth\Providers\Auth\Illuminate

Is anyone else facing these issues? Is there a solution?

[tdhsmith]

So the issue with this is that, with Illuminate\Contracts\Cache\Repository as CacheContract in constructor, it fails for some reason but if that is replaced with simple app('cache'), it works fine. Weird and not sure what's happening.

Hrm I'm not sure what's up there; I know very little about Lumen but they both should be using the same Cache module. I guess it could just be a difference in bindings/DI? In any case, you should probably be using app('cache.store') because cache alone points to the cache factory interface, and I'm pretty certain that's going to break once you rely on it.

2. Fatal error: Call to undefined method Illuminate\Auth\RequestGuard::onceUsingId()
   Path: Tymon\JWTAuth\Providers\Auth\Illuminate

Urgh ok this looks a lot trickier. Lumen "doesn't support" session-based auth, so by default it has you extend Auth with a custom, callback-based driver (see the first section in the docs here). Unfortunately, the Guard for this driver, RequestGuard, doesn't implement the methods we need. We don't actually care about sessions or statefulness (JWT is stateless), but we use Auth::onceUsingId($credentials) to support our User lookups (JWTAuth::authenticate).

So for now you either can not use authenticate and do your user lookups manually from the sub claim, or you can write a custom provider implementing Tymon\JWTAuth\Contracts\Providers\Auth that doesn't utilize onceUsingId to find the user.

[tdhsmith]

I think the long-term solutions to the second problem are:

1. Native support as an 5.2 auth driver
2. Packaging up an "auth provider" for jwt that actually sidesteps the whole auth system. (I've been kicking that around for a while since this keeps coming up. In fact, we could even do something crazy like shift the byId method to the JWTSubject interface.)

[GrahamCampbell]

1. Class cache.store does not exist

You need make the cache repository contract instead of container alias exclusive in laravel.

[irazasyed]

@tdhsmith

Hrm I'm not sure what's up there; I know very little about Lumen but they both should be using the same Cache module. I guess it could just be a difference in bindings/DI? In any case, you should probably be using app('cache.store') because cache alone points to the cache factory interface, and I'm pretty certain that's going to break once you rely on it.

I tried this already and it fails. Ends up throwing same "does not exist" error. Hence, I'm using the cache alias which seems to be working. Again this is still weird, since the repo is type hinted and it was supposed to work.

Urgh ok this looks a lot trickier. Lumen "doesn't support" session-based auth, so by default it has you extend Auth with a custom, callback-based driver (see the first section in the docs here). Unfortunately, the Guard for this driver, RequestGuard, doesn't implement the methods we need. We don't actually care about sessions or statefulness (JWT is stateless), but we use Auth::onceUsingId($credentials) to support our User lookups (JWTAuth::authenticate).

So for now you either can not use authenticate and do your user lookups manually from the sub claim, or you can write a custom provider implementing Tymon\JWTAuth\Contracts\Providers\Auth that doesn't utilize onceUsingId to find the user.

Yeah, I figured this after i posted the issue here and like you suggested, The long-term solution would be to have a native support. For the time being, I'll probably write a custom provider. Thanks.

@GrahamCampbell

You need make the cache repository contract instead of container alias exclusive in laravel.

You mean do this:
app(\Illuminate\Contracts\Cache\Repository::class); ? It still fails and originally this repo is type-hinted already in constructor of the provider and that's whats causing the issue. For some reason it's not able to resolve. The only way it works is using the alias which is why its weird.

Thanks.

[irazasyed]

I ended up creating a package with a custom Auth Driver for this package: https://github.com/irazasyed/jwt-auth-guard

Works good for me :)

[irazasyed]

So this PR should fix the number 1 issue i had: laravel/lumen-framework#324

[tdhsmith]

Aha! Great news, I was wondering what could be going on there.

[dani3l]

irazasyed, nice package! would love to see all those methods ending up in tymon's driver :)

[irazasyed]

@dani3l Thanks and yep, Would love to see that happening as well :)

[tymondesigns]

I think I will need to look at some of the points raised by @tdhsmith here, specifically around the fact that it feels a bit weird to add once() & onceById() methods to an already stateless auth driver. But I do appreciate that it is necessary right now, given that the JWTAuth instance requires those methods via the Illuminate Auth provider (oooh inception 😄 )

@irazasyed I know there are some subtle differences between our implementations of the Guard, but I would happily accept a PR to fill in the blanks so to speak, if you like?

[irazasyed]

@tymondesigns Sure! I can send a PR. Would you like me to add those once() & onceById() methods too? As you can see, I also have other methods like logout() (Which btw is more like a wrapper around invalidate but since the other guard too have this, So it's quite common for people to be using such method), generateTokenById(), refresh(), invalidate(), etc. some of which are just wrappers while others do have a real usage. So i gotta know what methods you want me to implement in your Guard or if you prefer everything, That's fine too.

As far as once() and onceById() is concerned, I know it might not really make sense to have those methods but they're there acting more as helpers and filling the requirements in your package, like you've already mentioned. Besides, It doesn't really return anything other than a boolean value. So they'll serve more like helpers with if/else statements i think, not a biggie.

There are some more issues though, Mostly with the Lumen, check out this: laravel/lumen-framework#291

Because of this issue, The parseToken method ends up throwing an error because it's trying to access route param using the method in Laravel which works there but not in Lumen. Hence, It aborts the whole app instead of throwing an exception with a message like, token not found. Besides this, There are few other minor things that needs to be fixed as well. Will get to that point later.

[tymondesigns]

@irazasyed I've already added the logout method, but we definitely need once() and onceUsingId() (and I get your point on this btw, as I say, I just want to have a think to see if there might be a better way at some point), oh and the refresh() method would be good.

I guess i'm saying add everything in there 😄 and I may end up tweaking things if necessary when I'm at my machine later.

Thanks for pointing out that issue with route params, maybe I will add the parser chain to the config - https://github.com/tymondesigns/jwt-auth/blob/develop/src/Providers/LumenServiceProvider.php#L155

Then you could just remove the RouteParams class, especially if it's not used, or indeed to add more to the chain.

[tdhsmith]

On the route params stuff, #361 has an example where it fails because there's no associated route() whatsoever (when a request is created manually rather than naturally), so there should probably be more configuration or safety there anyway.

[tdhsmith]

With configurable parser chain, Lumen users could sub in something like this until a change makes it to core:

class LumenCompatibleRouteParams extends RouteParams
{
    public function parse(Request $request)
    {
        return $request->route()[2][$this->key];
    }
}

(That magical 2 makes kittens cry. 😿)

I guess technically we can already configure the chain by abusing $parser->setChainOrder($arr)...

[tymondesigns]

@tdhsmith great point! Didn't think of the testing stuff

[tdhsmith]

Well I believe the PHPUnit helpers run everything through Kernel correctly, but who knows what other things exist in the wild, so we probably shouldn't assume route() works.

[tymondesigns]

I guess we could just throw a try catch around the damn thing and be done with it 😁 who uses the route parameters for this anyway? amirite

[irazasyed]

@tymondesigns Sure. I'll do that soon :)

As far as the route param issue is concerned, Since as @GrahamCampbell said, they're open for PRs. So i think we should just focus on fixing the root cause of this issue rather than making several changes in this project and making it more confusing for people (Though flexibility is good but too much of it also makes it look like its too complicated, it's simple until you make it complicated :P).

What you could also do is, Add the input key option in config. So people can actually set a different name if they'd like. So for example, If someone wants to change the name to api_token then they could just update in config. EDIT: Look at the Token Guard for example of what i meant.

I'll probably create a new PR to the core framework soon.

[tdhsmith]

@tymondesigns Unfortunately it's a fatal error, not an exception, so we can't actually take the easy way out. 😑 But we can do some method_exists checks and other things...

@irazasyed Yes I agree rapid changes and configuration bloat can be issues. As a compromise there could merely be programmatic access to the parsers through JWTAuth::getParser() and $parser->getChain()..?

[irazasyed]

@tdhsmith That's doable and sounds good. For more advance developers who really want to dig into it and customize in depth, that should be good enough 👍

[tymondesigns]

@irazasyed So i've gone ahead and added the missing methods. Should work as expected now I think

[lukeed]

so is Lumen 5.2 officially supported now?

[naerymdan]

I've been trying to setup your latest code on latest Lumen 5.2.3, but the function extendAuthGuard in LumenServiceProvider,php creates an infinite loop trying to satisfy the $app['tymon.jwt.auth'] dependency for JwtGuard.

All of it seems to come from AuthServiceProvider wanting to instantiate the guard to finish building the auth provider, specifically in function registerAuthenticator.

Any idea?

[naerymdan]

Nevermind, seems like the latest modifications have fixed the guard provider and the JWTGuard can be accessed with app('auth').

[irazasyed]

@tymondesigns I was away for sometime. Just saw, Everything seems to be good 👍

I'm closing this ticket now. Since all the issues mentioned seem to have been resolved in last few commits.