Summary

Check OAuth token in @webapi_check_local_site

Review Request #9079 — Created July 17, 2017 and submitted Aug. 30, 2017, 12:51 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

~~9078~~

Commit

b60473b...

Reviewers

Groups

reviewboard

People

Description

This patch updates @webapi_check_local_site to ensure that if the user
is using an OAuth2 access token that it is valid for the current Local
Site.

Web API test mixins classes have been added so that this logic can be
tested for all resources.

Testing Done

Ran unit tests.

Issues

Description	From	Last Updated
It looks like you posted the wrong change here?	david	Aug. 1, 2017, 4:45 p.m.
F401 'djblets.webapi.errors.NOT_LOGGED_IN' imported but unused	reviewbot	July 17, 2017, 3:54 p.m.
Why is this commented out? Same below.	david	July 20, 2017, 2:25 p.m.
E303 too many blank lines (2)	reviewbot	July 20, 2017, 2:38 p.m.
Here we compare directly against None, whereas below (in the next conditional), we just check truthiness. Is this correct? Or …	chipx86	Aug. 21, 2017, 1:34 p.m.
The other logging statements are out of date. These should now have a request=request argument, and the three initial request.* …	chipx86	Aug. 21, 2017, 1:53 p.m.
Swap these.	chipx86	Aug. 21, 2017, 1:59 p.m.
This method should really be purely about setting things up to work with a Local Site. I don't think we …	chipx86	Aug. 21, 2017, 1:56 p.m.
I don't really understand this. "Meant for a local site on the root"?	chipx86	Aug. 21, 2017, 1:56 p.m.
The two Local Site-related assignments are redundant.	chipx86	Aug. 21, 2017, 1:57 p.m.
These would be better as creates + IntegrityError checks. It'll keep query counts down (otherwise, we're doing 2 in most …	chipx86	Aug. 21, 2017, 1:58 p.m.
E999 SyntaxError: invalid syntax	reviewbot	Aug. 21, 2017, 2:14 p.m.
E999 SyntaxError: invalid syntax	reviewbot	Aug. 21, 2017, 2:21 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/tests/mixins.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F401 'djblets.webapi.errors.NOT_LOGGED_IN' imported but unused
```

reviewboard/webapi/tests/mixins.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Why is this commented out? Same below.
```

Change Summary:

Add missing tests

Diff:

Revision 2 (+669 -41)

Show changes

	reviewboard/webapi/decorators.py
	reviewboard/webapi/tests/mixins.py
	reviewboard/webapi/tests/test_api_token.py
	reviewboard/webapi/tests/test_oauth_token.py
	reviewboard/webapi/tests/test_review_group.py
	reviewboard/webapi/tests/test_review_request.py
	reviewboard/webapi/tests/test_screenshot_draft.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/tests/mixins.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```

Commit:

+	a93b44d74e75d235e09843a868e847fca5ecd97e

Diff:

Revision 3 (+1993 -108)

Show changes

	reviewboard/settings.py
	reviewboard/staticbundles.py
	reviewboard/urls.py
	reviewboard/accounts/backends.py
	reviewboard/accounts/pages.py
	reviewboard/accounts/forms/pages.py
	reviewboard/admin/siteconfig.py
	reviewboard/oauth/__init__.py
	25 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

It looks like you posted the wrong change here?

Diff:

Revision 4 (+668 -41)

Show changes

	reviewboard/webapi/decorators.py
	reviewboard/webapi/tests/mixins.py
	reviewboard/webapi/tests/test_api_token.py
	reviewboard/webapi/tests/test_oauth_token.py
	reviewboard/webapi/tests/test_review_group.py
	reviewboard/webapi/tests/test_review_request.py
	reviewboard/webapi/tests/test_screenshot_draft.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/webapi/decorators.py (Diff revision 4)

The issue has been resolved. Show all issues

Here we compare directly against None, whereas below (in the next conditional), we just check truthiness. Is this correct? Or can we just check truthiness in both?

reviewboard/webapi/decorators.py (Diff revision 4)

The issue has been resolved. Show all issues

The other logging statements are out of date. These should now have a request=request argument, and the three initial request.* args should be removed. The logging handler will take care to log these details.

reviewboard/webapi/tests/mixins.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Swap these.
```

reviewboard/webapi/tests/mixins.py (Diff revision 4)

The issue has been dropped. Show all issues

This method should really be purely about setting things up to work with a Local Site. I don't think we want to complicate it by then amking the usage of a Local Site optional. It seems that we only have the one basic test using this (and then the GET equivalent used the GET version of this test). Can we just change up the logic in those to use the non-site versions of this, and to perform the tests needed?

This applies to the equivalent versions of this.

reviewboard/webapi/tests/mixins.py (Diff revision 4)

The issue has been dropped. Show all issues

I don't really understand this. "Meant for a local site on the root"?

brennie Aug. 21, 2017, 1:59 p.m.

A local-site restricted token was used on the root, ie. /api/ instead of s/foo/api/

chipx86 Aug. 21, 2017, 5:55 p.m.

Hmm, okay. It's the "with access to a local site ... meant for a local site on the root" that's confusing. It's still not completely clear to me what this is testing for, from the docstring alone. Is it the token meant for a local site that the user has access to, being used on /api/? Might be better to explicitly say /api/ instead of "root", and to maybe just get rid of the "with access to a local site" bit?

reviewboard/webapi/tests/mixins.py (Diff revision 4)
The issue has been resolved. Show all issues
```
The two Local Site-related assignments are redundant.
```

reviewboard/webapi/tests/test_review_group.py (Diff revision 4)

The issue has been resolved. Show all issues

These would be better as creates + IntegrityError checks. It'll keep query counts down (otherwise, we're doing 2 in most cases now).

Commit:

-	a93b44d74e75d235e09843a868e847fca5ecd97e
+	b60473b5ef7713aead80668421d9e6058b651ce9

Diff:

Revision 5 (+682 -50)

Show changes

	reviewboard/webapi/decorators.py
	reviewboard/webapi/tests/mixins.py
	reviewboard/webapi/tests/test_api_token.py
	reviewboard/webapi/tests/test_oauth_token.py
	reviewboard/webapi/tests/test_review_group.py
	reviewboard/webapi/tests/test_review_request.py
	reviewboard/webapi/tests/test_screenshot_draft.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/decorators.py (Diff revision 5)
The issue has been resolved. Show all issues
```
E999 SyntaxError: invalid syntax
```

Diff:

Revision 6 (+682 -50)

Show changes

	reviewboard/webapi/decorators.py
	reviewboard/webapi/tests/mixins.py
	reviewboard/webapi/tests/test_api_token.py
	reviewboard/webapi/tests/test_oauth_token.py
	reviewboard/webapi/tests/test_review_group.py
	reviewboard/webapi/tests/test_review_request.py
	reviewboard/webapi/tests/test_screenshot_draft.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/decorators.py (Diff revision 6)
The issue has been resolved. Show all issues
```
E999 SyntaxError: invalid syntax
```

Diff:

Revision 7 (+682 -50)

Show changes

	reviewboard/webapi/decorators.py
	reviewboard/webapi/tests/mixins.py
	reviewboard/webapi/tests/test_api_token.py
	reviewboard/webapi/tests/test_oauth_token.py
	reviewboard/webapi/tests/test_review_group.py
	reviewboard/webapi/tests/test_review_request.py
	reviewboard/webapi/tests/test_screenshot_draft.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (f738b53)