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

The key authentication.currentUser was not bound to any value (mixed auth/noauth controller) #1275

Closed
joeytwiddle opened this issue Apr 24, 2018 · 9 comments

Comments

@joeytwiddle
Copy link
Contributor

joeytwiddle commented Apr 24, 2018

I have started from a generated server and tried following the authentication instructions.

But whenever I try to access an authenticated a route, I see this error:

Unhandled error in GET /whoami: 500 Error: The key authentication.currentUser was not bound to any value.

Where is the CURRENT_USER provider supposed to get added to the registry? ☑️ Answered below.

Is there an example server with working authentication anywhere? ☑️ Got one below. Thanks virkt25!

Another thing to note is that the verify() function in MyAuthStrategyProvider never gets called. Although it does get called in virkt25's example, so I need to work out what the difference is between our projects.

@joeytwiddle
Copy link
Contributor Author

joeytwiddle commented Apr 25, 2018

This might not be relevant, but I found basic-auth.acceptance.ts and noticed the setup is a bit different.

The README does:

.bind(AuthenticationBindings.STRATEGY).toProvider(MyPassportStrategyProvider);

to the app object but the acceptance test does it to the server object.

@joeytwiddle joeytwiddle changed the title Error: The key authentication.currentUser was not bound to any value Getting the authentication instructions to work Apr 25, 2018
@virkt25
Copy link
Contributor

virkt25 commented Apr 25, 2018

Hi @joeytwiddle ,

I created an app using the latest lb4 CLI and followed the instructions in the @loopback/authentication README and everything seems to be working fine for me. Here's my test app so you can spot anything you might be doing different.

https://github.com/virkt25/lb4-auth-test

If everything you're doing is the same and it's still not working for you can you please provide a reproduction sandbox. Thanks and hope this helps :)


Where is the CURRENT_USER provider supposed to get added to the registry?

This gets added from the AuthenticationComponent which you add in src/application.ts by doing app.component(AuthenticationComponent).

@shimks
Copy link
Contributor

shimks commented Apr 25, 2018

Taking a quick look, it seems like AuthenticationBindings.CURRENT_USER isn't bound to the app context. Have you made sure to register the AuthenticationComponent to the app context?

EDIT: @virkt25 beat me in terms of quality with his reply 😆

@joeytwiddle
Copy link
Contributor Author

joeytwiddle commented Apr 26, 2018

Thanks so much! I will compare our repos and feedback.

Perhaps your demo repository could be linked or imported into the loopback docs examples page, as an example for others. (If the authentication API is stable.)

@joeytwiddle
Copy link
Contributor Author

joeytwiddle commented May 3, 2018

OK, so apparently the demo works fine. The reason I was confused is this: my controller has some routes which require authentication, and other routes which should be publicly accessible.

(For example, we allow an unauthed POST /user to create a new account, but most other routes on the user controller require login.)

Unfortunately it seems if my controller uses AuthenticationBindings.CURRENT_USER then I must use the @authenticate decorator on all of its routes.

If I leave @authenticate off one of the routes, then when that route is accessed it will throw:

Error: The key authentication.currentUser was not bound to any value.

I forked a branch to reproduce that behaviour. This simple commit demonstrates the problem.

Does that seem like expected behaviour to you guys?

I was thinking of some possible ways to improve the situation:

  1. Allow a controller to have routes with and without the @authenticate decorator.
  2. Allow some routes to have optional auth (so this.user may or not be set when the route runs).
  3. (perhaps preferable) Offer auth strategies to provide behaviours 1 and 2. (I started working on those but never finished.)

@joeytwiddle
Copy link
Contributor Author

joeytwiddle commented May 16, 2018

Regarding 3, my approach was to create two new auth strategies:

  • NoAuthStrategy would always act like authentication had succeeded, and would always resolve, but it would resolve with user: null

    That would allow for routes which do not require authentication, but could live within a controller which uses the AuthenticationBindings.

  • OptionalAuthStrategy would be an adapter: it would accept a normal auth Strategy in its constructor. At auth time, it would try the provided strategy, but if that failed, it would still resolve, just with user: null.

    That would allow for routes which would run with or without an authenticated user.

Unfortunately I haven't had time to complete the module. You are very welcome to use the code or just use the idea.

If the problem I described in the previous comment (controllers with a mixture of auth and non-auth routes) is not really a problem, please feel free to close this Issue.

@joeytwiddle joeytwiddle changed the title Getting the authentication instructions to work The key authentication.currentUser was not bound to any value (mixed auth/noauth controller) May 16, 2018
@shimks
Copy link
Contributor

shimks commented May 16, 2018

I'm going to be investigating this.

Our intended use case was that users wouldn't have to decorate routes that don't require authentication.

Thanks for bring this up!

@shimks
Copy link
Contributor

shimks commented May 16, 2018

Just got back from a talk with @raymondfeng and told me what is happening here. Each time a route is accessed, a controller instance is created and resolves the injections given to it (in this case it'd be CURRENT_USER). When routes without @authenticate decoration are accessed, injections still try to be resolved, and in this case they will error out since CURRENT_USER hasn't been bound by the authentication function in the sequence.

Here are three approaches to get this working in the intended way:

  1. Pass in {optional: true} in @inject (@inject(AuthenticationBindings.CURRENT_USER, {optional: true}))
    This allows injection to not error out even if CURRENT_USER is undefined
  2. Inject CURRENT_USER as a method argument to operations requiring authentication
    This allows the injection to only occur when the authenticated methods are being invoked
  3. Use @inject.getter to retrieve the injection as the method is being invoked
    This allows injection to be resolved when the method is being invoked and not when the controller is being instantiated.

We also plan on documenting these approaches (#1334), as we don't feel it was adequately explained enough in the README, so stay tuned!

@shimks shimks removed the bug label May 16, 2018
@bajtos
Copy link
Member

bajtos commented Jun 25, 2018

Closing in favour of #1334

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants