Loopback-next: Document ways to properly authenticate only some methods in a controller

[shimks]

Description / Steps to reproduce / Feature proposal

Copying from #1275

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.

There should be documentation for handling this use case in the README

_See Reporting Issues for more tips on writing good issues_

[bajtos]

I see two design approaches:

1. Treat CURRENT_USER as an optional binding that is either resolved to a valid user or else an error is thrown. This will require all consumers to obtain it with optional: true flag or use one of the other approaches described above. Pro: when a piece of code receives a current user, it can safely assume the user is defined. Con: adding {optional:true} to every binding is cumbersome and removes the type safety, because we can end up with undefined user again.

2. Treat CURRENT_USER as a binding that's always defined, but provides undefined value when the user is not authenticated. Pro: it's easy to receive the current user from the context. Con: the type becomes UserProfile | undefined, consumers must be prepared to handle the case when the user is not defined.

Considering the options, I think I like the following one best:

• Treat CURRENT_USER as a binding that throws for anonymous users.
• Inject CURRENT_USER as a method argument to operations requiring authentication. IMO, this is the API that's easiest to test in isolation (think about unit tests).

However, instead of reporting "CURRENT_USER was not bound" error, we should report a more helpful error explaining that the request was not authenticated. This can be achieved for example by binding CURRENT_USER during component initialization and then re-binding the value to the actual user value once the user was authenticated.

Add to auth component setup:

app.bind(CURRENT_USER).toDynamicValue(() => {
  return Promise.reject(HttpErrors.Unauthorized());
});

On the second thought, since components can contribute only Providers, we may need to rewrite the code snippet above to use a Provider instead of a dynamic-value function.

Here is the existing code that will re-bind the current user for authenticated requests - no change should be needed there:

https://github.com/strongloop/loopback-next/blob/1b861c4958935930349e8f9dd927b7d48ad9cbeb/packages/authentication/src/providers/authentication.provider.ts#L78-L80

[virkt25]

+1 for a more helpful error message if we throw one (should be improved anyways in case someone accidentally went down this path).

I like the idea of injecting CURRENT_USER as a method argument / having it resolved as a getter (but with this approach I feel it's more complicated as a user would need to know they need to do this).

[jannyHou]

By injecting the CURRENT_USER when instantiate the controller class, each call has a consistent user if the invoked method calls other methods that need authentication.

While by using @inject.getter, does it still guarantee the user retrieved in a chain of methods is always the same?

[jannyHou]

Treat CURRENT_USER as a binding that's always defined, but provides undefined value when the user is not authenticated. Pro: it's easy to receive the current user from the context. Con: the type becomes UserProfile | undefined, consumers must be prepared to handle the case when the user is not defined.

Maybe we can define a default value for UserProfile that represents an anonymous user, then we don't need a union type to handle undefined...

[joeytwiddle]

I agree that it is preferable to have each route declare whether it requires authentication or not. 👍

Interesting questions janny. I can see how an anonymous user value might sometimes be useful, but other times we might want our route to always respond to unauthed requests with an error. (Empty responses like 200 [] may be more confusing to the caller than 401 You need to be authenticated)

I don't know how the injection will look on a route, but I do hope it will have a small footprint.

// Nice and short (the default strategy would be defined elsewhere)
@authenticate()
@get('/whoami')
whoAmI(): string {/*...*/}

// Acceptable
@authenticate('BasicStrategy')
@get('/whoami')
whoAmI(): string {/*...*/}

// Acceptable
@get('/whoami')
whoAmI(
  @injectAuth() user: UserProfile,
): string {/*...*/}

// Oh boy. This boiler plate is a bit heavy to put on every route!
@get('/whoami')
whoAmI(
  @inject(AuthenticationBindings.CURRENT_USER, { strategy: 'BasicStrategy' }) user: UserProfile,
): string {/*...*/}

[bajtos]

IIRC the constraints of our current authentication design, I think the following should be easy to achieve:

// Use the default strategy configured elsewhere
@authenticate()
@get('/whoami')
whoAmI(
  @injectCurrentUser() user: UserProfile
) {}

// Use a custom strategy
@authenticate('BasicStrategy')
@get('/whoami')
whoAmI(
  @injectCurrentUser() user: UserProfile
) {}

We can even shorten @injectCurrentUser to @currentUser or similar.

[joeytwiddle]

An afterthought: Would it be possible for @authenticate() or for @injectCurrentUser() to set this.user?

That might reduce the boilerplate, if we don't have to keep accepting the user object (as a parameter).

And it would also achieve parity with the current @inject(AuthenticationBindings.CURRENT_USER) in case we decide to keep that for developers who _do_ want authentication on every route.

[bajtos]

An afterthought: Would it be possible for @authenticate() or for @injectCurrentUser() to set this.user?

You can inject the current user in your controller constructor. Depending on how we decide to handle CURRENT_USER when no user is authenticated (throw an error or return undefined), it is probably safer to inject a getter instead.

class UserController {
  constructor(
    @inject.getter(AuthenticationBindings.CURRENT_USER) 
    public getCurrentUser: Getter<UserProfile>,
  ) {}

  @authenticate()
  @get('/whoami')
  whoAmI() {
     console.log('I am ', this.getCurrentUser());
  }
}

[bajtos]

Authentication is out of scope of 4.0 GA, removing this story from the release.

[dhmlau]

@emonddr , would this be covered by your recently created task to clean up the docs? https://github.com/strongloop/loopback-next/issues/2940

[emonddr]

@dhmlau , we can definitely have a section on this. I've only ever decorated a controller method with authenticate(name,options) IF I want to create an authenticated endpoint, and I've only ever injected the current user on a controller method as a parameter on an authenticated endpoint (never in a controller constructor), but I will discuss with @jannyHou and @raymondfeng . :)

[hazimdikenli]

I think you should use authentication decorator/injector either on class levels, or on method levels, does not make sense trying to support both options at the same time. If there is no way to prevent this decorator/injector from being used on class or method levels, let the developer handle the problems.

Like a controller might have only one method which needs authentication, let them use authentication decorator on method level. But if they use it on the class level and one of the methods does not need authentication, they should refactor their code,

1. either by adding another controller to house that method ; like splitting a users controller to have membership services, self sign-in services, or password recovery services etc.
2. or by removing authentication decorator from class level and applying it on method levels.

Considering that this will be used in parallel with authorization decorators (or guards) on methods, what you'll end up doing is, add an authenticaton decorator at the class (controller level) so you have access to the current user and you are preventing anonymous access, and if required, add an authorization decorator on the method level, which requires a current user to check against the specified privileges on the decorator.

[dhmlau]

Discussion with @raymondfeng @jannyHou @emonddr:

• To answer the original question, @authenticate decorator is applicable at the method level.
• If we want to allow decorators to be used at class level and method level can inherit from the class level, we can use interceptor.

Acceptance Criteria

• [x] Document how to use the @authenticate decorator so that it is applicable at the method level (which is the current usage anyway).

[dhmlau]

@emonddr , per the discussion in the estimation meeting, could you please review the original description and see if your docs PR is going to cover that? Thanks.

[emonddr]

@dhmlau , my recent docs PR https://github.com/strongloop/loopback-next/pull/2977 only covers adding the @authenticate decorator at the method level.

With regards to whether CURRENT_USER is injected in the constructor or in the method (and when {optional:true} should be specified)...I added a note in the Using the Authentication Decorator which addresses this. PR#3185