Adding remove_superuser and remove_system_auditors to the SAML user attribute map (#12522)

awx/sso/conf.py

@@ -1537,9 +1537,11 @@ register(
         ('is_superuser_attr', 'saml_attr'),
         ('is_superuser_value', 'value'),
         ('is_superuser_role', 'saml_role'),
+        ('remove_superusers', True),
         ('is_system_auditor_attr', 'saml_attr'),
         ('is_system_auditor_value', 'value'),
         ('is_system_auditor_role', 'saml_role'),
+        ('remove_system_auditors', True),
     ],
 )
 

awx/sso/fields.py

@@ -743,8 +743,10 @@ class SAMLUserFlagsAttrField(HybridDictField):
     is_superuser_attr = fields.CharField(required=False, allow_null=True)
     is_superuser_value = fields.CharField(required=False, allow_null=True)
     is_superuser_role = fields.CharField(required=False, allow_null=True)
+    remove_superusers = fields.BooleanField(required=False, allow_null=True)
     is_system_auditor_attr = fields.CharField(required=False, allow_null=True)
     is_system_auditor_value = fields.CharField(required=False, allow_null=True)
     is_system_auditor_role = fields.CharField(required=False, allow_null=True)
+    remove_system_auditors = fields.BooleanField(required=False, allow_null=True)
 
     child = _Forbidden()

awx/sso/pipeline.py

@@ -255,6 +255,7 @@ def _check_flag(user, flag, attributes, user_flags_settings):
     is_role_key = "is_%s_role" % (flag)
     is_attr_key = "is_%s_attr" % (flag)
     is_value_key = "is_%s_value" % (flag)
+    remove_setting = "remove_%ss" % (flag)
 
     # Check to see if we are respecting a role and, if so, does our user have that role?
     role_setting = user_flags_settings.get(is_role_key, None)
@@ -286,7 +287,7 @@ def _check_flag(user, flag, attributes, user_flags_settings):
             # if they don't match make sure that new_flag is false
             else:
                 logger.debug(
-                    "Refusing %s for %s because attr %s (%s) did not match value '%s'"
+                    "For %s on %s attr %s (%s) did not match expected value '%s'"
                     % (flag, user.username, attr_setting, attribute_value, user_flags_settings.get(is_value_key))
                 )
                 new_flag = False
@@ -295,8 +296,16 @@ def _check_flag(user, flag, attributes, user_flags_settings):
             logger.debug("Giving %s %s from attribute %s" % (user.username, flag, attr_setting))
             new_flag = True
 
-    # If the user was flagged and we are going to make them not flagged make sure there is a message
+    # Get the users old flag
     old_value = getattr(user, "is_%s" % (flag))
+
+    # If we are not removing the flag and they were a system admin and now we don't want them to be just return
+    remove_flag = user_flags_settings.get(remove_setting, True)
+    if not remove_flag and (old_value and not new_flag):
+        logger.debug("Remove flag %s preventing removal of %s for %s" % (remove_flag, flag, user.username))
+        return old_value, False
+
+    # If the user was flagged and we are going to make them not flagged make sure there is a message
     if old_value and not new_flag:
         logger.debug("Revoking %s from %s" % (flag, user.username))
 

awx/sso/tests/functional/test_pipeline.py

@@ -438,69 +438,92 @@ class TestSAMLAttr:
 @pytest.mark.django_db
 class TestSAMLUserFlags:
     @pytest.mark.parametrize(
-        "user_flags_settings, expected",
+        "user_flags_settings, expected, is_superuser",
         [
             # In this case we will pass no user flags so new_flag should be false and changed will def be false
             (
                 {},
                 (False, False),
+                False,
             ),
             # In this case we will give the user a group to make them an admin
             (
                 {'is_superuser_role': 'test-role-1'},
                 (True, True),
+                False,
             ),
             # In this case we will give the user a flag that will make then an admin
             (
                 {'is_superuser_attr': 'is_superuser'},
                 (True, True),
+                False,
             ),
             # In this case we will give the user a flag but the wrong value
             (
                 {'is_superuser_attr': 'is_superuser', 'is_superuser_value': 'junk'},
                 (False, False),
+                False,
             ),
             # In this case we will give the user a flag and the right value
             (
                 {'is_superuser_attr': 'is_superuser', 'is_superuser_value': 'true'},
                 (True, True),
+                False,
             ),
             # In this case we will give the user a proper role and an is_superuser_attr role that they dont have, this should make them an admin
             (
                 {'is_superuser_role': 'test-role-1', 'is_superuser_attr': 'gibberish', 'is_superuser_value': 'true'},
                 (True, True),
+                False,
             ),
             # In this case we will give the user a proper role and an is_superuser_attr role that they have, this should make them an admin
             (
                 {'is_superuser_role': 'test-role-1', 'is_superuser_attr': 'test-role-1'},
                 (True, True),
+                False,
             ),
             # In this case we will give the user a proper role and an is_superuser_attr role that they have but a bad value, this should make them an admin
             (
                 {'is_superuser_role': 'test-role-1', 'is_superuser_attr': 'is_superuser', 'is_superuser_value': 'junk'},
                 (False, False),
+                False,
             ),
             # In this case we will give the user everything
             (
                 {'is_superuser_role': 'test-role-1', 'is_superuser_attr': 'is_superuser', 'is_superuser_value': 'true'},
                 (True, True),
+                False,
             ),
             # In this test case we will validate that a single attribute (instead of a list) still works
             (
                 {'is_superuser_attr': 'name_id', 'is_superuser_value': 'test_id'},
                 (True, True),
+                False,
             ),
             # This will be a negative test for a single atrribute
             (
                 {'is_superuser_attr': 'name_id', 'is_superuser_value': 'junk'},
                 (False, False),
+                False,
+            ),
+            # The user is already a superuser so we should remove them
+            (
+                {'is_superuser_attr': 'name_id', 'is_superuser_value': 'junk', 'remove_superusers': True},
+                (False, True),
+                True,
+            ),
+            # The user is already a superuser but we don't have a remove field
+            (
+                {'is_superuser_attr': 'name_id', 'is_superuser_value': 'junk', 'remove_superusers': False},
+                (True, False),
+                True,
             ),
         ],
     )
-    def test__check_flag(self, user_flags_settings, expected):
+    def test__check_flag(self, user_flags_settings, expected, is_superuser):
         user = User()
         user.username = 'John'
-        user.is_superuser = False
+        user.is_superuser = is_superuser
 
         attributes = {
             'email': ['noone@nowhere.com'],

awx/sso/tests/unit/test_fields.py

@@ -123,9 +123,11 @@ class TestSAMLUserFlagsAttrField:
             {'is_superuser_attr': 'something'},
             {'is_superuser_value': 'value'},
             {'is_superuser_role': 'my_peeps'},
+            {'remove_superusers': False},
             {'is_system_auditor_attr': 'something_else'},
             {'is_system_auditor_value': 'value2'},
             {'is_system_auditor_role': 'other_peeps'},
+            {'remove_system_auditors': False},
         ],
     )
     def test_internal_value_valid(self, data):
@@ -165,6 +167,17 @@ class TestSAMLUserFlagsAttrField:
                     'junk2': ['Invalid field.'],
                 },
             ),
+            # make sure we can't pass a string to the boolean fields
+            (
+                {
+                    'remove_superusers': 'test',
+                    'remove_system_auditors': 'test',
+                },
+                {
+                    "remove_superusers": ["Must be a valid boolean."],
+                    "remove_system_auditors": ["Must be a valid boolean."],
+                },
+            ),
         ],
     )
     def test_internal_value_invalid(self, data, expected):