Add support for scoped access tokens

[bajtos]

Description

Define a new property AccessToken.scopes to contain the list of scopes granted to this access token.

Define a new remote method metadata accessScope to contain a single scope name required by this method.

Define a new remote method metadata accessScopes to contain a list of scope names required by this method.

Modify the authorization algorithm to ensure that at least one of the required scope is allowed by the scopes granted to the requesting access token.

To preserve backwards compatibility, remote methods without accessScope metadata can be invoked only by access tokens with undefined scopes property.

To preserve backwards compatibility:

• a built-in scope DEFAULT is defined
• remote methods with no accessScopes metadata are treated as accessScopes: ['DEFAULT']
• access tokens with no scopes property are treated as scopes: ['DEFAULT']

Impact on existing applications:

• Database schema must be updated after upgrading the loopback version

• If the application was already using a custom AccessToken.scopes
  property with a type different from an array, then the relevant code
  must be updated to work with the new type "array of strings".

This pull request is an incremental step towards #382 where we want to scope password-reset tokens to allow only password reset and no other operation.

cc @strongloop/sq-lb-epitome

Related issues

• connect to Tighten security for resetting password #382
• depends on Preserve custom remoting metadata (shared methods) strong-remoting#404

Checklist

• New tests added or existing tests modified to cover all changes
• Code conforms with the style
  guide

[bajtos]

Note: The implementation here does not take into account the built-in model Scope, because I was not able to find any documentation explaining how to setup such scopes to restrict which token can invoke which remote method.

[bajtos]

Question to consider during review:

• Is the impact on existing applications acceptable? If not, then I'll have to introduce a feature flag to preserve backwards compatibility. Besides the extra complexity, such feature flag will leave applications insecure until they opt-in.

• Is the user-facing API (accessScope property on shared methods, scopes as an array of strings) flexible enough to allow us to evolve this initial implementation closer towards OAuth2 scopes, while preserving backwards compatibility?

Please note that my intention is to get a minimal solution for scopes that would allow me to create access tokens scoped to a single password-setter function. Anything beyond that is out of scope of this pull request.

[ritch]

Nothing for setting scopes?

I like this implementation. I don't have any issues. Although I'm a bit apprehensive to move forward without docs on such a big feature.

[bajtos]

@ritch thank you for chiming in.

Nothing for setting scopes?

For the purpose of password reset, the scope will be set when creating the password-reset token - see User.resetPassword and User.prototype.createAccessToken that eventually calls

    this.accessTokens.create({
      ttl: ttl,
    }, cb);

I'm a bit apprehensive to move forward without docs on such a big feature.

I see your point. To be honest, I didn't really consider how to use this feature outside of LoopBack core.

Do you have any use case in mind where access-token scopes would be useful? (Other than OAuth2.)

Is this perhaps an indication that we should use a different mechanism for scoping down the password-reset token, i.e. do not treat the password-reset token as a regular access token, but treat it as a new domain entity, similarly how we treat email confirmation token now?

[ebarault]

Do you have any use case in mind where access-token scopes would be useful? (Other than OAuth2.)

It's a very powerful toolbox for fine grain access control if this feature lands in user domain (by opposition to keeping in internal to loopback) as it enables to create multiple access tokens for a same USER principal with different rights without adding/revoking roles.

I wonder if scopes as an embedded array will prevent some future evolutions compared to using an hasMany relations to a scope model.

@bajtos : have you checked in the OAuth2 standard if there are any conceptual incompatibilities with the proposed implementation we could not easily move away later on?

[raymondfeng]

@bajtos Your PR is a good starting point.

The enhanced access token model is already available at https://github.com/strongloop/loopback-component-oauth2/blob/master/common/models/oauth-token.json as part of oAuth2. If we align the base user/accessToken model with oAuth2 password flow, let's make sure they are consistent.

For accessScope, usually a method might allow one of a set of scopes. Should it be string[] instead?

[superkhau]

missing one %s (or %j)?

[bajtos]

Thank you all for your comments, this is a great discussion!

Here is an excerpt from OAuth2 specification describing the usage of AccessToken scopes

https://tools.ietf.org/html/rfc6749#section-3.3

The authorization and token endpoints allow the client to specify the
scope of the access request using the "scope" request parameter. In
turn, the authorization server uses the "scope" response parameter to
inform the client of the scope of the access token issued.

The value of the scope parameter is expressed as a list of space-
delimited, case-sensitive strings. The strings are defined by the
authorization server. If the value contains multiple space-delimited
strings, their order does not matter, and each string adds an
additional access range to the requested scope.

