Spike more flexible user profile #3771

jannyHou · 2019-09-19T15:44:59Z

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

bajtos

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.

packages/authentication/src/types.ts

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

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

raymondfeng · 2019-09-23T19:07:41Z

packages/authentication/src/types.ts

@@ -35,6 +35,10 @@ export interface AuthenticateFn {
  (request: Request): Promise<UserProfile | undefined>;
 }

+export interface UserToUserProfileConverterFn<U> {


Maybe name it as UserProfileBuilder?

+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.

raymondfeng · 2019-09-23T19:26:45Z

extensions/authentication-passport/spike-user-profile.md

+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.


UserProfileInDB?

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

bajtos

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.

raymondfeng · 2019-09-24T19:44:29Z

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

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


This should be optional.

@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)

agnes512

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

emonddr · 2019-09-27T14:59:37Z

extensions/authentication-passport/spike-user-profile.md

+  `@loopback/authentication` to help developers inject the factory wherever it's
+  needed.
+- Add `userProfileFactory` as in `StrategyAdapter`'s constructor.


Add userProfileFactory as in StrategyAdapter's constructor.

Add userProfileFactory in StrategyAdapter's constructor.

emonddr · 2019-09-27T15:12:23Z

packages/authentication/src/__tests__/fixtures/user-profile-spike/user.repository.ts

+  }
+}
+
+export const SAMPLE_USER_MIM_SET = {


SAMPLE_USER_MIM_SET

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

oh good catch on the typo

bajtos

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.

emonddr · 2019-09-30T15:46:55Z

extensions/authentication-passport/spike-user-profile.md

@@ -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


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.

emonddr · 2019-09-30T15:48:28Z

extensions/authentication-passport/spike-user-profile.md


 # 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


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

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

emonddr · 2019-09-30T15:49:27Z

extensions/authentication-passport/spike-user-profile.md


 # 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


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

emonddr · 2019-09-30T15:50:58Z

extensions/authentication-passport/spike-user-profile.md

+  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`


sure it returns a user profile in of type UserProfile

extensions/authentication-passport/spike-user-profile.md

emonddr · 2019-09-30T15:53:19Z

extensions/authentication-passport/spike-user-profile.md

+- The app has a user model called `MyUser`, which has
+
+  - a property defined in `UserProfile` with same type


same type as what?

jannyHou · 2019-09-30T22:11:16Z

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

jannyHou requested review from bajtos and raymondfeng as code owners September 19, 2019 15:44

jannyHou force-pushed the spike/user-profile-with-passport branch from 097f14f to d044af3 Compare September 19, 2019 15:47

jannyHou force-pushed the passport/move-to-extension branch from 33d1474 to 2c98238 Compare September 19, 2019 20:38

bajtos reviewed Sep 20, 2019

View reviewed changes

jannyHou force-pushed the passport/move-to-extension branch 3 times, most recently from b6b9d0a to 85fead5 Compare September 23, 2019 16:32

jannyHou changed the base branch from passport/move-to-extension to master September 23, 2019 18:36

jannyHou force-pushed the spike/user-profile-with-passport branch 3 times, most recently from 1ce6cb7 to d03e1ca Compare September 23, 2019 18:58

raymondfeng reviewed Sep 23, 2019

View reviewed changes

bajtos reviewed Sep 24, 2019

View reviewed changes

extensions/authentication-passport/src/strategy-adapter.ts Outdated Show resolved Hide resolved

bajtos reviewed Sep 24, 2019

View reviewed changes

raymondfeng reviewed Sep 24, 2019

View reviewed changes

jannyHou added 2 commits September 25, 2019 22:50

feat: spike for user profile converter

55fab7b

fixup!: apply feedback

0f60b7b

jannyHou force-pushed the spike/user-profile-with-passport branch 2 times, most recently from 124d669 to 344c1f1 Compare September 26, 2019 18:12

agnes512 reviewed Sep 27, 2019

View reviewed changes

jannyHou changed the title ~~Spike/user profile with passport~~ Spike more flexible user profile Sep 27, 2019

agnes512 approved these changes Sep 27, 2019

View reviewed changes

emonddr suggested changes Sep 27, 2019

View reviewed changes

bajtos approved these changes Sep 30, 2019

View reviewed changes

hacksparrow approved these changes Sep 30, 2019

View reviewed changes

feat(authentication): add user profile builder for custom strategy

8c1e9fe

jannyHou force-pushed the spike/user-profile-with-passport branch from 344c1f1 to 8c1e9fe Compare September 30, 2019 14:40

raymondfeng approved these changes Sep 30, 2019

View reviewed changes

emonddr suggested changes Sep 30, 2019

View reviewed changes

fixup!: apply feedback

61a8c1f

emonddr approved these changes Sep 30, 2019

View reviewed changes

jannyHou mentioned this pull request Sep 30, 2019

Add user profile factory for authentication modules #3846

Closed

4 tasks

jannyHou closed this Sep 30, 2019

jannyHou mentioned this pull request Sep 30, 2019

[Spike]Make model UserProfile more flexible #2246

Closed

6 tasks

jannyHou deleted the spike/user-profile-with-passport branch March 13, 2020 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spike more flexible user profile #3771

Spike more flexible user profile #3771

jannyHou commented Sep 19, 2019 •

edited

Loading

bajtos left a comment

raymondfeng Sep 23, 2019

bajtos Sep 24, 2019

raymondfeng Sep 23, 2019

bajtos left a comment

raymondfeng Sep 24, 2019

jannyHou Sep 24, 2019

agnes512 left a comment

emonddr Sep 27, 2019

emonddr Sep 27, 2019

jannyHou Sep 30, 2019

bajtos left a comment

emonddr Sep 30, 2019

emonddr Sep 30, 2019

emonddr Sep 30, 2019

emonddr Sep 30, 2019

emonddr Sep 30, 2019

jannyHou commented Sep 30, 2019

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

		- a property defined in `UserProfile` with same type

Spike more flexible user profile #3771

Spike more flexible user profile #3771

Conversation

jannyHou commented Sep 19, 2019 • edited Loading

Checklist

bajtos left a 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

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agnes512 left a 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

bajtos left a 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

jannyHou commented Sep 30, 2019

jannyHou commented Sep 19, 2019 •

edited

Loading