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

Group flattening can lead to conflicts due to non-uniqueness #199

Closed
philomory opened this issue Jun 15, 2024 · 2 comments · Fixed by #201
Closed

Group flattening can lead to conflicts due to non-uniqueness #199

philomory opened this issue Jun 15, 2024 · 2 comments · Fixed by #201
Assignees
Labels
bug Something isn't working

Comments

@philomory
Copy link

Describe the bug
If a user is both a direct and indirect member of the same group (or, probably, an indirect member of a group through multiple indirection paths), then ssosync tries to add the member to same group twice in a single invocation, which is not allowed, resulting in the error Notifying Lambda and mark this execution as Failure: ConflictException: Member and Group relationship already exists with the reason listed as UNIQUENESS_CONSTRAINT_VIOLATION. The sync then aborts mid-process.

On subsequent invocations, because the user was added to the group once before the failure, the sync process will see that the user is already a member of the group in question in AWS and not attempt to add them again, bypassing the failure. However, since the execution halts on the first such error each time, if you have a dense tree of nested groups with a number of redundant membership paths, it may take a significant number of executions before all of the errors are worked through.

To Reproduce
Steps to reproduce the behavior:

  1. Create two groups in Google Workspace, test-group-parent, and test-group-child.
  2. Create a user in Google Workspace, test-user.
  3. Add the user test-user as a member of both test-group-parent and test-group-child.
  4. Add the group test-group-child as a member of test-group-parent.
  5. Ran with --group-match "name:test-group*"
  6. See error

Expected behavior
Tool should handle case where user is a member of the same group via multiple "paths" without unhandled exceptions.

Additional context
I'm fairly certain the issue has its origin is this section of code that handles nested group membership: https://github.com/awslabs/ssosync/blob/6cb78e1225f5193ea996b215d1eca378046701dd/internal/sync.go#L1067C17-L1076C18

A simple fix would be to use a set data type instead of an array for memberUsers, except I've just learned Go doesn't have one of those (really?); making a poor man's set from a map, or just sticking with an array and removing duplicates before returning, would work as well.

Separately, it may seem reasonable to question, "why would you have redundant group membership in this way?", and indeed the example above seems rather arbitrary; however, there are fairly realistic scenarios in which this might occur naturally; for example, if a user is a member of a "developers" group and a "devops" group, and both of those groups are nested within an "engineering" group, then they'll show up twice within "engineering" using the group flattening method currently employed.

@CarlosCuevas
Copy link

experiencing the same issue. similarly, i've resorted to just letting it run over and over to get past it.

@ChrisPates ChrisPates self-assigned this Jun 18, 2024
@ChrisPates ChrisPates added the bug Something isn't working label Jun 18, 2024
@ChrisPates
Copy link
Contributor

I'll hopefully be able to look at this week and get a lease out next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants