Create a signal for expired API tokens.

Review Request #12589 — Created Sept. 12, 2022 and submitted

Information

Djblets
release-3.x

Reviewers

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.

  • 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.
Summary ID
Create a signal for expired API tokens.
b2c89cfc2dbf184b23b09e7819b1892ead21b5bc
Description From Last Updated

Can we make it so we set this key here in djblets instead of in Review Board? It seems fragile …

daviddavid

The summary has to be one line without wrapping.

chipx86chipx86

Too many "d"s.

chipx86chipx86

pytz is a third-party library, so it needs to be in the second import group. Although, we don't need it …

chipx86chipx86

Can we update this to explicitly specify the class that's being tested?

chipx86chipx86

No blank line here.

chipx86chipx86

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

chipx86chipx86
david
  1. 
      
  2. 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.

    1. 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?

    2. 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?

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

    4. 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.

  3. 
      
maubin
david
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. djblets/webapi/signals.py (Diff revision 2)
     
     
     

    The summary has to be one line without wrapping.

  3. djblets/webapi/signals.py (Diff revision 2)
     
     

    Too many "d"s.

  4. 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.

  5. djblets/webapi/tests/test_signals.py (Diff revision 2)
     
     
     

    Can we update this to explicitly specify the class that's being tested?

  6. djblets/webapi/tests/test_signals.py (Diff revision 2)
     
     
     
     

    No blank line here.

  7. 
      
maubin
chipx86
  1. 
      
  2. djblets/webapi/tests/test_signals.py (Diff revision 3)
     
     
     

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

  3. 
      
maubin
chipx86
  1. Ship It!
  2. 
      
maubin
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.x (c95fed7)
Loading...