App specific scopes #395
App specific scopes #395
Conversation
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): |
There was a problem hiding this comment.
Should there also be a data migration that sets the scopes for existing applications?
There was a problem hiding this comment.
I don't believe so, if no custom scope is set, the default scopes will still be used.
|
I'm not sure if it makes sense to try to include this in the AuthorizationView, it usually doesn't work in the initial_data, because when you don't know the client_id, you can't do anything yet, |
|
Thanks so much for doing the work to rebase my PR! I've been extremely busy and ended up getting tied up too much to do the work that I felt might be necessary before vetting it as a PR to the original master branch, which is why it was just a PR against my own. It'd be great to see this make it in! |
|
I should add, just in case it's not obvious from the code: There's no mechanism built into this to handle granting an application access to the requested scopes. In my actual use of this code, I utilized the django admin for manually granting scopes to apps. I'd trigger an email for any app that requested scopes we considered "elevated", and I'd manually copy them over in the django admin. A cleaner and more universally appropriate UX for app creators to request specific scopes (and see which are considered "elevated") and for site admins to grant those scopes would be optimal. However, this PR is still sufficient for people to use as long as they understand that they need to roll their own solution for granting scopes (and tracking requests of said scopes!) |
… app-specific-scopes
- filter scopes in both the get and validate functions, so the skip_authorization apps also get filtered scopes
- added documentation
- added a filter scope method to the application model
- also filter the scopes in the the validate_scopes function in the provider
|
@JensTimmerman: I am also keen on this functionality. It seems that the problem with the Travis build is the use of |
| =================== | ||
| You can restrict each individual application to a separate subset of scopes. | ||
| Using the ``allowed_scopes = 'scope1 scope2'`` field on the ``Application`` model you can limit the scopes an application | ||
| can get. This is usefull if you trust an application owner only for e.g. read access and don't want |
|
@mkjpryor-stfc I'll look into fixing some more issues I encountered while working on this. But I'm awaiting some feedback from the original developers on this. |
|
@JensTimmerman: No worries. Let me know if there's anything I can do to help this along. |
| """ | ||
| get an application from a client_id, this caches the applications | ||
| """ | ||
| app = self._application_cache.get(client_id, get_application_model().objects.get(client_id=client_id)) |
There was a problem hiding this comment.
this doesn't actually cache it, it is still recreated every time
|
@JensTimmerman are you waiting on me for feedback? I'm the original author but I don't recall seeing any open questions... perhaps I missed it? or do you mean the original authors of django-oauth-toolkit? |
|
I mean the original authors of django-oauth-toolkit. |
| @@ -6,7 +6,7 @@ | |||
| from django.conf import settings | |||
| from django.core.urlresolvers import reverse | |||
| from django.db import models, transaction | |||
| from django.utils import timezone | |||
| from django.utils import timezone, six, six | |||
There was a problem hiding this comment.
You're importing six twice.
There was a problem hiding this comment.
Perhaps you should use import six to avoid issues if/when Django moves/removes six.
There was a problem hiding this comment.
Considering DOT already depends on six being available, it would make the most sense to just import six instead.
| @@ -216,6 +238,8 @@ def allow_scopes(self, scopes): | |||
| """ | |||
| if not scopes: | |||
| return True | |||
| if isinstance(scopes, six.string_types): | |||
There was a problem hiding this comment.
When is scopes not a string? Are there steps that can be taken to ensure that it is always a string?
There was a problem hiding this comment.
if it is not a string it has to be a list, so then there is no need to split it into a list.
There was a problem hiding this comment.
I get that. Looking more closely at the existing code and docstring, this method expects an iterable of strings. You are weakening that contract to allow for a single string. However, the only place where you are consuming this is in tests. This change seems unnecessary. To keep the contract clean, if you do use allow_scopes, I would ensure that any new usage sends an iterable of strings rather than a delimited string.
There was a problem hiding this comment.
well a string is in fact an iterable, using it by accident might show up weird errors, this is a bit more developer friendly in that it will do the thing that you wanted to happen when you pass a string. But you are also right that it might encourage the use of a single string and confuse more people when they see inconsitent usage of 'scopes'
| def test_get_application(self): | ||
| """Test the get_application method from the AuthorizationView""" | ||
| self.assertEqual(self.app_foo_1, AuthorizationView().get_application(self.app_foo_1.client_id)) | ||
| # test twice to test the cache |
There was a problem hiding this comment.
Unless you're checking DB query counts or calls to the cache, you aren't truly validating caching behavior.
| @@ -76,6 +77,12 @@ def setUp(self): | |||
| self.app_bar_1 = self._create_application('app bar_user 1', self.bar_user) | |||
| self.app_bar_2 = self._create_application('app bar_user 2', self.bar_user) | |||
|
|
|||
| def test_get_application(self): | |||
| """Test the get_application method from the AuthorizationView""" | |||
There was a problem hiding this comment.
It would be helpful if the docstring explained what about the method you are testing. For example, Verify the method returns the application corresponding to the given client ID.
| @@ -41,6 +41,77 @@ def test_allow_scopes(self): | |||
| self.assertTrue(access_token.allow_scopes(['write', 'read', 'read'])) | |||
| self.assertTrue(access_token.allow_scopes([])) | |||
| self.assertFalse(access_token.allow_scopes(['write', 'destroy'])) | |||
| self.assertTrue(access_token.allow_scopes('read write')) | |||
There was a problem hiding this comment.
This test is a great candidate for parameterization.
| self.assertFalse(access_token.allow_scopes('write destroy')) | ||
|
|
||
|
|
||
| def test_get_allowed_scopes_from_scopes(self): |
There was a problem hiding this comment.
This test is also a great candidate for parameterization.
There was a problem hiding this comment.
This test also needs a docstring, and, based on the length (60 lines!), should be split into multiple test cases.
|
|
||
| skip_authorization_completely = False | ||
| def getscopes(self): |
| """ | ||
| get an application from a client_id, this caches the applications | ||
| """ | ||
| if client_id not in self._application_cache: |
There was a problem hiding this comment.
Suggested alternative:
application = self._application_cache.get(client_id)
if not application:
application = get_application_model().objects.get(client_id=client_id)
self._application_cache[client_id] = application
return application
There was a problem hiding this comment.
I don't think that's better. But that aside this cache can be pulled into its own PR.
|
@synasius @Girbons @JensTimmerman what is needed to get this merged? |
|
@clintonb I'm still awaiting a reply from [email protected] |
|
This is definitely a feature we want to merge but I do not like the current implementation. I've been working recently on a feature that adds support for scopes backends. You can see the code here https://github.com/evonove/django-oauth-toolkit/compare/scopes-backend and I wrote about it on a blog post: https://evonove.it/blog/django-oauth-toolkit-0110-released/ At the moment scopes can be changed through settings, but there is no way to implement a custom flow to obtain scopes (for instance I'd like to determine the available scopes based on the groups to which the user belongs). I'd love to base the App-specific scope feature on this new API. What do you think about it? |
|
@synasius this is great! It meets the immediate need, and allows for extensions that are not immediately known. When can we have it!? :) |
|
I wish the distaste with my initial approach was voiced long ago, I would have gladly redone / reworked it in a better way... in any case, I hope it eventually gets added. |
|
@honestbleeps I'm helping evonove maintain the library now, I'll be working with @synasius to land an improved version of this. |
|
@synasius Great, I like that approach even more! Hope to see more of it soon 👍 |
|
@synasius Great suggestion, improved flexibility. What needs to be done to move it forward? |
|
@jleclanche how can we get this merged and released? |
|
Ok, since @synasius isn't replying, I'd like to suggest taking over his design from the scopes-backend branch. I really like the plan outlined above. The backwards-compatible default scopes backend is also really cool. I can't see at first glance what's missing from that commit, so I'll be testing it in a few days (I'm currently trying to release something else on the side so I'm unable to fully focus on this just yet). @clintonb, I'd love your help. If you can help testing the scopes-backend branch (which I have just rebased), verify what's left to implement, I can finish it off myself. |
|
@jleclanche 😅 😅 😅 |
|
@jleclanche If it helps I can say that the code in |
|
Excellent, good to know! In that case I'll play around with it, write some documentation and land it hopefully this week (if you're reading this in the future and I haven't, nudge me). Once it lands, I'm in favour of doing a 0.12 release. We can then look into builtin app-specific scopes functionality for a 1.0 release (Seeing that DOT is widely used, works well and we're already careful with backwards compatibility, I don't see much point in staying on 0.x much longer). |
👍
👍
Totally agree! We've been careful since 0.1 😄 |
|
Great! Sounds really good to me. |
|
Guess you can close it now and reference it later, after the |
|
Someone can upload an example of this beacause i am trying i cant doit . thanks |
|
Yes i managed to do it, working perfectly in production |
|
@tximpa91 - Could please share a sample snippet of the same. I'm not able to understand the flow of it. |
|
@tximpa91 - I've seen this scope backed. How do I connect it to oauth-toolkit? |
rebased code from honestbleeps on latest master.
fixes #216 #130
possibly also helps to resolve issues like #318