Add web-based logon.

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

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.
Summary ID
Add web based login.
4ef1b2f41b5e863395ac0a3fca6de048a27e483a
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
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

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

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

    ): 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)
     
     
     

    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)
     
     
     

    These are missing documentation.

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

    , optional

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

    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)
     
     
     
     
     

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

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

    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)
     
     
     

    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)
     
     
     

    Blank line between statements and blocks.

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

    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)
     
     
     
     

    This should go before Returns.

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

    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)
     
     
     

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

  3. rbtools/utils/users.py (Diff revision 3)
     
     

    I think I'd prefer to revert this change.

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

    Type here doesn't match the type hint.

  5. 
      
maubin
Review request changed

Commits:

Summary ID
Add web based login.
2331634b5909c7d68b20584faeca064ad7c48d48
Add web based login.
4ef1b2f41b5e863395ac0a3fca6de048a27e483a

Diff:

Revision 4 (+1892 -24)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
david
  1. Ship It!
  2. 
      
Loading...