feat: extract local credentials into a new model by bajtos · Pull Request #385 · loopbackio/loopback4-example-shopping

bajtos · 2019-11-14T11:07:06Z

Introduce UserCredentials models to hold hashed passwords, add has-one relation from User to UserCredentials.

Rework authentication-related code to work with the new domain model.

See loopbackio/loopback-next#1996 for additional information.

I have to say the code in the example app is a bit messy. Some things are (unnecessarily) duplicated in multiple places, therefore my pull request had to touch quite few files to update all duplications. Considering our current priorities, I am leaving refactoring & cleanup out of scope of my PR.

emonddr

Great stuff. Just a few comments and questions. :)

packages/shopping/src/controllers/user.controller.ts

packages/shopping/src/repositories/user-credentials.repository.ts

jannyHou

@bajtos Thank you very much for creating the PR 👍 mostly LGTM, left a few comment.

jannyHou · 2019-11-14T18:07:24Z

packages/shopping/src/models/user-credentials.model.ts

+    required: true,
+    mongodb: {dataType: 'ObjectID'},
+  })
+  userId: string;


Hmm, why do we need an extra userId when id is already defined?

id is the primary key of UserCredentials model

userId is a foreign key referencing id of the user these credentials belong to

This is the classic HasOne/BelongsTo setup we use everywhere else in LB4.

It is possible to use User's id as the primary key of UserCredentials too, but we don't have first-class support for that in LoopBack yet.

Also note that depending on the password policy, we may want to store historical credentials and thus have more than on UserCredentials instance for a single user. Think of a password policy "the new value must be different than the last 3 values used".

packages/shopping/src/controllers/user.controller.ts

bajtos · 2019-11-15T07:56:56Z

@emonddr @jannyHou thank you for thoughtful comments. I have addressed some of them in 24ff52c and posted comments where I think no action is needed.

PTAL, LGTY now?

It makes me wonder, is there anything else we need to update as part of this change? E.g. documentation?

emonddr · 2019-11-15T15:05:07Z

@emonddr @jannyHou thank you for thoughtful comments. I have addressed some of them in 24ff52c and posted comments where I think no action is needed.

PTAL, LGTY now?

It makes me wonder, is there anything else we need to update as part of this change? E.g. documentation?

Please look at https://loopback.io/doc/en/lb4/Authentication-Tutorial.html , and double-check that any code examples shown or instructions to post or get a user match what you now have.

The Try it out section , https://loopback.io/doc/en/lb4/Authentication-Tutorial.html#try-it-out passes an id of "1" in the POST /users . This doesn't work in your PR now, please make sure a user is able to POST /users where id value is specified, and where id is not specified but autogenerated by mongo db.

emonddr

Adding a user with id specified fails. Please fix. thx. See my comment.

bajtos · 2019-11-15T15:23:34Z

The Try it out section , https://loopback.io/doc/en/lb4/Authentication-Tutorial.html#try-it-out passes an id of "1" in the POST /users . This doesn't work in your PR now, please make sure a user is able to POST /users where id value is specified, and where id is not specified but autogenerated by mongo db.

I find it very annoying that the documentation is depending on behavior that's not covered by automated tests :-/

I'll take a look and add the missing test.

bajtos · 2019-11-15T15:24:08Z

Thank you @emonddr for the pointers for documentation which I should check, this was very helpful 🙇

jannyHou · 2019-11-15T15:24:20Z

@bajtos Thank you,

It makes me wonder, is there anything else we need to update as part of this change? E.g. documentation?

You probably want to explain the UserCredential is separated from User as a single model in https://github.com/strongloop/loopback4-example-shopping#models

bajtos · 2019-11-25T08:32:21Z

Thank you all again for your feedback. I see the following major areas to improve:

Ensure we can provide a custom id when creating a new user, update docs and tests accordingly
Update "Authentication tutorial" to match the new example app

Here is what I did:

I followed all steps in Try it out
In Update Authentication Tutorial with UserCredentials loopback-next#4201, I changed the example data in "Try it out" section to use a valid ObjectID value instead of 1
In this pull request (e27afa7), I added a new test to verify that it's possible to create a new user using the payload shown in our docs
In Update Authentication Tutorial with UserCredentials loopback-next#4201, I updated the relevant parts of "Authentication Tutorial" to match the updates made by this pull request
Finally, in this pull request (e27afa7), I added UserCredentials model to README.

@emonddr @jannyHou LGTY now?

packages/shopping/src/controllers/user.controller.ts

emonddr · 2019-11-25T14:54:28Z

In loopbackio/loopback-next#4201, I changed the example data in "Try it out" section to use a valid ObjectID value instead of 1

As I mentioned in the PR referenced above, if we are not going to pass in an easy id like 1, 2, or 3, etc but an Object ID (which is harder for a person to think of), let's just avoid passing an id value when POST-ing a new user in the Try it Out section of the document.

emonddr

See my most recent comment. thx.

jannyHou

👏

Introduce `UserCredentials` models to hold hashed passwords, add has-one relation from `User` to `UserCredentials`. Rework authentication-related code to work with the new domain model. Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>

bajtos added Authentication feature labels Nov 14, 2019

bajtos requested review from deepakrkris, emonddr, jannyHou and raymondfeng November 14, 2019 11:07

bajtos self-assigned this Nov 14, 2019

bajtos mentioned this pull request Nov 14, 2019

Refactoring: Customer credentials loopbackio/loopback-next#1996

Closed

2 tasks

emonddr suggested changes Nov 14, 2019

View reviewed changes

packages/shopping/src/controllers/user.controller.ts Outdated Show resolved Hide resolved

packages/shopping/src/repositories/user-credentials.repository.ts Outdated Show resolved Hide resolved

jannyHou reviewed Nov 14, 2019

View reviewed changes

emonddr suggested changes Nov 15, 2019

View reviewed changes

raymondfeng approved these changes Nov 19, 2019

View reviewed changes

bajtos mentioned this pull request Nov 25, 2019

Update Authentication Tutorial with UserCredentials loopbackio/loopback-next#4201

Merged

3 tasks

jannyHou reviewed Nov 25, 2019

View reviewed changes

packages/shopping/src/controllers/user.controller.ts Show resolved Hide resolved

emonddr approved these changes Nov 25, 2019

View reviewed changes

jannyHou approved these changes Nov 25, 2019

View reviewed changes

bajtos force-pushed the feat/user-credentials branch from e27afa7 to 4a180bb Compare November 25, 2019 15:08

bajtos merged commit 6c3764c into master Nov 26, 2019

bajtos deleted the feat/user-credentials branch November 26, 2019 07:09

Conversation

bajtos commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emonddr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jannyHou left a comment

Choose a reason for hiding this comment

Uh oh!

jannyHou Nov 14, 2019

Choose a reason for hiding this comment

Uh oh!

bajtos Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bajtos commented Nov 15, 2019

Uh oh!

emonddr commented Nov 15, 2019

Uh oh!

emonddr left a comment

Choose a reason for hiding this comment

Uh oh!

bajtos commented Nov 15, 2019

Uh oh!

bajtos commented Nov 15, 2019

Uh oh!

jannyHou commented Nov 15, 2019

Uh oh!

bajtos commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

emonddr commented Nov 25, 2019

Uh oh!

emonddr left a comment

Choose a reason for hiding this comment

Uh oh!

jannyHou left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bajtos commented Nov 14, 2019 •

edited

Loading

bajtos commented Nov 25, 2019 •

edited

Loading