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.

Sign up for GitHub

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

Jump to bottom

Implement soft auto provisioning #730

Merged
merged 6 commits into from
Feb 23, 2024
Merged

Implement soft auto provisioning #730

merged 6 commits into from
Feb 23, 2024

Conversation

julien-nc
Copy link
Member

Problem: When auto provisioning is enabled, if a user with the same ID exists in another backend (Db, LDAP...), we create a second user in the user_oidc backend which leads to issues and confusion.

Here is a partial solution to this problem:
New config soft_auto_provision flag (enabled by default).

  • When enabled
    • If the user already exists in another backend, we don't create a new one in our backend. We update the information (mapped attributes) of the existing user.
    • If the user does not exist in another backend, we create it in ours
  • When disabled
    • We refuse login of users that already exist in other backends

This does not migrate existing users to the user_oidc backend but allows them to log in without creating a duplicate user.

Nothing was changed when auto provisioning is disabled.

Extra change: In the user backend, the ldap sync was not triggered when looking for an existing user.

@julien-nc julien-nc added the enhancement New feature or request label Dec 8, 2023
@julien-nc julien-nc force-pushed the enh/660/soft-auto-provisioning branch from 654b0a2 to 679c111 Compare December 8, 2023 12:36
nc-fkl
nc-fkl reviewed Dec 8, 2023
View reviewed changes
lib/Service/ProvisioningService.php Outdated
$this->eventDispatcher->dispatchTyped(new UserChangedEvent($user, 'displayName', $newDisplayName, $oldDisplayName));
}
} else {
$user->setDisplayName($newDisplayName);
Copy link
Contributor

@nc-fkl nc-fkl Dec 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure here if the $newDisplayName in L112 is always set as its only set once when the if condition in L101 was triggered

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch! Thanks

nc-fkl
nc-fkl suggested changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@nc-fkl nc-fkl left a comment

Choose a reason for hiding this comment

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

Please check my comment regarding $newDisplayName in ProvisioningService.php

@julien-nc julien-nc requested a review from nc-fkl December 8, 2023 14:27
nc-fkl
nc-fkl approved these changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@nc-fkl nc-fkl left a comment

Choose a reason for hiding this comment

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

Nice! :)

@github-actions GitHub Actions
Copy link

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

@julien-nc julien-nc force-pushed the enh/660/soft-auto-provisioning branch from 8f28992 to a1a85a4 Compare January 12, 2024 12:11
@julien-nc julien-nc force-pushed the enh/660/soft-auto-provisioning branch from 3b584ed to c48a5f5 Compare January 19, 2024 14:01
@julien-nc
Copy link
Member Author

@juliushaertl Could you check if this PR makes sense? 😁

@julien-nc julien-nc force-pushed the enh/660/soft-auto-provisioning branch from c48a5f5 to cb658a2 Compare January 23, 2024 15:33
juliusknorr
juliusknorr approved these changes Jan 31, 2024
View reviewed changes
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

The approach looks good to me, haven't tested though 👍

@julien-nc
implement soft auto provisioning: allow users from other backends to …
e31ff84 
…login, update their info

Signed-off-by: Julien Veyssier <[email protected]>
@julien-nc
fix isLdapDeletedUser calls with null user
ee1259c 
Signed-off-by: Julien Veyssier <[email protected]>
@julien-nc
fix setting display name
3e80a08 
Signed-off-by: Julien Veyssier <[email protected]>
@julien-nc
do not throttle when there is a user conflict with non-soft auto prov…
01c8098 
…isioning

Signed-off-by: Julien Veyssier <[email protected]>
@julien-nc
cs:fix
d52b05b 
Signed-off-by: Julien Veyssier <[email protected]>
@julien-nc julien-nc force-pushed the enh/660/soft-auto-provisioning branch from cb658a2 to d52b05b Compare February 23, 2024 09:36
@julien-nc julien-nc merged commit 84f46e1 into main Feb 23, 2024
40 checks passed
@julien-nc julien-nc deleted the enh/660/soft-auto-provisioning branch February 23, 2024 10:15
@julien-nc julien-nc mentioned this pull request Feb 28, 2024
Merged
@brtptrs
Copy link

brtptrs commented Feb 28, 2024 •
edited
Loading

Would it be possible to catch this Problem during while this provisioning?
#656
Maybe with a config flag to enable a lowercase - uppercase verification

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliusknorr juliusknorr juliusknorr approved these changes

@nc-fkl nc-fkl nc-fkl approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request feedback-requested
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@julien-nc @brtptrs @juliusknorr @nc-fkl