Skip to content

Conversation

@ebarault
Copy link
Contributor

@ebarault ebarault commented Mar 1, 2017

This is the PR for the documentation related to the feature introduced in strongloop/loopback#2971.

@crandmck, @beeman, @bajtos could you please take a look at this first draft?

I also performed a quick review of the Authentication, Authorization and Permissions section, with a few modifications. I will come back to this section later on in another PR, there are consistency issues to me, for example i don't quite figure out why this sub-section is included in the reviewed section.

@crandmck crandmck added the review label Mar 1, 2017
module.exports = function enableAuthentication(server) {
server.enableAuth();
};
```
Copy link
Member

Choose a reason for hiding this comment

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

This is usually not enough, one has to attach all authentication models (ACL, Role, etc.) to a datasource. In our project templates, this is done by listing these models in server/model-config.json. For users adding authentication to an empty-server project, I would recommend to let enableAuth to auto-attach any required models, so that server/model-config.json changes are not required.

server.enableAuth({ dataSource: 'db' });

Copy link
Contributor Author

@ebarault ebarault Mar 1, 2017

Choose a reason for hiding this comment

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

@bajtos : i updated the doc to let the users know about this feature anyhow


{% include note.html content="
Usually you don't need any extra configuration regarding built-in models `Role`, `RoleMapping`, and `ACL` which you can use without any customization. Just make sure they are declared in the `model-config.json` configuration file.
" %}
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

took care of merging


#### Access Control with a single user model

In the case your application leverages only one type of user extending the built-in `User` model (which should the case in a majority of configurations), you barely have no further configuration to do.
Copy link
Member

Choose a reason for hiding this comment

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

which should be the case (add be)


In order to allow the Loopback access control system to work with several models extending the built-in `User` model. The relations between the `users` models and the `AccessToken` should be modified to allow a single `AccessToken` model to host access tokens for multiple types of users while at the same time allowing each `user` models instances to be linked to their related access tokens in a non ambiguous way.

This is achieved by changing the **hasMany** relation from `User` to `AccessToken` and the **belongsTo** relation from `AccessToken` to `User` by their [polymorphic](Polymorphic-relations) equivalents, in which the `principalType` property is used as a **_discriminator_** to resolve which of the potential `user` model instance an 'accessToken' instance belongs to.
Copy link
Member

@bajtos bajtos Mar 1, 2017

Choose a reason for hiding this comment

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

[polymorphic](Polymorphic-relations)

I think this link won't work, I think the URL should include .html extension.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oddly enough, links do work either with or without the .html ext.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it works locally

Copy link
Member

Choose a reason for hiding this comment

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

Nice!

...
app.use(loopback.token({
model: app.models.customAccessToken
}));
Copy link
Member

@bajtos bajtos Mar 1, 2017

Choose a reason for hiding this comment

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

The idiomatic way is to configure middleware in server/middleware.json.

See https://github.com/strongloop/loopback-workspace/blob/fc4f761ed7b635cf18e091b6ad111c31c78880a9/templates/projects/empty-server/files/server/middleware.json#L34

{
  "auth": {
    "loopback#token": {
      "params": {
        "model": "customAccessToken"
      }
    }
  }
}

(I typed the example from my head, may not work out of the box.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bajtos I updated as per your snippet. I'd appreciate if you can link me to the exact reference, or assert this one is good

Copy link
Member

Choose a reason for hiding this comment

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

I think this doc page is the most relevant: https://docs.strongloop.com/display/public/LB/Defining+middleware

Copy link
Member

Choose a reason for hiding this comment

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

The new content showing loopback#token configuration looks good to me.

You must create your own custom model (named something other than \"User,\" for example \"Customer\" or \"Client\") that [extends the built-in User model](Extending-built-in-models.html) rather than use the built-in User model directly. The built-in User model provides a great deal of commonly-used functionality that you can use via your custom model.

LoopBack does not support multiple models based on the User model in a single application. That is, you cannot have more than one model derived from the built-in User model in a single app.
Since version 3, Loopback supports multiple models based on the User model in a single application. See [this section](...) on how to configure the application accordingly.
Copy link
Member

Choose a reason for hiding this comment

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

[this section](...)

Looks like a forgotten to-do :)

@bajtos bajtos requested a review from crandmck March 1, 2017 12:36
@bajtos
Copy link
Member

bajtos commented Mar 1, 2017

Great stuff, thank you @ebarault for this contribution!

I reviewed the content, the information looks mostly correct to me, see the few comments above.

@crandmck I'll leave the text formatting and grammar conventions for you to review.

@beeman
Copy link

beeman commented Mar 1, 2017

Looks good! I will give this a test later this week to see if I can get it up and running.

@ebarault
Copy link
Contributor Author

ebarault commented Mar 1, 2017

@crandmck, @bajtos : I just finished reviewing @bajtos comments

@crandmck
Copy link
Contributor

crandmck commented Mar 1, 2017

Thanks @ebarault I'd like a number of formatting/style/wording changes. Shall I just go ahead and make changes of this nature directly in the branch, rather than comment and ask you to edit?

@ebarault
Copy link
Contributor Author

ebarault commented Mar 1, 2017

@crandmck : yes sure go ahead, this will be way more efficient this way. Let me know when you need a review.

@crandmck
Copy link
Contributor

crandmck commented Mar 2, 2017

OK, I made a pass.

There is a lot here. Well done, @ebarault ! Please review my edits. I think it's mostly wording and formatting. There's enough information of a fairly specialized nature that this might ultimately deserve its own article, apart form the main "Authentication, authorization, and permissions." But that can always come later.

@bajtos I still think it needs a bit of work, but if you want to land it, I'm fine with just iterating on the published version.

One thing I noticed--really orthogonal to the rest of the PR, but still--"Authentication, authorization, and permissions" uses disableRemoteMethod(.., isStatic) instead of disableMethodByName([prototype.]...)per 3.0 RNs. IIUC, then this should change--either in this PR or we should log an issue.

@ebarault
Copy link
Contributor Author

ebarault commented Mar 2, 2017

@crandmck : thanks for the review. I'm good with your changes. 👍

There are a number of things that should be reviewed in the Auth... section, other PRs will be required. @bajtos i let you decide whether you'd like to include the changes regarding disableRemoteMethod in this one or not.

@bajtos
Copy link
Member

bajtos commented Mar 2, 2017

One thing I noticed--really orthogonal to the rest of the PR, but still--"Authentication, authorization, and permissions" uses disableRemoteMethod(.., isStatic) instead of disableMethodByName([prototype.]...)per 3.0 RNs. IIUC, then this should change--either in this PR or we should log an issue.

Oh, of course, please use disableRemoteMethodByName. I was typing the comments from memory and I am not used to the new API yet.

@ebarault
Copy link
Contributor Author

ebarault commented Mar 2, 2017

@bajtos : it's from the original text, not from your comments

You must create your own custom model (named something other than \"User,\" for example \"Customer\" or \"Client\") that [extends the built-in User model](Extending-built-in-models.html) rather than use the built-in User model directly. The built-in User model provides a great deal of commonly-used functionality that you can use via your custom model.

LoopBack does not support multiple models based on the User model in a single application. That is, you cannot have more than one model derived from the built-in User model in a single app.
Since version 3, Loopback supports multiple models based on the User model in a single application. See [this section](Authentication-authorization-and-permissions.html#access-control-with-multiple-user-models) on how to configure the application accordingly.
Copy link
Member

Choose a reason for hiding this comment

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

Should we spell out the specific LoopBack version where this feature was added?

- Since version 3, Loopback ...
+ Since version 3.3.0, Loopback ...

Also I think the correct spelling is LoopBack, not Loopback?

@bajtos
Copy link
Member

bajtos commented Mar 2, 2017

Let's keep disableRemoteMethod -> disableRemoteMethodByName out of scope of this pull request please, so that it can be landed sooner.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

The changes LGTM. I left one minor comment, it can be ignored or applied in a follow-up pull request.

@crandmck
Copy link
Contributor

crandmck commented Mar 2, 2017

I'll fix disableRemoteMethod -> disableRemoteMethodByName separately.

@crandmck
Copy link
Contributor

crandmck commented Mar 2, 2017

I made the last small changes @bajtos suggested. I'll merge this, and we can continue to improve it. As noted previously, at some point I may open another PR to move this into a separate article.

@crandmck crandmck merged commit fbcda78 into gh-pages Mar 2, 2017
@crandmck crandmck removed the review label Mar 2, 2017
@crandmck
Copy link
Contributor

crandmck commented Mar 2, 2017

Thanks again @ebarault! This is a great contribution.

@crandmck crandmck deleted the multiple-user-based-models branch March 2, 2017 22:10
@beeman
Copy link

beeman commented Mar 3, 2017

Great work @ebarault - I just implemented this in an example project and it works as expected: https://github.com/beeman/loopback-example-multiple-user-models

I might add some ACL's and a simple UI to demo the functionality a bit more.

Edit: that was a bit too fast. It seems that adding this line (https://github.com/beeman/loopback-example-multiple-user-models/blob/master/server/middleware.json#L37) that's noted in the docs causes an error message:

500 loopback.token() middleware requires a AccessToken model

Is there something wrong in the way I implemented it?

@bajtos
Copy link
Member

bajtos commented Mar 3, 2017

👏 thank you all! 👏

@ebarault
Copy link
Contributor Author

ebarault commented Mar 3, 2017

@beeman: thanks for providing this example 👍 .
I tested and I can confirm your setup is correct and there's a bug in the way the token middleware handles the auth.token config params. I'm opening a new PR to solve it. In the meantime you can continue testing by specifying the accessToken model within server.js or in a boot script.

cc: @bajtos

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants