Add a web-based logon flow for clients.

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

maubin
Review Board
release-5.0.x
13032
13045
reviewboard

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
Summary
Support web-based login for clients.

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
maubin
  1. 
      
  2. reviewboard/accounts/views.py (Diff revision 1)
     
     
     
     
     
     
     
     

    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
-
Support web-based login for RBTools.
+
Support web-based login for clients.

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
Review request changed
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. Blank line between these.

  3. Can you add -> None to each of these?

  4. 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. Blank line between this block and statement.

  6. Can you add -> None to each of these?

    Same with other test files.

  7. The docstring's indented too far.

  8. Blank line here.

  9. Blank line here.

  10. Blank line here.

  11. Blank line here.

  12. Blank line here.

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

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

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

    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)
     
     

    No need for the outer parens.

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

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

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

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

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

    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)
     
     
     
     
     
     

    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)
     
     
     
     
     
     

    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)
     
     
     

    Let's call this client_name.

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

    No need for parens here.

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

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

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

    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)
     
     
     
     
     

    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)
     
     
     

    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)
     
     
     
     
     

    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)
     
     

    Missing a trailing comma.

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

    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)
     
     
     

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

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

    Same comments as above.

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

    Same comments re: pulling out request.

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

    Maybe support a client-state here as well?

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

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

    Same with template tags below and in other files.

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

  36. 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. 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)
     
     
     
     

    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. Missing a trailing comma.

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

    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)
     
     
     

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

    Also, no space after value.

  42. 
      
Loading...