Add a web-based logon flow for clients.
Review Request #12982 — Created May 5, 2023 and submitted
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 |
---|---|
fffedca30a9cf151b8f2a6b116b0fd1dcc9d6249 | |
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 … |
![]() |
|
'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 … |
|
|
Missing semicolon. Column: 48 Error code: W033 |
![]() |
|
Missing semicolon. Column: 61 Error code: W033 |
![]() |
|
While here, can you add a module docstring? |
|
|
Can you add a Version Changed and mention the addition of client-name and client-url? |
|
|
Swap these around. Should go newest to oldest. |
|
|
Since this is multi-line, let's do one value per line. |
|
|
Let's inverse the attribute logic here. Compute a local value, then set it on the attribute. |
|
|
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 … |
|
|
Second sentence should be in the description area. |
|
|
This feels like it should be an independent block of work. The rest are just simply setting state, but this … |
|
|
These are both present in LoginView as well. Maybe we should centralize a list of allowed hosts somewhere? |
|
|
Small nit: We can get more out of a line if we start the text on the next line, indented … |
|
|
These lines are indented too far. |
|
|
Might be worth mentioning what happens if this is None. |
|
|
Alternatively, something like: if port: suffix = f':{port}' else: suffix = '' return { f'127.0.0.1{suffix}' f'localhost{suffix{' } Avoids having to … |
|
|
Just realized the naming conventions here. These should be _clientName, _clientURL, etc. in JavaScript. |
|
|
This can be const. |
|
|
Because these are now in a .es6.js (which Babel will compile), you can now leave out the interpolate. I think … |
|
|
Can you postfix this instead? Put the -- at the end? Just a small nit, but is more consistent with … |
|
|
Indentation is off on the description. |
|
|
We have a RB.navigateTo that should be used here instead. If _redirect is for unit test purposes, you can remove … |
|
|
In this case, you can combine these statements: const redirectCounter = --this._redirectCounter; (And in this case, -- beforehand makes sense, … |
|
|
Let's indent the blocktrans (space inside the {%). |
|
|
Indentation of tags should be one level higher, since they're inside a {% block %}. |
|
|
Same here. |
|
|
All user input should be escaped. What I recommend is to have a get_js_view_data() function that returns everything that should … |
|
|
Same notes about indentation in this file. |
|
|
Blank line between blocks. |
|
|
As above, let's have some central variables we can access for the URLs, so we can control it in one … |
|
|
Template tags should be indented relative to {% block %}. |
|

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

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 aclient-name
parameter/variable. - Handles
WebAPITokenGenerationError
s during the client login flow
Testing Done: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
|||||||||||||||
Diff: |
Revision 4 (+2435 -15) |
|||||||||||||||
Added Files: |
Checks run (1 failed, 1 succeeded)
JSHint
-
reviewboard/static/rb/js/views/clientLoginView.es6.js (Diff revision 4) Missing semicolon. Column: 48 Error code: W033
-
reviewboard/static/rb/js/views/clientLoginView.es6.js (Diff revision 4) Missing semicolon. Column: 61 Error code: W033

Change Summary:
Missed some semicolons.
Commits: |
|
|||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+2435 -15) |
Checks run (2 succeeded)

Change Summary:
Small bug fix.
Commits: |
|
|||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+2441 -15) |
Checks run (2 succeeded)

Change Summary:
Removed the client name when we display the "Redirecting to <client> in 3..." countdown. It's possible
to redirect to a local URL so the wording should be more general.
Commits: |
|
|||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+2441 -15) |
Checks run (2 succeeded)
-
-
-
reviewboard/accounts/views.py (Diff revision 7) Can you add a
Version Changed
and mention the addition ofclient-name
andclient-url
? -
reviewboard/accounts/views.py (Diff revision 7) Since this is multi-line, let's do one value per line.
-
reviewboard/accounts/views.py (Diff revision 7) Let's inverse the attribute logic here. Compute a local value, then set it on the attribute.
-
reviewboard/accounts/views.py (Diff revision 7) Looks like we compute the
?client-name=%s&client-url=%s%s
three times in this class. Maybe we should precompute this indispatch
as an attribute and then reference it? Along with a flag that can be used instead of pulling out and checkingclient_name
andclient_url
. -
-
reviewboard/accounts/views.py (Diff revision 7) These are both present in
LoginView
as well. Maybe we should centralize a list of allowed hosts somewhere? -
reviewboard/accounts/views.py (Diff revision 7) Small nit: We can get more out of a line if we start the text on the next line, indented 1 level.
-
-
reviewboard/static/rb/js/views/clientLoginView.es6.js (Diff revision 7) Because these are now in a
.es6.js
(which Babel will compile), you can now leave out theinterpolate
. 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. -
reviewboard/static/rb/js/views/clientLoginView.es6.js (Diff revision 7) Can you postfix this instead? Put the
--
at the end? Just a small nit, but is more consistent with usual styling. -
reviewboard/templates/accounts/client_login.html (Diff revision 7) Let's indent the
blocktrans
(space inside the{%
). -
reviewboard/templates/accounts/client_login.html (Diff revision 7) Indentation of tags should be one level higher, since they're inside a
{% block %}
. -
-
reviewboard/templates/accounts/client_login.html (Diff revision 7) 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 fromel
), and then|json_dumps_items
that, so we get a safe JSON payload to inject. -
reviewboard/templates/accounts/client_login_confirm.html (Diff revision 7) Same notes about indentation in this file.
-
reviewboard/templates/accounts/client_login_confirm.html (Diff revision 7) Blank line between blocks.
-
reviewboard/templates/accounts/client_login_confirm.html (Diff revision 7) 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.
-
reviewboard/templates/accounts/login.html (Diff revision 7) Template tags should be indented relative to
{% block %}
.

Change Summary:
- Added JS tests for
ClientLoginView
. - Made sure user input (client name and client url) are escaped and safe to inject into templates.
- Added central variables/attributes for common URLs and things needed for the client logon flow.
- Added a centralized set for allowed client hosts.
- Fixed indentation issues in all of the templates.
Testing Done: |
|
|||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
|||||||||||||||||||||
Diff: |
Revision 8 (+3051 -15) |
Checks run (2 succeeded)

Change Summary:
- Set the default client API token expiration to 1 year instead of
None
.
Commits: |
|
|||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 9 (+3051 -15) |
Checks run (2 succeeded)
-
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 :)
-
-
reviewboard/accounts/views.py (Diff revisions 7 - 9) 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.
-
-
reviewboard/accounts/views.py (Diff revisions 7 - 9) Might be worth mentioning what happens if this is
None
. -
reviewboard/accounts/views.py (Diff revisions 7 - 9) 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.
-
reviewboard/static/rb/js/views/clientLoginView.es6.js (Diff revisions 7 - 9) Just realized the naming conventions here. These should be
_clientName
,_clientURL
, etc. in JavaScript. -
reviewboard/static/rb/js/views/clientLoginView.es6.js (Diff revisions 7 - 9) Indentation is off on the description.
-
reviewboard/static/rb/js/views/clientLoginView.es6.js (Diff revisions 7 - 9) We have a
RB.navigateTo
that should be used here instead.If
_redirect
is for unit test purposes, you can remove it and spy onRB.navigateTo
instead. -
reviewboard/static/rb/js/views/clientLoginView.es6.js (Diff revisions 7 - 9) 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.)

Commits: |
|
|||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+3033 -15) |