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

Delete user if inactive only #61

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Luis-3M
Copy link

@Luis-3M Luis-3M commented Sep 22, 2021

Issue #59

Description of changes:

Problem
Noticed that if we delete a user on the Google side and then recreate it with the exact same details the following occurs:

  1. ssosync will consider that user as inactive based on the API response, hence deleting it from AWS SSO
    https://github.com/awslabs/ssosync/blob/master/internal/sync.go#L70-L103
    https://github.com/awslabs/ssosync/blob/master/internal/google/client.go#L64-L73

  2. ssosync will then list all the active users and it will see that it needs to add again the user that has just been deleted in the previous step.
    https://github.com/awslabs/ssosync/blob/master/internal/sync.go#L105-L152

  3. At this stage user will have no permission sets assigned on AWS SSO, and this will happen every time we run ssosync.

Possible Solution
I believe that it would be beneficial to change the SyncUsers function so it checks that all of the API returned deleted users are in fact inactive, and for the ones that are now active it just ignores them by building a new list of deleted users which we can iterate over just like its being done atm.

@Luis-3M Luis-3M changed the title IN-28: Delete user if inactive only Delete user if inactive only Sep 22, 2021
@Luis-3M
Copy link
Author

Luis-3M commented Sep 28, 2022

@ChrisPates could you review this PR pls ?

@Luis-3M
Copy link
Author

Luis-3M commented Jan 24, 2023

@ChrisPates What are your thoughts on this code optimisation?

@ChrisPates
Copy link
Contributor

So I have a number of issues to address around how the adds/updates/deletes are determined.

The part that is really the source of this undesirable behaviour is the call to retrieve deleted users from the Google side.

So I'm looking at performing a full comparison (which wasn't possible before the introduction of the identityStore api) and potentially making use of the external identifier fields in identity store so matching is more accurate.

Also this would provide a route back from the use of DisplayName which allows duplicates on the Google side (since they key on primary email address) but on the identity store side it must because it keys on displayName.

@Luis-3M
Copy link
Author

Luis-3M commented Jan 30, 2023

Awesome looking forward to it @ChrisPates - indeed relying on just the users list API call on the Google side (with ShowDeleted("true")) is the source of the problem.

Will keep an eye on this thread.

@ChrisPates
Copy link
Contributor

I've had to update the workflows, can you update your fork from master and update this PR, I'll get it merged into the main code base.

@ChrisPates ChrisPates self-assigned this Aug 28, 2024
Copy link
Contributor

@ChrisPates ChrisPates left a comment

Choose a reason for hiding this comment

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

Looks, good once the PR has been updated from master. I'll run through the dev pipeline and get it merged in to master.

@Luis-3M
Copy link
Author

Luis-3M commented Sep 3, 2024

Thanks @ChrisPates - I'll get this done as soon as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants