fix: passport strategy adapter must support oauth2 flows #4919

deepakrkris · 2020-03-18T23:50:26Z

fixes: #4902

highlights of changes :

packages/rest/src/writer.ts - modified to handle redirections, controller can choose to redirect an api call
extensions/authentication-passport/src/strategy-adapter.ts - adapter is modified to handle redirection calls in passport strategy
extensions/authentication-passport/src/__tests__/acceptance/fixtures/oauth2-provider.ts
- Mock oauth2 provider to mock the whole authorization flow
extensions/authentication-passport/src/_tests__/acceptance/authentication-with-passport-strategy-oauth2-adapter.acceptance.ts
- test cases for the whole authorization flow

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

jannyHou

@deepakrkris Great effort and awesome documentations, very detailed! I left two comments about the design in #4919 (comment) and #4919 (comment)

jannyHou · 2020-03-24T21:54:05Z

.../src/__tests__/acceptance/authentication-with-passport-strategy-oauth2-adapter.acceptance.ts

+    const passport = new Oauth2Strategy(oauth2Options, verify);
+
+    // passport-oauth2 base class leaves user profile creation to subclass implementations
+    passport.userProfile = (accessToken, done) => {


is userProfile a function required by the Oauth2Strategy middleware?

Good question

@jannyHou userprofile is a no-op function in 'Passport-oauth2' Strategy since it is a base class for all oauth2 implementations like 'passport-facebook', etc. we are just overriding it here for our oauth2 impl.

jannyHou · 2020-03-24T22:10:52Z

.../src/__tests__/acceptance/authentication-with-passport-strategy-oauth2-adapter.acceptance.ts

+        // HTTP status code 302 is returned to the browser
+        const response = await supertest('')
+          .post('http://localhost:9000/login_submit?' + params)
+          .send({username: 'user', password: 'abc'})


any reason repeat the username and password in the request body and the parameters?

I was having a hard time to submit the form with variables in the beginning, so tried all kinds of approach. I will remove the query parameters since form submission is working now.

jannyHou · 2020-03-24T22:44:56Z

extensions/authentication-passport/src/__tests__/acceptance/fixtures/mock-oauth2-social-app.ts

+ */
+app.post('/login_submit', urlencodedParser, async function (req, res) {
+  const user = findUser(
+    req.body.username || 'user1',


any reason to have a default user and password set here?

I will remove the default values, added them when I was trying to make the login page submit work.

jannyHou · 2020-03-24T23:15:40Z

extensions/authentication-passport/src/strategy-adapter.ts

@@ -42,9 +42,9 @@ export class StrategyAdapter<U> implements AuthenticationStrategy {
   *     3. authenticate using the strategy
   * @param request The incoming request.
   */
-  authenticate(request: Request): Promise<UserProfile> {
+  authenticate(request: Request): Promise<UserProfile | RedirectRoute> {


@deepakrkris could you explain more about why we have to allow returning a RedirectRoute here?

According to the passport-oauth2 module, it returns a user (which conceptually same as lb's userProfile) after the authentication done, wondering why we couldn't follow the same pattern here?

The configure-strategy deep-link you have commented above, is only to set the list of strategies in the base 'passport' module (which is heavily dependent on express).
The passport.authenticate() method that is handling the express routes interprets the incoming request and does the redirection. The base 'passport' module implements this as an express middleware without any code written by the user. For LoopBack, the strategy adapter will have to handle the redirect callback from the passport-strategy (done by the base passport module in express) + we also need the controller to configure the routes (done by the passport.authenticate() method) + the injected send() method in the sequence must be aware of this redirection (done by the base passport module in express).

This is the part I found tricky (due to my lack of knowledge in authentication when I started in Jan 2019), the authentication strategy code needs to execute before it reaches a controller method. If the user is not allowed to call this controller method, then they should get a not authenticated http error. So in the flow you're following in all the code updates you've made @deepakrkris , is a user able to access and get into controller method code before this oauth authentication flow is finished? Perhaps we need a chart explaining each step of the code path through our code and through the oauth provider backend?

@jannyHou @emonddr users are able to access the oauth2controller methods before authentication completes because they participate to get the user validated with an external system. Those methods are essentially for authenticating the user and are small parts of the larger oauth2 dialog.

the Oauth2Controller methods that we have defined are similar to the endpoints defined in the authenticate requests section of the passport oauth2 readme.

@deepakrkris Thank you for the explanation 👍 the redirection passes the authenticated user's info to the callback endpoint, which results in the controller function maps to /thirdparty won't have a user profile.

jannyHou · 2020-03-24T23:48:44Z

.../src/__tests__/acceptance/authentication-with-passport-strategy-oauth2-adapter.acceptance.ts

+  constructor() {}
+
+  // this configures the oauth2 strategy
+  @authenticate('oauth2')


@deepakrkris related to my comment in #4919 (comment)
I think the scenario here is different than what's expected from our authentication system...user profile injection should happen here, which means after the oauth2 strategy is invoked, a user profile should be returned as the result of the authentication action.
My confusion point is, why we have to handle the redirect then get the user profile?

My expectation would be, the user profile is injected at controller level instead of the endpoint level. So that when the controller functions are invoked, the user profile is ready to use, both /thirdparty and /callback can print it.

I am not very familiar with the newly created redirectRoute therefore having a hard time to illustrate which part should handle the callback redirection, the strategy? or the controller?

To explain that quickly, businessUsecase endpoints wont be configured with anoauth2 strategy. The OAuth2Controller is configured with an oauth2 strategy so that it can begin the Authorization Code Grant dialog. Once that flow is completed, the web client would switch to other strategies like session or jwt.

The longer version :) :
/thirdparty route acts as an interface to begin the oauth2 dialog with the thirdparty and /callback is for the third-party login to redirect back to LoopBack app. Once /callback endpoint has the oauth token + the user profile, it can proceed in three different ways.

It can create a browser session. This is the most popular one we use everyday to login to an app using facebook/google credentials. This means the client from there on would use the passport-session strategy to access other Usecase endpoints in the LoopBack App.

Or it can return the original oauth token from the third-party to the web client which can then call other Usecase endpoints using the token as a Bearer(jwt strategy)

Or it can create a new token with the same expiration time of the original thirdparty token (the exp field in that token says it) and send that to the web client. (jwt strategy)

emonddr · 2020-03-26T14:35:35Z

Thanks for adding more documentation; including that ASCII diagram in the comment ;)

raymondfeng · 2020-03-26T17:02:10Z

.../src/__tests__/acceptance/authentication-with-passport-strategy-oauth2-adapter.acceptance.ts

+//  |            |                                                              |  v           |
+//  |            |      <------------------------[8]-----------------------     | ****         |
+//  +------------+        LB App returns to browser the access token            +--------------+
+


Thank you for the detailed explanation. It might be better to have a separate README.md though.

raymondfeng · 2020-03-26T17:08:35Z

packages/authentication/src/providers/auth-action.provider.ts

+      // bind redirection url and status as 'authentication.oauth2.redirectUrl' to context
+      // controller should handle actual redirection
+      this.ctx
+        .bind('authentication.redirect.url')


Please define constants for such keys in keys.ts.

raymondfeng · 2020-03-26T17:09:41Z

packages/rest/src/router/redirect-route.ts

+  redirect(response: Response): void {
+    response.statusCode = this.statusCode || 302;
+    response.setHeader('Location', this.targetLocation);
+    response.setHeader('Content-Length', '0');


This is probably not needed.

raymondfeng · 2020-03-26T17:14:48Z

.../src/__tests__/acceptance/authentication-with-passport-strategy-oauth2-adapter.acceptance.ts

+  ) {
+    const redirectHandler = new RedirectRoute('/', redirectUrl, status);
+    redirectHandler.redirect(response);
+    return redirectHandler;


Change it to return response please.

@raymondfeng , if RedirectRoute is not returned from the controller, the writer.ts (send function) cannot do a type check for RedirectRoute.

If we should return the Response object, in the writer.ts should we do a lookup against the request context for a redirect url value instead of the RedirectRoute type check ?

Raymond I see your point, writer.ts returns immediately on a response result. Thank you. I will make the change.

jannyHou · 2020-03-26T18:45:46Z

packages/authentication/src/types.ts

@@ -87,7 +87,9 @@ export interface AuthenticationStrategy {
   *
   * @param request - Express request object
   */
-  authenticate(request: Request): Promise<UserProfile | undefined>;
+  authenticate(


Had a chat with @deepakrkris yesterday and I agree it's reasonable to allow returning a redirect route. Might be a breaking change.

deepakrkris · 2020-03-26T20:10:52Z

packages/rest/src/router/redirect-route.ts

-  OperationRetval,
-  PathParameterValues,
-} from '../types';
+import {OperationArgs, OperationRetval, PathParameterValues} from '../types';


effectively no change here, just undid the change for last commit

deepakrkris · 2020-03-27T19:36:00Z

@raymondfeng could you please review/approve, I have fixed your review comments.

raymondfeng · 2020-03-27T20:11:12Z

packages/authentication/src/providers/auth-action.provider.ts

@@ -29,6 +29,7 @@ export class AuthenticateActionProvider implements Provider<AuthenticateFn> {
    readonly getStrategy: Getter<AuthenticationStrategy>,
    @inject.setter(SecurityBindings.USER)
    readonly setCurrentUser: Setter<UserProfile>,
+    @inject.context() private ctx: Context,


inject.setter is a better option. See https://loopback.io/doc/en/lb4/Decorators_inject.html for examples.

We already use @inject.setter for user:

@inject.setter(SecurityBindings.USER) readonly setCurrentUser: Setter<UserProfile>,

raymondfeng · 2020-03-27T20:12:00Z

packages/authentication/src/providers/auth-action.provider.ts

+        .to(redirectOptions.targetLocation);
+      this.ctx
+        .bind(AuthenticationBindings.AUTHENTICATION_REDIRECT_STATUS)
+        .to(redirectOptions.statusCode);


These can be replaced with setters (https://loopback.io/doc/en/lb4/Decorators_inject.html).

deepakrkris · 2020-03-27T20:45:09Z

@raymondfeng ready for review again, thank you for the perfect comments

fixes: #4902

raymondfeng · 2020-03-27T21:32:25Z

packages/authentication/src/providers/auth-action.provider.ts

+    @inject.setter(AuthenticationBindings.AUTHENTICATION_REDIRECT_URL)
+    readonly setRedirectUrl: Setter<string>,
+    @inject.setter(AuthenticationBindings.AUTHENTICATION_REDIRECT_STATUS)
+    readonly setRedirectStatus: Setter<number>,


deepakrkris force-pushed the fix4902 branch 3 times, most recently from 54747ef to ee0198f Compare March 19, 2020 07:28

dhmlau added feature Authentication labels Mar 19, 2020

dhmlau assigned deepakrkris Mar 19, 2020

deepakrkris force-pushed the fix4902 branch 13 times, most recently from 0b8f894 to ae56e93 Compare March 20, 2020 11:30

deepakrkris marked this pull request as ready for review March 20, 2020 11:31

deepakrkris requested review from emonddr, hacksparrow and jannyHou as code owners March 20, 2020 11:31

deepakrkris requested a review from bajtos March 20, 2020 11:31

deepakrkris force-pushed the fix4902 branch 6 times, most recently from bc9d387 to fe38976 Compare March 20, 2020 18:46

deepakrkris force-pushed the fix4902 branch from 4f3bdba to 3292631 Compare March 24, 2020 23:48

jannyHou reviewed Mar 24, 2020

View reviewed changes

deepakrkris force-pushed the fix4902 branch 4 times, most recently from 231bc30 to 6756ead Compare March 26, 2020 02:48

raymondfeng reviewed Mar 26, 2020

View reviewed changes

deepakrkris force-pushed the fix4902 branch from 6756ead to 0f3badb Compare March 26, 2020 18:11

jannyHou reviewed Mar 26, 2020

View reviewed changes

deepakrkris commented Mar 26, 2020

View reviewed changes

deepakrkris force-pushed the fix4902 branch from 589db36 to 37ba8ed Compare March 27, 2020 18:59

raymondfeng reviewed Mar 27, 2020

View reviewed changes

deepakrkris force-pushed the fix4902 branch from 37ba8ed to 99ee2d3 Compare March 27, 2020 20:41

deepakrkris force-pushed the fix4902 branch 2 times, most recently from d869966 to b08a2ce Compare March 27, 2020 20:59

fix: passport strategy adapter must support oauth2 flows

3c222f2

fixes: #4902

deepakrkris force-pushed the fix4902 branch from b08a2ce to 3c222f2 Compare March 27, 2020 21:19

raymondfeng reviewed Mar 27, 2020

View reviewed changes

raymondfeng approved these changes Mar 27, 2020

View reviewed changes

deepakrkris merged commit 67c2f02 into master Mar 27, 2020

deepakrkris deleted the fix4902 branch March 27, 2020 22:35

dhmlau mentioned this pull request Mar 29, 2020

OAuth2 Strategy support for the authentication package #230

Closed

emonddr mentioned this pull request Mar 30, 2020

docs: migrate loopback-example-passport repo as lb4 example #4899

Merged

7 tasks

fix: passport strategy adapter must support oauth2 flows #4919

fix: passport strategy adapter must support oauth2 flows #4919

Conversation

deepakrkris commented Mar 18, 2020 • edited Loading

Checklist

jannyHou left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepakrkris Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emonddr commented Mar 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepakrkris commented Mar 27, 2020

Choose a reason for hiding this comment

raymondfeng Mar 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepakrkris commented Mar 27, 2020

Choose a reason for hiding this comment

deepakrkris commented Mar 18, 2020 •

edited

Loading

jannyHou left a comment •

edited

Loading

deepakrkris Mar 25, 2020 •

edited

Loading

jannyHou Mar 24, 2020 •

edited

Loading

raymondfeng Mar 27, 2020 •

edited

Loading