Update WebAPITokenManager to use token generators

Review Request #12419 — Created June 28, 2022 and submitted

Information

Djblets
release-3.x

Reviewers

Currently, token generation is baked into
djblets.webapi.managers.WebAPITokenManager. With our move to API Tokens v2,
we don't want token generation to be tightly coupled with Web API tokens. We
want tokens to be usable outside of just the API.

This change moves token generation out of WebAPITokenManager by making use of
token generators instead of directly generating tokens in WebAPITokenManager.
To do this we add two new parameters to WebAPITokenManager,
token_generator_id to decide what token generator to use and token_info to
pass along information needed for token generation to the token generator.

  • Updated WebAPITokenManager's unit tests to account for the use
    of token generators.
  • Ran all unit tests in djblets.webapi.tests with success.
Summary ID
Update WebAPITokenManager to use Token Generators
d39b84516abefb900321e80dd1d18f4524b0eb5d
Description From Last Updated

SyntaxError: invalid syntax Column: 1 Error code: E999

reviewbotreviewbot

Switch these two lines

daviddavid

Instead of instantiating the registry as an attribute on this manager class, we shoud have a global within djblets. That …

daviddavid

We should probably verify that this returns a value, and if not, raise WebAPITokenGenerationError.

daviddavid

Can we swap these two args? That way all the query args are together.

daviddavid

While you're here, can you move this import up into the 3rd party modules group?

daviddavid

Let's put a Version Added in each of these new ones. Helps to have a high-level in the function's description, …

chipx86chipx86

No need for the parens here, or the trailing comma.

chipx86chipx86

No need for the parens here, or the trailing comma.

chipx86chipx86

No blank line here.

chipx86chipx86

Let's do import kgb and use attribute accesses from there. That's the pattern we've been establishing elsewhere.

chipx86chipx86

Can we also check the warning text, to ensure we're getting what we think we're getting?

chipx86chipx86

Let's move this after auto_generated. Since Python by default allows positional arguments to be specified for keyword arguments (unless using …

chipx86chipx86
david
  1. Ship It!

  2. 
      
maubin
Review request changed
Change Summary:
  • Added deprecation warnings for when token_generator_id or token_info aren't set.
Commits:
Summary ID
Update WebAPITokenManager to use Token Generators
1bc481db2f099cc4127f48a2c3e6961a42bae5e0
Update WebAPITokenManager to use Token Generators
6fe60bdab5c0235031d62e7a701f672cf524a7d9

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
david
  1. 
      
  2. djblets/webapi/managers.py (Diff revision 3)
     
     
     
    Show all issues

    Switch these two lines

  3. djblets/webapi/managers.py (Diff revision 3)
     
     
    Show all issues

    Instead of instantiating the registry as an attribute on this manager class, we shoud have a global within djblets. That way extensions or other code can register additional generators easily.

    This should live in djblets/secrets/token_generators/__init__.py and use djblets.registries.importer.lazy_import_registry.

  4. djblets/webapi/tests/test_api_token.py (Diff revision 3)
     
     
    Show all issues

    While you're here, can you move this import up into the 3rd party modules group?

  5. 
      
maubin
maubin
david
  1. 
      
  2. djblets/webapi/managers.py (Diff revisions 3 - 5)
     
     
     
    Show all issues

    We should probably verify that this returns a value, and if not, raise WebAPITokenGenerationError.

  3. djblets/webapi/managers.py (Diff revisions 3 - 5)
     
     
     
    Show all issues

    Can we swap these two args? That way all the query args are together.

  4. 
      
chipx86
  1. 
      
  2. djblets/webapi/managers.py (Diff revision 5)
     
     
     
     
    Show all issues

    Let's put a Version Added in each of these new ones.

    Helps to have a high-level in the function's description, but also to see this on each parameter.

  3. djblets/webapi/managers.py (Diff revision 5)
     
     
     
    Show all issues

    No need for the parens here, or the trailing comma.

  4. djblets/webapi/managers.py (Diff revision 5)
     
     
     
    Show all issues

    No need for the parens here, or the trailing comma.

  5. djblets/webapi/managers.py (Diff revision 5)
     
     
     
     
     
     
     
     
    Show all issues

    No blank line here.

  6. djblets/webapi/tests/test_api_token.py (Diff revision 5)
     
     
    Show all issues

    Let's do import kgb and use attribute accesses from there. That's the pattern we've been establishing elsewhere.

  7. djblets/webapi/tests/test_api_token.py (Diff revision 5)
     
     
     
    Show all issues

    Can we also check the warning text, to ensure we're getting what we think we're getting?

  8. 
      
maubin
chipx86
  1. 
      
  2. djblets/webapi/managers.py (Diff revision 6)
     
     
     
     
     
     
     
     
    Show all issues

    Let's move this after auto_generated. Since Python by default allows positional arguments to be specified for keyword arguments (unless using keyword-only arguments in Python 3), introducing a new argument in the middle is technically an API-breaking change.

    This is why I want to eventually move more things to keyword-only arguments (but that has its own deprecation challenges).

  3. 
      
maubin
chipx86
  1. Ship It!
  2. 
      
maubin
Review request changed
Status:
Completed
Change Summary:
Pushed to release-3.x (b7f84ba)