Add SAML 2.0 SSO.

Review Request #12254 — Created April 24, 2022 and updated

david
Review Board
release-5.0.x
reviewboard

This change adds SAML 2.0 support for single-sign-on authentication. It
includes a few major pieces:

  • Infrastructure for SSO backends including registry, configuration
    machinery, and URL registration.
  • Updates to settings forms to allow me to toggle some subforms based on
    a checkbox enabler rather than via drop-downs.
  • SAML 2.0 backend which includes configuration, login/logout, and user
    provisioning.

The vast majority of the settings available are specific to SAML,
including the various URLs and method settings. Because of some user
feedback we've already recieved, I've added a toggle to control the
behavior of user provisioning. In most cases, the person using this has
absolute trust in the integrity and correctness of their IdP, but in
some cases that may not be true. If the "username" parameter can not
necessarily be trusted, there's an option to force existing users to log
in with their Review Board password at least once.

Testing was done with a test application on onelogin.com with the
relevant configuration to authenticate to my server on localhost.

  • Logged in with existing user that had a username match.
  • Logged in with user that had an email match and saw correct detection
    of the existing user.
  • Logged in with a user that had no existing match and saw provision
    screen. Proceeded and was logged in with a new user with correct
    name/email/etc.
  • Tested behavior of "require login" toggle.
  • Logged out from onelogin portal and saw GET request to the SLS
    endpoint which flushed the session and then redirected back to the
    onelogin login page.
  • Tested authentication configuration page, enabling/disabling settings,
    and making changes to settings. Tested behavior when the
    python3-saml package wasn't installed.
  • Tested text of button on login page, and that it redirected to the SSO
    login flow.
  • Ran unit tests.
Summary
Add SAML 2.0 SSO.

Description From Last Updated

Looks good. Since this is WIP, I won't give it a Ship It!, but we're ready to move onto the ...

chipx86chipx86

Do we need to do this, or can we rely on lazy population in the registry?

chipx86chipx86

Blank line between these.

chipx86chipx86

extra_data is worth having, but I think we may want to include something specific for credential data that we'd never ...

chipx86chipx86

Missing docstring.

chipx86chipx86

Rather than this format, cna we do: for _module, _backend_cls_name in (('saml.sso_backend', 'SAMLSSOBackend'),): ... Or define the list separately.

chipx86chipx86

Missing docs in this file.

chipx86chipx86

This should be after __init__.

chipx86chipx86

F821 undefined name 'ModuleNotFoundError'

reviewbotreviewbot

conf before contrib.

chipx86chipx86

404 before Response.

chipx86chipx86

These are in reverse order.

chipx86chipx86

These are in reverse order.

chipx86chipx86

Same with these.

chipx86chipx86

Missing docs.

chipx86chipx86

We should have some else case, even if that's just raising an exception.

chipx86chipx86

No need to .save() after .create().

chipx86chipx86

Shouldn't have a trailing comma for a function call.

chipx86chipx86

Missing a Version Added.

chipx86chipx86

Missing a Version Added. We should put them in the functions as well. Same comments for other new modules.

chipx86chipx86

These should be swapped.

chipx86chipx86

Let's wrap any calls in an exception, in case any extension-provided backends (or ours) ever misbehave.

chipx86chipx86

Should we conditionally save ones with _enabled?

chipx86chipx86

Typo: flieds.

chipx86chipx86

Col: 40 Missing semicolon.

reviewbotreviewbot

Missing ;

chipx86chipx86

Col: 64 Missing semicolon.

reviewbotreviewbot

Missing ;

chipx86chipx86

Thes should be indented 1 space. No need for />. Same elsewhere. These apply to the other templates as well.

chipx86chipx86

F821 undefined name 'ModuleNotFoundError'

reviewbotreviewbot

F401 'django.contrib.auth.login' imported but unused

reviewbotreviewbot

F401 'reviewboard.accounts.backends.standard.StandardAuthBackend' imported but unused

reviewbotreviewbot

F401 'django.utils.translation.gettext_lazy as _' imported but unused

reviewbotreviewbot
Checks run (2 failed)
flake8 failed.
JSHint failed.

flake8

JSHint

chipx86
  1. I think this all looks really good. There are a lot of moving pieces, so I'm not sure how much I've internalized (it would be nice to ultimately break this out into a change for the subform code, and then maybe SSO vs. SAML implementation), but nothing stands out as wrong.

    I have a handful of comments in here. Most are things like doc fixes or style/ordering fixes, which I stopped providing because I know it's WIP. Only a few are on design.

  2. reviewboard/accounts/__init__.py (Diff revision 1)
     
     
     
     
     

    Do we need to do this, or can we rely on lazy population in the registry?

    1. I prefer to have this explicit so we can be 100% sure that URLs are registered.

  3. reviewboard/accounts/models.py (Diff revision 1)
     
     
     

    Blank line between these.

  4. reviewboard/accounts/models.py (Diff revision 1)
     
     

    extra_data is worth having, but I think we may want to include something specific for credential data that we'd never want to expose via the API. HostingServiceAccount has a data = JSONField() where those go. Maybe a service_data = would be appropriate.

  5. Missing docstring.

  6. reviewboard/accounts/sso/backends/registry.py (Diff revision 1)
     
     
     
     

    Rather than this format, cna we do:

    for _module, _backend_cls_name in (('saml.sso_backend',
                                        'SAMLSSOBackend'),):
        ...
    

    Or define the list separately.

  7. Missing docs in this file.

  8. This should be after __init__.

  9. reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1)
     
     
     
     
     

    conf before contrib.

  10. reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1)
     
     
     
     
     
     

    404 before Response.

  11. These are in reverse order.

  12. These are in reverse order.

  13. Same with these.

  14. reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1)
     
     
     
     
     
     
     

    We should have some else case, even if that's just raising an exception.

  15. reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1)
     
     
     
     
     
     
     

    No need to .save() after .create().

  16. reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1)
     
     
     
     
     

    Shouldn't have a trailing comma for a function call.

  17. reviewboard/accounts/sso/errors.py (Diff revision 1)
     
     

    Missing a Version Added.

  18. reviewboard/accounts/sso/users.py (Diff revision 1)
     
     

    Missing a Version Added. We should put them in the functions as well.

    Same comments for other new modules.

  19. These should be swapped.

  20. Let's wrap any calls in an exception, in case any extension-provided backends (or ours) ever misbehave.

  21. reviewboard/admin/forms/auth_settings.py (Diff revision 1)
     
     
     

    Should we conditionally save ones with _enabled?

  22. Typo: flieds.

  23. Thes should be indented 1 space.

    No need for />. Same elsewhere.

    These apply to the other templates as well.

  24. 
      
david
Review request changed

Description:

   

This is the preliminary change for adding SAML 2.0 authentication. It

    includes a few major pieces:

   
   
  • Infrastructure for SSO backends including registry, configuration
    machinery, and URL registration.
   
  • Updates to settings forms to allow me to toggle some subforms based on
    a checkbox enabler rather than via drop-downs.
   
  • SAML 2.0 backend which includes configuration, login/logout, and user
    provisioning.
   
   

The vast majority of the settings available are specific to SAML,

    including the various URLs and method settings. Because of some user
    feedback we've already recieved, I've added a toggle to control the
    behavior of user provisioning. In most cases, the person using this has
    absolute trust in the integrity and correctness of their IdP, but in
    some cases that may not be true. If the "username" parameter can not
    necessarily be trusted, there's an option to force existing users to log
    in with their Review Board password at least once.

   
   

What's left to do:

   
   
  • "Login with SAML" button on the login screen.
   
  • Lots more unit testing.
   
  • Some refactoring of the link-user view to extract common functionality
    for other backends.
   
  • User/admin manual.
   
  • Ensure codebase docs are correct and consistent.
-  
  • Test behavior when python3-saml isn't installed.

Testing Done:

   

Testing was done with a test application on onelogin.com with the

    relevant configuration to authenticate to my server on localhost.

   
   
  • Logged in with existing user that had a username match.
   
  • Logged in with user that had an email match and saw correct detection
    of the existing user.
   
  • Logged in with a user that had no existing match and saw provision
    screen. Proceeded and was logged in with a new user with correct
    name/email/etc.
   
  • Tested behavior of "require login" toggle.
   
  • Logged out from onelogin portal and saw GET request to the SLS
    endpoint which flushed the session and then redirected back to the
    onelogin login page.
  +
  • Tested authentication configuration page, enabling/disabling settings,
    and making changes to settings. Tested behavior when the
    python3-saml package wasn't installed.

Commits:

Summary
-
[WIP] Add SAML 2.0 SSO.
+
[WIP] Add SAML 2.0 SSO.

Diff:

Revision 2 (+3742 -42)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
david
Review request changed

Change Summary:

  • Docstrings
  • Use common login_user method.

Description:

   

This is the preliminary change for adding SAML 2.0 authentication. It

    includes a few major pieces:

   
   
  • Infrastructure for SSO backends including registry, configuration
    machinery, and URL registration.
   
  • Updates to settings forms to allow me to toggle some subforms based on
    a checkbox enabler rather than via drop-downs.
   
  • SAML 2.0 backend which includes configuration, login/logout, and user
    provisioning.
   
   

The vast majority of the settings available are specific to SAML,

    including the various URLs and method settings. Because of some user
    feedback we've already recieved, I've added a toggle to control the
    behavior of user provisioning. In most cases, the person using this has
    absolute trust in the integrity and correctness of their IdP, but in
    some cases that may not be true. If the "username" parameter can not
    necessarily be trusted, there's an option to force existing users to log
    in with their Review Board password at least once.

   
   

What's left to do:

   
   
  • "Login with SAML" button on the login screen.
   
  • Lots more unit testing.
   
  • Some refactoring of the link-user view to extract common functionality
    for other backends.
   
  • User/admin manual.
-  
  • Ensure codebase docs are correct and consistent.

Commits:

Summary
-
[WIP] Add SAML 2.0 SSO.
+
[WIP] Add SAML 2.0 SSO.

Diff:

Revision 4 (+3890 -42)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. 
      
  2. Looks good. Since this is WIP, I won't give it a Ship It!, but we're ready to move onto the next stage.

  3. 
      
david
david
david
david
Review request changed

Change Summary:

Out of WIP.

Summary:

-[WIP] Add SAML 2.0 SSO.
+Add SAML 2.0 SSO.

Description:

~  

This is the preliminary change for adding SAML 2.0 authentication. It

  ~

This change adds SAML 2.0 support for single-sign-on authentication. It

    includes a few major pieces:

   
   
  • Infrastructure for SSO backends including registry, configuration
    machinery, and URL registration.
   
  • Updates to settings forms to allow me to toggle some subforms based on
    a checkbox enabler rather than via drop-downs.
   
  • SAML 2.0 backend which includes configuration, login/logout, and user
    provisioning.
   
   

The vast majority of the settings available are specific to SAML,

    including the various URLs and method settings. Because of some user
    feedback we've already recieved, I've added a toggle to control the
    behavior of user provisioning. In most cases, the person using this has
    absolute trust in the integrity and correctness of their IdP, but in
    some cases that may not be true. If the "username" parameter can not
    necessarily be trusted, there's an option to force existing users to log
    in with their Review Board password at least once.

-  
-  

What's left to do:

-   - A bit more unit testing.

Commits:

Summary
-
[WIP] Add SAML 2.0 SSO.
+
Add SAML 2.0 SSO.

Diff:

Revision 8 (+4838 -46)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
Review request changed
Loading...