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

New sync mechanism #41016

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

New sync mechanism #41016

wants to merge 4 commits into from

Conversation

jvillafanez
Copy link
Member

@jvillafanez jvillafanez commented Oct 9, 2023

Description

New generic sync mechanism, aiming to fix some flaws the current mechanism has. In particular, the errors that could happen during syncing will be reported and be visible to the admin, so he'll know if something goes wrong; the command will fail (exit code different than 0) if something goes wrong.
The new sync mechanism will use specialized interfaces, not existing ones.

Advantages:

  • Apps can add their own sync services in order to sync calendar, contacts, federated data, or whatever they want.
  • Apps can also overwrite sync services to provide additional features or optimizations.
  • Expected error handling is documented in the interface in order to be able to forward the exceptions and make them visible to the admin.

This PR includes the user sync service, which will allow admins to sync users from multiple backends. Right now, the DB backend is provided, and support for LDAP will be added in the user_ldap app (owncloud/user_ldap#808). Additional backends can be added in the respective apps.

Some simple examples:

  • occ sync:sync user -> check and sync users from all registered backends (DB included). Missing users will be disabled
  • occ sync:sync user -o 'missingAction=remove' -> check and sync users from all registered backends (DB included). Missing users will be removed
  • occ sync:sync user -o 'backends=OCA\User_LDAP\User_Proxy' -> check and sync users just for that backend (DB users will be ignored). Missing users (from that backend) will be disabled
  • occ sync:sync user -o 'missingAction=remove' -o 'backends=OCA\User_LDAP\User_Proxy' -> check and sync users just for that backend. Missing users will be removed
  • occ sync:sync user --only-one 59a09cfe-f23f-103d-992b-5b46a8a66263 -> check and sync only the user with that uid. If it's missing, the user will be disabled
  • occ sync:sync user --only-one 59a09cfe-f23f-103d-992b-5b46a8a66263 -o 'backends=OC\User\Database' -o 'missingAction=remove' -> check the user with that uid in the specified backend. If the user isn't in that backend (it might be from a different backend), it won't do anything; it will only remove the user if the backend matches and the user is missing. The command will try to sync the user from that backend (which should fail if it belongs to another backend).

Related Issue

https://github.com/owncloud/enterprise/issues/5775

Motivation and Context

The current sync mechanism has an important problems when syncing LDAP users. There could be name collisions and the LDAP part could return less users than requested, causing the sync mechanism to stop and prevent syncing the rest of the LDAP users.
The new sync mechanism fixes that problem, and also make the error visible to the admin so it doesn't need to monitor the logs for specific errors.

How Has This Been Tested?

Manually tested, running the command in multiple scenarios

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@update-docs
Copy link

update-docs bot commented Oct 9, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@jvillafanez
Copy link
Member Author

Unit tests for the UserSyncer class are incomplete at the moment. New tests need to be added.

@jvillafanez jvillafanez force-pushed the new_sync_mech branch 2 times, most recently from efb31c9 to e95a108 Compare October 20, 2023 11:05
@sonarcloud
Copy link

sonarcloud bot commented Oct 20, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 21 Code Smells

59.5% 59.5% Coverage
1.4% 1.4% Duplication

@jvillafanez
Copy link
Member Author

This is ready to review.
The PR could be tested without any additional change in order to sync the DB users (not much useful). LDAP syncing is in owncloud/user_ldap#808 ; the code can be run although it isn't fully finished yet due to missing tests.

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

1 participant