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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spike more flexible user profile #3771

Closed
wants to merge 4 commits into from

Conversation

@jannyHou
Copy link
Contributor

jannyHou commented Sep 19, 2019

connect to #2246
Reviewers can read spike-user-profile.md to start.

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 jannyHou requested review from bajtos and raymondfeng as code owners Sep 19, 2019
@jannyHou jannyHou force-pushed the spike/user-profile-with-passport branch from 097f14f to d044af3 Sep 19, 2019
@jannyHou jannyHou force-pushed the passport/move-to-extension branch from 33d1474 to 2c98238 Sep 19, 2019
Copy link
Member

bajtos left a comment

I am not able to review the high-level design and how it fits into the wider context of our authentication architecture, I'll leave that up to others (@raymondfeng?).

I have few minor comments to consider, see below.

@jannyHou jannyHou force-pushed the passport/move-to-extension branch 3 times, most recently from b6b9d0a to 85fead5 Sep 23, 2019
@jannyHou jannyHou changed the base branch from passport/move-to-extension to master Sep 23, 2019
@jannyHou jannyHou force-pushed the spike/user-profile-with-passport branch 3 times, most recently from 1ce6cb7 to d03e1ca Sep 23, 2019
@@ -35,6 +35,10 @@ export interface AuthenticateFn {
(request: Request): Promise<UserProfile | undefined>;
}

export interface UserToUserProfileConverterFn<U> {

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Sep 23, 2019

Member

Maybe name it as UserProfileBuilder?

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 24, 2019

Member

+1 to find a better name.

Personally, I would use UserProfileFactory (see Factory pattern).

The name UserProfileBuilder suggest a well-known Builder pattern, which we are NOT following in this API.

See the corresponding change made in file 'authentication-with-passport-strategy-adapter.acceptance.ts':

- Type `UserProfileInDB` is defined to describe the custom user. In a real application it should be a custom User model.
- Define a converter function `converter` that turns an `UserProfileInDb` instance into a user profile. It's provided in the constructor when create the adapter.

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Sep 23, 2019

Member

UserProfileInDB?

Copy link
Member

bajtos left a comment

I don't have enough knowledge of our authentication layer to be able to review this proposal in full depth.

I don't see any obvious problem on the first sight.

However, I am missing a list of the proposed follow-up tasks (the implementation plan) in the spike document.

private readonly strategy: Strategy,
readonly name: string,
// The default converter returns an user as user profile
private userConverter: UserToUserProfileConverterFn<U> = (u: unknown) => {

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Sep 24, 2019

Member

This should be optional.

This comment has been minimized.

Copy link
@jannyHou

jannyHou Sep 24, 2019

Author Contributor

@raymondfeng a default converter is provided (return the user as it is) therefore the converter here is not marked as optional :)

See discussion in #3771 (comment)

jannyHou added 2 commits Sep 18, 2019
@jannyHou jannyHou force-pushed the spike/user-profile-with-passport branch 2 times, most recently from 124d669 to 344c1f1 Sep 26, 2019
Copy link
Contributor

agnes512 left a comment

馃憤 Good write up. I think I got the idea even I am not that familiar with this part of code base.

@jannyHou jannyHou changed the title Spike/user profile with passport Spike more flexible user profile Sep 27, 2019
`@loopback/authentication` to help developers inject the factory wherever it's
needed.
- Add `userProfileFactory` as in `StrategyAdapter`'s constructor.

This comment has been minimized.

Copy link
@emonddr

emonddr Sep 27, 2019

Contributor

Add userProfileFactory as in StrategyAdapter's constructor.

Add userProfileFactory in StrategyAdapter's constructor.

}
}

export const SAMPLE_USER_MIM_SET = {

This comment has been minimized.

Copy link
@emonddr

emonddr Sep 27, 2019

Contributor

SAMPLE_USER_MIM_SET

What's a MIM set? Did you mean MIN as in minimum?

This comment has been minimized.

Copy link
@jannyHou

jannyHou Sep 30, 2019

Author Contributor

oh good catch on the typo

@bajtos
bajtos approved these changes Sep 30, 2019
Copy link
Member

bajtos left a comment

All details LGTM. As I commented earlier, I don't have enough knowledge to review the big picture. Please get an approval from @raymondfeng and @emonddr before landing.

@jannyHou jannyHou force-pushed the spike/user-profile-with-passport branch from 344c1f1 to 8c1e9fe Sep 30, 2019
@@ -2,21 +2,77 @@

connect to story https://github.com/strongloop/loopback-next/issues/2246

I picked the `authentication-passport` module to start the spike for more flexible user profile because compared with the custom authentication strategies, users have less control to the returned user when using the passport adapter. I believe if we could find a solution for the passport based strategies, applying similar approach to a custom strategy would be easy.
I picked the `authentication-passport` module to start the spike for more

This comment has been minimized.

Copy link
@emonddr

emonddr Sep 30, 2019

Contributor

I picked the authentication-passport module to start the spike for a more flexible user profile because, compared with the custom authentication
strategies, users have less control to over the returned user when using the passport
adapter. I believe that if we could find a solution for the passport based
strategies, applying a similar approach to a custom strategy would be easy.


# Solution

A converter function is introduced to be passed into the `StrategyAdapter`'s constructor. It takes in a custom user, converts it to a user profile described by `UserProfile` then returns it.
A converter function is introduced to be passed into the `StrategyAdapter`'s

This comment has been minimized.

Copy link
@emonddr

emonddr Sep 30, 2019

Contributor

A converter function is introduced to be passed into the StrategyAdapter's

A converter function is introduced to be passed into the StrategyAdapter's


# Solution

A converter function is introduced to be passed into the `StrategyAdapter`'s constructor. It takes in a custom user, converts it to a user profile described by `UserProfile` then returns it.
A converter function is introduced to be passed into the `StrategyAdapter`'s
constructor. It takes in a custom user, converts it to a user profile described

This comment has been minimized.

Copy link
@emonddr

emonddr Sep 30, 2019

Contributor

It takes in a custom user, converts it to a user profile described by UserProfile , and then returns it.

instance into a user profile. It's provided in the constructor when creating
the adapter.
- The converter is invoked in the strategy's `authentication()` function to make
sure it returns a user profile in type `UserProfile`

This comment has been minimized.

Copy link
@emonddr

emonddr Sep 30, 2019

Contributor

sure it returns a user profile in of type UserProfile

- The app has a user model called `MyUser`, which has

- a property defined in `UserProfile` with same type

This comment has been minimized.

Copy link
@emonddr

emonddr Sep 30, 2019

Contributor

same type as what?

@jannyHou jannyHou referenced this pull request Sep 30, 2019
0 of 4 tasks complete
@jannyHou

This comment has been minimized.

Copy link
Contributor Author

jannyHou commented Sep 30, 2019

I am closing this pull request as the spike is done. Creating follow-up stories:

@jannyHou jannyHou closed this Sep 30, 2019
@jannyHou jannyHou referenced this pull request Sep 30, 2019
0 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can鈥檛 perform that action at this time.