Add web-based logon.

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

Information

RBTools
release-5.x

Reviewers

This adds web-based logon for to RBTools. This allows users of RBTools 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 RBTools. To do this we spin up a
local server which redirects to Review Board's login page and then receives
an API token from the Review Board server to authenticate with.

  • Ran unit tests.
  • Manually tested the login flow with successful logins and failed logins.
  • Tested logging in again while logged in and logging out
  • Tested with automatically opening a browser (the -o flag).
  • Tested logging in/out with the traditional login flow.
  • Tested exiting the command.
  • Tested running two instances of the web login server at the same time.
    Saw that the servers ran on different ports and was able to complete
    the login flow on both.
  • Tested timing out the web login server.
  • Tested on RB5, RB6 and RB7.
  • Tested using a version that is too old to support web-based login.
  • Tested using a version that have the client_web_login server
    capabilities bug (5.0.5-5.0.7 and 6.0-6.0.2).
  • Tested using RB7 with the capabilities bug fix.
Summary ID
Add web based login.
2182d1057713651347ffa618c02240d58d179eb7
Description From Last Updated

There's one design consideration we should figure out. This all works great for the case of rbt commands, but RBTools …

chipx86chipx86

I'm not sure of how I should add unit tests for this request handler. I'm not sure how to mock …

maubinmaubin

line too long (80 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

): should be on the next line. Can you update this to specify types while here?

chipx86chipx86

Let's require keyword arguments here. Helps with letting us add/rearrange arguments as we go without breaking API.

chipx86chipx86

These are missing documentation.

chipx86chipx86

, optional

chipx86chipx86

I think we should use the same user agent we use for API requests (defined in api/request.py), so we can …

chipx86chipx86

Let's make these keyword-only. Same with ones below.

chipx86chipx86

Version Added will show as part of the description, so this will need to be above Args.

chipx86chipx86

For logging statements, we should avoid f-strings and instead pass parameters to the logging function. These get tracked separately, allowing …

chipx86chipx86

Blank line between statements and blocks.

chipx86chipx86

Since we're repeating this block of code, we should have a standard function somewhere for opening pages in a browser.

chipx86chipx86

This should go before Returns.

chipx86chipx86

Can we use constants to define this range? Even if just as arguments to this function. That'll help document them …

chipx86chipx86

Hmm. Instead of "traditional", maybe "console" or "terminal"?

daviddavid

I think I'd prefer to revert this change.

daviddavid

Type here doesn't match the type hint.

daviddavid

Typo: loggin -> logging

daviddavid

Let's plumb the capabilities object in the command through to here, so we can use capabilities.has_capability('authentication', 'client_web_login')

daviddavid

Too many logins. Let's say "The login command now directs the user to log in via the Review Board web …

daviddavid

And here.

daviddavid

Could do: if capabilities is None: capabilities = Capabilities(server_info.capabilities) return capabilities.has_capability(...)

daviddavid

This is technically not a Python Standard Library, so it should go in a 3rd-party group.

chipx86chipx86

"shut down"

chipx86chipx86

Types should go on the next line, indented 4. Same below.

chipx86chipx86

Summary must be a single line. Same with ones below.

chipx86chipx86

Let's call this timeout_time_secs, so it's always clear what the unit is.

chipx86chipx86

Let's add a unit of time here as well. Does it need to be a float?

chipx86chipx86

We should just use the local variables here.

chipx86chipx86

Both of these should be , optional.

chipx86chipx86

I'd rather we avoid a SystemExit here, since potentially this is being used in a third-party codebase. It'd be better …

chipx86chipx86

Same comment as above re: the type being on the next line. This applies throughout the file.

chipx86chipx86

Missing docs for this.

chipx86chipx86

There's an extra blank line here.

chipx86chipx86

This string should use single quotes. Also noticed that the casing is different than the other headers. Pretty sure these …

chipx86chipx86

This function doesn't have a return type, so no need for the return None.

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

maubin
  1. 
      
  2. rbtools/utils/web_login_server.py (Diff revision 1)
     
     
    Show all issues

    I'm not sure of how I should add unit tests for this request handler. I'm not sure how to mock an HTTPServer in order to test things, or if I should just run a server on localhost:8000 or something during the tests.

  3. 
      
maubin
chipx86
  1. 
      
  2. Show all issues

    There's one design consideration we should figure out. This all works great for the case of rbt commands, but RBTools can also be consumed via services that may need to talk to multiple Review Board servers (in theory) and may or may not be multi-threaded.

    Right now, the web server and its state are global, so the first login to a server wins. There's also only one server that can run at a time. And we have to block on response.

    I think an alternative approach could provide wider compatibility with different use cases:

    1. Have a class that manages all this state and the server. WebLoginManager or something. This would contain the functionality for creating a server, starting it, and querying successful login.

    2. Allow this to take a callback function for when login is successful, which is called immediately after setting self.login_successful.

    3. Add a wait_successful() method or something that can do the blocking loop waiting for success/fail (you'll probably want a time.sleep(0.1) to avoid high-CPU usage), making it easy to use for the rbt case.

    4. Introduce timeouts. This should probably be on WebLoginManager. After a certain amount of time, the server shuts down and any blocking operations give up. This avoids the situation where a background process outputs a URL and just stalls for ever, and covers the blocking loop and async results usages.

    This would allow for multiple (even concurrent) web login processes, and avoid global state to avoid threading issues. It'd also give async processes a way to wait for successful login/timeout (important for UI processes, such as possible IDE integrations)

  3. rbtools/utils/users.py (Diff revision 2)
     
     
    Show all issues

    ): should be on the next line.

    Can you update this to specify types while here?

  4. rbtools/utils/web_login_server.py (Diff revision 2)
     
     
     
    Show all issues

    Let's require keyword arguments here. Helps with letting us add/rearrange arguments as we go without breaking API.

  5. rbtools/utils/web_login_server.py (Diff revision 2)
     
     
     
    Show all issues

    These are missing documentation.

  6. rbtools/utils/web_login_server.py (Diff revision 2)
     
     
    Show all issues

    , optional

  7. rbtools/utils/web_login_server.py (Diff revision 2)
     
     
    Show all issues

    I think we should use the same user agent we use for API requests (defined in api/request.py), so we can more easily trace user agent activity in logs.

  8. rbtools/utils/web_login_server.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    Let's make these keyword-only. Same with ones below.

  9. rbtools/utils/web_login_server.py (Diff revision 2)
     
     
     
     
    Show all issues

    Version Added will show as part of the description, so this will need to be above Args.

  10. rbtools/utils/web_login_server.py (Diff revision 2)
     
     
     
    Show all issues

    For logging statements, we should avoid f-strings and instead pass parameters to the logging function. These get tracked separately, allowing the message and the arguments to be pulled out separately by logging implementations.

    Same elsewhere.

  11. rbtools/utils/web_login_server.py (Diff revision 2)
     
     
     
    Show all issues

    Blank line between statements and blocks.

  12. rbtools/utils/web_login_server.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Since we're repeating this block of code, we should have a standard function somewhere for opening pages in a browser.

  13. rbtools/utils/web_login_server.py (Diff revision 2)
     
     
     
     
    Show all issues

    This should go before Returns.

  14. rbtools/utils/web_login_server.py (Diff revision 2)
     
     
    Show all issues

    Can we use constants to define this range? Even if just as arguments to this function. That'll help document them a bit better.

  15. 
      
maubin
david
  1. 
      
  2. rbtools/commands/login.py (Diff revision 3)
     
     
     
    Show all issues

    Hmm. Instead of "traditional", maybe "console" or "terminal"?

  3. rbtools/utils/users.py (Diff revision 3)
     
     
    Show all issues

    I think I'd prefer to revert this change.

  4. rbtools/utils/web_login.py (Diff revision 3)
     
     
    Show all issues

    Type here doesn't match the type hint.

  5. 
      