Should we use the same format (a single string containing a space-delimited list of scopes) too?

On the other hand, our OAuth2AccessToken model uses an array of strings, as can be seen in loopback-component-oauth2:common/models/oauth-token.json#L29

@ebarault

I wonder if scopes as an embedded array will prevent some future evolutions compared to using an hasMany relations to a scope model.

I am pretty sure we cannot use hasMany scopes relation, because that's 1:n relation, where the scope object contains id of the AccessToken it belongs to. I suppose we could use hasAndBelongsToMany relation where the pairs of (accessTokenId, scopeId) are stored in a new model/table, but it seems to me like an overkill.

have you checked in the OAuth2 standard if there are any conceptual incompatibilities with the proposed implementation we could not easily move away later on?
If we align the base user/accessToken model with oAuth2 password flow, let's make sure they are consistent.

👍

AFAICT, the proposal in this patch should be consistent with oAuth2 flow as I understand it. @raymondfeng you are much more familiar with oAuth2 flow, do you see any potential inconsistency?

For accessScope, usually a method might allow one of a set of scopes. Should it be string[] instead?

I'll trust your judgment here.

What is the scope check algorithm when both the access token and the protected method/resource define multiple scopes?

If I understand the oAuth2 quote above correctly, then my expectation is that the request is allowed as long as there is at least one scope in the access token that is also in the list defined by the protected method/resource. For example:

// method scopes
accessScopes: ['write:user', 'write:user:password']

// token scopes
scopes: ['write:user', 'read:products', 'read:categories']

// -> request is allowed

[bajtos]

For accessScope, usually a method might allow one of a set of scopes. Should it be string[] instead?

I think the more important question to answer is how to handle scopes that are a superset of another scope. Examples:

https://developer.github.com/v3/oauth/#scopes

user - Grants read/write access to profile info only. Note that this scope includes user:email and user:follow.
user:email - Grants read access to a user's email addresses.
user:follow - Grants access to follow or unfollow other users.

https://brandur.org/oauth-scope

read: Read access to all a user’s apps and their subresources, except for protected subresources like config vars and releases.
write: Write access to apps and unprotected subresources. Superset of read.
read-protected: Read including protected subresources. Superset of read.
write-protected: Write including protected subresources. Superset of read-protected and write.

If we allow remote methods to specify multiple scopes, then a poor-man's solution for superset/included scopes is to always specify all scopes which are allowed for the method, i.e. duplicate the superset/include relations in every shared method. While that doesn't like a good solution to me, perhaps it's a sign that as long as methods can define multiple scopes, then an incremental backwards-compatible solution for supserset/included scopes can be implemented later, to keep this initial pull request small and focused.

// An example to illustrate my poor-man's solution
acessScopes: ['user', 'user:email']

Thoughts?

[raymondfeng]

If I understand the oAuth2 quote above correctly, then my expectation is that the request is allowed as long as there is at least one scope in the access token that is also in the list defined by the protected method/resource.

Yes.

The only missing piece against the oAuth2 access token is the optional belongsTo relation to Application model, which represents the client application.

[bajtos]

The only missing piece against the oAuth2 access token is the optional belongsTo relation to Application model, which represents the client application.

Great! I think this relation can be implemented in the future as an incremental addition, let's leave it out of scope of this pull request.

[ebarault]

I think the more important question to answer is how to handle scopes that are a superset of another scope.

Does the standard precise anything about that or it is left to implementation details?
In the absence of any strong directive, i upvote your proposal for a simple solution.

Should we make the function for access scopes hierarchy resolving overridable from the start, so that if someone wants to have a policy like "a parent includes any parent:child" he can easily implement it?

using hasAndBelongsToMany relation AccessTokens<>scopes

yes that'll be overkill

[bajtos]

Does the standard precise anything about that or it is left to implementation details?

AFAICT, the standard does not say anything about this.

Should we make the function for access scopes hierarchy resolving overridable from the start, so that if someone wants to have a policy like "a parent includes any parent:child" he can easily implement it?

I prefer to leave that out of scope of this initial pull request to stay focused on fixing the password-reset flow.

[bajtos]

coverage/coveralls — Coverage decreased (-0.02%) to 89.305%

I believe this is caused by the new lines added to AccessContext.prototype.debug, which is invoked only when debug logs are enabled via DEBUG env var. Nothing to worry about IMO.

[bajtos]

I pushed few new commits, I think all comments should be addressed now.

