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

Required to annotate with @authenticate for all the routes! #2460

Closed
4 tasks
vishalvisd opened this issue Feb 23, 2019 · 13 comments
Closed
4 tasks

Required to annotate with @authenticate for all the routes! #2460

vishalvisd opened this issue Feb 23, 2019 · 13 comments
Assignees
Labels
Authentication developer-experience Issues affecting ease of use and overall experience of LB users feature

Comments

@vishalvisd
Copy link

vishalvisd commented Feb 23, 2019

Current Behavior

I have to annotate all of my controller routes with @authenticate('startegry_name'), but in my case I just have a single strategy and don't like the compulsion of adding the @autheticate annotation to each of my 20-30 routes. Though, one or two routes are there where I don't need any authentication and should be available for public.

Expected Behavior

Would be better if there is a way to have authentication default for all routes except for one or two. IF there is already some sort of way available please let me know what would be that.

Acceptance criteria

  • if a method is decorated with @authenticate, the method would use the metadata as is without checking the class
  • if a method is not decorated with @authenticate, the method would check the class level
  • introduce @authenticate.skip or @authenticate('skip') to exclude a method to inherit @authenticate from the class
  • update relevant docs in the site
@dougal83
Copy link
Contributor

except for one or two.

A whitelist effectively? I do think that is a nice to have(if not already). Low priority.

@vishalvisd
Copy link
Author

So, is it nice to have all paths which may go up to any number (in my case around 30) and all to be annotated with @authenticate !?. How about the case when there is just one strategy and all the paths (ignore my mention about one or two paths and assume all paths) needs to be authenticated with that strategy

Also, is it good way - for example if for some reason, if someone forget to annotate, than that path will be public.

Unless I missed something, I feel I am asking a very simple question about a common scenario that many will be facing.

@vishalvisd vishalvisd changed the title Required to annotate with authenticate for all the routes! Required to annotate with @authenticate for all the routes! Feb 23, 2019
@jannyHou
Copy link
Contributor

@vishalvisd I also discussed with team about having a controller class decorator that sets one authentication strategy for all the methods. Will update the progress here when we start, now as a workaround, you can hardcode what strategy to return in the strategy resolver, see this example.

In the example above, the strategy resolver returns the strategy according to a controller function's metadata, in your case you can just ignore the metadata and return the particular strategy you want.

@vishalvisd
Copy link
Author

I know about this, but this need to still annotate all routes which I think is not a workaround.

Thanks for taking it up for discussion, hope to see an update on this soon. Loopback is a great framework and I really appreciate the effort you guys are putting into it!

@dhmlau
Copy link
Member

dhmlau commented May 31, 2019

@jannyHou @emonddr , could you please help groom this task so that the team can estimate? Thanks!

@raymondfeng
Copy link
Contributor

We can use the same pattern as @intercept - https://github.com/strongloop/loopback-next/blob/master/packages/context/src/interceptor.ts#L339.

The decorator allows to be applied at both class and method level.

@dhmlau
Copy link
Member

dhmlau commented Jun 10, 2019

Related to #1334

@emonddr
Copy link
Contributor

emonddr commented Jun 25, 2019

I need some time to study the interceptors pattern being recommended here, to understand how this would be implemented...

But in the meantime, here are some things we need to consider:

If we have the @authenticate('some_strategy') decorator at the controller class level, this would mean ALL controller functions will go through authentication.

If we want a few controller methods to avoid authentication in this scenario, we would need another decorator like @unauthenticated to be placed on these few controller methods. (to avoid authenticating these requests)

There's a PR in the works about how default and request-level options should be handled
#3194 . The current suggested approach is that a strategy class should load its default options as a class property, and that any option overrides should be specified at the request-level in the @authenticate decorator on a controller function.

So authentication with default options, the decorator parameters at the class level would look like this:

@authenticate('jwt_strategy')

No options are passed as a second parameter (no need to, the strategy class loads defaults)

If user wants to override some default option for a specific authenticated controller function, then
the @authenticate decorator would need to be place on that controller method and would look like this:

@authenticate('jwt_strategy',{ someOption: "different_value" })

@jannyHou, @raymondfeng , what are your thoughts on this? thanks

@raymondfeng
Copy link
Contributor

@emonddr Good idea to list various combinations. In general, the class level provides default settings and the method level can override it. To make things simple, I suggest the following:

  1. If a method is decorated with @authenticate, uses the metadata as is without checking the class
  2. If a method is not decorated with @authenticate, checks the class level
  3. Introduce @authenticate.skip or @authenticate('skip') to exclude a method to inherit @authenticate from the class

@jannyHou
Copy link
Contributor

Thanks @emonddr for the summary,

I need some time to study the interceptors pattern being recommended here

I think Raymond means the class or method decorator can be created like this.

And I am good with @raymondfeng 's proposal:

If a method is decorated with @authenticate, uses the metadata as is without checking the class
If a method is not decorated with @authenticate, checks the class level
Introduce @authenticate.skip or @authenticate('skip') to exclude a method to inherit @authenticate from the class

No options are passed as a second parameter (no need to, the strategy class loads defaults)

I think the class level decorator can still take options, like each controller has its own configurations. We can leverage @config(). These are implementation details we can discuss more when start to work on this story.

@dhmlau dhmlau added the developer-experience Issues affecting ease of use and overall experience of LB users label Jul 16, 2019
@dhmlau dhmlau added the p2 label Aug 20, 2019
@dhmlau dhmlau added 2019Q4 and removed 2019Q3 labels Sep 18, 2019
@dhmlau
Copy link
Member

dhmlau commented Sep 18, 2019

Moving to Q4.

@raymondfeng
Copy link
Contributor

See #3762

@dhmlau
Copy link
Member

dhmlau commented Sep 23, 2019

Closing as done.

@dhmlau dhmlau added this to the Sept 2019 milestone milestone Sep 23, 2019
@dhmlau dhmlau closed this as completed Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Authentication developer-experience Issues affecting ease of use and overall experience of LB users feature
Projects
None yet
Development

No branches or pull requests

7 participants