From 4c8ba38d929aa32ecab20c8a3c5c298a8dc64f65 Mon Sep 17 00:00:00 2001
From: Valentin Deniaud <vdeniaud@entrouvert.com>
Date: Tue, 20 Oct 2020 10:07:30 +0200
Subject: [PATCH] auth_saml: rename toggle-role action to add-role (#46857)

---
 src/authentic2_auth_saml/adapters.py |  5 +-
 tests/test_auth_saml.py              | 75 +++++++++++++++++-----------
 2 files changed, 51 insertions(+), 29 deletions(-)

diff --git a/src/authentic2_auth_saml/adapters.py b/src/authentic2_auth_saml/adapters.py
index 2f0a42a3..26645672 100644
--- a/src/authentic2_auth_saml/adapters.py
+++ b/src/authentic2_auth_saml/adapters.py
@@ -245,7 +245,7 @@ class AuthenticAdapter(DefaultAdapter):
         except Exception as e:
             raise MappingError('condition evaluation failed', details={'error': six.text_type(e)})
 
-    def action_toggle_role(self, user, idp, saml_attributes, mapping):
+    def action_add_role(self, user, idp, saml_attributes, mapping):
         role = self.get_role(mapping)
         if self.evaluate_condition(user, saml_attributes, mapping):
             if role not in user.roles.all():
@@ -256,5 +256,8 @@ class AuthenticAdapter(DefaultAdapter):
                 logger.info('auth_saml: removing role "%s"', role, extra={'user': user})
                 user.roles.remove(role)
 
+    def action_toggle_role(self, *args, **kwargs):
+        return self.action_add_role(*args, **kwargs)
+
     def auth_login(self, request, user):
         utils.login(request, user, 'saml')
diff --git a/tests/test_auth_saml.py b/tests/test_auth_saml.py
index 9e334910..9a564b59 100644
--- a/tests/test_auth_saml.py
+++ b/tests/test_auth_saml.py
@@ -52,7 +52,7 @@ def test_providers_on_login_page(db, app, settings):
     assert response.pyquery('button[name="login-saml-1"]')
 
 
-def test_provision_attributes(db, caplog, simple_role):
+def test_provision_attributes(db, caplog):
     from authentic2_auth_saml.adapters import AuthenticAdapter
 
     adapter = AuthenticAdapter()
@@ -79,16 +79,6 @@ def test_provision_attributes(db, caplog, simple_role):
             {
                 'attribute': 'first_name',
                 'saml_attribute': 'first_name',
-            },
-            {
-                'action': 'toggle-role',
-                'role': {
-                    'name': simple_role.name,
-                    'ou': {
-                        'name': simple_role.ou.name,
-                    },
-                },
-                'condition': "roles == 'A'",
             }
         ]
     }
@@ -106,11 +96,56 @@ def test_provision_attributes(db, caplog, simple_role):
     assert user.email == 'john.doe@example.com'
     assert user.attributes.title == 'Mr.'
     assert user.first_name == 'John'
+    user.delete()
+
+    # on missing mandatory attribute, no user is created
+    del saml_attributes['mail']
+    assert adapter.lookup_user(idp, saml_attributes) is None
+
+    # 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)
+
+
+@pytest.mark.parametrize('action_name', ['add-role', 'toggle-role'])
+def test_provision_add_role(db, simple_role, action_name):
+    from authentic2_auth_saml.adapters import AuthenticAdapter
+
+    adapter = AuthenticAdapter()
+    User = get_user_model()
+    user = User.objects.create()
+    idp = {
+        'A2_ATTRIBUTE_MAPPING': [
+            {
+                'action': action_name,
+                'role': {
+                    'name': simple_role.name,
+                    'ou': {
+                        'name': simple_role.ou.name,
+                    },
+                },
+                'condition': "roles == 'A'",
+            }
+        ]
+    }
+
+    saml_attributes = {
+        'issuer': 'https://idp.com/',
+        'name_id_content': 'xxx',
+        'name_id_format': lasso.SAML2_NAME_IDENTIFIER_FORMAT_PERSISTENT,
+    }
+    user = adapter.lookup_user(idp, saml_attributes)
+    user.refresh_from_db()
     assert simple_role not in user.roles.all()
     user.delete()
 
     # if a toggle-role is mandatory, failure to evaluate condition block user creation
-    assert idp['A2_ATTRIBUTE_MAPPING'][-1]['action'] == 'toggle-role'
+    assert idp['A2_ATTRIBUTE_MAPPING'][-1]['action'] == action_name
     idp['A2_ATTRIBUTE_MAPPING'][-1]['mandatory'] = True
     assert adapter.lookup_user(idp, saml_attributes) is None
 
@@ -130,22 +165,6 @@ def test_provision_attributes(db, caplog, simple_role):
     # condition failed, so role should be removed
     assert simple_role not in user.roles.all()
 
-    user.delete()
-
-    # on missing mandatory attribute, no user is created
-    del saml_attributes['mail']
-    assert adapter.lookup_user(idp, saml_attributes) is None
-
-    # 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)
-
-
 
 def test_login_with_conditionnal_authenticators(db, app, settings, caplog):
 
-- 
2.20.1