search

awslabs / ssosync

Populate AWS SSO directly with your G Suite users and groups using either a CLI or AWS Lambda
Apache License 2.0
512 stars 175 forks source link

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

Closed philomory closed 2 months ago

philomory commented 2 months ago

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 commented 2 months ago

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

ChrisPates commented 2 months ago

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