From 2468d5eb1c3ee0435bfab431e26943ca7fadba74 Mon Sep 17 00:00:00 2001 From: Vincent Hatakeyama Date: Tue, 12 Nov 2024 13:12:33 +0100 Subject: [PATCH] [IMP] auth_saml: only write value that changes When using mapping, not writing the value systematically avoids getting security mail on login/email changes when there is no change. Also use SQL for blanking passwords avoids the security update mails. --- auth_saml/models/res_users.py | 35 ++++++++++++++++++++++------------ auth_saml/readme/HISTORY.md | 10 ++++++++-- auth_saml/tests/test_pysaml.py | 32 +++++++++++++++++++++---------- 3 files changed, 53 insertions(+), 24 deletions(-) diff --git a/auth_saml/models/res_users.py b/auth_saml/models/res_users.py index 412b5c6994..1a7435147a 100644 --- a/auth_saml/models/res_users.py +++ b/auth_saml/models/res_users.py @@ -55,7 +55,14 @@ def _auth_saml_signin(self, provider: int, validation: dict, saml_response) -> s user_saml.with_env(new_env).write({"saml_access_token": saml_response}) if validation.get("mapped_attrs", {}): - user.write(validation.get("mapped_attrs", {})) + # Only write field that changes to avoid generating Security Update on users + # when login/email changes (from mail module) + vals = {} + for key, value in validation.get("mapped_attrs", {}).items(): + if not isinstance(value, str) or getattr(user, key) != value: + vals[key] = value + if vals: + user.write(vals) return user.login @@ -158,27 +165,31 @@ def _set_password(self): # pylint: disable=protected-access super(ResUser, non_blank_password_users)._set_password() if blank_password_users: - # similar to what Odoo does in Users._set_encrypted_password - self.env.cr.execute( - "UPDATE res_users SET password = NULL WHERE id IN %s", - (tuple(blank_password_users.ids),), - ) - blank_password_users.invalidate_recordset(fnames=["password"]) + blank_password_users._set_password_blank() return + def _set_password_blank(self): + """Set the password to a value that prohibits logging.""" + # Use SQL to blank the password to avoid sending security messages (done in + # mail module) to end users. + _logger.debug("Removing password from %s user(s)", len(self.ids)) + # similar to what Odoo does in Users._set_encrypted_password + self.env.cr.execute( + "UPDATE res_users SET password = NULL WHERE id IN %s", + (tuple(self.ids),), + ) + self.invalidate_recordset(fnames=["password"]) + def allow_saml_and_password_changed(self): """Called after the parameter is changed.""" if not self.allow_saml_and_password(): # sudo because the user doing the parameter change might not have the right # to search or write users - users_to_blank_password = self.sudo().search( + blank_password_users = self.sudo().search( [ "&", ("saml_ids", "!=", False), ("id", "not in", list(self._saml_allowed_user_ids())), ] ) - _logger.debug( - "Removing password from %s user(s)", len(users_to_blank_password) - ) - users_to_blank_password.write({"password": False}) + blank_password_users._set_password_blank() diff --git a/auth_saml/readme/HISTORY.md b/auth_saml/readme/HISTORY.md index 89020f8c3c..27737662f0 100644 --- a/auth_saml/readme/HISTORY.md +++ b/auth_saml/readme/HISTORY.md @@ -1,3 +1,9 @@ -## 16.0.1.0.0 +## 17.0.1.1.0 -Initial migration for 16.0. +When using attribute mapping, only write value that changes. +No writing the value systematically avoids getting security mail on login/email +when there is no real change. + +## 17.0.1.0.0 + +Initial migration for 17.0. diff --git a/auth_saml/tests/test_pysaml.py b/auth_saml/tests/test_pysaml.py index 9eedaa5405..78d87b0b20 100644 --- a/auth_saml/tests/test_pysaml.py +++ b/auth_saml/tests/test_pysaml.py @@ -133,17 +133,11 @@ def test__compute_sp_metadata_url__provider_has_sp_baseurl(self): self.assertEqual(self.saml_provider.sp_metadata_url, expected_url) self.saml_provider.sp_baseurl = temp - def test__hook_validate_auth_response(self): - # Create a fake response with attributes - fake_response = DummyResponse(200, "fake_data") - fake_response.set_identity( - {"email": "new_user@example.com", "first_name": "New", "last_name": "User"} - ) - - # Add attribute mappings to the provider + def _add_mapping_to_provider(self): + """Add mapping to the provider""" self.saml_provider.attribute_mapping_ids = [ - (0, 0, {"attribute_name": "email", "field_name": "login"}), - (0, 0, {"attribute_name": "first_name", "field_name": "name"}), + (0, 0, {"attribute_name": "mail", "field_name": "login"}), + (0, 0, {"attribute_name": "givenName", "field_name": "name"}), ( 0, 0, @@ -151,6 +145,13 @@ def test__hook_validate_auth_response(self): ), # This attribute is not in attrs ] + def test__hook_validate_auth_response(self): + # Create a fake response with attributes + fake_response = DummyResponse(200, "fake_data") + fake_response.set_identity( + {"mail": "new_user@example.com", "givenName": "New", "last_name": "User"} + ) + self._add_mapping_to_provider() # Call the method result = self.saml_provider._hook_validate_auth_response( fake_response, "test@example.com" @@ -261,6 +262,17 @@ def test_login_with_saml(self): # User should now be able to log in with the token self.authenticate(user="test@example.com", password=token) + def test_login_with_saml_mapping_attributes(self): + """Test login with SAML on a provider with mapping attributes""" + self.assertEqual(self.user.name, "User") + self.assertEqual(self.user.login, "test@example.com") + self._add_mapping_to_provider() + self.test_login_with_saml() + # Changed due to mapping and FakeIDP returning another value + self.assertEqual(self.user.name, "Test") + # Not changed + self.assertEqual(self.user.login, "test@example.com") + def test_disallow_user_password_when_changing_ir_config_parameter(self): """Test that disabling users from having both a password and SAML ids remove users password."""