Summary

[OAuth2Provider][WIP] Enabling OAuth2 authorization on Review Board Web API

Review Request #7996 — Created Feb. 25, 2016 and discarded Sept. 7, 2017, 9 p.m.

Information

Owner

lehoangm

Repository

Review Board

Branch

master

Bugs

Depends On

~~7941~~, ~~7997~~

Blocks

7997, 8095

Reviewers

Groups

reviewboard

People

brennie, chipx86

Description

Prior to this commit. A user can manage list of all OAuth2 client applications
and OAuth2 tokens but there is no mechanism to authorize resources using OAuth2
protocol. In this commit, OAuth2 authorization mechanism is added to
the current authorization mechanism for Review Board web API resources.
Services now can follow OAuth2 protocol, send a request with appropriate
authorization header to work with Review Board's resources. All the scopes
are loaded at RB start time, and external services can request a token with
specify scope through OAuth2 protocol. When using Review Board's API resource,
the scope of a token is checked to make sure that the external service has
the right permission the resource.

Since Django OAuth toolkit does not support dynamic scope, modifications
are needed. OAuth2Validator.validate_scopes now always use the dynamic
dict of scopes for scope validation. OAUTH2_PROVIDER['SCOPES'] is a
dynamic dict of scopes. OAuth2ProviderSettings does not cache any scope related
information and always provive most up to date scope information.

Testing Done

Manual test. Missing unit tests. I will be added latter

Issues

Description	From	Last Updated
We can't actually import this here, or certain things will break due to circular references/initialization orders. I'll go into this …	chipx86	April 2, 2016, 9:31 p.m.
'django_reset' imported but unused	reviewbot	March 6, 2016, 1:45 a.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	March 6, 2016, 1:45 a.m.
So we can't actually do this for a few reasons: 1) It's not safe to import things like API resource …	chipx86	April 2, 2016, 9:31 p.m.
Col: 37 W292 no newline at end of file	reviewbot	March 6, 2016, 1:50 a.m.
This should be part of the same import group, in alphabetical order, since these are all outside modules.	chipx86	March 6, 2016, 1:54 a.m.
'AccessToken' imported but unused	reviewbot	March 6, 2016, 1:50 a.m.
These should all be part of the same improt group, in alphabetical order.	chipx86	March 6, 2016, 1:53 a.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	March 6, 2016, 1:54 a.m.
Is WebAPIToken what we want? I thought we'd be using something different.	chipx86	March 6, 2016, 1:54 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	April 2, 2016, 9:33 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	April 2, 2016, 9:33 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	April 2, 2016, 9:33 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	April 2, 2016, 9:33 p.m.
'django_reset' imported but unused	reviewbot	April 2, 2016, 9:34 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	April 2, 2016, 9:34 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	April 2, 2016, 9:33 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	April 2, 2016, 9:33 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/webapi/base.py
    reviewboard/webapi/auth_backends.py
    reviewboard/webapi/resources/oauth2_token.py
    reviewboard/settings.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/webapi/base.py
    reviewboard/webapi/auth_backends.py
    reviewboard/webapi/resources/oauth2_token.py
    reviewboard/settings.py

reviewboard/settings.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 'django_reset' imported but unused
```
reviewboard/settings.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 'from settings_local import *' used; unable to detect undefined names
```
reviewboard/settings.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 37
 W292 no newline at end of file
```
reviewboard/webapi/base.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'AccessToken' imported but unused
```
reviewboard/webapi/base.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```

reviewboard/settings.py (Diff revision 1)

The issue has been resolved. Show all issues

We can't actually import this here, or certain things will break due to circular references/initialization orders. I'll go into this more in the next comment.

reviewboard/settings.py (Diff revision 1)

The issue has been resolved. Show all issues

So we can't actually do this for a few reasons:

1) It's not safe to import things like API resource code within settings.py.
2) Iterating here will lead to slowdown at startup.
3) API is dynamic. If an extension is enabled, and it provides API, we'll have new possible scopes. That needs to be taken into consideration.

What we need to do instead is to not have a hard-coded list of scopes, and instead be able to check scopes dynamically as needed.

I don't recall the details exactly, but when we first investigated OAuth2 support, we determined this was doable, but it would require additional work. It just depends on the parts of oauth2_provider that we access.

A lot of what oauth2_provider provides (mixins for access control on views, decorators) checks against SCOPES, but if we don't use them, or we provide our own versions, we can do a better job of this. Since we're really just trying to protect the API (and disallow access to non-API), we only care about access control at the API level. So, any code that we'd have to execute that would check against SCOPES can instead check against the policy_id each resource has.

We can talk about this more on Slack.

reviewboard/webapi/auth_backends.py (Diff revision 1)

The issue has been resolved. Show all issues

This should be part of the same import group, in alphabetical order, since these are all outside modules.

reviewboard/webapi/base.py (Diff revision 1)

The issue has been resolved. Show all issues

These should all be part of the same improt group, in alphabetical order.

reviewboard/webapi/base.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Is WebAPIToken what we want? I thought we'd be using something different.
```

Change Summary:

Update my monkey bussiness :)

Description:

		Prior to this commit. A user can manage list of all OAuth2 client applications
		and OAuth2 tokens but there is no mechanism to authorize resources using OAuth2
		protocol. In this commit, OAuth2 authorization mechanism is added to
		the current authorization mechanism for Review Board web API resources.
		Services now can follow OAuth2 protocol, send a request with appropriate
		authorization header to work with Review Board's resources. All the scopes
		are loaded at RB start time, and external services can request a token with
		specify scope through OAuth2 protocol. When using Review Board's API resource,
		the scope of a token is checked to make sure that the external service has
~		the right permission the resource. This commit affects both djblets and
~		reviewboard files at the same time.
	~	the right permission the resource.
	~
	+	Since Django OAuth toolkit does not support dynamic scope, modifications
	+	are needed. OAuth2Validator.validate_scopes now always use the dynamic
	+	dict of scopes for scope validation. OAUTH2_PROVIDER['SCOPES'] is a
	+	dynamic dict of scopes. OAuth2ProviderSettings does not cache any scope related
	+	information and always provive most up to date scope information.

Diff:

Revision 2 (+159 -6)

Show changes

	reviewboard/settings.py
	reviewboard/admin/siteconfig.py
	reviewboard/oauth2/dynamic_scope_dictionary.py
	reviewboard/oauth2/dynamic_scope_patcher.py
	reviewboard/webapi/auth_backends.py
	reviewboard/webapi/base.py

Fix it!

Tool: Pyflakes
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/webapi/base.py
    reviewboard/settings.py
    reviewboard/oauth2/dynamic_scope_dictionary.py
    reviewboard/webapi/auth_backends.py
    reviewboard/oauth2/dynamic_scope_patcher.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/webapi/base.py
    reviewboard/settings.py
    reviewboard/oauth2/dynamic_scope_dictionary.py
    reviewboard/webapi/auth_backends.py
    reviewboard/oauth2/dynamic_scope_patcher.py

reviewboard/oauth2/dynamic_scope_patcher.py (Diff revision 2)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/oauth2/dynamic_scope_patcher.py (Diff revision 2)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/oauth2/dynamic_scope_patcher.py (Diff revision 2)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/oauth2/dynamic_scope_patcher.py (Diff revision 2)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/settings.py (Diff revision 2)
The issue has been dropped. Show all issues
```
 'django_reset' imported but unused
```
reviewboard/settings.py (Diff revision 2)
The issue has been dropped. Show all issues
```
 'from settings_local import *' used; unable to detect undefined names
```
reviewboard/settings.py (Diff revision 2)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/settings.py (Diff revision 2)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Status:: Discarded