-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
docs: migrate loopback-example-passport repo as lb4 example #4899
Conversation
45e0402
to
6a68ec4
Compare
175eb29
to
0d84b39
Compare
7b25fdf
to
8b1d786
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job, Deepak. I left questions and comments. Also, please make sure any comments from #4919 that also have relevance here are applied accordingly. thx.
@@ -0,0 +1,68 @@ | |||
// Copyright IBM Corp. 2019. All Rights Reserved. | |||
// Node module: @loopback/rest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've noticed in a few files the Node module
comment seems to have different values:
// Node module: @loopback/authentication or @loopback/rest
Please make them all the same value appropriate for this example. See other examples. thx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
* 4. provides state methods to the strategy instance | ||
* see: https://github.com/jaredhanson/passport | ||
*/ | ||
export class StrategyAdapter<U> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is another strategy adapter being defined here? you should use: https://github.com/strongloop/loopback-next/blob/master/extensions/authentication-passport/src/strategy-adapter.ts .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were you waiting for #4919 to land?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @emonddr yes exactly, I was waiting for 4919 to be landed, the changes will have to be published as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
symlinks with master package is enabled now, please see PR description for manual test steps
import {User} from '../models'; | ||
import {asSpecEnhancer, OASEnhancer} from '@loopback/openapi-v3'; | ||
|
||
const options: StrategyOption = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these real ids? Perhaps remove them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emonddr , yep I created a test app in facebook, it is not a security risk as of now. but I am going to remove it shortl
); | ||
} | ||
modifySpec( | ||
spec: import('@loopback/rest').OpenAPIObject, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is being done here exactly? What is being added to the app's open api spec? thx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not relevant, removed now.
) { | ||
const userProfile: UserProfile = mapFaceBookProfile(profile); | ||
this.userService | ||
.findOrCreateExternalUser(userProfile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is meant by external user? If you could clarify. thx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are creating a local profile on behalf of an external provider, added additional comments to clarify this now.
response.redirect(this.statusCode, this.targetLocation); | ||
} | ||
|
||
updateBindings(requestContext: RequestContext) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this as an empty method? Please clarify. thx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, removed
|
||
import {Entity, model, property} from '@loopback/repository'; | ||
|
||
@model({settings: {strict: false}}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are settings required in the @model decorator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link https://loopback.io/doc/en/lb4/Model.html#using-the-juggler-bridge shows 2 examples. One where no settings are placed in @model decorator, and one with . If you could elaborate. thx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still looking at this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the strict: false setting now
8b1d786
to
26b6d7c
Compare
// This file is licensed under the MIT License. | ||
// License text available at https://opensource.org/licenses/MIT | ||
|
||
import {Getter, inject, Provider, Setter} from '@loopback/context'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, wouldn't this action be provided by the @loopback/authentication
module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
// This file is licensed under the MIT License. | ||
// License text available at https://opensource.org/licenses/MIT | ||
|
||
import {UserProfileFactory} from '@loopback/authentication'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should leverage the adapter from https://github.com/strongloop/loopback-next/tree/master/extensions/authentication-passport
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, symlinks to master packages enabled now.
26b6d7c
to
df6288c
Compare
df6288c
to
038ae79
Compare
4dce345
to
0f5b8c3
Compare
@@ -0,0 +1,67 @@ | |||
{ | |||
"name": "passport-oauth2-login", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name should be something like @loopback/example-passport-oauth2
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
@@ -0,0 +1,29 @@ | |||
const application = require('./dist'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copyright/license header is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
@@ -0,0 +1,93 @@ | |||
// Copyright IBM Corp. 2014,2015. All Rights Reserved. | |||
// Node module: loopback-example-passport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The module name does not match package.json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
1c027c3
to
0c72bef
Compare
@deepakrkris , please see my comments from 4 to 5 days ago. Thanks. |
@emonddr the authentication endpoints are used in this example to create an authenticated user session for the express web application. This is similar to https://github.com/strongloop/loopback-example-passport . To authenticate the api endpoints we would need to add a different authentication strategy like 'jwt'. |
@emonddr thanks for testing this. I have fixed this now. |
agree that the menu is very naive, but I just copied most of the UI templates from https://github.com/strongloop/loopback-example-passport |
@emonddr the logout function is fixed now . I have also added acceptance test cases for these scenarios. |
@emonddr I have added details on facebook login in the README.md now. |
@deepakrkris , apologies. I don't follow. Isn't the whole purpose of this example to see how someone can use facebook or google login to get authenticated, and then get access to LB4 APIs? |
@emonddr the purpose is to authenticate with facebook/google, YES. But not to validate user access to other LB4 APIs, but to validate access to the express web application. In this case the path |
@deepakrkris , I don't see belongsTo decorator in UserIdentity or UserCredential but User has @hasone and @hasmany respectively. Also in latest commit, if you are using Also, perhaps rename : |
examples/passport-login/data/db.json
Outdated
"UserCredentials": 3 | ||
"User": 9, | ||
"UserIdentity": 104566694532676, | ||
"UserCredentials": 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for the this file to zero entries in User, UserIdentity, and UserCredentials, and have the "ids" have entries with value 1 or something. Right now the "ids".UserCredentials is 4, but looking in UserCredentials, the userId is 6. So it looks weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emonddr I have enabled clearing test data after every test run
export class PingController { | ||
constructor(@inject(RestBindings.Http.REQUEST) private req: Request) {} | ||
|
||
@authenticate('session') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User should always be able to visit /ping without authentication (even if logged in with an auth strategy)
Let's leave the /ping without the @authenticate
decorator
but have another endpoint in this controller like
@authenticate('session')
@get(/whoAmi )
which returns the currently logged in user.
What will the request headers show?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, /whoAmI
or /me
prints the user info, /ping
is usually public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, this is done now
@deepakrkris , thanks for addressing my comments yesterday. We're almost there. |
Hi @deepakrkris , so I spoke with @agnes512 . So it turns out you were correct. You don't need the belongsTo in UserCredential and UserIdentity if you don't plan on navigating from any of these back to the original User. If you only need to navigate from User to UserIdentity or User to UserCredentials, then you only need the hasMany and hasOne. If you need to navigate from parent to child and child to parent, then you need to use hasMany and belongsTo, or hasOne and belongsTo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deepakrkris Great effort!!! a few questions & suggestions(commented with details):
- the method in the ping controller looks good in general, but it's better to have it in the existing user controller and transfer the method to
/whoAmI
or/me
- renaming
UserIdentity
? - maybe more explanation about the the app/server structure
- data in db.json
I need more time to review each file in details but other than questions above I don't see obvious problem.
export interface UserIdentityService<I, U> { | ||
/** | ||
* find or create a local user using a profile from an external source | ||
* @param userIdentity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe write it more clear that userIdentity
here refers to the external identity.
): Promise<User> { | ||
let profile; | ||
try { | ||
profile = await this.userIdentityRepository.findById(userIdentity.id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Followed the conversation here, a suggestion for UserIdentity
: maybe rename it to something that clearly say it's an EXTERNAL identity?
Like ExternalIdentity
, ThirdPartyIdentity
? just to distinguish it with the local UserPorfile
.
Same for the newly added interface UserIdentity
in @loopback/authentication
.
WDYT? cc @deepakrkris @emonddr
Feel free to ignore it if it's only my own concern :) I understand that renaming takes huge refactor effort.
* a. LB4 application provides passport login services for the express app | ||
* b. The LB4 login apis are wrapped with session middleware to allow client sessions with user profiles | ||
*/ | ||
export class ExpressServer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused about the server+app structure here...so you setup an express server, and use the webApp
and lbApp
as two routers?
examples/passport-login/data/db.json
Outdated
"1003": "{\"id\":\"1003\",\"provider\":\"custom-oauth2\",\"profile\":{\"emails\":[{\"value\":\"[email protected]\"}]},\"authScheme\":\"custom-oauth2\",\"created\":\"2020-04-13T22:01:23.556Z\",\"userId\":6}", | ||
"104566694532668": "{\"id\":\"104566694532668\",\"provider\":\"facebook\",\"profile\":{\"emails\":[{\"value\":\"[email protected]\"}]},\"authScheme\":\"facebook\",\"created\":\"2020-04-13T23:18:46.875Z\",\"userId\":8}" | ||
}, | ||
"UserCredentials": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one User
should have one UserCredentials
, any reason the db here has 3 users but only 1 entry of credentials..?
export class PingController { | ||
constructor(@inject(RestBindings.Http.REQUEST) private req: Request) {} | ||
|
||
@authenticate('session') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, /whoAmI
or /me
prints the user info, /ping
is usually public.
Hey @jannyHou thanks for the review,
|
5d6de44
to
d501f45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great improvements since last time. thank you.
Please update the instructions on how to interact with sample. I have a added a suggested list. Also, I am still seeing a JSON object as a string on the Login screen after I sign up with facebook.
$ npm start | ||
``` | ||
|
||
- Open your browser to the example app with, `http://localhost:3000` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deepakrkris , thanks for making the UI changes and addressing my earlier comments.
Let's update this section with the latest UI changes in mind (some of the instructions are out of date.)
But also it is important to register a local user BEFORE any social media login, otherwise there will be issues.
- Open browser to localhost:3000
- Sign up as a local user with the email of a facebook test user you created above, and provide a password for local purposes. This registers a local user, and it will eventually be linked to social media credentials in the following steps.
- Login as this local user. You will be brought to the "View Account" tab ( and describe what they should see)
- Click on Sign Up or Login, and the pages indicate you've already logged in.
- Click on [Current User] button, and it calls https://localhost:3000/api/whoAmI endpoint and displays currently authenticated user.
- Click on Logout.
- Click on View Account, and you are not permitted since you are logged out.
- Look at the db.json and mention what they should see in there. User with specific email, and in UserCredentials, the password you enterered when you signed up.
- Now click on Login, and choose
Login with Facebook
. Enter the email of your facebook test user (same email you entered above), but specify the test user's facebook password (not local password). - Upon successful FaceBook login, you are brought to
View Account
. (Explain the data they should see) - Click on Sign Up or Login, and the pages indicate you are already logged in.
- Click on [Current User] to view the currently authenticated user.
- Click on Logout
- Click on
View Account
, and you are not allowed. - Look at the db.json, there is still one user. It still has the UserCredentials entry as earlier, But now there is an entry in the UserIdentity section which contains the facebook profile info.
At any time (authenticated or not), open http://localhost:3000/api/ping in the browser and you should see the ping info returned. This endpoint doesn't require authentication.
Please re-word my list above to your liking. But we need to hold the user's hand to explain our UI to have a sense of what the ui can do based on the endpoints we've secured. As well as the database info that helps tie the local and social stuff together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emonddr agreed, this is fixed now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deepakrkris , thanks for making the UI changes and addressing my earlier comments.
Let's update this section with the latest UI changes in mind (some of the instructions are out of date.)
But also it is important to register a local user BEFORE any social media login, otherwise there will be issues.
- Open browser to localhost:3000
- Sign up as a local user with the email of a facebook test user you created above, and provide a password for local purposes. This registers a local user, and it will eventually be linked to social media credentials in the following steps.
- Login as this local user. You will be brought to the "View Account" tab ( and describe what they should see)
- Click on Sign Up or Login, and the pages indicate you've already logged in.
- Click on [Current User] button, and it calls https://localhost:3000/api/whoAmI endpoint and displays currently authenticated user.
- Click on Logout.
- Click on View Account, and you are not permitted since you are logged out.
- Look at the db.json and mention what they should see in there. User with specific email, and in UserCredentials, the password you enterered when you signed up.
- Now click on Login, and choose
Login with Facebook
. Enter the email of your facebook test user (same email you entered above), but specify the test user's facebook password (not local password).- Upon successful FaceBook login, you are brought to
View Account
. (Explain the data they should see)- Click on Sign Up or Login, and the pages indicate you are already logged in.
- Click on [Current User] to view the currently authenticated user.
- Click on Logout
- Click on
View Account
, and you are not allowed.- Look at the db.json, there is still one user. It still has the UserCredentials entry as earlier, But now there is an entry in the UserIdentity section which contains the facebook profile info.
At any time (authenticated or not), open http://localhost:3000/api/ping in the browser and you should see the ping info returned. This endpoint doesn't require authentication.Please re-word my list above to your liking. But we need to hold the user's hand to explain our UI to have a sense of what the ui can do based on the endpoints we've secured. As well as the database info that helps tie the local and social stuff together.
@emonddr I have incorporated these steps as suitable for user readability. please review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor suggestions LGTM :)
I'll leave the UI related code review for Dom, thanks!
expect(url.parse(callbackToLbApp).query).to.containEql('code'); | ||
}); | ||
|
||
it('access code can be exchanged for token', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm how does the code and token exchange get tested in it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
authorization code exchange test is also done in acceptance tests of extension-passport
* map passport profile to user profile | ||
* @param user | ||
*/ | ||
mapProfile(user: User): UserProfile { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the mapProfile
functions are same in all strategies, you can extract them into utils.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
} | ||
|
||
@bind(asAuthStrategy) | ||
export class Oauth2AuthStrategy implements AuthenticationStrategy { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emonddr The auth provider should be decorated with @extensionPoint
, see example in https://loopback.io/doc/en/lb4/Loopback-component-authentication.html#registering-a-custom-authentication-strategy
The class here is just an auth strategy.
} | ||
|
||
@bind(asAuthStrategy) | ||
export class Oauth2AuthStrategy implements AuthenticationStrategy { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emonddr @deepakrkris See the comment in https://github.com/strongloop/loopback-next/blob/master/packages/core/src/extension-point.ts#L67-L71
The extension name can be provided directly by the 1st param in @extensions()
, @extensionPoint()
is another way to provide the name.
That's why Deepak's code works.
async authenticate( | ||
request: RequestWithSession, | ||
): Promise<UserProfile | RedirectRoute | undefined> { | ||
if (!request.user) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm why the session strategy doesn't retrieve the user from request's session but from request directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
done: (error: any, user?: any, info?: any) => void, | ||
) => void; | ||
|
||
export namespace PassportAuthenticationBindings { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please move the bindings into keys.ts
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
*/ | ||
loginToThirdParty( | ||
@param.path.string('provider') provider: string, | ||
@inject('authentication.redirect.url') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a reminder: doesn't have to apply in this PR, please create binding keys for the redirect url and status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you and please fix the lint error before merge: https://travis-ci.com/github/strongloop/loopback-next/jobs/320670018
And please also squash the commits and rename the first commit to be feat:
since it's a code contribution with over 5k lines 🎉 .
fda0515
to
24b5fec
Compare
satisfies: #2624
Test steps
** This example repo would need manual test
preconditions: create a test app and test user in facebook, and make sure to give
email
permission to test user. (emonddr : visit https://developers.facebook.com to do this.)preconditions: Copy test app's client id and secret and update those in the examples/passport-poauth2-login/authentication-strategies/oauth2-strategy.ts
Run
npm i
from loopback-next root folder, to create symlinks with master packagescd examples/passport-oauth2-login & npm start
open browser -> http://localhost:3000 (emonddr : Open a new window in Incognito mode, otherwise may not work)
click login -> login with facebook
page redirects to facebook, enter test user credentials
if authentication was successful, page redirects to login profiles page and shows linked facebook account details along with access token
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