Add SAML 2.0 SSO.

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

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

"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)
     
     
     
     
     

    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
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. "Single Sign-On" is the more "official" way it's written. Let's use that everywhere.

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

    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)
     
     
     
     
     

    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)
     
     

    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)
     
     

    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)
     
     
     

    We could probably put that in the help text.

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

    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. Let's standardize on "Single Sign-On".

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

    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)
     
     
     
     
     

    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. Since this is a new registry, we can probably avoid entrypoints altogether, just require extensions to register items.

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

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

    We can probably remove this blank line.

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

    These can probably begin on the include( line.

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

    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. This label should be in sentence casing.

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

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

    These labels should also be in sentence casing.

  22. "Identity Provider"?

  23. Do we want to remove these?

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

    Each of these could start on the path( line.

  25. Same notes as on the base implementation.

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

  27. 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. Should this be @cached_property?

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

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

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

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

    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)
     
     
     
     
     
     
     
     

    This will need a Raises:

  34. Can we put quotes around the values?

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

    Returns goes before Raises.

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

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

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

    Can you wrap find_suggested_username in :py:func:?

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

    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)
     
     
     

    Swap these.

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

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

  41. This should go above the previous sso imports.

  42. No trailing period (here and elsewhere).

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

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

    Version Added?

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

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

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

  50. Can we remove the blank line?

  51. Can we remove the blank line?

  52. Can we remove the blank line?

  53. No need for the />.

  54. Can we remove the blank line?

  55. Can we remove the blank line?

  56. Can we remove the blank line?

  57. 
      
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
-
Add SAML 2.0 SSO.
+
Add SAML 2.0 SSO.

Diff:

Revision 10 (+4898 -52)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
Review request changed

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. Can this be super()?

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

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

  4. Can this be super()?

  5. Can this be super()?

  6. Can this be super()?

  7. Can this be super()?

  8. Can this be super()?

  9. 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. Can this be super()?

  11. Can this be super()?

  12. Can this be super()?

  13. 
      
david
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

Status: Closed (submitted)

Change Summary:

Pushed to release-5.0.x (3615e82)
Loading...