-
-
Notifications
You must be signed in to change notification settings - Fork 792
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
add TokenHasMethodScopeAlternative #563
add TokenHasMethodScopeAlternative #563
Conversation
This is embarrassing. FYI, when I run tox locally it's throwing this flake8 error:
but when I run flake8 manually it seems to work OK. No excuse for forgetting to run flake8 manually. |
Mmh, looks good. Second opinions? |
@jleclanche I'm thinking about adding another permission class named something like 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 |
@jleclanche @symptog pinging you for thoughts on proposed IsRegisteredClient. |
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. |
@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;-) |
Regarding |
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: |
…on classes. Also improved one of the prior test cases to demonstrate that required scopes of more than one scope work.
@jleclanche @symptog I've refactored to what I hope makes more sense. |
@jleclanche just a reminder that this looks like it's ready to merge. @symptog's thumbs-up sounds like a second opinion;-) |
Sorry for the delay. This will land after 1.1.0. Could you rebase against master please? |
Pull Request Test Coverage Report for Build 899
💛 - Coveralls |
@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;-) |
@jleclanche @symptog OK to merge? |
Can you fixup&rebase it please? I've been pretty busy but I'll try to do a final review asap. |
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>
.
|
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 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>
.
|
@jleclanche I'm looking at a screen that shows three green check-marks: Where are you seeing conflicts? |
@jleclanche said:
I did exactly this. test code was significantly changed and I had to reapply my changes manually. |
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. |
@jleclanche Thanks! Agree about the name. It's too long. Maybe drop "Alternative" from the end? Use OAS nomenclature and call it |
|
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) andTokenHasMethodPathScope
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: