Introduce client API tokens.

Review Request #13032 — Created May 19, 2023 and submitted

Information

Review Board
release-5.0.x

Reviewers

This change introduces client API tokens which are API tokens that belong
specifically to a given client. Clients (e.g. RBTools) will use these tokens
to authenticate to Review Board. These tokens are important for an upcoming
change that allows users to authenticate clients to Review Board via a
web-based login flow.

During the web-based login flow, client API tokens are automatically created
if they do not exist or if previous client tokens are expired. Each client
will have their own client tokens, and the tokens will be marked with a note
that says the token was automatically created for the given client.

  • Ran unit tests.
  • Tested creating a normal token and saw the right email being sent.
  • Tested creating a client token and saw the right email being sent.
  • Manually tested with the upcoming client web-based login flow change.
Summary ID
Add support for client API tokens.
98e1dc56139935bbc7bb26c58727b00680da7888
Description From Last Updated

To help with readability, can you update this to uncollapse newlines? So, one '\n' for each blank line, instead of …

chipx86chipx86

The {% should be unindented. Same with the closing tag.

chipx86chipx86

Let's move these together onto the same line, to reduce the number of newlines we end up with. Text e-mails …

chipx86chipx86

We only use this for typing, so let's move this into TYPE_CHECKING.

chipx86chipx86

For now, we need to use Union[...] instead of |. The | syntax is new as of Python 3.10 (technically …

chipx86chipx86

Can we call this client_name?

chipx86chipx86

Hmm, I don't think this is particularly future-proof. This only works if the only thing in extra data is the …

daviddavid

This can fail with a WebAPITokenGenerationError exception. We should either document a re-raise, or handle it.

chipx86chipx86

Can we call this client_name? I want to give us some room to expand on this if we need to …

chipx86chipx86

Swap these.

chipx86chipx86

datetime and typing should be in their own import group at the top.

chipx86chipx86

This can just be super().setUp().

chipx86chipx86

It doesn't look like this ever changes. Maybe just make it a constant on the class?

chipx86chipx86
david
  1. 
      
  2. reviewboard/webapi/managers.py (Diff revision 1)
     
     
     
     

    Hmm, I don't think this is particularly future-proof. This only works if the only thing in extra data is the client name, but we don't have exclusive control of this--a big part of the point of extra data is that any third party can do what they want with it.

    I think in this case it probably makes sense to add a new field to the token model that we can query on.

    1. Woops, I see. Another thing that would work is using this query instead extra_data__contains=f'"client": "{client}"' (querying for the key and value pair instead of the dictionary as a whole). However, I can see that these types of queries have a dependency on our DjbletsJSONEncoder. Our DjbletsJSONEncoder (which encodes the data for our JSONFields) uses : to separate keys and values, but if we were to ever change that (for example to : with no space), then the query would break. It seems unlikely that we would ever change that, but still something to think about.

      I'm leaning towards avoiding adding a new field. I think the above solution would work well, I've added a unit test for it and tested it out manually with tokens that have multiple things in their extra_data. What do you think? Also, is it ok to warrant a database upgrade for point releases, or is this something that should only happen for major releases?

    2. __contains on an extra_data can be very slow. The database has to read the contents of every single field and do a string search, and depending on how it decides to do this, the result can be a full table scan. We want to avoid those wherever possible. On top of this, the above query will match:

      {
        "extension-provided key": {
          "client": "foo"
        }
      }
      

      Instead, a faster and safer way to accomplish this is by handling matching on the Python side. We query all tokens for the user and then check within extra_data for the matching value.

      I think that's fast enough. Most users will only have a handful of tokens, so this won't add any noticeable overhead. Plus we're only using this during a login flow, not per-request or anything.

      Since this is informational, I think we're okay using extra_data and just doing this sort of lookup this way. If we needed to filter based off the field per-request or something, I'd say we introduce one in RB6. But I don't see us needing to do that for anything currently planned.

  3. 
      
chipx86
  1. 
      
  2. reviewboard/notifications/tests/test_email_sending.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     

    To help with readability, can you update this to uncollapse newlines? So, one '\n' for each blank line, instead of ...\n\n'.

  3. The {% should be unindented. Same with the closing tag.

  4. Let's move these together onto the same line, to reduce the number of newlines we end up with.

    Text e-mails are a bit annoying to templatize.

  5. reviewboard/webapi/managers.py (Diff revision 1)
     
     
     

    We only use this for typing, so let's move this into TYPE_CHECKING.

  6. reviewboard/webapi/managers.py (Diff revision 1)
     
     

    For now, we need to use Union[...] instead of |. The | syntax is new as of Python 3.10 (technically mypy can deal with it, but it's not safe otherwise).

  7. reviewboard/webapi/managers.py (Diff revision 1)
     
     

    Can we call this client_name?

  8. reviewboard/webapi/managers.py (Diff revision 1)
     
     

    Can we call this client_name? I want to give us some room to expand on this if we need to later (say, introduce a client dictionary with additional information, if we later find we need to track more than this).

  9. reviewboard/webapi/models.py (Diff revision 1)
     
     
     

    Swap these.

  10. reviewboard/webapi/tests/test_managers.py (Diff revision 1)
     
     
     
     
     
     
     
     

    datetime and typing should be in their own import group at the top.

  11. This can just be super().setUp().

  12. It doesn't look like this ever changes. Maybe just make it a constant on the class?

  13. 
      
chipx86
  1. 
      
  2. reviewboard/webapi/managers.py (Diff revision 1)
     
     

    This can fail with a WebAPITokenGenerationError exception. We should either document a re-raise, or handle it.

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

Status: Closed (submitted)

Change Summary:

Pushed to release-5.0.x (877bd91)
Loading...