While working on the documentation (see loopbackio/loopback.io#338), I discovered one scenario that I consider important to cover: how to allow models to add new scopes to built-in methods contributed e.g. by a database datasource (PersistedModel). In 6f9ae2a, I added code to read Model.settings.accessScopes and add any scopes defined in that object to scopes already defined at shared-method level.

@raymondfeng @ritch @superkhau @ebarault I consider this pull request as done and ready for your final review. I'll squash the commits after I get your LGTMs.

[ebarault]

"The access scopes that can authorize access to the invoked resource"

• one missing s
• otherwise we can read that all the scopes are required to authorize access to the resource

[ebarault]

authorizedScopes is more appropriate

[bajtos]

I see your point. I find authorizedScopes confusing, because it may also mean scopes the access token is authorized to access. I am going to use resourceScopes names, inspired by #3313 (comment)

[ebarault]

@bajtos : So, you removed the definition of scopes at model level ? that's too bad, i believe it was very close to be ok.

I went through your PR, and i believe this can evolve in backward compatible ways towards the vision we've built in this discussion.

My main concern is the following
People will generally implement scopes at model json definition level, as they do for ACLs. I am not expecting them to define them at method levels, and then navigating from ACLs to scopes defined in each method.

Plus, having 2 different places to define scopes we will have to handle considerations as: "how do we merge scopes defined at method level with scopes defined model level?" and "how to explain this in a way that does not confuse developers ?"

I'd have only implemented (or at least promoted) the definition of scopes at model level to keep it as simple as possible. It will make even more sense to define them only there when we will evolve the ACLs to reference scopes.

I left 2 cosmetic comments in the PR.

[raymondfeng]

@ebarault I think you misunderstood my view a bit. Essentially, there are two perspectives for authorization scopes.

1. authorizedScopes in access token which represents the grant of limited permissions by the resource owner (user/system) to the client application. For example, to allow a mobile app to see my Facebook posts but not albums, the access token can be (app1, 'raymond', ['posts:read:']).

2. protectedResourceScopes at the app/model config level which represents a list of protected resources, for example, posts:read --> ['Post.find', 'Post.findById']. It maps named permissions to access of protected resources. We can even think Post.find as a fine protected resource scope which only contains Post.find method access.

ACL controls if a principal (user/app/role) can access a protected resource (scope/method/property). For example:

• ('role:$owner', scopes: ['posts:read'], 'allow')
• ('role:$owner', property: ['Post.find', 'Post.findById'], 'allow') (The same as above)

Now let's look at two requests for GET /api/users/raymond/posts?access_token=...:
The resolved method for the REST call is Post.find.

1. AccessToken is ('app1', 'raymond', ['posts:read'])

• If Post.find is not tagged with any resource scopes, we'll only apply the property based ACL. In this case, it will be allowed as the access token has raymond maps to $owner role.
• If Post.find is tagged as part of posts:read scope, we'll first check the authorizedScopes from the access token, which is in ['posts:read']. We then apply ACL rules. In this case, it will be allowed as the access token has raymond which maps to $owner role.

2. AccessToken is ('app1', 'raymond', ['albums:read'])

• If Post.find is not tagged with any resource scopes, we'll only apply the property based ACL. In this case, it will be allowed as the access token has raymond. No scope check is involved here.
• If Post.find is tagged as part of posts:read scope, we'll first check the authorizedScopes from the access token, which is NOT in ['posts:read'], it will be denied even before we further check ACL rules.

Tagging a method to a protected resource scope not only allows ACL to define rules against the scope, but also enforce the scope matching between AccessToken and targeted protected resource scope. If a method is not tagged at all, we can treat it as part of DEFAULT scope and each access token implicitly has DEFAULT scope.

In summary, there will be two step check:

• Check if authorizedScopes from the access token is in the list of allowed protected resource scopes if tagged for the target method invocation.
• Check if established principal has authorization to perform the target method invocation

WDYT?

[ebarault]

@raymondfeng : prior i can respond, could you review the last part of you previous comment?
i think there's something wring with

If Post.find is tagged as part of posts:read scope, we'll first check the authorizedScopes from the access token, which is NOT in ['posts:read'], it will be denied by we further check ACL rules.

this is the essential part actually :-)

Also, regarding this part:

Tagging a method to a protected resource scope not only allows ACL to define rules against the scope but ...

I'm not sure how to take this with regard to

there will be two step check

which i understand as successive and cumulative steps (1 then 2, not 1 or 2)

if the two steps are successive and cumulative, then ACL rules precisely can't go against the method scope tagging. As per your example 2, even if a role is authorized a given method, if this method is tagged with only scopeA but the accessToken has scopeB, the request will be denied.

[raymondfeng]

@ebarault

it will be denied by we further check ACL rules.

