[17.0][IMP] auth_saml: only write value that changes

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

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.

auth_saml/tests/test_pysaml.py

@@ -133,24 +133,25 @@ 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": "[email protected]", "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,
                 {"attribute_name": "nick_name", "field_name": "name"},
             ),  # 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": "[email protected]", "givenName": "New", "last_name": "User"}
+        )
+        self._add_mapping_to_provider()
         # Call the method
         result = self.saml_provider._hook_validate_auth_response(
             fake_response, "[email protected]"
@@ -261,6 +262,17 @@ def test_login_with_saml(self):
         # User should now be able to log in with the token
         self.authenticate(user="[email protected]", 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, "[email protected]")
+        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, "[email protected]")
+
     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."""