maubin
david
  1. Ship It!
  2. 
      
maubin
maubin
david
  1. 
      
  2. rbtools/commands/login.py (Diff revisions 4 - 6)
     
     
    Show all issues

    Typo: loggin -> logging

  3. rbtools/utils/web_login.py (Diff revisions 4 - 6)
     
     
    Show all issues

    Let's plumb the capabilities object in the command through to here, so we can use capabilities.has_capability('authentication', 'client_web_login')

    1. I'll do this, but in order to not break compatibility for get_authenticated_session() I'll have to make capabilities an optional argument that defaults to None. And when that gets passed all the way to here I'll fall back on using server_info.capabilities if the given capabilities is None.

  4. docs/rbtools/rbt/commands/login.rst (Diff revision 6)
     
     
     
    Show all issues

    Too many logins.

    Let's say "The login command now directs the user to log in via the Review Board web page, instead of ..."

  5. docs/rbtools/rbt/commands/login.rst (Diff revision 6)
     
     
     
    Show all issues

    And here.

  6. 
      
maubin
david
  1. 
      
  2. rbtools/utils/web_login.py (Diff revisions 6 - 7)
     
     
     
     
     
     
     
     
    Show all issues

    Could do:

    if capabilities is None:
        capabilities = Capabilities(server_info.capabilities)
    
    return capabilities.has_capability(...)
    
  3. 
      
maubin
david
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. rbtools/utils/web_login.py (Diff revision 8)
     
     
    Show all issues

    This is technically not a Python Standard Library, so it should go in a 3rd-party group.

  3. rbtools/utils/web_login.py (Diff revision 8)
     
     
    Show all issues

    "shut down"

  4. rbtools/utils/web_login.py (Diff revision 8)
     
     
    Show all issues

    Types should go on the next line, indented 4.

    Same below.

  5. rbtools/utils/web_login.py (Diff revision 8)
     
     
     
    Show all issues

    Summary must be a single line.

    Same with ones below.

  6. rbtools/utils/web_login.py (Diff revision 8)
     
     
    Show all issues

    Let's call this timeout_time_secs, so it's always clear what the unit is.

  7. rbtools/utils/web_login.py (Diff revision 8)
     
     
    Show all issues

    Let's add a unit of time here as well.

    Does it need to be a float?

  8. rbtools/utils/web_login.py (Diff revision 8)
     
     
     
    Show all issues

    We should just use the local variables here.

  9. rbtools/utils/web_login.py (Diff revision 8)
     
     
     
     
     
    Show all issues

    Both of these should be , optional.

  10. rbtools/utils/web_login.py (Diff revision 8)
     
     
    Show all issues

    I'd rather we avoid a SystemExit here, since potentially this is being used in a third-party codebase. It'd be better to have a more relevant exception to raise.

  11. rbtools/utils/web_login.py (Diff revision 8)
     
     
    Show all issues

    Same comment as above re: the type being on the next line.

    This applies throughout the file.

  12. rbtools/utils/web_login.py (Diff revision 8)
     
     
    Show all issues

    Missing docs for this.

  13. rbtools/utils/web_login.py (Diff revision 8)
     
     
     
     
     
    Show all issues

    There's an extra blank line here.

  14. rbtools/utils/web_login.py (Diff revision 8)
     
     
    Show all issues

    This string should use single quotes.

    Also noticed that the casing is different than the other headers. Pretty sure these get normalized, but for consistency, maybe we want to use Content-Type here.

  15. rbtools/utils/web_login.py (Diff revision 8)
     
     
     
    Show all issues

    This function doesn't have a return type, so no need for the return None.

  16. 
      
maubin
david
  1. Ship It!
  2. 
      
chipx86
  1. Ship It!
  2. 
      
maubin
Review request changed
Status:
Completed
Change Summary:
Pushed to release-5.x (0119859)