Add a web-based logon flow for clients.

Review Request #12982 — Created May 5, 2023 and submitted

Information

Review Board
release-5.0.x

Reviewers

This adds supporting views and endpoints for allowing users to authenticate
clients to the Review Board server via a web browser. This is useful for
users who log in to Review Board using SSO or similar methods, and would
like to use those methods to authenticate the clients. Right now our only
client is RBTools, but in the future we could have other tools and services
that would need to authenticate to Review Board, so this lays the foundation
for supporting web-based logon for any client.

Specifically, we're adding a view that sends authentication data to a client
URL (with the data being an API token created for the client). We also add a
view for prompting the user to authenticate the client as the currently logged
in user, if they are already logged in on their browser during the web login
flow.

  • Ran unit tests and JS unit tests.
  • Manually tested authentication to RBTools with the normal login flow, SSO,
    and SSO auto login.
  • Manually tested with successful logins, incorrect password/username attempts,
    logins where the RBTools server is down or returns bad responses
  • Tested with redirect URLs.
  • Tested the normal login flow and with redirect URLs.
  • Tested for XSS vulnerability by using <script>..</script> values in
    the client name, client URL and redirect query parameters.
Summary ID
Add support for client API tokens.
fffedca30a9cf151b8f2a6b116b0fd1dcc9d6249
Support web-based login for clients.
e6a7e979b9a12b87bab50b999f091e023fb35df8

Description From Last Updated

Would it make sense to build this payload differently according to which external client we're dealing with? For example maybe …

maubinmaubin

'datetime' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

'json' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

'django.contrib.auth.models.User' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

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

reviewbotreviewbot

Blank line between these.

chipx86chipx86

Can you add -> None to each of these?

chipx86chipx86

We're a bit all over the place with the formatting of view docstrings, but we mostly settle on Testing ViewName …

chipx86chipx86

Blank line between this block and statement.

chipx86chipx86

Can you add -> None to each of these? Same with other test files.

chipx86chipx86

The docstring's indented too far.

chipx86chipx86

Blank line here.

chipx86chipx86

Blank line here.

chipx86chipx86

Blank line here.

chipx86chipx86

Blank line here.

chipx86chipx86

Blank line here.

chipx86chipx86

While here, can you add a __future__ ... annotations import?

chipx86chipx86

Can you add typing to these? In fact, we should move these to the class body as instance variables, document …

chipx86chipx86

No need for the outer parens.

chipx86chipx86

Can we name this client-name? (I made some comments about why on /r/13032).

chipx86chipx86

request is local, so we don't need to access self.request here.

chipx86chipx86

To better convey intent here (this is only for GET requests), let's check the method first.

chipx86chipx86

We should probably preserve ?next=, in case the caller wants to eventually redirect back somewhere. In fact, I wonder if …

chipx86chipx86

I think dispatch should be responsible for pulling these out for other methods to use. State set on self within …

chipx86chipx86

Let's call this client_name.

chipx86chipx86

No need for parens here.

chipx86chipx86

We can ditch the else, since it's implied.

chipx86chipx86

We access self.request a lot, so let's pull it out so we don't have to perform attribute lookups more than …

chipx86chipx86

This is a bit tricky to read. Let's pull this out into a variable and then reference that in the …

chipx86chipx86

I think we'll want to include 127.0.0.1:{client_url_port} in this list. For get_host(), do we want to include the port as …

chipx86chipx86

I just made a comment on /r/13032/ about this, but generate_token() can raise a WebAPITokenGenerationError, which would bubble up to …

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

To keep this consistent, how about prefixing this with client? And maybe flip it, so it's client_allowed = [True,False]?

chipx86chipx86

For logging-related statements, pass arguments as positional arguments, rather than using %.

chipx86chipx86

Same comments as above.

chipx86chipx86

Same comments re: pulling out request.

chipx86chipx86

Maybe support a client-state here as well?

chipx86chipx86

No space before the {% blocktrans %}. Indentation happens within the {% ... %}. Same with template tags below and …

chipx86chipx86

Let's put a blank line between these blocks, so they don't blur together.

