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

[for 10.4] Receive multiple users from backend for user sync #36576

Merged
merged 1 commit into from Dec 17, 2019

Conversation

sharidas
Copy link
Contributor

@sharidas sharidas commented Dec 11, 2019

Receive multiple users from the backend for user sync
instead of checking for single user. From the received
users check if the user to be synced is available in
the list.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

Problem: When admin tries to sync a user say for ldap backend, there can be cases where names like mhof, mhoff, mhoffg exist in the ldap. And when admin tries to sync mhof, the admin gets Multiple user exists exception.

What this PR proposes:
Grab all the users available from the user backend, which the admin had searched for. And then try to match the users with the user to be searched for. If there are multiple users with same user name available then throw the exception. If only one user is available then sync it, else as usual show message saying user does not exist in the backend.

Related Issue

  • Fixes enterprise ticket 3447

Motivation and Context

Do not check for the count of users. Instead check if the users received from the backend contains the exact match or not.

How Has This Been Tested?

  • Created LDAP setup
  • Created LDAP users mhof, mhoff, mhoffg
  • Now as oC admin tried to sync the user mhof
sujith@sujith-ownCloud  ~/test/owncloud   fix-sync-user ●  ./occ user:sync "OCA\User_LDAP\User_Proxy" -u 'mhof'          
Cannot load Xdebug - it was already loaded
If unknown users are found, what do you want to do with their accounts? (removing the account will also remove its data)
  [0] disable
  [1] remove
  [2] ask later
 > 0
Syncing mhof ...

 sujith@sujith-ownCloud  ~/test/owncloud   fix-sync-user ● 
  • As admin user try to sync mhoffg123 which does not exist:
sujith@sujith-ownCloud  ~/test/owncloud   fix-sync-user ●  ./occ user:sync "OCA\User_LDAP\User_Proxy" -u 'mhoffg123'
Cannot load Xdebug - it was already loaded
If unknown users are found, what do you want to do with their accounts? (removing the account will also remove its data)
  [0] disable
  [1] remove
  [2] ask later
 > 0
Syncing mhoffg123 ...
Disabling accounts:
mhoffg123, ,  (no longer exists in the backend)

 sujith@sujith-ownCloud  ~/test/owncloud   fix-sync-user ● 

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

@sharidas sharidas self-assigned this Dec 11, 2019
@sharidas sharidas added this to the development milestone Dec 11, 2019
@codecov
Copy link

codecov bot commented Dec 11, 2019

Codecov Report

Merging #36576 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #36576      +/-   ##
============================================
+ Coverage     64.66%   64.66%   +<.01%     
- Complexity    19049    19051       +2     
============================================
  Files          1269     1269              
  Lines         74498    74504       +6     
  Branches       1311     1311              
============================================
+ Hits          48171    48177       +6     
  Misses        25941    25941              
  Partials        386      386
Flag Coverage Δ Complexity Δ
#javascript 54.02% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.83% <100%> (ø) 19051 <0> (+2) ⬆️
Impacted Files Coverage Δ Complexity Δ
core/Command/User/SyncBackend.php 79.1% <100%> (+0.64%) 38 <0> (+2) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 591e3b6...a00e9c3. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 11, 2019

Codecov Report

Merging #36576 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #36576      +/-   ##
============================================
+ Coverage     64.66%   64.66%   +<.01%     
- Complexity    19050    19052       +2     
============================================
  Files          1268     1268              
  Lines         74491    74495       +4     
  Branches       1311     1311              
============================================
+ Hits          48171    48175       +4     
  Misses        25934    25934              
  Partials        386      386
Flag Coverage Δ Complexity Δ
#javascript 54.02% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.84% <100%> (ø) 19052 <0> (+2) ⬆️
Impacted Files Coverage Δ Complexity Δ
core/Command/User/SyncBackend.php 78.89% <100%> (+0.43%) 38 <0> (+2) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ad2d29...dd84405. Read the comment docs.

core/Command/User/SyncBackend.php Outdated Show resolved Hide resolved
core/Command/User/SyncBackend.php Outdated Show resolved Hide resolved
core/Command/User/SyncBackend.php Outdated Show resolved Hide resolved
@sharidas sharidas force-pushed the fix-sync-user branch 2 times, most recently from 5cb24c2 to eb81bab Compare December 13, 2019 09:28
Copy link
Member

@jvillafanez jvillafanez left a comment

Choose a reason for hiding this comment

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

Code looks good.

@phil-davis
Copy link
Contributor

I added issue #36597 to make CLI acceptance tests.
We do not seem to have any acceptance tests for the user:sync command - so we won't hold up merging this PR.

Recieve multiple users from the backend for user sync
instead of checking for single user. From the received
users check if the user to be synced is available in
the list.

Signed-off-by: Sujith H <sharidasan@owncloud.com>
@phil-davis phil-davis changed the title Recieve multiple users from backend for user sync Receive multiple users from backend for user sync Dec 17, 2019
@phil-davis
Copy link
Contributor

@sharidas

  1. Is there an issue to link to in the top post?
  2. needs a changelog item

@sharidas
Copy link
Contributor Author

@phil-davis

  • I have updated the enterprise ticket in the pr template
  • Also the changelog was updated changelog/unreleased/36576 in this PR.

@phil-davis
Copy link
Contributor

Looks good - I will merge.

@phil-davis phil-davis changed the title Receive multiple users from backend for user sync [for 10.4] Receive multiple users from backend for user sync Dec 17, 2019
@phil-davis phil-davis merged commit 52539fa into master Dec 17, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix-sync-user branch December 17, 2019 08:08
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

3 participants