Add settings for client web-based logon flow.

Review Request #13045 — Created May 19, 2023 and updated

maubin
Review Board
release-5.0.x
13051, 12982
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
Support web-based login for clients.
Add settings for client web-based logon.

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

chipx86chipx86

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 …

chipx86chipx86

No need for parens here.

chipx86chipx86

Formatting should be more like: widget=AmountSelectorWidget( unit_choices=[ ... ], number_attrs={ ... } ) That way there's no confusion about which …

chipx86chipx86

I don't think anything on this form affects Django settings, so we can probably skip this.

chipx86chipx86

Can we pull these out into a siteconfig_settings variable that get passed in? That'll help avoid the sort of weirdness …

chipx86chipx86

This should be assertIsNone(). Same with similar assertions below.

chipx86chipx86

No trailing period here. Same below.

chipx86chipx86
Checks run (2 succeeded)
flake8 passed.
JSHint passed.
chipx86
  1. 
      
  2. Two things to consider here:

    1. We don't use "logon" anywhere else in the codebase. "Authentication" is probably more correct here.
    2. "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.

  3. 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()
    
  4. No need for parens here.

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

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

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

  8. This should be assertIsNone(). Same with similar assertions below.

  9. No trailing period here. Same below.

  10. 
      
Loading...