chipx86chipx86

Instead of "Blocking log in" (which I feel may be misunderstood by some users), let's try to help them out …

chipx86chipx86

We should put this in scripts-post, so it doesn't block anything on page load.

chipx86chipx86

We know the value of this variable at template time, so let's use it to wire off the entire <script>, …

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

We can't safely use these template literals in here. We'd have to call interpolate(gettext('...', [params...])) directly. We also can't localize …

chipx86chipx86

No need for the />. That's a legacy holdover that we don't need to continue with. Also, no space after …

chipx86chipx86

Missing semicolon. Column: 48 Error code: W033

reviewbotreviewbot

Missing semicolon. Column: 61 Error code: W033

reviewbotreviewbot

While here, can you add a module docstring?

chipx86chipx86

Can you add a Version Changed and mention the addition of client-name and client-url?

chipx86chipx86

Swap these around. Should go newest to oldest.

chipx86chipx86

Since this is multi-line, let's do one value per line.

chipx86chipx86

Let's inverse the attribute logic here. Compute a local value, then set it on the attribute.

chipx86chipx86

Looks like we compute the ?client-name=%s&client-url=%s%s three times in this class. Maybe we should precompute this in dispatch as an …

chipx86chipx86

Second sentence should be in the description area.

chipx86chipx86

This feels like it should be an independent block of work. The rest are just simply setting state, but this …

chipx86chipx86

These are both present in LoginView as well. Maybe we should centralize a list of allowed hosts somewhere?

chipx86chipx86

Small nit: We can get more out of a line if we start the text on the next line, indented …

chipx86chipx86

These lines are indented too far.

chipx86chipx86

Might be worth mentioning what happens if this is None.

chipx86chipx86

Alternatively, something like: if port: suffix = f':{port}' else: suffix = '' return { f'127.0.0.1{suffix}' f'localhost{suffix{' } Avoids having to …

chipx86chipx86

Just realized the naming conventions here. These should be _clientName, _clientURL, etc. in JavaScript.

chipx86chipx86

This can be const.

chipx86chipx86

Because these are now in a .es6.js (which Babel will compile), you can now leave out the interpolate. I think …

chipx86chipx86

Can you postfix this instead? Put the -- at the end? Just a small nit, but is more consistent with …

chipx86chipx86

Indentation is off on the description.

chipx86chipx86

We have a RB.navigateTo that should be used here instead. If _redirect is for unit test purposes, you can remove …

chipx86chipx86

