Add settings for client web-based logon flow.

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

Information

Review Board
release-5.0.x

Reviewers

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
Add support for client API tokens.
a2ff67c1013efcbe576b0880500c894fd321ea8e
Support web-based login for clients.
6e448f9b47d091acf624cef63bee6aff1df4b198
Support web-based login for clients.
635733e2536f8c06a78768af015ca5df6d558f9a
Add settings for client web-based logon.
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. …

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

line too long (80 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot
chipx86
  1. 
      
  2. Show all issues

    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)
     
     
     
     
     
     
     
     
    Show all issues

    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()
    
    1. I'm going to drop this since I'm no longer using the subform pattern for the API authentication settings.

  4. Show all issues

    No need for parens here.

  5. reviewboard/admin/forms/auth_settings.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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)
     
     
     
    Show all issues

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

  7. Show all issues

    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. Show all issues

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

  9. Show all issues

    No trailing period here. Same below.

  10. 
      
maubin
Review request changed

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:

   

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.

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

Testing Done:

   
  • Ran unit tests.
~  
  • Manually tested the settings page, enabling and disabling the flow, putting
    different expirations, saving the form.
  ~
  • Manually tested the settings page putting different expirations,
    saving the form, playing with other settings.

Commits:

Summary ID
Support web-based login for clients.
0540962a83a1b9e1f652c60a13a4cdc22ea5e46c
Add settings for client web-based logon.
283fd7d68173d9a5d14e13e117aee8e3d094b180
Add support for client API tokens.
91be88b04b398689dd794e53111cbe9ba8dfc658
Support web-based login for clients.
8b7d7bbe83a258088b0786b28f831924809e4096
Support web-based login for clients.
aea87d105b6dc931e9c68d106db00841cccc0fcd
Add settings for client web-based logon.
ec11605e1bb459b6dbde38b40b4e745451201d94

Diff:

Revision 2 (+281 -15)

Show changes

Removed Files:

Added Files:

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
david
  1. Ship It!
  2. 
      
chipx86
  1. Ship It!
  2. 
      
maubin
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-5.0.x (4d0e851)
Loading...