add TokenHasMethodScopeAlternative

[n2ygk]

These add a couple more permission classes for scope matching. TokenHasMethodScope allows specifying required scope(s) on a per-method basis (in the case where READ_SCOPE and WRITE_SCOPE are not granular enough) and TokenHasMethodPathScope adds regex path matching and alternative lists of required scopes for a given regex match which is aligned with OpenAPI Spec (OAS) security requirement object list of alternative matching:

"When a list of Security Requirement Objects is defined on the Open API object or Operation Object, only one of Security Requirement Objects in the list needs to be satisfied to authorize the request."

[coveralls]

Coverage decreased (-0.04%) to 93.076% when pulling 9709ea9 on n2ygk:multiple_scopes_alternatives into c2ca9cc on evonove:master.

[n2ygk]

This is embarrassing.

FYI, when I run tox locally it's throwing this flake8 error:

ERROR:   flake8: Error creating virtualenv. Note that some special characters (e.g. ':' and unicode symbols) in paths are not supported by virtualenv. Error details: InvocationError('/Users/alan/src/django-oauth-toolkit/env/bin/python3 -m virtualenv --python /Users/alan/src/django-oauth-toolkit/env/bin/python3 flake8 (see /Users/alan/src/django-oauth-toolkit/.tox/flake8/log/flake8-0.log)', 100)

but when I run flake8 manually it seems to work OK. No excuse for forgetting to run flake8 manually.

[jleclanche]

Mmh, looks good. Second opinions?

[n2ygk]

@jleclanche I'm thinking about adding another permission class named something like IsRegisteredClient which calls Oauth2Validator.validate_client_id.

In the case of using the built-in oauth2 provider this validation is implicit. For an external oauth2 provider, this would confirm that the client app is "trusted" by this resource server which involves registering the client application.

My example use case is server-to-server trust using Client Credentials grant. This is analogous to the non-OAuth way of using Basic Auth in which each server has a registry of clients it trusts. It probably applies to Authorization Code grants as well.... Just because a client is registered with AS doesn't mean it should be permitted to talk to every RS that can introspect from the same AS.

Does this make sense?

@symptog I'm thinking of changing oauth2_validators._get_token_from_authentication_server to change application=None to do a lookup of the application by client_id. Is that reasonable?

[n2ygk]

@jleclanche @symptog pinging you for thoughts on proposed IsRegisteredClient.

[jleclanche]

I've never encountered this use case myself so it's hard for me to make a judgement on it. I'd really like to be able to get opinions from other folks but I'm not sure anyone's listening.

[n2ygk]

@jleclanche on further consideration, I think the best way to do this is to use a resource server-specific scope that only "approved" clients can use. Similar to how "introspection" scope is currently used in oauth2_provider. So ignore my ramblings;-)

[n2ygk]

Regarding TokenHasMethodPathScope do you think that makes sense or would it make more sense to build this into a url router, using TokenHasMethodScope? A URL router that magically adds the RelationshipView would be a cool thing....

[symptog]

@symptog I'm thinking of changing oauth2_validators._get_token_from_authentication_server to change application=None to do a lookup of the application by client_id. Is that reasonable?

My thoughts were that the AS knows about the applications, clients and scopes and the RS only validates the token towards the AS and so the RS don't need to know anything else except if the token is valid for the requested resource.

Towards your commit:
The TokenHasMethodPathScope doesn't open to me. As you usually map URLs to view functions/classes, adding required scopes to those functions/methods is more intuitive and more clear.

[n2ygk]

@jleclanche @symptog I've refactored to what I hope makes more sense.

[n2ygk]

@jleclanche just a reminder that this looks like it's ready to merge. @symptog's thumbs-up sounds like a second opinion;-)

[jleclanche]

Sorry for the delay. This will land after 1.1.0. Could you rebase against master please?

[coveralls]

Pull Request Test Coverage Report for Build 899

• 0 of 0 (NaN%) changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 92.948%

---

Totals
Change from base Build 895:	0.1%
Covered Lines:	1173
Relevant Lines:	1262

---

💛 - Coveralls

[n2ygk]

@jleclanche Sorry for the multiple attempts, but ready for merge now, including some test coverage improvements to pre-existing code, leaving the campground cleaner than I found it;-)

[n2ygk]

@jleclanche @symptog OK to merge?

[jleclanche]

Can you fixup&rebase it please? I've been pretty busy but I'll try to do a final review asap.

[n2ygk]

Already did rebase it (merged master into my branch as there were too many conflicts to do a clean rebase).

…

On Sat, May 5, 2018 at 4:04 PM, Jerome Leclanche ***@***.***> wrote: Can you fixup&rebase it please? I've been pretty busy but I'll try to do a final review asap. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#563 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEJ5d229VI_tlE3Hdi28znGoael7UJzTks5tvgXdgaJpZM4SZPmy> .

[jleclanche]

That's messing with things. I can't merge something with conflicts either way; if it's a problem I recommend cherry-picking and re-applying the conflicting parts manually.

[n2ygk]

I wasn’t aware there were still conflicts. Maybe some more recent commits? I’ll take another look but thought it was clean and mergeable.

…

On Sat, May 5, 2018 at 5:48 PM Jerome Leclanche ***@***.***> wrote: That's messing with things. I can't merge something with conflicts either way; if it's a problem I recommend cherry-picking and re-applying the conflicting parts manually. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#563 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEJ5d2r6qJxIKDFB1-39V4pqyYlEaLaqks5tvh5HgaJpZM4SZPmy> .

[n2ygk]

@jleclanche I'm looking at a screen that shows three green check-marks:
[x] Review requested
[x] All checks have passed
[x] This branch has no conflicts with the base branch
and, I assume since I've recently joined Jazzband, "Merge pull request" is available for me.

Where are you seeing conflicts?

[n2ygk]

@jleclanche said:

That's messing with things. I can't merge something with conflicts either way; if it's a problem I recommend cherry-picking and re-applying the conflicting parts manually.

I did exactly this. test code was significantly changed and I had to reapply my changes manually.

[jleclanche]

Sorry again for the delays. I've merged this now. I'm not super happy with the naming but we can change that before 1.2.0 release if needs be.

[n2ygk]

@jleclanche Thanks! Agree about the name. It's too long. Maybe drop "Alternative" from the end? Use OAS nomenclature and call it TokenMeetsSecurityRequirements or TokenMeetsOpenApiRequirements? Ugh.

[jleclanche]

TokenMatchesOASRequirements or something similar sounds good.