Allow saml_admin_attr to work in conjunction with SAML Org Map

[john-westcott-iv]

SUMMARY

From the SAML redesign, we found a regression where an admin specified by saml_admin_attr property of SOCIAL_AUTH_SAML_ORGANIZATION_ATTR setting but the user was not a member of the admin role from the
admins property of the SOCIAL_AUTH_SAML_ORGANIZATION_MAP setting the user would not be an admin of the organization. We fixed this by doing an or condition between the existing desired_org_state and the returned values.

ISSUE TYPE

• New or Enhanced Feature

COMPONENT NAME

• API

AWX VERSION

ADDITIONAL INFORMATION

[AlanCoding]

Not getting it.

    def test__update_user_orgs_with_role_name(self, backend, users):
        u1, u2, u3 = users

        # Test user membership logic with regular expressions
        backend.setting('ORGANIZATION_MAP')['Default']['role_name'] = 'auditor_role'
        backend.setting('ORGANIZATION_MAP')['Default']['users'] = re.compile('.*')

        # desired_org_state = {'Default': {'role_name': 'auditor_role'}}
        desired_org_state = {}
        orgs_to_create = []
        print((backend, desired_org_state, orgs_to_create, u1))
        _update_user_orgs(backend, desired_org_state, orgs_to_create, u1)
        _update_user_orgs(backend, desired_org_state, orgs_to_create, u2)
        _update_user_orgs(backend, desired_org_state, orgs_to_create, u3)

        assert desired_org_state == {'Default': {'member_role': False, 'admin_role': False, 'auditor_role': True}}

Trying and failing to write a unit test at awx/sso/tests/functional/test_saml_pipeline.py and it's not clicking.

Here's thing thing - even if I artificially put in 'role_name' key into desired_org_state (under the organization), what would the value be? If it's a role name, then I presume something like "auditor_role". But your line:

desired_org_state[organization_name][role_name] = desired_org_state[organization_name].get('role_name', False) or has_role

means that if the first condition is met, it gives data like {"member_role": "auditor_role"}. Otherwise it's of {"member_role": True} format. It just makes no sense. It's true I don't know what might be setting "role_name" but yeah, that blank needs to be filled in. I don't see how this could be coherent.

[john-westcott-iv]

@AlanCoding The way the data structure works we need desired_org_state[organization_name][role_name] to be either True or False to know whether the role should be granted or not. The problem we have with the SAML adapter is that there are more than one way a user can be mapped as an admin or auditor. i.e. this can come from an org map or saml_admin_attr. And they would overwrite each other. So if the first method evaluated to True and the second evaluated to False you ended up without the permissions.

What I am doing here to solve this is creating an or condition so that if the field is already set to True from the first method and False from the second we still end up with True in the end. So take the existing value already set in desired_org_state[organization_name].get('role_name') (or default to False if not found) and or that value with he value from the second method returned in the variable has_role. So we assume they will start without the permissions and then if anything sets it to True they get the permission.

So a test, imho, would look more like:

  @pytest.mark.parameterize(
      "desired_org_state,update_m2m_return.expected",
      (
         # Set the existing auditor_role to either true or false and the call to _update_m2m_from_expression to either return true or faslse
          ( {'Default': {'auditor_role': False} }, True, True, ), 
          ( {'Default': {'auditor_role': True} }, True, True, ), 
          ( {'Default': {'auditor_role': False} }, False, False, ), 
          ( {'Default': {'auditor_role': True} }, False, True, ), 
       )
   )
  def test__update_user_orgs_with_role_name(self, backend, users, desired_org_state, update_m2m_return, expected):
        u1, u2, u3 = users

        # Test user membership logic with regular expressions
        backend.setting('ORGANIZATION_MAP')['Default']['role_name'] = 'auditor_role'
        backend.setting('ORGANIZATION_MAP')['Default']['users'] = re.compile('.*')

       with mock.patch('awx.sso.saml_pipeline._update_m2m_from_expression', return_value=update_m2m_return):
           _update_user_orgs(backend, desired_org_state, [], u1)

       assert desired_org_state['Default']['auditor_role'] == expected

Note: didn't test this to make sure it worked, there might be more you have to mock

[AlanCoding]

In the line before what you change:

has_role = _update_m2m_from_expression(user, is_member_expression, remove_members)

Logically, I look at has_role and expect a boolean. I check the method definition, and yeah, it is clearly written to return True or False or None.

So why would the test set this?

backend.setting('ORGANIZATION_MAP')['Default']['role_name'] = 'auditor_role'

Just based on variable type, we established that desired_org_state[organization_name]['role_name'] should be True, False, or None. Why is it all of a sudden a mapping to the role name like 'auditor_role'? In the entire awx/sso/saml_pipeline.py file I can't find any other case where it sets the 'role_name' key. It's this string vs. variable issue that's killing me.

Instead of 'role_name' in this diff, would role_name be correct? Like

desired_org_state[organization_name][role_name] = desired_org_state[organization_name].get(role_name, False) or has_role

If I squint and try not to think too hard about it, that seems like it might hit at your objective:

if the field is already set to True from the first method and False from the second we still end up with True in the end

[john-westcott-iv]

Yea, the test should probably make that look like a valid value for ORGANIZATION_MAP (I just copied that from whatever you had). In thinking harder, that mapping (other than the name of the org itself) is actually probably irrelevant if we mock the return of _update_m2m_from_expression.

[AlanCoding]

yeah, so maybe we can get a test that more faithfully does a set up according to

The problem we have with the SAML adapter is that there are more than one way a user can be mapped as an admin or auditor. i.e. this can come from an org map or saml_admin_attr

So the input itself is a the settings that makes a user admin via one method and auditor via the other method. Just thinking out loud now.

[AlanCoding]

I think I might have gotten it. Here is the fix along with a new test for it:

https://github.com/ansible/awx/compare/devel...AlanCoding:saml_admin_option2?expand=1

This test will fail on the line

assert desired_org_state['o1_alias']['admin_role'] is True

This is because the 2nd rule which adds the user as a member will revert the first rule that added the user as an admin.

This changes 'role_name' to role_name, which I believe is a legitimate mistake. To me, this justifies the need for actual testing. We also need some better tracking for the actual issue. Otherwise I'm ready to go with this fix and test and you let me know how you want to move forward. I had to rebase that branch, so we need a rebase of your branch if we're going to cherry pick it as a patch and test.

[jay-steurer]

Hi Alan, I think the link you shared is missing the PR part.

[AlanCoding]

It depends on whether John wants to keep this PR or close it and open a new one.

[AlanCoding]

Also, I still don't have an issue for this.

[john-westcott-iv]

@AlanCoding thanks for catching this!

[AlanCoding]

Current status is that I believe this is a good and valid patch.

However, we have not obtained confirmation by any experiencing the bug that it fixed their problem.