Skip to content

Conversation

@deepakrkris
Copy link
Contributor

@deepakrkris deepakrkris commented May 5, 2020

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 👈

return userProfile;
};

export class FacebookOauth implements Provider<GenericInterceptor<InvocationContext>> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GenericInterceptor<InvocationContext> can be shorten as Interceptor.

this.bind('passport-facebook').toProvider(FacebookOauth);
this.bind('passport-google').toProvider(GoogleOauth);
this.bind('passport-oauth2').toProvider(CustomOauth2);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can compose 5 interceptors into one (depending on #5343)

import {composeInterceptors} from '@loopback/context';
const authInterceptor = composeInterceptors(
  'passport-init-mw', // or use `toInterceptor(passport.initialize())` directly without binding it
  'passport-session-mw', // or use `toInterceptor(passport.session())` 
  'passport-facebook', 
  'passport-google', 
  'passport-oauth2');

// ...
@intercept(authInterceptor)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perfect, thank you

@deepakrkris deepakrkris force-pushed the passport-login-interceptor branch 5 times, most recently from 0f3f486 to baafe2c Compare May 6, 2020 00:50
"1": "{\"name\":\"Deepak Rajamohan\",\"username\":\"deepak.r.kris@gmail.com\",\"email\":\"deepak.r.kris@gmail.com\",\"id\":1}"
},
"UserIdentity": {
"3463934540288186": "{\"id\":\"3463934540288186\",\"provider\":\"facebook\",\"profile\":{\"emails\":[{\"value\":\"deepak.r.kris@gmail.com\"}]},\"authScheme\":\"facebook\",\"created\":\"2020-05-06T00:48:12.355Z\",\"userId\":1}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember that we had a discussion not to include db.json as the data change over time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I did include in the git ignore, not sure how this gets included every time, I will fix that as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i just removed the file altogether and its reference

* 'passport-google',
* 'passport-oauth2'
*/
export function OAuth2InterceptExpressMiddleware() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the name can be simpler as oauth2. This way, the decorator will be @oauth2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, but I have to simplify as well as be clear this is via interceptors middleware chain

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try simplifying

@deepakrkris deepakrkris force-pushed the passport-login-interceptor branch 2 times, most recently from 14482a7 to 9ca3f83 Compare May 6, 2020 01:13
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍 Awesome improvement! LGTM and please make sure tests pass.

// if provider name is given then use this middleware otherwise pass along
if (request.query['oauth2-provider-name'] === 'oauth2') {
return toInterceptor(passport.authenticate('oauth2'))(
return toInterceptor(passport.authenticate('facebook'))(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this function out as we don't want to call passport.authenticate multiple times.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the code below, we probably should use return next(); instead of just next();.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

@deepakrkris deepakrkris force-pushed the passport-login-interceptor branch 2 times, most recently from b5ae95d to 1ba52ea Compare May 7, 2020 01:37
@raymondfeng
Copy link
Contributor

@deepakrkris Please run npm run prettier:fix to fix the issue - https://travis-ci.com/github/strongloop/loopback-next/jobs/329111814

@@ -0,0 +1,38 @@
// Copyright IBM Corp. 2020. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: we typically name the file as facebook.interceptor.ts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

ExpressRequestHandler,
} from '@loopback/rest';

export class FacebookOauthInterceptor implements Provider<Interceptor> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's mark it with @bind({scope: BindingScope.SINGLETON}).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

Copy link
Contributor

@raymondfeng raymondfeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment.

this.bind('passport-facebook').toProvider(FacebookOauthInterceptor);
this.bind('passport-google').toProvider(GoogleOauthInterceptor);
this.bind('passport-oauth2').toProvider(CustomOauth2Interceptor);
this.bind('set-session-user').toProvider(SessionAuth);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to make these interceptor providers singleton too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

@deepakrkris deepakrkris force-pushed the passport-login-interceptor branch from f81a1e3 to 5402e37 Compare May 7, 2020 07:25
@deepakrkris deepakrkris merged commit b5c5ab7 into master May 7, 2020
@deepakrkris deepakrkris deleted the passport-login-interceptor branch May 7, 2020 15:54
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

Successfully merging this pull request may close these issues.

5 participants