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

Ldap sync - remove deleted accounts #116

Closed
gerardorourke opened this issue Apr 16, 2020 · 6 comments
Closed

Ldap sync - remove deleted accounts #116

gerardorourke opened this issue Apr 16, 2020 · 6 comments
Labels
enhancement New feature or request question Further information is requested

Comments

@gerardorourke
Copy link

I have confirmed that when syncing as per below documentation - that any Active Directory account which is disabled will gets soft deleted - so this is great.

However if the account has been deleted from AD, there is no update on this account on the Laravel User Sync. Ideally deleted AD accounts - would get soft deleted in Laravel Users Table.

I assume this be achieved that after a sync - any accounts which have not been confirmed existing - get soft deleted.

In summary - the below soft deletes disabled AD accounts - but not deleted AD accounts.
Ref: https://ldaprecord.com/docs/laravel/auth/importing/

protected function schedule(Schedule $schedule)
{
// Import LDAP users hourly.
$schedule->command('ldap:import ldap', [
'--no-interaction',
'--restore',
'--delete',
'--filter' => '(objectclass=user)',
])->hourly();
}

@stevebauman stevebauman added enhancement New feature or request question Further information is requested labels Apr 16, 2020
@stevebauman
Copy link
Member

stevebauman commented Apr 16, 2020

Hi @gerardorourke,

I've worried about adding this specific feature in as to avoid soft-deleting accounts that were not discovered for specific imports (such as manual imports via command line using a filter, or importing a single user).

For example, say I wanted to manually import one user into my application, and I also wanted to soft-delete their model if they are disabled:

php artisan ldap:import sbauman --delete

If I implement a change to soft-delete all users whom are missing with this flag, then all other database users will be soft-deleted.

What are your thoughts on having a specific flag for this use case such as --delete-missing?

Example:

php artisan ldap:import --delete-missing

@gerardorourke
Copy link
Author

Steve,

You are correct to be worried. With this feature it is best to be cautious!
Your option of a "delete-missing" sounds perfect and seems like the ideal solution.
The soft delete would only occur if the connection to LDAP was successfully and fully completed.

Not sure if an additional "safety" parameter should also be added i.e. to only do the delete-missing if the number of missing records to be deleted is less than "10%" (configurable) of your total user count?

I don't think this safety parameter is 100% necessary (but it would be nice) - as the great thing about a soft deletes is that is always possible to do an undelete if necessary with a simple SQL update.

Gerry

@stevebauman
Copy link
Member

stevebauman commented Apr 16, 2020

Great, I'm glad you're on board with the flag! 👍

Not sure if an additional "safety" parameter should also be added i.e. to only do the delete-missing if the number of missing records to be deleted is less than "10%" (configurable) of your total user count?

I think what we can do is collect all of the Object GUIDs of users that are imported, and then run a query to soft-delete all of the users that have Object GUIDs, but are not existing in the list of import GUIDs. For example (psuedo-code):

$importedGuids = $this->import();

User::whereNotNull('guid')
    ->whereNotIn($importedGuids)
    ->where('domain', '=', $domain)
    ->update(['deleted_at' => now()]);

There's two ways we can do the above though:

  1. (Shown above) We can either run a single query to soft-delete all of the users whom are missing from the imported -- but this has a caveat as Eloquent events are not fired for this type of update, such as the deleting and deleted events.
  2. Update each user individually (more resource intensive), but all Eloquent events are fired.

What are your thoughts?

@gerardorourke
Copy link
Author

gerardorourke commented Apr 16, 2020

Ldap integrations can and often will have 1000s of records - so I think your first option makes more sense.

If any deleted entries were detailed in the Laravel log file - this would be useful.

@gerardorourke
Copy link
Author

Steve,
Thank you. I have tested this and it is working lovely!
Gerry

@stevebauman
Copy link
Member

stevebauman commented Apr 20, 2020

Awesome, glad to hear @gerardorourke!

I've just released v1.3.0 with this included, so we're good to go 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants