Add settings for client web-based logon flow.
Review Request #13045 — Created May 19, 2023 and updated
Information | |
---|---|
maubin | |
Review Board | |
release-5.0.x | |
13051, 12982 | |
Reviewers | |
reviewboard | |
This adds some settings to the Authentication settings page for the
upcoming client web-based logon flow. There is a checkbox for
enabling/disabling the flow, and a field for setting the expiration of
client API tokens.
- Ran unit tests.
- Manually tested the settings page, enabling and disabling the flow, putting
different expirations, saving the form.
Summary |
---|
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. |
|
-
-
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.