Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing bug in LDAP reconcile loop #13342

Merged
merged 2 commits into from
Jan 9, 2023

Conversation

john-westcott-iv
Copy link
Member

SUMMARY

The LDAP reconcile loop did not account for the same team name in multiple organizations. It would add or remove the user from any team by a name (which is not unique). This change now forces the reconcile loop to consider the team along with the organization.

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
COMPONENT NAME
  • API
AWX VERSION
awx: 21.9.1.dev95+g14fa2685d5
ADDITIONAL INFORMATION

awx/sso/backends.py Outdated Show resolved Hide resolved
continue
if role_name not in roles:
roles.append(role_name)
model_roles = Team.objects.filter(name__in=team_names).values_list('name', 'organization__name', *roles, named=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now this query performs the same filter as before, but it returns more values so that you capture the organization name... (continuing walk-through)

if object_type == 'organization':
desired_state = desired_states.get(row.name, {})
else:
desired_state = desired_states.get(row.organization__name, {}).get(row.name, {})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, we have the possibility that the team in question is unwanted, because it has the same name as a team listed in the configuration, but it's in another organization. In that event, desired_states should be lacking either the team's org name key or the team name key, and this should return {}

# The mapping was not defined for this [org/team]/role so we can just pass
pass
continue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here, where we have an unwanted "extra" team returned by the query, we hit the continue because it was not in the configuration (the desired_states). We don't remove or add the user, and that fixes the bug.

Copy link
Member

@AlanCoding AlanCoding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know tests are still failing, and I don't know why, but I can follow everything here and I believe it fixes the bug.

We should add test coverage, even unit tests feel workable. But for the core code changes, I can read this and see through to the fix.

@john-westcott-iv
Copy link
Member Author

We should def add test coverage for this function.

@john-westcott-iv john-westcott-iv self-assigned this Jan 9, 2023
@john-westcott-iv john-westcott-iv merged commit d73cc50 into ansible:devel Jan 9, 2023
TheRealHaoLiu pushed a commit to TheRealHaoLiu/awx that referenced this pull request Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants