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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: extract local credentials into a new model #385

Merged
merged 1 commit into from
Nov 26, 2019

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Nov 14, 2019

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.

Copy link
Contributor

@emonddr emonddr left a comment

Choose a reason for hiding this comment

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

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

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.

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

required: true,
mongodb: {dataType: 'ObjectID'},
})
userId: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

  • 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 Outdated Show resolved Hide resolved
@bajtos
Copy link
Member Author

bajtos commented Nov 15, 2019

@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
Copy link
Contributor

emonddr commented Nov 15, 2019

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

Copy link
Contributor

@emonddr emonddr left a comment

Choose a reason for hiding this comment

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

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

@bajtos
Copy link
Member Author

bajtos commented Nov 15, 2019

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
Copy link
Member Author

bajtos commented Nov 15, 2019

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

@jannyHou
Copy link
Contributor

@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
Copy link
Member Author

bajtos commented Nov 25, 2019

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:

@emonddr @jannyHou LGTY now?

@emonddr
Copy link
Contributor

emonddr commented Nov 25, 2019

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.

Copy link
Contributor

@emonddr emonddr 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 most recent comment. thx.

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.

👏

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š <[email protected]>
@bajtos bajtos force-pushed the feat/user-credentials branch from e27afa7 to 4a180bb Compare November 25, 2019 15:08
@bajtos bajtos merged commit 6c3764c into master Nov 26, 2019
@bajtos bajtos deleted the feat/user-credentials branch November 26, 2019 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants