Refactor LDAP sync to sync service structure #340
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JustSamuel
force-pushed
the
refactor/user-sync-service
branch
from
a7fc805 to
28aa07a
Compare
Yoronex
reviewed
Sep 30, 2024
JustSamuel
force-pushed
the
refactor/user-sync-service
branch
10 times, most recently
from
4a524be to
446691f
Compare
JustSamuel
changed the title
JustSamuel
force-pushed
the
refactor/user-sync-service
branch
from
cc05292 to
72f2e5b
Compare
JustSamuel
force-pushed
the
refactor/user-sync-service
branch
4 times, most recently
from
ef207b5 to
24c9f3c
Compare
JustSamuel
force-pushed
the
refactor/user-sync-service
branch
8 times, most recently
from
1c88b3d to
df93c65
Compare
JustSamuel
force-pushed
the
refactor/user-sync-service
branch
from
df93c65 to
302f5cc
Compare
JustSamuel
force-pushed
the
refactor/user-sync-service
branch
2 times, most recently
from
4321116 to
bc3d5ab
Compare
This was
linked to
issues
Jan 24, 2025
Closed
JustSamuel
force-pushed
the
refactor/user-sync-service
branch
from
bc3d5ab to
ea55643
Compare
Yoronex
reviewed
Jan 26, 2025
JustSamuel
force-pushed
the
refactor/user-sync-service
branch
4 times, most recently
from
c66f410 to
a982f76
Compare
JustSamuel
force-pushed
the
refactor/user-sync-service
branch
from
a982f76 to
d514c79
Compare
Yoronex
approved these changes
Feb 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR changes the syncing of users to a user-first syncing. This means that instead of having different services scattered throughout the codebase, we focus on having a single interface that can be implemented and used in the general user sync.
A
sync-serviceshould implement async,fetchanddownfunction. It can also be decided to override theguardfunction if needed. The new syncing structure is as follows:The
user-sync-serviceis akin to a compound design pattern and takes an array of services to use. Each of these services implements thesyncfunction, enabling the user to be synced with an external service. Thissynchas a result object which can indicate one of the following:A bad sync can be the case that the user does no longer exist in LDAP or the GEWISDB.
After the user has been synced by all the services, the following happens:
downa user if all services returned a bad sync, or else do nothing.downis the function which should clean up or remove data related to the user entity. For the LDAP sync this will be removing the LDAPAuthentication entry, but for the GEWISDB sync this will mean deactivating the user in SudoSOS as no longer existing in the GEWISDB is a reason to be deactivated. This bring us to the following question:Currently
downis only called when all syncs are bad. Is this desired?Take the cases that a user becomes inactive and removed from AD. The LDAP sync will start to report bad syncs. This could be grounds to remove the user from the LDAP table. However only calling
downwhen all syncs report bad feels safer as it makes for a build in safeguard. This is exceptionally clear from the other case, where a user is removed from GEWISDB but still exists in AD. This is strange, and we would expect a user to also be removed from LDAP. However if they are still in AD, they could still "login", meaning that removing their whole SudoSOS account could potentially lead to a duplicate being created if they login with LDAP.Therefore I am more inclined to go with the "all syncs should report bad" option. In the end, everything will still be cleaned. It will just take a bit longer with the safer route.
Related issues/external references
#330
Types of changes