From 60ad5a9a8c49419f35ce4b7bc0cbd4b47d37c345 Mon Sep 17 00:00:00 2001
From: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date: Thu, 15 Oct 2020 17:46:22 +0200
Subject: [PATCH] auth_saml: always add mapping as MappingError details
 (#47760)

---
 src/authentic2_auth_saml/adapters.py | 24 +++++++++++++-----------
 tests/test_auth_saml.py              | 24 +++++++++++++++++-------
 2 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/src/authentic2_auth_saml/adapters.py b/src/authentic2_auth_saml/adapters.py
index 2f0a42a3..62131096 100644
--- a/src/authentic2_auth_saml/adapters.py
+++ b/src/authentic2_auth_saml/adapters.py
@@ -36,8 +36,7 @@ class MappingError(Exception):
     details = None
 
     def __init__(self, message, details=None):
-        if details:
-            self.details = details
+        self.details = details or {}
         super(MappingError, self).__init__(message)
 
     def __str__(self):
@@ -104,6 +103,10 @@ class AuthenticAdapter(DefaultAdapter):
         if not isinstance(attribute_mapping, list):
             raise MappingError('invalid A2_ATTRIBUTE_MAPPING')
 
+        if self.apply_attribute_mapping(user, idp, saml_attributes, attribute_mapping):
+            user.save()
+
+    def apply_attribute_mapping(self, user, idp, saml_attributes, attribute_mapping):
         user_modified = False
         for mapping in attribute_mapping:
             if not isinstance(mapping, dict):
@@ -116,21 +119,20 @@ class AuthenticAdapter(DefaultAdapter):
                     method = getattr(self, 'action_' + action.replace('-', '_'))
                 except AttributeError:
                     pass
-            if not method:
-                raise MappingError('invalid action %r' % action)
             try:
+                if not method:
+                    raise MappingError('invalid action')
                 logger.debug('auth_saml: applying provisionning mapping %s', mapping)
                 if method(user, idp, saml_attributes, mapping):
                     user_modified = True
             except MappingError as e:
+                e.details['mapping'] = mapping
                 if mandatory:
                     # it's mandatory, provisionning should fail completely
                     raise e
                 else:
-                    logger.debug('auth_saml: action mapping %r failed: %s', mapping, e)
-
-        if user_modified:
-            user.save()
+                    logger.warning('auth_saml: mapping action failed: %s', e)
+        return user_modified
 
     def action_rename(self, user, idp, saml_attributes, mapping):
         from_name = mapping.get('from')
@@ -152,16 +154,16 @@ class AuthenticAdapter(DefaultAdapter):
             raise MappingError('missing saml_attribute key')
 
         if saml_attribute not in saml_attributes:
-            raise MappingError('unknown saml_attribute', details={'saml_attribute': saml_attribute})
+            raise MappingError('unknown saml_attribute')
         value = saml_attributes[saml_attribute]
         return self.set_user_attribute(user, attribute, value)
 
     def set_user_attribute(self, user, attribute, value):
         if isinstance(value, list):
             if len(value) == 0:
-                raise MappingError('no value for %s' % attribute, details={'attribute': attribute})
+                raise MappingError('no value')
             if len(value) > 1:
-                raise MappingError('too many values for %s' % attribute, details={'attribute': attribute})
+                raise MappingError('too many values', details={'value': value})
             value = value[0]
         if attribute in ('first_name', 'last_name', 'email', 'username'):
             if getattr(user, attribute) != value:
diff --git a/tests/test_auth_saml.py b/tests/test_auth_saml.py
index 9e334910..991a9abc 100644
--- a/tests/test_auth_saml.py
+++ b/tests/test_auth_saml.py
@@ -15,7 +15,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
-import logging
+import re
 
 import pytest
 
@@ -138,13 +138,23 @@ def test_provision_attributes(db, caplog, simple_role):
 
     # simulate no attribute value
     saml_attributes['first_name'] = []
-    mapping = {
-        'attribute': 'first_name',
-        'saml_attribute': 'first_name',
-    }
-    with pytest.raises(MappingError, match='no value for first_name'):
-        adapter.action_set_attribute(user, idp, saml_attributes, mapping)
+    attribute_mapping = [
+        {
+            'mandatory': True,
+            'attribute': 'first_name',
+            'saml_attribute': 'first_name',
+        }
+    ]
+
+    # fail fast
+    with pytest.raises(MappingError, match='no value.*first_name'):
+        adapter.apply_attribute_mapping(user, idp, saml_attributes, attribute_mapping)
 
+    # or log a warning
+    caplog.clear()
+    del attribute_mapping[0]['mandatory']
+    adapter.apply_attribute_mapping(user, idp, saml_attributes, attribute_mapping)
+    assert re.match('.*no value.*first_name', caplog.records[0].message)
 
 
 def test_login_with_conditionnal_authenticators(db, app, settings, caplog):
-- 
2.28.0