Summary

Allow API tokens to be flagged as deprecated.

Review Request #12627 — Created Sept. 26, 2022 and submitted Oct. 27, 2022, 8:48 p.m.

Information

Owner

maubin

Repository

Djblets

Branch

release-3.x

Bugs

Depends On

Blocks

12631

Reviewers

Groups

djblets

People

Description

This change introduces a concept of deprecated token generators and allows API

tokens to be considered as deprecated if they were generated by a deprecated

token generator. The deprecated token generators can be set via the

DJBLETS_DEPRECATED_TOKEN_GENERATORS setting.
Now when a deprecated token is used for authentication, a deprecation notice

will be sent via a response header. Previously, the base WebAPIResource

class would include headers set by authentication backends in the response

only when the authentication failed. For the purposes of including

deprecation notices in successful responses, we modified the WebAPIResource

to always include any headers set by authentication backends.

Testing Done


Ran all unit tests
Made requests using deprecated and non deprecated tokens to some

  Reviewboard API endpoints (specifically GET and POST requests to

/review-requests/id/reviews/)
Ran all webapi tests in the Reviewboard test suite.

Commits

Summary	ID
Allow API tokens to be flagged as deprecated.	d05fdaa1ac8507c72f043fbfbcba13c798c16449

Issues

Description	From	Last Updated
For type comparisons, it's best to use is instead of ==. Though in this case, an assertion isn't going to …	chipx86	Sept. 27, 2022, 9:30 p.m.
Blank line between statements and comments.	chipx86	Sept. 27, 2022, 9:31 p.m.
This is best as "Returns whether ...", since it's a function and not a property.	chipx86	Sept. 27, 2022, 9:31 p.m.
Do we need both this and setting headers= above in some classes?	chipx86	Sept. 27, 2022, 9:40 p.m.
Let's alphabetize the keys.	chipx86	Sept. 27, 2022, 9:36 p.m.
Returns -> Return	david	Oct. 20, 2022, 11:17 a.m.
Might as well update this to "Return" while we're here too.	david	Oct. 20, 2022, 11:18 a.m.
The final .0 isn't needed here.	chipx86	Oct. 27, 2022, 8:24 p.m.
Same here. This can be 3.1.	chipx86	Oct. 27, 2022, 8:32 p.m.

flake8 passed.

JSHint passed.

Summary:

-	[WIP] Allow API tokens to be flagged as deprecated.
+	Allow API tokens to be flagged as deprecated.

Description:

		This change introduces a concept of deprecated token generators and allows API
		tokens to be considered as deprecated if they were generated by a deprecated
~		token generator. The set of deprecated token generators can be set via the
	~	token generator. The deprecated token generators can be set via the
		`DJBLETS_DEPRECATED_TOKEN_GENERATORS` setting.

~		This change is still a `[WIP]` due to the uncertainty over where to set
~		response headers for deprecation notices when using deprecated API tokens to
~		authenticate through the API.
~
~		Question: Should we have a setting for toggling the deprecation notices? Then
~		use this to decide if we should include the deprecation response
	~	Now when a deprecated token is used for authentication, a deprecation notice
	~	will be sent via a response header. Previously, the base `WebAPIResource`
	~	class would include headers set by authentication backends in the response
	~	only when the authentication failed. For the purposes of including
	~	deprecation notices in successful responses, we modified the `WebAPIResource`
	~	to always include any headers set by authentication backends.
-		header and display the deprecation notice from the tokens config page.

Testing Done:

	+	Ran all unit tests
	+	Made requests using deprecated and non deprecated tokens to some Reviewboard API endpoints (specifically GET and POST requests to `/review-requests/id/reviews/`)
	+	Ran all webapi tests in the Reviewboard test suite.

Commits:

	Summary	ID
	Allow API tokens to be flagged as deprecated.	71f4f9244887b8dab56d0bf4acfb5f95dbaa8b35
	Allow API tokens to be flagged as deprecated.	d1863d4f37528d02583c42431f3a2ea794a06046

Diff:

Revision 2 (+376 -38)

Show changes

	djblets/secrets/tests/test_token_generator_registry.py
	djblets/secrets/token_generators/registry.py
	djblets/webapi/models.py
	djblets/webapi/auth/backends/api_tokens.py
	djblets/webapi/resources/base.py
	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_api_token.py
	djblets/webapi/tests/test_webapiresource.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

djblets/secrets/token_generators/registry.py (Diff revision 2)

The issue has been resolved. Show all issues

For type comparisons, it's best to use is instead of ==.

Though in this case, an assertion isn't going to tell a consumer much. Assertions are best done when we control the input and output.

Instead, if we want to check this, we should do a conditional that raises an ImproperlyConfigured, which is a Django exception that means "this setting (or something similar) is wrong."

djblets/webapi/auth/backends/api_tokens.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line between statements and comments.
```

djblets/webapi/models.py (Diff revision 2)

The issue has been resolved. Show all issues

This is best as "Returns whether ...", since it's a function and not a property.

djblets/webapi/resources/base.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Do we need both this and setting headers= above in some classes?
```
djblets/webapi/tests/test_api_token.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Let's alphabetize the keys.
```

Commits:

	Summary	ID
	Allow API tokens to be flagged as deprecated.	d1863d4f37528d02583c42431f3a2ea794a06046
	Allow API tokens to be flagged as deprecated.	8d47a8d14201aba5de3623b9960dcdc032abb07c

Diff:

Revision 3 (+378 -38)

Show changes

	djblets/secrets/tests/test_token_generator_registry.py
	djblets/secrets/token_generators/registry.py
	djblets/webapi/models.py
	djblets/webapi/auth/backends/api_tokens.py
	djblets/webapi/resources/base.py
	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_api_token.py
	djblets/webapi/tests/test_webapiresource.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Changed the deprecation notice message.

Commits:

	Summary	ID
	Allow API tokens to be flagged as deprecated.	8d47a8d14201aba5de3623b9960dcdc032abb07c
	Allow API tokens to be flagged as deprecated.	dbc99923d3c41f3a4977b5c62374126613416e4a

Diff:

Revision 4 (+382 -38)

Show changes

	djblets/secrets/tests/test_token_generator_registry.py
	djblets/secrets/token_generators/registry.py
	djblets/webapi/models.py
	djblets/webapi/auth/backends/api_tokens.py
	djblets/webapi/resources/base.py
	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_api_token.py
	djblets/webapi/tests/test_webapiresource.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

djblets/webapi/models.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Returns -> Return
```
djblets/webapi/models.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Might as well update this to "Return" while we're here too.
```

Change Summary:

Changed "Returns" to "Return" in some docstrings.

Commits:

	Summary	ID
	Allow API tokens to be flagged as deprecated.	dbc99923d3c41f3a4977b5c62374126613416e4a
	Allow API tokens to be flagged as deprecated.	7eb80d5904f4b4ceab0c62921db5afefac73364e

Diff:

Revision 5 (+384 -40)

Show changes

	djblets/secrets/tests/test_token_generator_registry.py
	djblets/secrets/token_generators/registry.py
	djblets/webapi/models.py
	djblets/webapi/auth/backends/api_tokens.py
	djblets/webapi/resources/base.py
	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_api_token.py
	djblets/webapi/tests/test_webapiresource.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Changed Version Added:'s to 3.1.0 and added type hints to some functions.

Commits:

	Summary	ID
	Allow API tokens to be flagged as deprecated.	7eb80d5904f4b4ceab0c62921db5afefac73364e
	Allow API tokens to be flagged as deprecated.	d4e64ce8df5143b3577e1e296723743455bc45ac

Diff:

Revision 6 (+394 -42)

Show changes

	djblets/secrets/tests/test_token_generator_registry.py
	djblets/secrets/token_generators/registry.py
	djblets/webapi/models.py
	djblets/webapi/auth/backends/api_tokens.py
	djblets/webapi/resources/base.py
	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_api_token.py
	djblets/webapi/tests/test_webapiresource.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

Looks great! Two tiny nits that are just for keeping consistency in docs, but all the code looks great! Feel free to make these changes and land it.

djblets/secrets/token_generators/registry.py (Diff revision 6)
The issue has been resolved. Show all issues
```
The final .0 isn't needed here.
```
djblets/webapi/models.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Same here. This can be 3.1.
```

Commits:

	Summary	ID
	Allow API tokens to be flagged as deprecated.	d4e64ce8df5143b3577e1e296723743455bc45ac
	Allow API tokens to be flagged as deprecated.	d05fdaa1ac8507c72f043fbfbcba13c798c16449

Diff:

Revision 7 (+394 -42)

Show changes

	djblets/secrets/tests/test_token_generator_registry.py
	djblets/secrets/token_generators/registry.py
	djblets/webapi/models.py
	djblets/webapi/auth/backends/api_tokens.py
	djblets/webapi/resources/base.py
	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_api_token.py
	djblets/webapi/tests/test_webapiresource.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Status: Closed (submitted)

Change Summary:

Pushed to release-3.x (c96db6a)