- Download Diff

Summary:

Add SAML 2.0 SSO.

Review Request #12254 — Created April 24, 2022 and updated May 11, 2022, 9:48 a.m.

Information
Owner:	david
Repository:	Review Board
Branch:	release-5.0.x
Bugs:
Depends On:
Reviewers
Groups:	reviewboard
People:

Description

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.

Commits

Summary
	Add SAML 2.0 SSO.

Screenshots

Files

Issues

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 ...	chipx86	April 26, 2022, 2:46 p.m.
Do we need to do this, or can we rely on lazy population in the registry?	chipx86	April 25, 2022, 1:49 p.m.
Blank line between these.	chipx86	April 25, 2022, 9:20 a.m.
extra_data is worth having, but I think we may want to include something specific for credential data that we'd never ...	chipx86	April 25, 2022, 9:20 a.m.
Missing docstring.	chipx86	April 25, 2022, 9:21 a.m.
Rather than this format, cna we do: for _module, _backend_cls_name in (('saml.sso_backend', 'SAMLSSOBackend'),): ... Or define the list separately.	chipx86	April 25, 2022, 9:21 a.m.
Missing docs in this file.	chipx86	April 25, 2022, 9:23 a.m.
This should be after __init__.	chipx86	April 25, 2022, 9:22 a.m.
F821 undefined name 'ModuleNotFoundError'	reviewbot	April 25, 2022, 2:16 p.m.
conf before contrib.	chipx86	April 25, 2022, 9:23 a.m.
404 before Response.	chipx86	April 25, 2022, 9:23 a.m.
These are in reverse order.	chipx86	April 25, 2022, 9:23 a.m.
These are in reverse order.	chipx86	April 25, 2022, 9:23 a.m.
Same with these.	chipx86	April 25, 2022, 9:23 a.m.
Missing docs.	chipx86	April 25, 2022, 9:24 a.m.
We should have some else case, even if that's just raising an exception.	chipx86	April 25, 2022, 9:25 a.m.
No need to .save() after .create().	chipx86	April 25, 2022, 9:25 a.m.
Shouldn't have a trailing comma for a function call.	chipx86	April 25, 2022, 9:25 a.m.
Missing a Version Added.	chipx86	April 25, 2022, 9:25 a.m.
Missing a Version Added. We should put them in the functions as well. Same comments for other new modules.	chipx86	April 25, 2022, 9:26 a.m.
These should be swapped.	chipx86	April 25, 2022, 9:26 a.m.
Let's wrap any calls in an exception, in case any extension-provided backends (or ours) ever misbehave.	chipx86	April 25, 2022, 9:27 a.m.
Should we conditionally save ones with _enabled?	chipx86	April 25, 2022, 9:28 a.m.
Typo: flieds.	chipx86	April 25, 2022, 9:27 a.m.
Col: 40 Missing semicolon.	reviewbot	April 25, 2022, 9:20 a.m.
Missing ;	chipx86	April 25, 2022, 9:28 a.m.
Col: 64 Missing semicolon.	reviewbot	April 25, 2022, 9:20 a.m.
Missing ;	chipx86	April 25, 2022, 9:28 a.m.
Thes should be indented 1 space. No need for />. Same elsewhere. These apply to the other templates as well.	chipx86	April 25, 2022, 1:50 p.m.
F821 undefined name 'ModuleNotFoundError'	reviewbot	April 25, 2022, 2:17 p.m.
F401 'django.contrib.auth.login' imported but unused	reviewbot	April 26, 2022, 1:42 p.m.
F401 'reviewboard.accounts.backends.standard.StandardAuthBackend' imported but unused	reviewbot	April 26, 2022, 1:42 p.m.
F401 'django.utils.translation.gettext_lazy as _' imported but unused	reviewbot	May 11, 2022, 9:48 a.m.

flake8 failed.

JSHint failed.

flake8

reviewboard/accounts/sso/backends/saml/sso_backend.py (Diff revision 1)
Show all issues
```
F821 undefined name 'ModuleNotFoundError'
```

JSHint

reviewboard/static/rb/js/ui/views/formView.es6.js (Diff revision 1)
Show all issues
```
Col: 40
 Missing semicolon.
```
reviewboard/static/rb/js/ui/views/formView.es6.js (Diff revision 1)
Show all issues
```
Col: 64
 Missing semicolon.
```

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.

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?

david April 25, 2022, 1:50 p.m.

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

reviewboard/accounts/models.py (Diff revision 1)
Show all issues
```
Blank line between these.
```

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.

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

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.

reviewboard/accounts/sso/backends/saml/settings.py (Diff revision 1)
Show all issues
```
Missing docs in this file.
```
reviewboard/accounts/sso/backends/saml/sso_backend.py (Diff revision 1)
Show all issues
```
This should be after __init__.
```
reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1)
Show all issues
```
conf before contrib.
```
reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1)
Show all issues
```
404 before Response.
```
reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1)
Show all issues
```
These are in reverse order.
```
reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1)
Show all issues
```
These are in reverse order.
```
reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1)
Show all issues
```
Same with these.
```
reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1)
Show all issues
```
Missing docs.
```

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.

reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1)
Show all issues
```
No need to .save() after .create().
```
reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1)
Show all issues
```
Shouldn't have a trailing comma for a function call.
```
reviewboard/accounts/sso/errors.py (Diff revision 1)
Show all issues
```
Missing a Version Added.
```

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.

reviewboard/accounts/tests/test_sso_backend_registry.py (Diff revision 1)
Show all issues
```
These should be swapped.
```

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

Show all issues

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

reviewboard/admin/forms/auth_settings.py (Diff revision 1)
Show all issues
```
Should we conditionally save ones with _enabled?
```
reviewboard/admin/forms/auth_settings.py (Diff revision 1)
Show all issues
```
Typo: flieds.
```
reviewboard/static/rb/js/ui/views/formView.es6.js (Diff revision 1)
Show all issues
```
Missing ;
```
reviewboard/static/rb/js/ui/views/formView.es6.js (Diff revision 1)
Show all issues
```
Missing ;
```

reviewboard/templates/accounts/sso/link-user-connect-existing.html (Diff revision 1)

Show all issues

Thes should be indented 1 space.

No need for />. Same elsewhere.

These apply to the other templates as well.

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

	setup.py
	reviewboard/accounts/__init__.py
	reviewboard/accounts/admin.py
	reviewboard/accounts/models.py
	reviewboard/accounts/urls.py
	reviewboard/accounts/sso/__init__.py
	reviewboard/accounts/sso/errors.py
	reviewboard/accounts/sso/users.py
	20 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/accounts/sso/backends/saml/sso_backend.py (Diff revision 2)
Show all issues
```
F821 undefined name 'ModuleNotFoundError'
```

Change Summary:

Switch to ImportError for import test, since that's a parent class of ModuleNotFoundError, and works with python 2.7 and the version of flake8 we're running.

Commits:

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

Diff:

Revision 3 (+3742 -42)

Show changes

	setup.py
	reviewboard/accounts/__init__.py
	reviewboard/accounts/admin.py
	reviewboard/accounts/models.py
	reviewboard/accounts/urls.py
	reviewboard/accounts/sso/__init__.py
	reviewboard/accounts/sso/errors.py
	reviewboard/accounts/sso/users.py
	20 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

	setup.py
	reviewboard/accounts/__init__.py
	reviewboard/accounts/admin.py
	reviewboard/accounts/models.py
	reviewboard/accounts/urls.py
	reviewboard/accounts/sso/__init__.py
	reviewboard/accounts/sso/errors.py
	reviewboard/accounts/sso/users.py
	20 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/accounts/sso/backends/saml/views.py (Diff revision 4)
Show all issues
```
F401 'django.contrib.auth.login' imported but unused
```

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

Show all issues

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

Commits:

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

Diff:

Revision 5 (+3886 -42)

Show changes

	setup.py
	reviewboard/accounts/__init__.py
	reviewboard/accounts/admin.py
	reviewboard/accounts/models.py
	reviewboard/accounts/urls.py
	reviewboard/accounts/sso/__init__.py
	reviewboard/accounts/sso/errors.py
	reviewboard/accounts/sso/users.py
	20 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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.

Change Summary:


Added login button.
Added docs to admin manual.
Validation for settings form fields.
Removed configurable NameID format, since we require NameID to be the username (and anything else doesn't make sense for us).
Added a bunch of unit tests.

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:
	~	What's left to do:
	+	- A bit more unit testing.

~		"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.
	+	Tested text of button on login page, and that it redirected to the SSO login flow.
	+	Ran unit tests.

Commits:

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

Diff:

Revision 6 (+4432 -46)

Show changes

	setup.py
	docs/manual/admin/configuration/authentication-settings.rst
	reviewboard/accounts/__init__.py
	reviewboard/accounts/admin.py
	reviewboard/accounts/models.py
	reviewboard/accounts/urls.py
	reviewboard/accounts/views.py
	reviewboard/accounts/sso/__init__.py
	25 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Fix up description/testing done.

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

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.

Change Summary:

Remove debug print statements.

Commits:

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

Diff:

Revision 7 (+4420 -46)

Show changes

	setup.py
	docs/manual/admin/configuration/authentication-settings.rst
	reviewboard/accounts/__init__.py
	reviewboard/accounts/admin.py
	reviewboard/accounts/models.py
	reviewboard/accounts/urls.py
	reviewboard/accounts/views.py
	reviewboard/accounts/sso/__init__.py
	25 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

	setup.py
	docs/manual/admin/configuration/authentication-settings.rst
	reviewboard/accounts/__init__.py
	reviewboard/accounts/admin.py
	reviewboard/accounts/models.py
	reviewboard/accounts/urls.py
	reviewboard/accounts/views.py
	reviewboard/accounts/sso/__init__.py
	25 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/accounts/sso/backends/saml/views.py (Diff revision 8)
Show all issues
```
F401 'django.utils.translation.gettext_lazy as _' imported but unused
```

Commits:

	Summary
-		Add SAML 2.0 SSO.
+		Add SAML 2.0 SSO.

Diff:

Revision 9 (+4836 -46)

Show changes

	setup.py
	docs/manual/admin/configuration/authentication-settings.rst
	reviewboard/accounts/__init__.py
	reviewboard/accounts/admin.py
	reviewboard/accounts/models.py
	reviewboard/accounts/urls.py
	reviewboard/accounts/views.py
	reviewboard/accounts/sso/__init__.py
	25 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

You have a pending review.

Add SAML 2.0 SSO.

flake8

JSHint

Description:

Testing Done:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Change Summary:

Commits:

Diff:

Checks run (2 succeeded)

Change Summary:

Description:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commits:

Diff:

Checks run (2 succeeded)

Change Summary:

Description:

Commits:

Diff:

Checks run (2 succeeded)

Change Summary:

Description:

Testing Done:

Change Summary:

Commits:

Diff:

Checks run (2 succeeded)

Change Summary:

Summary:

Description:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commits:

Diff:

Checks run (2 succeeded)