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
Create a signal for expired API tokens.

Screenshots

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)

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
-	Create a signal for expired API tokens.
+	Create a signal for expired API tokens.

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)

Show all issues

The summary has to be one line without wrapping.

djblets/webapi/signals.py (Diff revision 2)
Show all issues
```
Too many "d"s.
```

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

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)

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)
Show all issues
```
No blank line here.
```

Commits:

	Summary
-	Create a signal for expired API tokens.
+	Create a signal for expired API tokens.

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)

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
-	Create a signal for expired API tokens.
+	Create a signal for expired API tokens.

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: Closed (submitted)

Change Summary:

Pushed to release-3.x (c95fed7)