Add a web-based logon flow for clients.
Review Request #12982 — Created May 5, 2023 and updated
Information | |
---|---|
maubin | |
Review Board | |
release-5.0.x | |
13032 | |
13045 | |
Reviewers | |
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 |
---|
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 … |
![]() |
|
'datetime' imported but unused Column: 1 Error code: F401 |
![]() |
|
'json' imported but unused Column: 1 Error code: F401 |
![]() |
|
'django.contrib.auth.models.User' imported but unused Column: 1 Error code: F401 |
![]() |
|
'django.utils.timezone' imported but unused Column: 1 Error code: F401 |
![]() |
|
Blank line between these. |
|
|
Can you add -> None to each of these? |
|
|
We're a bit all over the place with the formatting of view docstrings, but we mostly settle on Testing ViewName … |
|
|
Blank line between this block and statement. |
|
|
Can you add -> None to each of these? Same with other test files. |
|
|
The docstring's indented too far. |
|
|
Blank line here. |
|
|
Blank line here. |
|
|
Blank line here. |
|
|
Blank line here. |
|
|
Blank line here. |
|
|
While here, can you add a __future__ ... annotations import? |
|
|
Can you add typing to these? In fact, we should move these to the class body as instance variables, document … |
|
|
No need for the outer parens. |
|
|
Can we name this client-name? (I made some comments about why on /r/13032). |
|
|
request is local, so we don't need to access self.request here. |
|
|
To better convey intent here (this is only for GET requests), let's check the method first. |
|
|
We should probably preserve ?next=, in case the caller wants to eventually redirect back somewhere. In fact, I wonder if … |
|
|
I think dispatch should be responsible for pulling these out for other methods to use. State set on self within … |
|
|
Let's call this client_name. |
|
|
No need for parens here. |
|
|
We can ditch the else, since it's implied. |
|
|
We access self.request a lot, so let's pull it out so we don't have to perform attribute lookups more than … |
|
|
This is a bit tricky to read. Let's pull this out into a variable and then reference that in the … |
|
|
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 … |
|
|
I just made a comment on /r/13032/ about this, but generate_token() can raise a WebAPITokenGenerationError, which would bubble up to … |
|
|
Missing a trailing comma. |
|
|
To keep this consistent, how about prefixing this with client? And maybe flip it, so it's client_allowed = [True,False]? |
|
|
For logging-related statements, pass arguments as positional arguments, rather than using %. |
|
|
Same comments as above. |
|
|
Same comments re: pulling out request. |
|
|
Maybe support a client-state here as well? |
|
|
No space before the {% blocktrans %}. Indentation happens within the {% ... %}. Same with template tags below and … |
|
|
Let's put a blank line between these blocks, so they don't blur together. |
|
|
Instead of "Blocking log in" (which I feel may be misunderstood by some users), let's try to help them out … |
|
|
We should put this in scripts-post, so it doesn't block anything on page load. |
|
|
We know the value of this variable at template time, so let's use it to wire off the entire <script>, … |
|
|
Missing a trailing comma. |
|
|
We can't safely use these template literals in here. We'd have to call interpolate(gettext('...', [params...])) directly. We also can't localize … |
|
|
No need for the />. That's a legacy holdover that we don't need to continue with. Also, no space after … |
|

-
-
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?

Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Depends On: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+1386 -12) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
Checks run (1 failed, 1 succeeded)
flake8
-
reviewboard/accounts/tests/test_client_login_confirmation_view.py (Diff revision 2) 'datetime' imported but unused Column: 1 Error code: F401
-
reviewboard/accounts/tests/test_client_login_confirmation_view.py (Diff revision 2) 'json' imported but unused Column: 1 Error code: F401
-
reviewboard/accounts/tests/test_client_login_confirmation_view.py (Diff revision 2) 'django.contrib.auth.models.User' imported but unused Column: 1 Error code: F401
-
reviewboard/accounts/tests/test_client_login_confirmation_view.py (Diff revision 2) 'django.utils.timezone' imported but unused Column: 1 Error code: F401

Change Summary:
Got rid of some unused imports.
Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+1374 -12) |
Checks run (2 succeeded)
-
-
reviewboard/accounts/tests/test_client_login_confirmation_view.py (Diff revision 3) Blank line between these.
-
reviewboard/accounts/tests/test_client_login_confirmation_view.py (Diff revision 3) Can you add
-> None
to each of these? -
reviewboard/accounts/tests/test_client_login_confirmation_view.py (Diff revision 3) 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. -
reviewboard/accounts/tests/test_client_login_confirmation_view.py (Diff revision 3) Blank line between this block and statement.
-
reviewboard/accounts/tests/test_client_login_view.py (Diff revision 3) Can you add
-> None
to each of these?Same with other test files.
-
reviewboard/accounts/tests/test_client_login_view.py (Diff revision 3) The docstring's indented too far.
-
-
-
-
-
-
reviewboard/accounts/views.py (Diff revision 3) While here, can you add a
__future__ ... annotations
import? -
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).
-
-
reviewboard/accounts/views.py (Diff revision 3) Can we name this
client-name
? (I made some comments about why on /r/13032). -
reviewboard/accounts/views.py (Diff revision 3) request
is local, so we don't need to accessself.request
here. -
reviewboard/accounts/views.py (Diff revision 3) To better convey intent here (this is only for GET requests), let's check the method first.
-
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.
-
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 aView
subclass will only live for the lifetime of the request. -
-
-
-
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. -
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
. -
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? -
reviewboard/accounts/views.py (Diff revision 3) I just made a comment on /r/13032/ about this, but
generate_token()
can raise aWebAPITokenGenerationError
, which would bubble up to here. We should handle this gracefully. -
-
reviewboard/accounts/views.py (Diff revision 3) To keep this consistent, how about prefixing this with
client
? And maybe flip it, so it'sclient_allowed = [True,False]
? -
reviewboard/accounts/views.py (Diff revision 3) For logging-related statements, pass arguments as positional arguments, rather than using
%
. -
-
-
-
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.
-
reviewboard/templates/accounts/client_login.html (Diff revision 3) Let's put a blank line between these blocks, so they don't blur together.
-
reviewboard/templates/accounts/client_login.html (Diff revision 3) 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?
-
reviewboard/templates/accounts/client_login.html (Diff revision 3) We should put this in
scripts-post
, so it doesn't block anything on page load. -
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.
-
-
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. -
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
.