Summary

Create a signal for expired API tokens.

Review Request #12589 — Created Sept. 12, 2022 and submitted Sept. 15, 2022, 1:45 p.m.

Information

Owner

maubin

Repository

Djblets

Branch

release-3.x

Bugs

Depends On

Blocks

12591

Reviewers

Groups

djblets

People

Description

Create a signal for expired API tokens. This signal gets triggered when an
expired API token is used for authentication.

We also switch from using naive date objects to timezone-aware date objects in
API token related unit tests to ensure that test data can be properly compared
to the API token's timezone-aware date objects.

Testing Done


Created unit tests for the new signal and ran tests in

djblets/webapi/tests/test_signals.py
Manually tested authenticating with expired and non expired tokens, confirmed

  that appropriate notifications were sent and triggered by the signal.

Commits

Summary	ID
Create a signal for expired API tokens.	b2c89cfc2dbf184b23b09e7819b1892ead21b5bc

Issues

Description	From	Last Updated
Can we make it so we set this key here in djblets instead of in Review Board? It seems fragile …	david	Sept. 13, 2022, 1:44 p.m.
The summary has to be one line without wrapping.	chipx86	Sept. 14, 2022, 10:12 a.m.
Too many "d"s.	chipx86	Sept. 14, 2022, 10:12 a.m.
pytz is a third-party library, so it needs to be in the second import group. Although, we don't need it …	chipx86	Sept. 14, 2022, 10:11 a.m.
Can we update this to explicitly specify the class that's being tested?	chipx86	Sept. 14, 2022, 10:14 a.m.
No blank line here.	chipx86	Sept. 14, 2022, 10:15 a.m.
This should be outside the try, since if it were to fail (highly theoretical), the disconnect would have nothing to …	chipx86	Sept. 14, 2022, 3:21 p.m.

flake8 passed.

JSHint passed.

djblets/webapi/auth/backends/api_tokens.py (Diff revision 1)

The issue has been resolved. Show all issues

Can we make it so we set this key here in djblets instead of in Review Board? It seems fragile and confusing to have this responsibility split between the two packages.

We might also want to change "email" to "notification" just to be a bit more agnostic as to how the notification occurs.

maubin Sept. 13, 2022, 1:33 p.m.

I agree it is weird to have the responsibility split between two packages. My reasoning for setting it in Review Board was that in Review Board I can make sure that the email was actually sent before setting the flag (see lines 300-302 here). If the email fails to send, we'll send another email upon the next use of the token. Here, I have no way of knowing if the email was actually sent. But that's also not a super common case and not that big of an issue, so should I go ahead with setting the flag here?

david Sept. 13, 2022, 1:35 p.m.

An alternative would be to have this signal fire every time an expired API token is attempted to be used, and then handle the checking and notification entirely in Review Board. Perhaps Christian has an opinion?

maubin Sept. 13, 2022, 1:44 p.m.

I like that alternative. I'll bring it up in our meeting today and see what Christian thinks.

chipx86 Sept. 13, 2022, 2:03 p.m.

Yeah I like that. That keeps Djblets from knowing how the consumer wants to use this, and gives us freedom in Review Board to handle it however we choose.

Change Summary:

No longer checks the token to see if an expired notification has already been sent.

Description:

		Create a signal for expired API tokens. This signal gets triggered when an
~		expired API token is used for authentication, with the purpose of signaling an
	~	expired API token is used for authentication.
-		email to be sent to the user to notify them of the expired token. An
-		`expired_email_sent` flag may be set in the token's `extra_data`, and when set
-		the signal will not be triggered even when the expired API token is used. This
-		way the email will only be sent once, upon first use of an expired token.

		We also switch from using naive date objects to timezone-aware date objects in
		API token related unit tests to ensure that test data can be properly compared
		to the API token's timezone-aware date objects.

Testing Done:

		Created unit tests for the new signal and ran tests in `djblets/webapi/tests/test_signals.py`
~		Manually tested authenticating with expired and non expired tokens, confirmed that appropriate emails were sent and triggered by the signal.
	~	Manually tested authenticating with expired and non expired tokens, confirmed that appropriate notifications were sent and triggered by the signal.

Commits:

	Summary	ID
	Create a signal for expired API tokens.	89d3defa21d263b06d9be996aa4a60f10cdb5ade
	Create a signal for expired API tokens.	a8fadd9d397f8a9bba97795acd7573f2b97a1c11

Diff:

Revision 2 (+180 -16)

Show changes

	djblets/webapi/signals.py
	djblets/webapi/auth/backends/api_tokens.py
	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_signals.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

djblets/webapi/signals.py (Diff revision 2)
The issue has been resolved. Show all issues
```
The summary has to be one line without wrapping.
```
djblets/webapi/signals.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Too many "d"s.
```

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 2)

The issue has been resolved. Show all issues

pytz is a third-party library, so it needs to be in the second import group.
Although, we don't need it for UTC. Django has its own thing we can use.

djblets/webapi/tests/test_signals.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can we update this to explicitly specify the class that's being tested?
```
djblets/webapi/tests/test_signals.py (Diff revision 2)
The issue has been resolved. Show all issues
```
No blank line here.
```

Commits:

	Summary	ID
	Create a signal for expired API tokens.	a8fadd9d397f8a9bba97795acd7573f2b97a1c11
	Create a signal for expired API tokens.	167262ee55eae0c5540d3981d54d47d7fb9f61b1

Diff:

Revision 3 (+180 -10)

Show changes

	djblets/webapi/signals.py
	djblets/webapi/auth/backends/api_tokens.py
	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_signals.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

djblets/webapi/tests/test_signals.py (Diff revision 3)

The issue has been resolved. Show all issues

This should be outside the try, since if it were to fail (highly theoretical), the disconnect would have nothing to try to disconnect.

Change Summary:

Moved the signal.connect() calls outside of the try blocks.

Commits:

	Summary	ID
	Create a signal for expired API tokens.	167262ee55eae0c5540d3981d54d47d7fb9f61b1
	Create a signal for expired API tokens.	b2c89cfc2dbf184b23b09e7819b1892ead21b5bc

Diff:

Revision 4 (+194 -20)

Show changes

	djblets/webapi/signals.py
	djblets/webapi/auth/backends/api_tokens.py
	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_signals.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-3.x (c95fed7)