Update APITokenResource for API Tokens v2

Review Request #12474 — Created July 15, 2022 and submitted

Information

Review Board
release-5.0.x

Reviewers

This change updates the APITokenResource according to the new WebAPIToken
fields addded as part of our move to API Tokens v2. The API now creates tokens
in the vendor checksum format rather than the legacy SHA1 format. Users can also
invalidate tokens via the API.

This change also corrects some PUT, POST and DELETE tests for for the
APITokenResource. These tests were all calling self.api_get instead of the
appropriate HTTP method.

It also updates test utilities that are related to WebAPITokens.

Ran unit tests in reviewboard/webapi/tests.

Summary ID
Update APITokenResource for API Tokens v2
6c5d95516bc6417b362923f5ae4a4f0f7ffca848
Description From Last Updated

Can we update this to use the imperative mood? ("Creates" -> "Create")

daviddavid

'django.utils.timezone' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

Can you alphabetize all the keys? Helps keep it clear where new ones go. Also, all these should have a …

chipx86chipx86

This has to go before Args. Under the hood, Version Added is a .. versionadded::, which is a paragraph-following/generating construct. …

chipx86chipx86

For the else clause, are we handling None/''? We should probably just be explicit with one or the other (probably …

chipx86chipx86

Let's be explicit and say invalid_date here.

daviddavid

Let's be explicit and say last_updated here.

daviddavid

Same here.

daviddavid

Same here.

daviddavid

If we're not modifying this, we should provide it inline where we're using it.

chipx86chipx86

We should maybe create some central concept of what the default token generator is. Maybe this lives in Djblets, as …

chipx86chipx86

Let's simplify this a bit. valid = kwargs.get('valid') if valid is True: return INVALID_FORM_DATA, .... elif value is False: ...

chipx86chipx86

I know we should already have some validation from the field type, but can we explicitly do kwargs['valid'] != False?

daviddavid

Let's avoid setting the key twice and instead compute as one statement.

chipx86chipx86

Paren should be on the previous line, with no trailing , (given that it's a function call).

chipx86chipx86

Let's make this part of the call, so we have that state all together. Same below.

chipx86chipx86

"expiration date" is more in line with the language we tend to use in the docs (I just found out, …

chipx86chipx86

This form for function definitions should only be used if we can't fit argument names and values on a single …

chipx86chipx86

Rather than this form, let's pull this out and pre-compute the generator ID. In fact, we should allow the caller …

chipx86chipx86

As above, we standardize on "expiration date."

chipx86chipx86

No blank line here.

chipx86chipx86

Let's pull out the ID before we call this, to avoid the chaining.

chipx86chipx86

Rather than True or False (here and in the resulting errors), we should use true or false, because these will …

chipx86chipx86

Let's pull this out to avoid the chaining.

chipx86chipx86

We try hard to avoid the ternary if .. else in statements. They're a bit harder to read and format. …

chipx86chipx86
maubin
Review request changed

Change Summary:

  • Serialize the DateTime fields to ISO format.
  • Uses WebAPITokenManager to handle invalidating tokens.

Commits:

Summary ID
Update APITokenResource for API Tokens v2
3008c645dfec118e41186664e2df56564a27413b
Update APITokenResource for API Tokens v2
815e7c9dcfad53f78f3576f77dee392aa4f44b3f

Diff:

Revision 2 (+652 -40)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
  1. 
      
  2. reviewboard/testing/testcase.py (Diff revision 2)
     
     
    Show all issues

    Can we update this to use the imperative mood? ("Creates" -> "Create")

  3. Show all issues

    Let's be explicit and say invalid_date here.

  4. Show all issues

    Let's be explicit and say last_updated here.

  5. Show all issues

    Same here.

  6. Show all issues

    Same here.

  7. Show all issues

    I know we should already have some validation from the field type, but can we explicitly do kwargs['valid'] != False?

  8. 
      
chipx86
  1. 
      
  2. reviewboard/webapi/resources/api_token.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Can you alphabetize all the keys? Helps keep it clear where new ones go.

    Also, all these should have a 'added_in': '5.0'. Same with all places where we describe a new field.

  3. reviewboard/webapi/resources/api_token.py (Diff revision 2)
     
     
     
     
    Show all issues

    This has to go before Args.

    Under the hood, Version Added is a .. versionadded::, which is a paragraph-following/generating construct. Args, Returns, and Raises are special block-generating directives meant to appear after the paragraph-generating content is done.

    Same below.

  4. reviewboard/webapi/resources/api_token.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    For the else clause, are we handling None/''? We should probably just be explicit with one or the other (probably None).

    Same below.

  5. reviewboard/webapi/resources/api_token.py (Diff revision 2)
     
     
     
     
    Show all issues

    If we're not modifying this, we should provide it inline where we're using it.

  6. Show all issues

    We should maybe create some central concept of what the default token generator is. Maybe this lives in Djblets, as a DJBLETS_DEFAULT_API_TOKEN_GENERATOR, and a function for getting it from the registry?

    This can be a follow-up. Not a P0 for this change or related changes. Given time constraints, this can also just be tracked in Asana for now.

  7. reviewboard/webapi/resources/api_token.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Let's simplify this a bit.

    valid = kwargs.get('valid')
    
    if valid is True:
        return INVALID_FORM_DATA, ....
    elif value is False:
        ...
    
  8. reviewboard/webapi/tests/test_api_token.py (Diff revision 2)
     
     
     
    Show all issues

    Let's avoid setting the key twice and instead compute as one statement.

  9. reviewboard/webapi/tests/test_api_token.py (Diff revision 2)
     
     
     
    Show all issues

    Paren should be on the previous line, with no trailing , (given that it's a function call).

  10. reviewboard/webapi/tests/test_api_token.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    Let's make this part of the call, so we have that state all together.

    Same below.

  11. 
      
maubin
chipx86
  1. 
      
  2. Show all issues

    "expiration date" is more in line with the language we tend to use in the docs (I just found out, this is a regional difference).

  3. reviewboard/testing/testcase.py (Diff revision 3)
     
     
     
     
     
     
     
     
    Show all issues

    This form for function definitions should only be used if we can't fit argument names and values on a single line. These will all fit, so let's start them off on the create_webapi_token line and then align each next one with that.

  4. reviewboard/testing/testcase.py (Diff revision 3)
     
     
     
    Show all issues

    Rather than this form, let's pull this out and pre-compute the generator ID.

    In fact, we should allow the caller to specify a generator ID and token info (default to None, set to these if not specified), so that we can more easily test with variations on the tokens.

  5. Show all issues

    As above, we standardize on "expiration date."

  6. reviewboard/webapi/resources/api_token.py (Diff revision 3)
     
     
     
     
    Show all issues

    No blank line here.

  7. reviewboard/webapi/resources/api_token.py (Diff revision 3)
     
     
     
    Show all issues

    Let's pull out the ID before we call this, to avoid the chaining.

  8. reviewboard/webapi/resources/api_token.py (Diff revision 3)
     
     
     
    Show all issues

    Rather than True or False (here and in the resulting errors), we should use true or false, because these will be values going into the API rather than the Python constants for them.

  9. reviewboard/webapi/tests/mixins.py (Diff revision 3)
     
     
     
    Show all issues

    Let's pull this out to avoid the chaining.

  10. reviewboard/webapi/tests/test_api_token.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    We try hard to avoid the ternary if .. else in statements. They're a bit harder to read and format.

    Instead, can we pre-compute the expected values at the top of the function, and compare against those?

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

Status: Closed (submitted)

Change Summary:

Pushed to release-5.0.x (2d66417)
Loading...