diff --git a/awx/sso/conf.py b/awx/sso/conf.py index 29d7f401d3..4517c387f3 100644 --- a/awx/sso/conf.py +++ b/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), ], ) diff --git a/awx/sso/fields.py b/awx/sso/fields.py index 9ad016f594..011aedcffd 100644 --- a/awx/sso/fields.py +++ b/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() diff --git a/awx/sso/pipeline.py b/awx/sso/pipeline.py index 73d2edfce5..079cd6587d 100644 --- a/awx/sso/pipeline.py +++ b/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)) diff --git a/awx/sso/tests/functional/test_pipeline.py b/awx/sso/tests/functional/test_pipeline.py index f0d6427a34..3e64114db5 100644 --- a/awx/sso/tests/functional/test_pipeline.py +++ b/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'], diff --git a/awx/sso/tests/unit/test_fields.py b/awx/sso/tests/unit/test_fields.py index 463daa3b3e..02803d5e15 100644 --- a/awx/sso/tests/unit/test_fields.py +++ b/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):