Review Request #9002 — Created June 8, 2017 and submitted
OAuth applications can now be created and managed via the
oauth-apps/
API endpoint. Application information is only available to (1) the user
who created the app, (2) site administrators, and (3) Local Site
administrators when the application is associated with a Local Site.
Ran unit tests.
| Description | From | Last Updated |
|---|---|---|
|
W293 blank line contains whitespace |
 reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W291 trailing whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
E303 too many blank lines (2) |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
E702 multiple statements on one line (semicolon) |
reviewbot
|
|
|
E305 expected 2 blank lines after class or function definition, found 1 |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
E303 too many blank lines (2) |
reviewbot
|
|
|
E303 too many blank lines (5) |
reviewbot
|
|
|
F811 redefinition of unused 'test_get_filtered' from line 77 |
reviewbot
|
|
|
E303 too many blank lines (2) |
reviewbot
|
|
|
): should be on the previous line. Also, docstring? |
 david
|
|
|
Module docstring? |
david
|
|
|
Swap these two lines. |
david
|
|
|
Swap these two lines. |
david
|
|
|
You can enforce the actual values by doing: 'type': (Application.GRANT_AUTHORIZATION_CODE, Application.GRANT_CLIENT_CREDENTIALS, Application.GRANT_IMPLICIT, Application.GRANT_PASSWORD) |
david
|
|
|
This can also use 'type': (Application.CLIENT_CONFIDENTIAL, Application.CLIENT_PUBLIC) |
david
|
|
|
Can this (and the other changes in this file) be rolled up into existing review requests? |
 chipx86
|
|
|
Needs a space before "stylesheet" |
chipx86
|
|
|
, optional |
chipx86
|
|
|
, optional |
chipx86
|
|
|
, optional |
chipx86
|
|
|
, optional |
chipx86
|
|
|
Can you put GRANT_PASSWORD on its own line? |
chipx86
|
|
|
I'd like to include more information on what this is, for those new to OAuth2. |
chipx86
|
|
|
Can you break the string after the newlines, so it reads more like paragraphs? |
chipx86
|
|
|
Missing a period. Should also include a bit more information on what this is. In general, the more explicit the … |
chipx86
|
|
|
Can you break the strings at newlines? |
chipx86
|
|
|
Can you break at the newlines? |
chipx86
|
|
|
Local Site |
chipx86
|
|
|
Local Site |
chipx86
|
|
|
Rather than having all this here, let's add permission checks to Application. There should be an is_accessible_by for this and … |
chipx86
|
|
|
Should use local_site.is_mutable_by(request.user). |
chipx86
|
|
|
This can fit on one line. |
chipx86
|
|
|
, optional |
chipx86
|
|
|
, optional |
chipx86
|
|
|
local_site.is_mutable_by(request.user) |
chipx86
|
|
|
This is already provided by the base class. |
chipx86
|
|
|
We need a @webapi_request_fields that documents username=. |
chipx86
|
|
|
These are already provided by the base class. |
chipx86
|
|
|
No blank line. |
chipx86
|
|
|
I don't really love the complexity of building a temp function that does its own caching. The logic should probably … |
chipx86
|
|
|
Can you put this in parens? (the superuser or ...). |
chipx86
|
|
|
local_site.is_mutable_by(request.user). |
chipx86
|
|
|
How about "You don't have permission to set this field."? We may very well want special permissions for modifying OAuth2 … |
chipx86
|
|
|
values_list would go on the next line. |
chipx86
|
|
|
We should default this to None up above, instead of setting it here and in the else: further down. |
chipx86
|
|
|
Let's remove the "on this LocalSite." RBCommons users are just going to ask questions about it. From their point of … |
chipx86
|
|
|
How about instead of this, we have a not_owner_status_code and not_owner_error, defaulting to 403 and PERMISSION_DENIED, respectively. Then it's a … |
chipx86
|
|
|
resources should go before tests. |
chipx86
|
|
|
Missing a docstring. |
chipx86
|
|
|
This sort of thing is repeated an aweful lot. How about giving _make_applications a filter argument? |
chipx86
|
|
|
Can we use keyword arguments for all these? It'll make it more self-documenting. |
chipx86
|
|
|
Missing a docstring. |
chipx86
|
|
|
No need for these blank lines. They're all setting common state for the tests. |
chipx86
|
|
|
Can you put a blank line before/after the api_get? It's helpful to have a block of setup code, a block … |
chipx86
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
F401 'djblets.webapi.errors.PERMISSION_DENIED' imported but unused |
reviewbot
|
|
|
E131 continuation line unaligned for hanging indent |
reviewbot
|
|
|
E131 continuation line unaligned for hanging indent |
reviewbot
|
|
|
F401 'djblets.webapi.errors.DOES_NOT_EXIST' imported but unused |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
Blank line between these two. |
david
|
|
|
i before s |
david
|
| reviewboard/webapi/resources/oauth_app.py (Diff revision 1) |
|---|
E702 multiple statements on one line (semicolon)
| reviewboard/webapi/resources/oauth_app.py (Diff revision 1) |
|---|
E305 expected 2 blank lines after class or function definition, found 1
whitespace
| reviewboard/webapi/tests/test_oauth_app.py (Diff revision 2) |
|---|
F811 redefinition of unused 'test_get_filtered' from line 77
flake8
Diff: |
Revision 3 (+1007 -10)
|
|---|
| reviewboard/testing/testcase.py (Diff revision 3) |
|---|
):should be on the previous line. Also, docstring?
| reviewboard/webapi/resources/oauth_app.py (Diff revision 3) |
|---|
You can enforce the actual values by doing:
'type': (Application.GRANT_AUTHORIZATION_CODE, Application.GRANT_CLIENT_CREDENTIALS, Application.GRANT_IMPLICIT, Application.GRANT_PASSWORD)
| reviewboard/webapi/resources/oauth_app.py (Diff revision 3) |
|---|
This can also use
'type': (Application.CLIENT_CONFIDENTIAL, Application.CLIENT_PUBLIC)
Diff: |
Revision 4 (+1038 -10)
|
|---|
Lock the skip_authorization field to admins. Cleanups. More tests.
Diff: |
Revision 5 (+1293 -10)
|
|---|
Diff: |
Revision 6 (+1294 -10)
|
|---|
Diff: |
Revision 7 (+1293 -9)
|
|---|
| reviewboard/oauth/forms.py (Diff revision 7) |
|---|
Can this (and the other changes in this file) be rolled up into existing review requests?
| reviewboard/webapi/resources/oauth_app.py (Diff revision 7) |
|---|
Can you put
GRANT_PASSWORDon its own line?
| reviewboard/webapi/resources/oauth_app.py (Diff revision 7) |
|---|
I'd like to include more information on what this is, for those new to OAuth2.
| reviewboard/webapi/resources/oauth_app.py (Diff revision 7) |
|---|
Can you break the string after the newlines, so it reads more like paragraphs?
| reviewboard/webapi/resources/oauth_app.py (Diff revision 7) |
|---|
Missing a period.
Should also include a bit more information on what this is. In general, the more explicit the descriptions/help text, the more time we save others and ourselves.
| reviewboard/webapi/resources/oauth_app.py (Diff revision 7) |
|---|
Rather than having all this here, let's add permission checks to
Application. There should be anis_accessible_byfor this andis_mutable_byfor the modify/delete checks (which are free to checkis_accessible_by).
| reviewboard/webapi/resources/oauth_app.py (Diff revision 7) |
|---|
Should use
local_site.is_mutable_by(request.user).
| reviewboard/webapi/resources/oauth_app.py (Diff revision 7) |
|---|
This is already provided by the base class.
| reviewboard/webapi/resources/oauth_app.py (Diff revision 7) |
|---|
We need a
@webapi_request_fieldsthat documentsusername=.
| reviewboard/webapi/resources/oauth_app.py (Diff revision 7) |
|---|
These are already provided by the base class.
| reviewboard/webapi/resources/oauth_app.py (Diff revision 7) |
|---|
I don't really love the complexity of building a temp function that does its own caching. The logic should probably change instead.
How about:
skip_authorization = parsed_request_fields.get('skip_authorization', False) is_owner = (username is not None and username != request.user.username) if skip_authorization or is_owner: # The user wants to set a fields only available to administrators. # Check for permission first. if not (request.user.is_authenticated() and (request.user.is_superuser or (...))): # The user doesn't have permission to modify these fields, so # return error messages for each of the fields they attempted to # modify. if skip_authorization: errors['skip_authorization'].append(...) if is_owner: errors['user'].append(...) else: # Start handling those fields. if is_owner: ...This gets rid of the need to build a temp function that caches on itself, and it separates out the permission checking from the rest of the logic.
The User lookups would go in the
else:. If the user doesn't have permission to set these fields, we shouldn't be giving them any information on whether the user they've set is valid. They should just get denied up-front.
| reviewboard/webapi/resources/oauth_app.py (Diff revision 7) |
|---|
Can you put this in parens? (the
superuser or ...).
| reviewboard/webapi/resources/oauth_app.py (Diff revision 7) |
|---|
How about "You don't have permission to set this field."? We may very well want special permissions for modifying OAuth2 entries for organizations without granting full admin privileges down the road.
| reviewboard/webapi/resources/oauth_app.py (Diff revision 7) |
|---|
We should default this to
Noneup above, instead of setting it here and in theelse:further down.
| reviewboard/webapi/resources/oauth_app.py (Diff revision 7) |
|---|
Let's remove the "on this LocalSite." RBCommons users are just going to ask questions about it. From their point of view, their team's users are the entire set of users.
| reviewboard/webapi/tests/mixins.py (Diff revision 7) |
|---|
How about instead of this, we have a
not_owner_status_codeandnot_owner_error, defaulting to403andPERMISSION_DENIED, respectively. Then it's a little more immediately clear what the purpose is, and it's not hard-coded for one specific use case.These should also have doc comments.
| reviewboard/webapi/tests/test_oauth_app.py (Diff revision 7) |
|---|
This sort of thing is repeated an aweful lot. How about giving
_make_applicationsafilterargument?
| reviewboard/webapi/tests/test_oauth_app.py (Diff revision 7) |
|---|
Can we use keyword arguments for all these? It'll make it more self-documenting.
| reviewboard/webapi/tests/test_oauth_app.py (Diff revision 7) |
|---|
No need for these blank lines. They're all setting common state for the tests.
| reviewboard/webapi/tests/test_oauth_app.py (Diff revision 7) |
|---|
Can you put a blank line before/after the
api_get?It's helpful to have a block of setup code, a block of test execution, and a block of assertions.
Here and below.
Addressed Christian's issues.
Diff: |
Revision 8 (+1361 -6)
|
|---|
| reviewboard/webapi/resources/oauth_app.py (Diff revision 8) |
|---|
F401 'djblets.webapi.errors.PERMISSION_DENIED' imported but unused
| reviewboard/webapi/resources/oauth_app.py (Diff revision 8) |
|---|
E131 continuation line unaligned for hanging indent
| reviewboard/webapi/resources/oauth_app.py (Diff revision 8) |
|---|
E131 continuation line unaligned for hanging indent
| reviewboard/webapi/tests/mixins.py (Diff revision 8) |
|---|
F401 'djblets.webapi.errors.DOES_NOT_EXIST' imported but unused
Diff: |
Revision 9 (+1360 -5)
|
|---|
Light refactoring, address david's issues, allow regenerating client secrets.
Description: |
|
||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+1405 -4) |