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

Command occ 'user:report -> count user dirs system wide #39254

Merged
merged 2 commits into from
Sep 23, 2021

Conversation

AlexAndBear
Copy link

@AlexAndBear AlexAndBear commented Sep 20, 2021

Description

Bugfix: Command occ 'user:report' might not count 'user directories' correctly

Before this PR the underlying function of 'user:report' just looked up in the
'datadirectory' set in 'config.php'.
This implies that user directories which are symlinks or even not in the '
datadirectory', have not been taken into account.
With this PR we check if the user's home path exist and increase the
'user directories' count.

Related Issue

Motivation and Context

How Has This Been Tested?

  • Create user admin and user2
  • Run occ user:report
  • | user directories | 2 | -> correct

  • Run php occ user:move-home user2 /home/test and remove the old home
  • Run occ user:report
  • | user directories | 1 | -> not correct

  • Apply this patch
  • Run occ user:report
  • | user directories | 2 | -> correct

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

@AlexAndBear
Copy link
Author

@pmaier1 FYI

@AlexAndBear AlexAndBear marked this pull request as ready for review September 20, 2021 07:34
@owncloud owncloud deleted a comment from update-docs bot Sep 20, 2021
@AlexAndBear AlexAndBear changed the title Count user dirs system wide Command occ 'user:report -> count user dirs system wide Sep 20, 2021
@AlexAndBear
Copy link
Author

AlexAndBear commented Sep 20, 2021

Note: This also fixed the issue that folders in the datadirectory and are non-user folders will be taken into account,
previously this has been overcome by adust User\Constants::DIRECTORIES_THAT_ARE_NOT_USERS

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiProxySmoke-8-8-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/32497/172/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiProxySmoke-8-3-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/32497/167/1

@jvillafanez
Copy link
Member

Some things to test:

  • After syncing all LDAP users (assume 500-1000 users) and log in with some of them (2-4)
  • Home folder outside data directory (the user_ldap app has that option)
  • Object storage as primary storage

The first 2 should be working with the patch I think (needs confirmation), but I'm not sure about the last one.

@AlexAndBear
Copy link
Author

AlexAndBear commented Sep 20, 2021

We could also improve the performance by calling countForSeenUsers instead of using countForAllUsers,
what do you think @jvillafanez

tests/Core/Command/User/ReportTest.php Outdated Show resolved Hide resolved
changelog/unreleased/39254 Outdated Show resolved Hide resolved
changelog/unreleased/39254 Outdated Show resolved Hide resolved
@AlexAndBear
Copy link
Author

AlexAndBear commented Sep 20, 2021

@jvillafanez

Home folder outside data directory (the user_ldap app has that option)

Where do I find this setting?

@jvillafanez
Copy link
Member

If I remember correctly, in the advanced tab of the wizard there is the "user home folder naming rule". You can set an LDAP attribute there, such as "attr:userhome" (the "attr:" prefix is mandatory). The value for the attribute could contain something like "/opt/user01" for the user01. Each user is expected to have a different home folder.

I don't know if there is any specific attribute you can use for this, so maybe you need to use any unused one.

You could search for "homeFolderNamingRule" in the user_ldap app to check for additional things.

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline webUIComments-chrome-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/32507/124/1

@AlexAndBear
Copy link
Author

@jvillafanez
I checked case 1, which works fine.
I tried to import users with the attr:userhome but my import tool breaks, do you dare to check if case 2 is working?

@owncloud owncloud deleted a comment from ownclouders Sep 20, 2021
@owncloud owncloud deleted a comment from ownclouders Sep 20, 2021
@jvillafanez
Copy link
Member

I tried to import users with the attr:userhome but my import tool breaks, do you dare to check if case 2 is working?

Looks good to me.

Tried using "attr:title" as home attribute, with the following values:

  • aaliyah_aañote => /opt/aañote
  • aaliyah_bähr => /opt/bähr
  • aaliyah_abernathy => /opt/aa/abernathy
  • aaliyah_adams => /opt/aa/adams
www-data@73f0601c68e3: ~/owncloud # ./occ user:report
+--------------------------+---+
| User Report              |   |
+--------------------------+---+
| OCA\User_LDAP\User_Proxy | 4 |
| OC\User\Database         | 1 |
|                          |   |
| guest users              | 0 |
|                          |   |
| total users              | 5 |
|                          |   |
| user directories         | 5 |
+--------------------------+---+

www-data@73f0601c68e3: ~/owncloud # ls data/
admin  avatars  files_external  index.html  owncloud.log

www-data@73f0601c68e3: ~/owncloud # ls /opt
 aa  'aa'$'\303\261''ote'  'b'$'\303\244''hr'

root@73f0601c68e3: /var/www/owncloud # ls /opt/aa
abernathy  adams

Ignore the weird folder name inside "/opt", the container has a posix locale by default. Using en_US.UTF-8 as locale fixes that. Nothing to do there.

It seems we're requiring that all users have the configured home attribute set (if active in the wizard), so I haven't been able to sync all the users. Anyway, I don't expect a wrong count

@AlexAndBear
Copy link
Author

Awesome thx, we are checking s3 primary storage

@owncloud owncloud deleted a comment from ownclouders Sep 20, 2021
@owncloud owncloud deleted a comment from ownclouders Sep 20, 2021
@owncloud owncloud deleted a comment from ownclouders Sep 20, 2021
@owncloud owncloud deleted a comment from ownclouders Sep 20, 2021
@owncloud owncloud deleted a comment from ownclouders Sep 20, 2021
@owncloud owncloud deleted a comment from ownclouders Sep 20, 2021
@AlexAndBear
Copy link
Author

@jvillafanez User reoprt directory count, does not work properly with s3 primary storage, tough it also does not work with master.
I suggest documenting this as a known limitation and leave it as it is, so far.

@jvillafanez
Copy link
Member

Using a view object should work with S3 as primary storage, but it might break with LDAP home folder.
I'm not sure, but if the S3 home uses relative paths, we could use a view object if the path is relative and check the underlying FS if it's an absolute path. Hopefully we don't need something more smart than that.

@AlexAndBear
Copy link
Author

@jvillafanez We try it with s3, creating a new View and calling view->is_dir($userHomePath) is returning false, any idea?

@JammingBen
Copy link
Contributor

I don't think we can work with the View (or any OC filesystem methods) here as the implementation circumvents all this stuff. It "just" speaks directly via the S3 API as far as I can tell.

@AlexAndBear
Copy link
Author

Note: As said in comment #39254 (comment), before this PR, user directory count was also 0 (not correct)

@sonarcloud
Copy link

sonarcloud bot commented Sep 21, 2021

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 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

LGTM - code is simple and logical.
We have some acceptance tests, which still pass - good. They are "simple" because they only have users whose home dir is the the default data. But IMO that is OK.

@AlexAndBear AlexAndBear merged commit ebe89d8 into master Sep 23, 2021
@delete-merged-branch delete-merged-branch bot deleted the enterprise/issues/4742 branch September 23, 2021 11:21
@AlexAndBear
Copy link
Author

Merged and opened issue ##39278

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants