Add SAML 2.0 SSO.

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

Information

Review Board
release-5.0.x

Reviewers

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 ID
Add SAML 2.0 SSO.
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 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. - Tested text of button on login page, and that it redirected to the SSO login flow. - Ran unit tests.
bf918a95fe5de7d72edf7932c56b0d7e4c1f70f4

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

"Single Sign-On" is the more "official" way it's written. Let's use that everywhere.

chipx86chipx86

We use "Identity Provider", "IdP", and "identity provider" in the document. We should stick with one term and capitalization. We …

chipx86chipx86

What do you think about breaking this down in bullet point form, like: You Identity Provider should provide the following: …

chipx86chipx86

We should probably make it clear that these are examples and make sure they don't link: Example: ``https://.../` Worth describing …

chipx86chipx86

Trying to start moving our initial signal setup into the AppConfig and signal_handlers.py, so we can avoid side-effects on module …

chipx86chipx86

I have a couple of additional thoughts regarding this model. These are pure discussion topics. I don't have concrete answers. …

chipx86chipx86

I hope 64 is fine, but I wonder if we might want to give some more breathing room. Maybe 128?

chipx86chipx86

We could probably put that in the help text.

chipx86chipx86

If we do add a LocalSite relation, we'll need that in here. However, I'm not sure that we want to …

chipx86chipx86

Let's standardize on "Single Sign-On".

chipx86chipx86

We can probably just make this a urls = [] attribute by default, and let subclasses decide whether to use …

chipx86chipx86

Can we break this down into a numbered list of items, for readability? This should also document what the second …

chipx86chipx86

Since this is a new registry, we can probably avoid entrypoints altogether, just require extensions to register items.

chipx86chipx86

If we don't need to support entrypoints, can we remove this?

chipx86chipx86

We can probably remove this blank line.

chipx86chipx86

These can probably begin on the include( line.

chipx86chipx86

I wonder if we could find a better place for this, because there are other places where we're going to …

chipx86chipx86

This label should be in sentence casing.

chipx86chipx86

This label should be in sentence casing (unless "Issuer" is generally capitalized here).

chipx86chipx86

These labels should also be in sentence casing.

chipx86chipx86

"Identity Provider"?

chipx86chipx86

Do we want to remove these?

chipx86chipx86

Each of these could start on the path( line.

chipx86chipx86

Same notes as on the base implementation.

chipx86chipx86

We may want to say "... and restart the web server"

chipx86chipx86

Should this be @cached_property?

chipx86chipx86

Should this be @cached_property?

chipx86chipx86

No need for str() here (unless we're testing via spies an want to ease checking this argument's value).

chipx86chipx86

Is this related to the TODO above? Should we drop this, document this?

chipx86chipx86

Can we use :term: for things like IdP, and define in glossary.rst?

chipx86chipx86

Worth starting to use Enum for these things? (Maybe not -- just throwing this out there.)

chipx86chipx86

This will need a Raises:

chipx86chipx86

Can we put quotes around the values?

chipx86chipx86

Returns goes before Raises.

chipx86chipx86

Can we provide the username in question as an argument to this?

chipx86chipx86

Can you wrap find_suggested_username in :py:func:?

chipx86chipx86

I think I wrote this initially, but it might make sense to limit this to one database query: q = …

chipx86chipx86

Swap these.

chipx86chipx86

For readability, can we move the name= to the next line? This all kind of blurs together.

chipx86chipx86

This should go above the previous sso imports.

chipx86chipx86

No trailing period (here and elsewhere).

chipx86chipx86

To reduce the amount of code and repeated attribute lookups, can we pull out siteconfig into a local variable?

chipx86chipx86

To just reduce database hits during tests, let's just fetch all linked accounts as list(...), and then we can check …

chipx86chipx86

Version Added?

chipx86chipx86

We do this whole fetch of everything leading up to 'fields' up to twice per forloop iteration. Let's pull this …

chipx86chipx86

How was load() being called before? Are we at risk of calling it twice now?

chipx86chipx86

We do so many of these. Wonder if it'd make sense to have a private function that yields sso_backend_id, form, …

chipx86chipx86

Leftover debugging.

chipx86chipx86

Can we remove the blank line?

chipx86chipx86

No need for /> anymore.

chipx86chipx86

Can we remove the blank line?

chipx86chipx86

Can we remove the blank line?

chipx86chipx86

No need for the />.

chipx86chipx86

Can we remove the blank line?

chipx86chipx86

Can we remove the blank line?

chipx86chipx86

No need for the />

chipx86chipx86

Can we remove the blank line?

chipx86chipx86

F401 'django_evolution.mutations.AddField' imported but unused

reviewbotreviewbot

E402 module level import not at top of file

reviewbotreviewbot

Can this be super()?

chipx86chipx86

Since we've eliminated the entrypoint support, we no longer need this.

chipx86chipx86

Can this be super()?

chipx86chipx86

Can this be super()?

chipx86chipx86

Can this be super()?

chipx86chipx86

Can this be super()?

chipx86chipx86

Can this be super()?

chipx86chipx86

We should probably get that into Asana for tracking. What's involved here? Can we address this for 5.0?

chipx86chipx86

Can this be super()?

chipx86chipx86

Can this be super()?

chipx86chipx86

Can this be super()?

chipx86chipx86

'djblets.registries.registry.LOAD_ENTRY_POINT' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

'djblets.db.query.get_object_or_none' imported but unused Column: 1 Error code: F401

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

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

    Blank line between these.

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

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

    Missing docstring.

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

    Rather than this format, cna we do:

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

    Or define the list separately.

  7. Show all issues

    Missing docs in this file.

  8. Show all issues

    This should be after __init__.

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

    conf before contrib.

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

    404 before Response.

  11. Show all issues

    These are in reverse order.

  12. Show all issues

    These are in reverse order.

  13. Show all issues

    Same with these.

  14. Show all issues

    Missing docs.

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

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

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

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

  17. reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1)
     
     
     
     
     
    Show all issues

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

  18. reviewboard/accounts/sso/errors.py (Diff revision 1)
     
     
    Show all issues

    Missing a Version Added.

  19. reviewboard/accounts/sso/users.py (Diff revision 1)
     
     
    Show all issues

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

    Same comments for other new modules.

  20. Show all issues

    These should be swapped.

  21. Show all issues

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

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

    Should we conditionally save ones with _enabled?

  23. Show all issues

    Typo: flieds.

  24. Show all issues

    Missing ;

  25. Show all issues

    Missing ;

  26. Show all issues

    Thes should be indented 1 space.

    No need for />. Same elsewhere.

    These apply to the other templates as well.

  27. 
      
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 ID
[WIP] Add SAML 2.0 SSO.
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. 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.
93052180b2f08ac863c6fc17e843abe1978382a7
[WIP] Add SAML 2.0 SSO.
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. 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.
a03de0ec350f55ef6b238bf5bc7fda62b3884103

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 ID
[WIP] Add SAML 2.0 SSO.
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. 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.
81ee98de7add6622dce178d20f52510a02c2405a
[WIP] Add SAML 2.0 SSO.
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. 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.
580aaefd8444bbe4036e8e449530b45987bcea2b

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. 
      
  2. Show all issues

    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 ID
[WIP] Add SAML 2.0 SSO.
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: - A bit more unit testing. 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. - Tested text of button on login page, and that it redirected to the SSO login flow. - Ran unit tests.
b8d560c0c13852a8b3042ef74e4fe44bff49d503
Add SAML 2.0 SSO.
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 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. - Tested text of button on login page, and that it redirected to the SSO login flow. - Ran unit tests.
e41fe7a85fc282934718c0587c9b80d55b1e5ee2

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. This is great. Thanks again for taking this on!

    Lots of comments in here, since it's a large. Mostly style, some doc suggestions, food-for-thought, and some things you're more than welcome to defer (future code improvement stuff). There is one larger topic about LinkedAccount model fields that I think we'll want to figure out.

  2. Show all issues

    "Single Sign-On" is the more "official" way it's written. Let's use that everywhere.

  3. Show all issues

    We use "Identity Provider", "IdP", and "identity provider" in the document. We should stick with one term and capitalization.

    We could wrap each in :term: and then define it more closely in glossary.rst, if that's worth it. That at least would also allow us to group the variations (Identity Provider, IdP).

    1. I'll just change it to be "Identity Provider" everywhere. I don't think it really fits in the glossary, at least with the glossary we have today.

  4. Show all issues

    What do you think about breaking this down in bullet point form, like:

    You Identity Provider should provide the following:
    
    1. URLs to fill out for the Issuer and SAML/SLO endpoints.
    2. A copy of your X.509 certificate.
    3. ...
    
  5. docs/manual/admin/configuration/authentication-settings.rst (Diff revision 9)
     
     
     
     
     
     
     
     
     
    Show all issues

    We should probably make it clear that these are examples and make sure they don't link:

    Example: ``https://.../`
    

    Worth describing these at all?

    1. I don't think we need explanations. These should map directly to fields they'll see in their identity provider configuration UI.

  6. reviewboard/accounts/__init__.py (Diff revision 9)
     
     
     
     
     
    Show all issues

    Trying to start moving our initial signal setup into the AppConfig and signal_handlers.py, so we can avoid side-effects on module import. I just did this with /r/12299/, for reference. Can we adopt that pattern here?

  7. reviewboard/accounts/models.py (Diff revision 9)
     
     
    Show all issues

    I have a couple of additional thoughts regarding this model.

    These are pure discussion topics. I don't have concrete answers.

    Local Sites?

    Do we want to bind this to a LocalSite?

    An argument for that would be that if an organization shuts down their, say, RBCommons account, and a user retains their linked account, both we and they retain some link to an account that the organization may have expected to have been deleted.

    Having a LocalSite relation would mean that a user would have to link up their accounts per-LocalSite, but that's probably a lot less common and maybe even preferable.

    Linked account type

    I think it'd be really useful to be able to identify the type of a linked account. I know we can query by service, but something more broad can be useful for linked account management.

    I don't know what this would look like, or at what level we would want to clarify it.

    Maybe we differentiate "auth" from "chat" from "scm" or something.

    Or maybe instead of/in addition, we want to bind an integration ID, for any integrations that will use this.

    Again, no idea, but for querying/management purposes, I think we'd want something beyond service_id.

    1. I don't think it makes sense to make this LocalSite specific. User auth has always been a global thing, and in the future where we potentially have the ability to connect multiple SSO services, a removed account will always need to be removed from their IdP and the local site in question. Having a relation here doesn't help us at all with that.

      Right now, I'm using sso:saml as our one and only service ID. I think I'd like to keep that kind of string-based namespacing for now, especially since there's no clear idea of how we would use some kind of differentiation. If we decide in the future we need something, we can always add it later.

  8. reviewboard/accounts/models.py (Diff revision 9)
     
     
    Show all issues

    I hope 64 is fine, but I wonder if we might want to give some more breathing room. Maybe 128?

    1. This is entirely internal to implementation details of code that uses these, so we can always choose IDs that fit. Plus I'd prefer to avoid having to have any evolutions for people that are using this in pre-production.

  9. reviewboard/accounts/models.py (Diff revision 9)
     
     
     
    Show all issues

    We could probably put that in the help text.

  10. reviewboard/accounts/models.py (Diff revision 9)
     
     
    Show all issues

    If we do add a LocalSite relation, we'll need that in here.

    However, I'm not sure that we want to limit 1 user account per service ID. Instead, do we want this to be ('service_user_id', 'service_id')?

  11. Show all issues

    Let's standardize on "Single Sign-On".

  12. reviewboard/accounts/sso/backends/base.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
    Show all issues

    We can probably just make this a urls = [] attribute by default, and let subclasses decide whether to use @property.

  13. reviewboard/accounts/sso/backends/base.py (Diff revision 9)
     
     
     
     
     
    Show all issues

    Can we break this down into a numbered list of items, for readability?

    This should also document what the second item looks like if the first is True.

  14. Show all issues

    Since this is a new registry, we can probably avoid entrypoints altogether, just require extensions to register items.

  15. Show all issues

    If we don't need to support entrypoints, can we remove this?

  16. reviewboard/accounts/sso/backends/registry.py (Diff revision 9)
     
     
     
     
    Show all issues

    We can probably remove this blank line.

  17. reviewboard/accounts/sso/backends/registry.py (Diff revision 9)
     
     
     
     
    Show all issues

    These can probably begin on the include( line.

  18. reviewboard/accounts/sso/backends/saml/forms.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    I wonder if we could find a better place for this, because there are other places where we're going to want X.509 certs to be validated (such as self-signed certs for repos/LDAP/etc. eventually).

    We probably should have a reviewboard.crypto module, or even a djblets.crypto. We currently have some crypto utils in reviewboard.scmtools.crypto_utils, and we could put this in there for now, but it might be a good time to think about a better location.

    This does not have to be part of this change.

    1. Will put this on the back burner for thought but not going to do anything for now.

  19. Show all issues

    This label should be in sentence casing.

  20. Show all issues

    This label should be in sentence casing (unless "Issuer" is generally capitalized here).

  21. reviewboard/accounts/sso/backends/saml/forms.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    These labels should also be in sentence casing.

  22. Show all issues

    "Identity Provider"?

  23. Show all issues

    Do we want to remove these?

  24. reviewboard/accounts/sso/backends/saml/sso_backend.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Each of these could start on the path( line.

  25. Show all issues

    Same notes as on the base implementation.

  26. Show all issues

    We may want to say "... and restart the web server"

  27. Show all issues

    Should this be @cached_property?

    1. Not this one, since the login flow via the SAML provider can give different redirect URLs in certain cases.

  28. Show all issues

    Should this be @cached_property?

  29. Show all issues

    No need for str() here (unless we're testing via spies an want to ease checking this argument's value).

  30. reviewboard/accounts/sso/backends/saml/views.py (Diff revision 9)
     
     
     
     
     
    Show all issues

    Is this related to the TODO above? Should we drop this, document this?

  31. Show all issues

    Can we use :term: for things like IdP, and define in glossary.rst?

  32. reviewboard/accounts/sso/backends/saml/views.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
    Show all issues

    Worth starting to use Enum for these things? (Maybe not -- just throwing this out there.)

  33. reviewboard/accounts/sso/backends/saml/views.py (Diff revision 9)
     
     
     
     
     
     
     
     
    Show all issues

    This will need a Raises:

  34. Show all issues

    Can we put quotes around the values?

  35. reviewboard/accounts/sso/users.py (Diff revision 9)
     
     
     
     
     
     
     
     
    Show all issues

    Returns goes before Raises.

  36. reviewboard/accounts/sso/users.py (Diff revision 9)
     
     
    Show all issues

    Can we provide the username in question as an argument to this?

  37. reviewboard/accounts/sso/users.py (Diff revision 9)
     
     
    Show all issues

    Can you wrap find_suggested_username in :py:func:?

  38. reviewboard/accounts/sso/users.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    I think I wrote this initially, but it might make sense to limit this to one database query:

    q = Q()
    
    if username:
        q |= Q(username=username)
    
    q |= Q(email=email) | Q(username=alternate_username)
    
    return get_object_or_none(User, q)
    

    Actually though, whether we do this or not, we can't rely on get_object_or_none() alone when querying email, as that can match multiple users. We need to handle that case.

    1. Unfortunately, that doesn't quite capture the same behavior as a prioritized list of possibilities. We might have a user that matches the provided username as well as a user that matches the alternate username, but the provided username is definitely the one we want to prefer.

      I'll fix up the issue with email to only fetch max 1 user.

    2. Oh, I was thinking we could fetch all the candidates and then go through it in the priority order. Just save the extra database queries.

  39. reviewboard/accounts/sso/views.py (Diff revision 9)
     
     
     
    Show all issues

    Swap these.

  40. reviewboard/accounts/sso/views.py (Diff revision 9)
     
     
    Show all issues

    For readability, can we move the name= to the next line? This all kind of blurs together.

  41. Show all issues

    This should go above the previous sso imports.

  42. Show all issues

    No trailing period (here and elsewhere).

  43. Show all issues

    To reduce the amount of code and repeated attribute lookups, can we pull out siteconfig into a local variable?

  44. Show all issues

    To just reduce database hits during tests, let's just fetch all linked accounts as list(...), and then we can check length and inspect the first result without further queries.

    Same below.

  45. reviewboard/accounts/views.py (Diff revision 9)
     
     
    Show all issues

    Version Added?

  46. reviewboard/admin/forms/auth_settings.py (Diff revision 9)
     
     
     
    Show all issues

    We do this whole fetch of everything leading up to 'fields' up to twice per forloop iteration. Let's pull this out before the loop. That'll help it read a bit better, too.

  47. Show all issues

    How was load() being called before? Are we at risk of calling it twice now?

    1. It's still called from the superclass init(). It does end up getting called twice now, but that's not problematic.

  48. reviewboard/admin/forms/auth_settings.py (Diff revision 9)
     
     
     
    Show all issues

    We do so many of these. Wonder if it'd make sense to have a private function that yields sso_backend_id, form, enable_field_id.

    Just a cleanup suggestion, but not critical.

  49. Show all issues

    Leftover debugging.

  50. Show all issues

    Can we remove the blank line?

  51. Show all issues

    No need for /> anymore.

  52. Show all issues

    Can we remove the blank line?

  53. Show all issues

    Can we remove the blank line?

  54. Show all issues

    No need for the />.

  55. Show all issues

    Can we remove the blank line?

  56. Show all issues

    Can we remove the blank line?

  57. Show all issues

    No need for the />

  58. Show all issues

    Can we remove the blank line?

  59. 
      
david
Review request changed
Change Summary:
  • Made requested changes
  • Change SAML request to get hostname/port from the configured server
    URL rather than the request. This prevents issues with mismatched
    URLs when the server is running internally on a different port than
    the public endpoint.
  • Fix error reporting for ACS view to get useful messages instead of
    just "invalid response".
Commits:
Summary ID
Add SAML 2.0 SSO.
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 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. - Tested text of button on login page, and that it redirected to the SSO login flow. - Ran unit tests.
62545c8cc6f7b3ac619978ef3559f1f9a4abc012
Add SAML 2.0 SSO.
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 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. - Tested text of button on login page, and that it redirected to the SSO login flow. - Ran unit tests.
6ee8f3ec540f29f5410a07186a85b6e651f8fcc9

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
Review request changed
Commits:
Summary ID
Add SAML 2.0 SSO.
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 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. - Tested text of button on login page, and that it redirected to the SSO login flow. - Ran unit tests.
6ee8f3ec540f29f5410a07186a85b6e651f8fcc9
Add SAML 2.0 SSO.
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 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. - Tested text of button on login page, and that it redirected to the SSO login flow. - Ran unit tests.
00db71cadf9ebab745a02588c2811f7c3fedf5d2

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
david
chipx86
  1. This looks great. I'm happy with it. I just have a few small optional nits, a thing for tracking a TODO, and then I have a reply to the 3 queries bit in my previous review.

  2. Show all issues

    Can this be super()?

  3. reviewboard/accounts/sso/backends/registry.py (Diff revision 13)
     
     
     
     
    Show all issues

    Since we've eliminated the entrypoint support, we no longer need this.

  4. Show all issues

    Can this be super()?

  5. Show all issues

    Can this be super()?

  6. Show all issues

    Can this be super()?

  7. Show all issues

    Can this be super()?

  8. Show all issues

    Can this be super()?

  9. Show all issues

    We should probably get that into Asana for tracking.

    What's involved here? Can we address this for 5.0?

    1. This is something that's listed in the docs but it's really unclear to me exactly what it means, and of course none of their examples actually do this. I'll get a task filed.

  10. Show all issues

    Can this be super()?

  11. Show all issues

    Can this be super()?

  12. Show all issues

    Can this be super()?

  13. 
      
david
Review request changed
Commits:
Summary ID
Add SAML 2.0 SSO.
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 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. - Tested text of button on login page, and that it redirected to the SSO login flow. - Ran unit tests.
8e214fbfb7ade198e46b4c56e19553e2b3166816
Add SAML 2.0 SSO.
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 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. - Tested text of button on login page, and that it redirected to the SSO login flow. - Ran unit tests.
8244640bd790d9f37f972e082fd435bc1b493209

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-5.0.x (3615e82)