This was a typo. Now I fixed it as it will be denied even before we further check ACL rules.

It's a two step process.

1 and 2. If 1 fails, we will deny without trying 2.

In my example, let's assume the ACL allows an owner to see posts and albums, but not delete a post (i.e., 3 scopes - posts:read, posts:delete, and albums:read).

• If the access token has posts:read scope for GET .../posts, the access to Post.find will be allowed during step 1 and step 2.
• If the access token has albums:read scope for GET .../posts, the access to Post.find will be denied during step 1.
• If the access token has posts:delete scope for DELETE .../posts, the access to Post.delete will be allowed during step 1 but denied in step 2.

[ebarault]

@raymondfeng : so i can confirm we now speak 99% the same language 👍

Only remaining subtlety : there's still something wrong for me with

Tagging a method to a protected resource scope not only allows ACL to define rules against the scope, but ...

Still IMO, with all this discussed, i can't see how an ACL could go against a scope as scope checkup denies in step 1.

---

Regarding the confusion: My intention was to use protectedResourceScopes as aliases for resources groups so that we can avoid duplicating ACLs like

• ('role:$owner', scopes: ['posts:read'], 'allow')
• ('role:$owner', property: ['Post.find', 'Post.findById'], 'allow')

when posts:read --> ['Post.find', 'Post.findById']

A resource group alias would allow to keep just one.

However i had a slightly different idea on how backward compatibility would work, which led me to reuse the protectedResourceScopes for this aliasing purpose.

Your proposal makes sense, so let's go !

[ebarault]

that being said: do you have any particular comment regarding my earlier comment?

settling this will allow finalizing this PR

[raymondfeng]

@ebarault I see resource group alias the same as protectedResourceScope. To me, it is a named permission group to a set of protected resources (such as Post.find and Post.findById). Please note a protected resource is (Model/Method/Property, READ/WRITE/EXECUTE). The concept is close to Java Permissions, such as:

• https://docs.oracle.com/javase/7/docs/api/java/io/FilePermission.html
• https://docs.oracle.com/javase/7/docs/api/java/net/SocketPermission.html

To declare protectedResourceScopes, I see multiple ways:

1. At app level - server/config.json
2. At 'model` level - server/model-config.json
3. At method/property level - tagging the method/property with an array of scope names

We can start with 3 as the PR implements and introduce 1 & 2.

[superkhau]

Code looks good, as for functionality, I don't have enough expertise in this area to give a good opinion. @raymondfeng's logic seems sound.

[drmikecrowe]

Are there plans for loopback-swagger in place, or may I submit a PR for that project that supports these changes?

[ebarault]

just left 2 cosmetic comments
i think you could have kept the code to define scopes at model level as it was close to be ok

[bajtos]

@drmikecrowe thank you for chiming in.

Are there plans for loopback-swagger in place, or may I submit a PR for that project that supports these changes?

I am not sure if this implementation of scopes is compatible with OAuth2 scopes enough to make sense to expose these scopes via Swagger. Could you please open an issue in loopback-swagger repository where we can discuss this proposal before you start writing code?

[bajtos]

I created #3339 for the follow-up discussion about defining scopes at app and model level.

[bajtos]

@ebarault

i think you could have kept the code to define scopes at model level as it was close to be ok

My concern is that "close to be ok" may take too long to get into state when it can be landed and I don't want to delay this pull request much longer.

However, I reviewed the removed commit 52ef73c and cherry-picked the preparation/refactoring changes back into this pull request - see fab857d.

I have also addressed your earlier two comments, I squashed those two fixes in the first commit, sorry for that.

Does the patch still LGTY now?

[ebarault]

I reviewed again and the code looks fine to me.

I suggest we add AccessToken.DEFAULT_SCOPE = 'DEFAULT' to avoid people having to manipulate directly the 'DEFAULT' string when creating access tokens

(or AccessToken.DEFAULT_SCOPE = AccessContext.DEFAULT)

Note: this can also be added in a future PR

[ebarault]

why 'DEFAULT' rather than the accurate ['DEFAULT'] ?

[bajtos]

Good catch, this should have been using DEFAULT_SCOPES for the token, and no || clause for context scopes (getScopes() should be already handling the default case). I'll fix this ASAP.

[bajtos]

I suggest we add AccessToken.DEFAULT_SCOPE = 'DEFAULT' to avoid people having to manipulate directly the 'DEFAULT' string when creating access tokens

I personally see little value in such aliases, it's just one or another string to me. However, I am happy to add that such alias to make the code/API easier to use for others.

[ebarault]

👏 🎉 @bajtos