In this case, you can combine these statements: const redirectCounter = --this._redirectCounter; (And in this case, -- beforehand makes sense, …

chipx86chipx86

Let's indent the blocktrans (space inside the {%).

chipx86chipx86

Indentation of tags should be one level higher, since they're inside a {% block %}.

chipx86chipx86

Same here.

chipx86chipx86

All user input should be escaped. What I recommend is to have a get_js_view_data() function that returns everything that should …

chipx86chipx86

Same notes about indentation in this file.

chipx86chipx86

Blank line between blocks.

chipx86chipx86

As above, let's have some central variables we can access for the URLs, so we can control it in one …

chipx86chipx86

Template tags should be indented relative to {% block %}.

chipx86chipx86
maubin
  1. 
      
  2. reviewboard/accounts/views.py (Diff revision 1)
     
     
     
     
     
     
     
     
    Show all issues

    Would it make sense to build this payload differently according to which external client we're dealing with? For example maybe one client expects some extra data in the payload, or some different authentication details other than a session cookie. If so, then for building the payload should I have a registry of external clients? Then choose which external client to use from the registry based on the external_client param in the request, and then have that build the payload?

  3. 
      
maubin
Review request changed

Summary:

-[WIP] Support web-based logon for external clients.
+Add a web-based logon flow for clients.

Description:

~  

This adds supporting views and endpoints for allowing external clients to

~   authenticate to the Review Board server via a web browser. This is useful for
~   users who authenticate to Review Board using SSO or similar methods, and would
~   like to use those methods to authenticate to the external clients. Right now
~   our only external client is RBTools, but in the future we could have other
~   tools and services that would need to authenticate to Review Board, like IDE
~   integrations.

  ~

This adds supporting views and endpoints for allowing users to authenticate

  ~ clients to the Review Board server via a web browser. This is useful for
  ~ users who log in to Review Board using SSO or similar methods, and would
  ~ like to use those methods to authenticate the clients. Right now our only
  ~ client is RBTools, but in the future we could have other tools and services
  ~ that would need to authenticate to Review Board, so this lays the foundation
  ~ for supporting web-based logon for any client.

   
~  

This currently sends session cookie data to the external clients for

~   authentication, but I will be changing it so that we use API tokens instead.

  ~

Specifically, we're adding a view that sends authentication data to a client

  ~ URL (with the data being an API token created for the client). We also add a
  + view for prompting the user to authenticate the client as the currently logged
  + in user, if they are already logged in on their browser during the web login
  + flow.

Testing Done:

~  
  • Ran unit tests
  ~
  • Ran unit tests and JS unit tests.
   
  • Manually tested authentication to RBTools with the normal login flow, SSO,
    and SSO auto login.
   
  • Manually tested with successful logins, incorrect password/username attempts,
    logins where the RBTools server is down or returns bad responses
-  
-  

TODO:

-   - Add unit tests for the new views.

Commits:

Summary ID
Support web-based login for RBTools.
bb73fdbc51c33c4bd810f3b314f1dabab977af29
Support web-based login for clients.
7d235a21d9526e47b7e51035264413b3cfdd8d62

Depends On:

+13032 - Introduce client API tokens.

Diff:

Revision 2 (+1386 -12)

Show changes

Added Files:

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
chipx86
  1. 
      
    1. I know this is a lot of comments, but this is looking great :) Many of these are repeated comments (blank lines, extra parens, etc.). There's a couple larger topics here, but most are small

  2. Show all issues

    Blank line between these.

  3. Show all issues

    Can you add -> None to each of these?

  4. Show all issues

    We're a bit all over the place with the formatting of view docstrings, but we mostly settle on Testing ViewName GET [...] for the docstrings.

  5. Show all issues

    Blank line between this block and statement.

  6. Show all issues

    Can you add -> None to each of these?

    Same with other test files.

  7. Show all issues

    The docstring's indented too far.

  8. Show all issues

    Blank line here.

  9. Show all issues

    Blank line here.

  10. Show all issues

    Blank line here.

  11. Show all issues

    Blank line here.

  12. Show all issues

    Blank line here.

  13. reviewboard/accounts/views.py (Diff revision 3)
     
     
    Show all issues

    While here, can you add a __future__ ... annotations import?

  14. reviewboard/accounts/views.py (Diff revision 3)
     
     
     
    Show all issues

    Can you add typing to these?

    In fact, we should move these to the class body as instance variables, document them, add typing, and set them in here (I have a comment about this below, but I've gone back in time to edit this one).

  15. reviewboard/accounts/views.py (Diff revision 3)
     
     
    Show all issues

    No need for the outer parens.

  16. reviewboard/accounts/views.py (Diff revision 3)
     
     
     
    Show all issues

    Can we name this client-name? (I made some comments about why on /r/13032).

  17. reviewboard/accounts/views.py (Diff revision 3)
     
     
     
     
     
     
     
     
    Show all issues

    request is local, so we don't need to access self.request here.

  18. reviewboard/accounts/views.py (Diff revision 3)
     
     
     
    Show all issues

    To better convey intent here (this is only for GET requests), let's check the method first.

  19. reviewboard/accounts/views.py (Diff revision 3)
     
     
     
     
     
     
    Show all issues

    We should probably preserve ?next=, in case the caller wants to eventually redirect back somewhere.

    In fact, I wonder if we'd want to provide some client-state= parameter we'd pass back to the client?

    Same applies below.

    1. Hmm, I wouldn't be able to preserve ?next= because I override it in the client-login-confirm view. If the user clicks "No, login as a different user", I send the user to the logout page and set ?next= to the login URL with the client-name and client-url params so that we redirect to the login page after logging out the user. However if the user clicks "Yes" to sign in as the current user, then I redirect to the client login page and would be able to preserve ?next= in that case. It's probably best to just have some client-redirect parameter for this instead (see below).

      I currently don't redirect anywhere after the client login page though. I just POST to the client URL and if I receive a successful response the page goes from displaying "Logging in to Client..." to "Succesffuly logged in to Client as User. You can now close this page.".

      I could set things up so that I redirect back to some client URL after a successful POST. I'm not sure that it'd be ok to redirect back to client-url though since that is supposed to be the endpoint that receives POST data containing the authentication details. I think I'd have to add another parameter like client-redirect or something. And then in that case we could have a client-state parameter that we can pass back when we redirect to the client. I'm assuming the client-state would be some information set by the client that we don't modify, just pass back, is that right?

      I think it's probably worth it to have some client-state and client-redirect params in case the client does want to redirect back to themselves. I'll only redirect if the client-redirect param exists in the request. For RBTools I don't see any reason to redirect back to the RBTools server or pass back any state. But I think it would be useful for other clients.

  20. reviewboard/accounts/views.py (Diff revision 3)
     
     
     
     
     
     
    Show all issues

    I think dispatch should be responsible for pulling these out for other methods to use.

    State set on self within a View subclass will only live for the lifetime of the request.

  21. reviewboard/accounts/views.py (Diff revision 3)
     
     
     
    Show all issues

    Let's call this client_name.

  22. reviewboard/accounts/views.py (Diff revision 3)
     
     
    Show all issues

    No need for parens here.

  23. reviewboard/accounts/views.py (Diff revision 3)
     
     
     
     
    Show all issues

    We can ditch the else, since it's implied.

  24. reviewboard/accounts/views.py (Diff revision 3)
     
     
     
    Show all issues

    We access self.request a lot, so let's pull it out so we don't have to perform attribute lookups more than once.

  25. reviewboard/accounts/views.py (Diff revision 3)
     
     
     
     
     
    Show all issues

    This is a bit tricky to read. Let's pull this out into a variable and then reference that in the if.

  26. reviewboard/accounts/views.py (Diff revision 3)
     
     
     
    Show all issues

    I think we'll want to include 127.0.0.1:{client_url_port} in this list.

    For get_host(), do we want to include the port as well?

    1. Good idea, I'll add 127.0.0.1:{client_url_port} to the list.

      For get_host() it should already include the port unless the host is using the default port. get_host() will either return the value from HTTP_X_FORWARDED_HOST (if Django's USE_X_FORWARDED_HOST is enabled) which may or may not include the port, HOST which may or may not include the port, or this will return SERVER_NAME:SERVER_PORT which will definitely include the port. So I don't think we really need any extra logic for tacking the port onto get_host() if it isn't already there.

  27. reviewboard/accounts/views.py (Diff revision 3)
     
     
     
     
     
    Show all issues

    I just made a comment on /r/13032/ about this, but generate_token() can raise a WebAPITokenGenerationError, which would bubble up to here. We should handle this gracefully.

  28. reviewboard/accounts/views.py (Diff revision 3)
     
     
    Show all issues

    Missing a trailing comma.

  29. reviewboard/accounts/views.py (Diff revision 3)
     
     
    Show all issues

    To keep this consistent, how about prefixing this with client? And maybe flip it, so it's client_allowed = [True,False]?

  30. reviewboard/accounts/views.py (Diff revision 3)
     
     
     
    Show all issues

    For logging-related statements, pass arguments as positional arguments, rather than using %.

  31. reviewboard/accounts/views.py (Diff revision 3)
     
     
     
     
     
    Show all issues

    Same comments as above.

  32. reviewboard/accounts/views.py (Diff revision 3)
     
     
     
    Show all issues

    Same comments re: pulling out request.

  33. reviewboard/accounts/views.py (Diff revision 3)
     
     
     
     
     
    Show all issues

    Maybe support a client-state here as well?

  34. reviewboard/templates/accounts/client_login.html (Diff revision 3)
     
     
     
     
    Show all issues

    No space before the {% blocktrans %}. Indentation happens within the {% ... %}.

    Same with template tags below and in other files.

  35. Show all issues

    Let's put a blank line between these blocks, so they don't blur together.

  36. Show all issues

    Instead of "Blocking log in" (which I feel may be misunderstood by some users), let's try to help them out a bit. How about something along the lines of:

    For security reasons, clients logging in to the Review Board API must pass a <tt>client-url</tt> that points to the local host. This may be a bug in your software.

    I think we'll only hit that under this case, right?

    1. Yeah that sounds good.

  37. Show all issues

    We should put this in scripts-post, so it doesn't block anything on page load.

  38. reviewboard/templates/accounts/client_login.html (Diff revision 3)
     
     
     
     
    Show all issues

    We know the value of this variable at template time, so let's use it to wire off the entire <script>, rather than injecting it and bailing within the code.

    I sorta wonder if all this JavaScript code should just live in the main JavaScript codebase instead of within this page, due to the limitations outlined below.

  39. Show all issues

    Missing a trailing comma.

  40. reviewboard/templates/accounts/client_login.html (Diff revision 3)
     
     
     
     
    Show all issues

    We can't safely use these template literals in here. We'd have to call interpolate(gettext('...', [params...])) directly.

    We also can't localize text that hard-codes a dynamic value coming from the Django template. The localization string scanner will see the literal {{username}}, and will never match at runtime. So any values like this must be provided as parameters.

  41. reviewboard/templates/accounts/login.html (Diff revision 3)
     
     
     
    Show all issues

    No need for the />. That's a legacy holdover that we don't need to continue with.

    Also, no space after value.

  42. 
      
maubin
Review request changed

Change Summary:

  • Moved the logic on the client login page into a JavaScript view instead of having it
    all in the template.
  • Made a base class for views dealing with client web-based login.
  • Preserve ?next= during the client web-based login flow and redirect to it after
    successfully sending authentication data to the client.
  • Handle when query parameters are passed in the ?next= URL, preserving them until
    the redirect.
  • Changed from client to a client-name parameter/variable.
  • Handles WebAPITokenGenerationErrors during the client login flow

Testing Done:

   
  • Ran unit tests and JS unit tests.
   
  • Manually tested authentication to RBTools with the normal login flow, SSO,
    and SSO auto login.
   
  • Manually tested with successful logins, incorrect password/username attempts,
    logins where the RBTools server is down or returns bad responses
  +
  • Tested with redirect URLs.
  +
  • Tested normal logins and with redirect URLs.

Commits:

Summary ID
Support web-based login for clients.
6bc50cdc50b52d39ecbc7b9050c742a4c137b2fd
Add support for client API tokens.
fffedca30a9cf151b8f2a6b116b0fd1dcc9d6249
Support web-based login for clients.
3c258d273f61bb9c3e29472708218c6468df400e

Diff:

Revision 4 (+2435 -15)

Show changes

Added Files:

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

maubin
maubin
maubin
chipx86
  1. 
      
  2. reviewboard/accounts/views.py (Diff revision 7)
     
     
    Show all issues

    While here, can you add a module docstring?

  3. reviewboard/accounts/views.py (Diff revision 7)
     
     
     
    Show all issues

    Can you add a Version Changed and mention the addition of client-name and client-url?

  4. reviewboard/accounts/views.py (Diff revision 7)
     
     
     
    Show all issues

    Since this is multi-line, let's do one value per line.

  5. reviewboard/accounts/views.py (Diff revision 7)
     
     
     
     
     
     
     
     
    Show all issues

    Let's inverse the attribute logic here. Compute a local value, then set it on the attribute.

  6. reviewboard/accounts/views.py (Diff revision 7)
     
     
     
     
     
     
    Show all issues

    Looks like we compute the ?client-name=%s&client-url=%s%s three times in this class. Maybe we should precompute this in dispatch as an attribute and then reference it? Along with a flag that can be used instead of pulling out and checking client_name and client_url.

  7. reviewboard/accounts/views.py (Diff revision 7)
     
     
    Show all issues

    Second sentence should be in the description area.

  8. reviewboard/accounts/views.py (Diff revision 7)
     
     
     
    Show all issues

    These are both present in LoginView as well. Maybe we should centralize a list of allowed hosts somewhere?

  9. reviewboard/accounts/views.py (Diff revision 7)
     
     
     
     
    Show all issues

    Small nit: We can get more out of a line if we start the text on the next line, indented 1 level.

  10. Show all issues

    This can be const.

  11. reviewboard/static/rb/js/views/clientLoginView.es6.js (Diff revision 7)
     
     
     
     
     
     
     
     
     
    Show all issues

    Because these are now in a .es6.js (which Babel will compile), you can now leave out the interpolate. I think you'll need to pull out the client name and username into local variables first, but I might be wrong there.

    You can just reference local variables in the template literal as usual.

    The plugin will take care of wrapping it in interpolate() and passing in those variables.

  12. Show all issues

    Can you postfix this instead? Put the -- at the end? Just a small nit, but is more consistent with usual styling.

  13. reviewboard/templates/accounts/client_login.html (Diff revision 7)
     
     
     
     
    Show all issues

    Let's indent the blocktrans (space inside the {%).

  14. reviewboard/templates/accounts/client_login.html (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Indentation of tags should be one level higher, since they're inside a {% block %}.

  15. Show all issues

    Same here.

  16. reviewboard/templates/accounts/client_login.html (Diff revision 7)
     
     
     
     
     
     
     
    Show all issues

    All user input should be escaped.

    What I recommend is to have a get_js_view_data() function that returns everything that should go in here (aside from el), and then |json_dumps_items that, so we get a safe JSON payload to inject.

  17. Show all issues

    Same notes about indentation in this file.

  18. Show all issues

    Blank line between blocks.

  19. Show all issues

    As above, let's have some central variables we can access for the URLs, so we can control it in one place and don't have to repeat.

    That'll also be safer, since we don't have any risk of injection attacks here.

  20. reviewboard/templates/accounts/login.html (Diff revision 7)
     
     
     
     
     
    Show all issues

    Template tags should be indented relative to {% block %}.

  21. 
      
maubin
maubin
chipx86
  1. Looking good! There's a few very small issues to fix, and some inconsistencies in variable names in JavaScript. But I think this is just about done :)

  2. reviewboard/accounts/views.py (Diff revisions 7 - 9)
     
     
     
     
     
     
     
     
    Show all issues

    Swap these around. Should go newest to oldest.

  3. reviewboard/accounts/views.py (Diff revisions 7 - 9)
     
     
     
     
     
    Show all issues

    This feels like it should be an independent block of work. The rest are just simply setting state, but this is generating, updating, and then setting state. It might be more readable as its own block.

  4. reviewboard/accounts/views.py (Diff revisions 7 - 9)
     
     
     
     
    Show all issues

    These lines are indented too far.

  5. reviewboard/accounts/views.py (Diff revisions 7 - 9)
     
     
     
    Show all issues

    Might be worth mentioning what happens if this is None.

  6. reviewboard/accounts/views.py (Diff revisions 7 - 9)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Alternatively, something like:

    if port:
        suffix = f':{port}'
    else:
        suffix = ''
    
    return {
        f'127.0.0.1{suffix}'
        f'localhost{suffix{'
    }
    

    Avoids having to loop over results and build a new set.

  7. reviewboard/static/rb/js/views/clientLoginView.es6.js (Diff revisions 7 - 9)
     
     
     
     
     
     
     
     
    Show all issues

    Just realized the naming conventions here. These should be _clientName, _clientURL, etc. in JavaScript.

  8. Show all issues

    Indentation is off on the description.

  9. Show all issues

    We have a RB.navigateTo that should be used here instead.

    If _redirect is for unit test purposes, you can remove it and spy on RB.navigateTo instead.

  10. Show all issues

    In this case, you can combine these statements:

    const redirectCounter = --this._redirectCounter;
    

    (And in this case, -- beforehand makes sense, because we're decrementing and then storing the result.)

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

Status: Closed (submitted)

Change Summary:

Pushed to release-5.0.x (aa70b43)
Loading...