Summary

Respect User.is_active when logging in with SAML SSO.

Review Request #14331 — Created Feb. 6, 2025 and submitted Feb. 14, 2025, 8:50 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-6.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This change makes it so the SAML ACS and link-user views check the
is_active flag. In the link-user flow, we rely on the authentication
view/form to show the same error that someone would get if they were
logging in with a username and password. In the ACS flow, we just
redirect to a permission denied page.

While writing tests for this, things were getting a little unwieldy, so
I split up the SAML view tests into separate classes.

Testing Done


Set my user account to inactive and tried to log in with SAML. Saw

  that things worked as expected.
Ran unit tests.

Commits

Summary	ID
Respect User.is_active when logging in with SAML SSO. When we built the SSO support, we made the assumption that all user management would happen on the IdP side. We've had a report of a user that wants to connect their IdP generally, but selectively mark users as active or not on the Review Board side. This change makes it so the SAML ACS and link-user views check the `is_active` flag. In the link-user flow, we rely on the authentication view/form to show the same error that someone would get if they were logging in with a username and password. In the ACS flow, we just redirect to a permission denied page. While writing tests for this, things were getting a little unwieldy, so I split up the SAML view tests into separate classes. Testing Done: - Set my user account to inactive and tried to log in with SAML. Saw that things worked as expected. - Ran unit tests.	b39328342a3e5456f66b3ed0b2ba8e3a3e3bef41

Summary

Respect User.is_active when logging in with SAML SSO.

When we built the SSO support, we made the assumption that all user management would happen on the IdP side. We've had a report of a user that wants to connect their IdP generally, but selectively mark users as active or not on the Review Board side. This change makes it so the SAML ACS and link-user views check the `is_active` flag. In the link-user flow, we rely on the authentication view/form to show the same error that someone would get if they were logging in with a username and password. In the ACS flow, we just redirect to a permission denied page. While writing tests for this, things were getting a little unwieldy, so I split up the SAML view tests into separate classes. Testing Done: - Set my user account to inactive and tried to log in with SAML. Saw that things worked as expected. - Ran unit tests.

b39328342a3e5456f66b3ed0b2ba8e3a3e3bef41

Issues

Description	From	Last Updated
'reviewboard.accounts.models.LinkedAccount' imported but unused Column: 1 Error code: F401	reviewbot	Feb. 6, 2025, 12:13 p.m.
expected 2 blank lines, found 1 Column: 1 Error code: E302	reviewbot	Feb. 6, 2025, 12:13 p.m.
expected 2 blank lines, found 1 Column: 1 Error code: E302	reviewbot	Feb. 6, 2025, 12:13 p.m.
Since they're targeting a 7.0.3 upgrade, I'm tempted to say let's just target 7.0.4 for this.	chipx86	Feb. 13, 2025, 10:35 a.m.
We should be localizing the errors.	chipx86	Feb. 13, 2025, 10:33 a.m.
Doesn't this type to forms.BooleanField automatically?	chipx86	Feb. 13, 2025, 10:34 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/accounts/tests/test_saml_views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
'reviewboard.accounts.models.LinkedAccount' imported but unused

Column: 1
Error code: F401
```
reviewboard/accounts/tests/test_saml_views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
expected 2 blank lines, found 1

Column: 1
Error code: E302
```
reviewboard/accounts/tests/test_saml_views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
expected 2 blank lines, found 1

