flake8
-
reviewboard/accounts/sso/backends/saml/sso_backend.py (Diff revision 1) Show all issues
Review Request #12254 — Created April 24, 2022 and updated
Information | |
---|---|
david | |
Review Board | |
release-5.0.x | |
Reviewers | |
reviewboard | |
This change adds SAML 2.0 support for single-sign-on authentication. It
includes a few major pieces:
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.
python3-saml
package wasn't installed.Summary | |
---|---|
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 ... |
|
|
Do we need to do this, or can we rely on lazy population in the registry? |
|
|
Blank line between these. |
|
|
extra_data is worth having, but I think we may want to include something specific for credential data that we'd never ... |
|
|
Missing docstring. |
|
|
Rather than this format, cna we do: for _module, _backend_cls_name in (('saml.sso_backend', 'SAMLSSOBackend'),): ... Or define the list separately. |
|
|
Missing docs in this file. |
|
|
This should be after __init__. |
|
|
F821 undefined name 'ModuleNotFoundError' |
![]() |
|
conf before contrib. |
|
|
404 before Response. |
|
|
These are in reverse order. |
|
|
These are in reverse order. |
|
|
Same with these. |
|
|
Missing docs. |
|
|
We should have some else case, even if that's just raising an exception. |
|
|
No need to .save() after .create(). |
|
|
Shouldn't have a trailing comma for a function call. |
|
|
Missing a Version Added. |
|
|
Missing a Version Added. We should put them in the functions as well. Same comments for other new modules. |
|
|
These should be swapped. |
|
|
Let's wrap any calls in an exception, in case any extension-provided backends (or ours) ever misbehave. |
|
|
Should we conditionally save ones with _enabled? |
|
|
Typo: flieds. |
|
|
Col: 40 Missing semicolon. |
![]() |
|
Missing ; |
|
|
Col: 64 Missing semicolon. |
![]() |
|
Missing ; |
|
|
Thes should be indented 1 space. No need for />. Same elsewhere. These apply to the other templates as well. |
|
|
F821 undefined name 'ModuleNotFoundError' |
![]() |
|
F401 'django.contrib.auth.login' imported but unused |
![]() |
|
F401 'reviewboard.accounts.backends.standard.StandardAuthBackend' imported but unused |
![]() |
|
F401 'django.utils.translation.gettext_lazy as _' imported but unused |
![]() |
reviewboard/accounts/sso/backends/saml/sso_backend.py (Diff revision 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.
reviewboard/accounts/__init__.py (Diff revision 1) |
---|
Do we need to do this, or can we rely on lazy population in the registry?
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 adata = JSONField()
where those go. Maybe aservice_data =
would be appropriate.
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.
reviewboard/accounts/sso/backends/saml/sso_backend.py (Diff revision 1) |
---|
This should be after
__init__
.
reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1) |
---|
We should have some
else
case, even if that's just raising an exception.
reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1) |
---|
No need to
.save()
after.create()
.
reviewboard/accounts/sso/backends/saml/views.py (Diff revision 1) |
---|
Shouldn't have a trailing comma for a function call.
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.
reviewboard/admin/forms/auth_settings.py (Diff revision 1) |
---|
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) |
---|
Should we conditionally save ones with
_enabled
?
reviewboard/templates/accounts/sso/link-user-connect-existing.html (Diff revision 1) |
---|
Thes should be indented 1 space.
No need for
/>
. Same elsewhere.These apply to the other templates as well.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+3742 -42) |
reviewboard/accounts/sso/backends/saml/sso_backend.py (Diff revision 2) |
---|
F821 undefined name 'ModuleNotFoundError'
Switch to
ImportError
for import test, since that's a parent class ofModuleNotFoundError
, and works with python 2.7 and the version of flake8 we're running.
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+3742 -42) |
login_user
method.Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+3890 -42) |
reviewboard/accounts/sso/backends/saml/views.py (Diff revision 4) |
---|
F401 'django.contrib.auth.login' imported but unused
reviewboard/accounts/sso/backends/saml/views.py (Diff revision 4) |
---|
F401 'reviewboard.accounts.backends.standard.StandardAuthBackend' imported but unused
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+3886 -42) |
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 6 (+4432 -46) |
Fix up description/testing done.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
Remove debug print statements.
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+4420 -46) |
Out of WIP.
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commits: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 8 (+4838 -46) |
reviewboard/accounts/sso/backends/saml/views.py (Diff revision 8) |
---|
F401 'django.utils.translation.gettext_lazy as _' imported but unused
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 9 (+4836 -46) |