Add settings for client web-based logon flow.
Review Request #13045 — Created May 19, 2023 and submitted
This adds some settings to the Authentication settings page for the
upcoming client web-based logon flow. There is a new section for
general API Authentication settings, with a field for setting the
automatic expiration of client API tokens.
- Ran unit tests.
- Manually tested the settings page putting different expirations,
saving the form, playing with other settings.
Summary | ID |
---|---|
a2ff67c1013efcbe576b0880500c894fd321ea8e | |
6e448f9b47d091acf624cef63bee6aff1df4b198 | |
635733e2536f8c06a78768af015ca5df6d558f9a | |
38cc49b61b5c553c4f4720a42dcded074528002d |
Description | From | Last Updated |
---|---|---|
Two things to consider here: We don't use "logon" anywhere else in the codebase. "Authentication" is probably more correct here. … |
|
|
Might make sense at this point to do: for forms in (self._iter_sso_backend_forms(), self._iter_client_web_login_forms()): for form, enable_field_id in forms: ... Same … |
|
|
No need for parens here. |
|
|
Formatting should be more like: widget=AmountSelectorWidget( unit_choices=[ ... ], number_attrs={ ... } ) That way there's no confusion about which … |
|
|
I don't think anything on this form affects Django settings, so we can probably skip this. |
|
|
Can we pull these out into a siteconfig_settings variable that get passed in? That'll help avoid the sort of weirdness … |
|
|
This should be assertIsNone(). Same with similar assertions below. |
|
|
No trailing period here. Same below. |
|
|
line too long (80 > 79 characters) Column: 80 Error code: E501 |
![]() |
-
-
Two things to consider here:
- We don't use "logon" anywhere else in the codebase. "Authentication" is probably more correct here.
- "Client" is so general a term that I'm not sure admins are going to understand what this is for as-is.
I think a description would help, and we should mention RBTools in there.
Thinking about the title, though... hmm.. Maybe we make this more broad. Something like:
API Authentication Settings =========================== <description> Automatic token expiration: [...]
This gives us a place to expand into if we need to do anything else with authentication later. For example, if we decide to start making password-based authentication optional down the road.
-
reviewboard/admin/forms/auth_settings.py (Diff revision 1) Might make sense at this point to do:
for forms in (self._iter_sso_backend_forms(), self._iter_client_web_login_forms()): for form, enable_field_id in forms: ...
Same with the other form loops below.
In fact, maybe we want to have a method that iterates all these subforms so all these loops dealing with subforms can remain simple, automatically handling new ones going forward? It could do:
yield from self._iter_sso_backend_forms() yield from self._iter_client_web_login_forms()
-
-
reviewboard/admin/forms/auth_settings.py (Diff revision 1) Formatting should be more like:
widget=AmountSelectorWidget( unit_choices=[ ... ], number_attrs={ ... } )
That way there's no confusion about which parts of this belong to the
IntegerField
vs. the widget. -
reviewboard/admin/forms/auth_settings.py (Diff revision 1) I don't think anything on this form affects Django settings, so we can probably skip this.
-
reviewboard/admin/tests/test_web_client_logon_settings_form.py (Diff revision 1) Can we pull these out into a
siteconfig_settings
variable that get passed in? That'll help avoid the sort of weirdness of the formatting here. -
reviewboard/admin/tests/test_web_client_logon_settings_form.py (Diff revision 1) This should be
assertIsNone()
. Same with similar assertions below. -
reviewboard/admin/tests/test_web_client_logon_settings_form.py (Diff revision 1) No trailing period here. Same below.

Change Summary:
- Removed the checkbox for enabling/disabling web-based login for clients.
- Improved the language in the settings so that it's less about "client"
authentication and more about general API authentication for services.
Description: |
|
|||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||
Commits: |
|
|||||||||||||||||||||
Diff: |
Revision 2 (+281 -15) |
|||||||||||||||||||||
Removed Files: |
||||||||||||||||||||||
Added Files: |
Checks run (1 failed, 1 succeeded)
flake8
-
reviewboard/admin/tests/test_authentication_settings_form.py (Diff revision 2) line too long (80 > 79 characters) Column: 80 Error code: E501

Commits: |
|
|||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+283 -15) |