Column: 1
Error code: E302
```

Commits:

	Summary	ID
	Respect User.is_active when logging in with SAML SSO. When we built the SSO support, we made the assumption that all user management would happen on the IdP side. We've had a report of a user that wants to connect their IdP generally, but selectively mark users as active or not on the Review Board side. This change makes it so the SAML ACS and link-user views check the `is_active` flag. In the link-user flow, we rely on the authentication view/form to show the same error that someone would get if they were logging in with a username and password. In the ACS flow, we just redirect to a permission denied page. While writing tests for this, things were getting a little unwieldy, so I split up the SAML view tests into separate classes. Testing Done: - Set my user account to inactive and tried to log in with SAML. Saw that things worked as expected. - Ran unit tests.	f75190d7dd73cc53abac84e7886a9bc2be5935fc
	Respect User.is_active when logging in with SAML SSO. When we built the SSO support, we made the assumption that all user management would happen on the IdP side. We've had a report of a user that wants to connect their IdP generally, but selectively mark users as active or not on the Review Board side. This change makes it so the SAML ACS and link-user views check the `is_active` flag. In the link-user flow, we rely on the authentication view/form to show the same error that someone would get if they were logging in with a username and password. In the ACS flow, we just redirect to a permission denied page. While writing tests for this, things were getting a little unwieldy, so I split up the SAML view tests into separate classes. Testing Done: - Set my user account to inactive and tried to log in with SAML. Saw that things worked as expected. - Ran unit tests.	9e050bf8dd4f42eb0a76fe42a9ce2c6857567aed

Diff:

Revision 2 (+554 -100)

Show changes

	reviewboard/accounts/errors.py
	reviewboard/accounts/sso/backends/base.py
	reviewboard/accounts/sso/backends/saml/forms.py
	reviewboard/accounts/sso/backends/saml/sso_backend.py
	reviewboard/accounts/sso/backends/saml/views.py
	reviewboard/accounts/tests/test_saml_forms.py
	reviewboard/accounts/tests/test_saml_views.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

```
Looks great. Couple small things.
```

reviewboard/accounts/errors.py (Diff revision 2)

The issue has been dropped. Show all issues

Since they're targeting a 7.0.3 upgrade, I'm tempted to say let's just target 7.0.4 for this.

david

Feb. 13, 2025, 10:41 a.m.

I'm not sure when that's planned for, and this seems generally useful for larger, more upgrade-shy orgs anyway.

reviewboard/accounts/errors.py (Diff revision 2)
The issue has been resolved. Show all issues
```
We should be localizing the errors.
```

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

The issue has been dropped. Show all issues

Doesn't this type to forms.BooleanField automatically?

david

Feb. 13, 2025, 10:41 a.m.

It seems like the general consensus I'm seeing on this topic is that it does, but best practice is to add an annotation for things unless the class is decorated with @final.

Commits:

	Summary	ID
	Respect User.is_active when logging in with SAML SSO. When we built the SSO support, we made the assumption that all user management would happen on the IdP side. We've had a report of a user that wants to connect their IdP generally, but selectively mark users as active or not on the Review Board side. This change makes it so the SAML ACS and link-user views check the `is_active` flag. In the link-user flow, we rely on the authentication view/form to show the same error that someone would get if they were logging in with a username and password. In the ACS flow, we just redirect to a permission denied page. While writing tests for this, things were getting a little unwieldy, so I split up the SAML view tests into separate classes. Testing Done: - Set my user account to inactive and tried to log in with SAML. Saw that things worked as expected. - Ran unit tests.	9e050bf8dd4f42eb0a76fe42a9ce2c6857567aed
	Respect User.is_active when logging in with SAML SSO. When we built the SSO support, we made the assumption that all user management would happen on the IdP side. We've had a report of a user that wants to connect their IdP generally, but selectively mark users as active or not on the Review Board side. This change makes it so the SAML ACS and link-user views check the `is_active` flag. In the link-user flow, we rely on the authentication view/form to show the same error that someone would get if they were logging in with a username and password. In the ACS flow, we just redirect to a permission denied page. While writing tests for this, things were getting a little unwieldy, so I split up the SAML view tests into separate classes. Testing Done: - Set my user account to inactive and tried to log in with SAML. Saw that things worked as expected. - Ran unit tests.	b39328342a3e5456f66b3ed0b2ba8e3a3e3bef41

Diff:

Revision 3 (+560 -100)

Show changes

	reviewboard/accounts/errors.py
	reviewboard/accounts/sso/backends/base.py
	reviewboard/accounts/sso/backends/saml/forms.py
	reviewboard/accounts/sso/backends/saml/sso_backend.py
	reviewboard/accounts/sso/backends/saml/views.py
	reviewboard/accounts/tests/test_saml_forms.py
	reviewboard/accounts/tests/test_saml_views.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-6.x (07115dc)