nextcloud / server

☁️ Nextcloud server, a safe home for all your data
https://nextcloud.com
GNU Affero General Public License v3.0
27.57k stars 4.08k forks source link

countSeenUsers() function returns wrong amount of LDAP active users #38681

Open Anth0rx opened 1 year ago

Anth0rx commented 1 year ago

⚠️ This issue respects the following points: ⚠️

Bug description

The function countSeenUsers() seems to return the amount of users that have ever been registered, not only those active and registered currently:

public function countSeenUsers() {
    $queryBuilder = \OC::$server->getDatabaseConnection()->getQueryBuilder();
    $queryBuilder->select($queryBuilder->func()->count('*'))
        ->from('preferences')
        ->where($queryBuilder->expr()->eq('appid', $queryBuilder->createNamedParameter('login')))
        ->andWhere($queryBuilder->expr()->eq('configkey', $queryBuilder->createNamedParameter('lastLogin')));

    $query = $queryBuilder->execute();

    $result = (int)$query->fetchOne();
    $query->closeCursor();

    return $result;
}

The following SQL statement to the Nextcloud database replicates the return value of this function:

SELECT COUNT(*)
FROM oc_preferences
WHERE appid = 'login' AND configkey = 'lastLogin';

The following SQL statement also outputs the displayName alongside the userid to better see which users are considered by countSeenUsers():

SELECT userid, configvalue
FROM oc_preferences
WHERE userid IN
    (SELECT userid
     FROM oc_preferences
     WHERE appid = 'login' AND configkey = 'lastLogin')
AND configkey = 'displayName';

The output of the SQL statement above includes already deleted LDAP accounts. So maybe this is a problem with the LDAP implementation of Nextcloud. Deleted users should not appear in the database.

Steps to reproduce

  1. Execute the following SQL statement to replicate the users considered by countSeenUsers():
    SELECT userid, configvalue
    FROM oc_preferences
    WHERE userid IN
    (SELECT userid
     FROM oc_preferences
     WHERE appid = 'login' AND configkey = 'lastLogin')
    AND configkey = 'email';
  2. Compare the output above with the output of php occ user:list

Expected behavior

The expected behaviour would be that the function countSeenUsers() returns the same amount of users as the OCC command php occ user:list. Deleted (LDAP) users should not appear in the database.

Installation method

Official All-in-One appliance

Nextcloud Server version

25

Operating system

Other

PHP engine version

PHP 8.0

Web server

Apache (supported)

Database engine version

PostgreSQL

Is this bug present after an update or on a fresh install?

Fresh Nextcloud Server install

Are you using the Nextcloud Server Encryption module?

Encryption is Disabled

What user-backends are you using?

Configuration report

{
    "system": {
        "profile.enabled": false,
        "enable_avatars": false,
        "memcache.distributed": "\\OC\\Memcache\\Redis"
        "memcache.locking": "\\OC\\Memcache\\Redis",
        "memcache.local": "\\OC\\Memcache\\APCu",
        "apps_paths": [
            {
                "path": "\/var\/www\/html\/apps",
                "url": "\/apps",
                "writable": false
            },
            {
                "path": "\/var\/www\/html\/custom_apps",
                "url": "\/custom_apps",
                "writable": true
            }
        ],
        "redis": {
            "host": "***REMOVED SENSITIVE VALUE***",
            "password": "***REMOVED SENSITIVE VALUE***",
            "port": 6379
        },
        "overwritehost": "***REMOVED SENSITIVE VALUE***",
        "overwriteprotocol": "https",
        "passwordsalt": "***REMOVED SENSITIVE VALUE***",
        "secret": "***REMOVED SENSITIVE VALUE***",
        "trusted_domains": [
            "localhost",
            "***REMOVED SENSITIVE VALUE***"
        ],
        "datadirectory": "***REMOVED SENSITIVE VALUE***",
        "dbtype": "pgsql",
        "version": "25.0.7.1",
        "overwrite.cli.url": "https:\/\/***REMOVED SENSITIVE VALUE***\/",
        "dbname": "***REMOVED SENSITIVE VALUE***",
        "dbhost": "***REMOVED SENSITIVE VALUE***",
        "dbport": "",
        "dbtableprefix": "oc_",
        "dbuser": "***REMOVED SENSITIVE VALUE***",
        "dbpassword": "***REMOVED SENSITIVE VALUE***",
        "installed": true,
        "instanceid": "***REMOVED SENSITIVE VALUE***",
        "loglevel": "2",
        "log_type": "file",
        "logfile": "\/var\/www\/html\/data\/nextcloud.log",
        "log_rotate_size": "10485760",
        "log.condition": {
            "apps": [
                "admin_audit"
            ]
        },
        "preview_max_x": "2048",
        "preview_max_y": "2048",
        "jpeg_quality": "60",
        "enabledPreviewProviders": {
            "1": "OC\\Preview\\Image",
            "2": "OC\\Preview\\MarkDown",
            "3": "OC\\Preview\\MP3",
            "4": "OC\\Preview\\TXT",
            "5": "OC\\Preview\\OpenDocument",
            "6": "OC\\Preview\\Movie"
        },
        "enable_previews": true,
        "upgrade.disable-web": true,
        "mail_smtpmode": "smtp",
        "trashbin_retention_obligation": "auto, 30",
        "versions_retention_obligation": "auto, 30",
        "activity_expire_days": "30",
        "simpleSignUpLink.shown": false,
        "share_folder": "\/Freigaben\/",
        "tempdirectory": "\/mnt\/ncdata\/tmp\/",
        "one-click-instance": true,
        "one-click-instance.user-limit": 100,
        "one-click-instance.link": "https:\/\/nextcloud.com\/all-in-one\/",
        "htaccess.RewriteBase": "\/",
        "files_external_allow_create_new_local": true,
        "trusted_proxies": "***REMOVED SENSITIVE VALUE***",
        "allow_local_remote_servers": true,
        "mail_from_address": "***REMOVED SENSITIVE VALUE***",
        "mail_sendmailmode": "smtp",
        "mail_domain": "***REMOVED SENSITIVE VALUE***",
        "mail_smtpauthtype": "PLAIN",
        "mail_smtpsecure": "ssl",
        "mail_smtphost": "***REMOVED SENSITIVE VALUE***",
        "mail_smtpauth": 1,
        "mail_smtpport": "465",
        "mail_smtpname": "***REMOVED SENSITIVE VALUE***",
        "mail_smtppassword": "***REMOVED SENSITIVE VALUE***",
        "ldapProviderFactory": "OCA\\User_LDAP\\LDAPProviderFactory",
        "maintenance": false,
        "default_language": "de",
        "default_locale": "de",
        "updatedirectory": "\/nc-updater",
    }
}

List of activated Apps

Enabled:
  - activity: 2.17.0
  - admin_audit: 1.15.0
  - appointments: 1.15.1
  - calendar: 4.3.4
  - circles: 25.0.0
  - cloud_federation_api: 1.8.0
  - comments: 1.15.0
  - contacts: 5.2.0
  - contactsinteraction: 1.6.0
  - dashboard: 7.5.0
  - dav: 1.24.0
  - deck: 1.8.5
  - federatedfilesharing: 1.15.0
  - files: 1.20.1
  - files_external: 1.17.0
  - files_fulltextsearch: 25.0.0
  - files_pdfviewer: 2.6.0
  - files_rightclick: 1.4.0
  - files_sharing: 1.17.0
  - files_trashbin: 1.15.0
  - files_versions: 1.18.0
  - fulltextsearch: 25.0.0
  - fulltextsearch_elasticsearch: 25.0.0
  - groupfolders: 13.1.3
  - logreader: 2.10.0
  - lookup_server_connector: 1.13.0
  - nextcloud-aio: 0.3.0
  - nextcloud_announcements: 1.14.0
  - notifications: 2.13.1
  - notify_push: 0.6.3
  - oauth2: 1.13.0
  - password_policy: 1.15.0
  - privacy: 1.9.0
  - provisioning_api: 1.15.0
  - recommendations: 1.4.0
  - related_resources: 1.0.4
  - richdocuments: 7.1.4
  - serverinfo: 1.15.0
  - settings: 1.7.0
  - sharebymail: 1.15.0
  - spreed: 15.0.6
  - support: 1.8.0
  - survey_client: 1.13.0
  - systemtags: 1.15.0
  - tasks: 0.15.0
  - text: 3.6.0
  - theming: 2.0.1
  - twofactor_backupcodes: 1.14.0
  - twofactor_totp: 7.0.0
  - user_ldap: 1.15.0
  - viewer: 1.9.0
  - workflowengine: 2.7.0
Disabled:
  - bruteforcesettings
  - encryption
  - federation: 1.15.0
  - firstrunwizard: 2.14.0
  - photos: 2.0.1
  - suspicious_login
  - user_status: 1.5.0
  - weather_status: 1.5.0

Nextcloud Signing status

No response

Nextcloud Logs

No response

Additional info

No response

joshtrichards commented 1 year ago

It looks to me like countSeenUsers() is working as intended:

    /**
     * returns how many users have logged in once
     *

But there is a misnomer and it's in occ user:report. It reports active users which is just the result of countSeenUsers() (I'm assuming that's where you got the "active" terminology.)

I interpret "active users" as being recently active or, at the very list, not disabled. But that's not what occ user:report populates that field with. So I get where you're coming from. :-)

So that field should probably be re-labeled to match the function populating it: seen users or used accounts or something. And a new field added to the report that is called enabled users which is "total users minus disabled users".

So the following current behavior:

www-data@4fbd80fb2bc6:~/html$ ./occ user:list
  - curluser: cURL
  - disableduser: disableduser
  - frank: frank
  - ncadmin: ncadmin
  - ncuser: ncuser
www-data@4fbd80fb2bc6:~/html$ ./occ user:report
+------------------+---+
| User Report      |   |
+------------------+---+
| Database         | 5 |
|                  |   |
| total users      | 5 |
|                  |   |
| user directories | 1 |
| active users     | 4 |
| disabled users   | 1 |
+------------------+---+

becomes:

www-data@4fbd80fb2bc6:~/html$ ./occ user:report
+------------------+---+
| User Report      |   |
+------------------+---+
| Database         | 5 |
|                  |   |
| total users      | 5 |
|                  |   |
| user directories | 1 |
| seen users       | 4 |
| enabled users    | 4 |
| disabled users   | 1 |
+------------------+---+

I believe that occ user:list as currently implemented will always output the same number as total users since it doesn't appear to have any filters on it (i.e. it includes disabled users):

www-data@4fbd80fb2bc6:~/html$ ./occ user:list -i
  - curluser:
    - user_id: curluser
    - display_name: cURL
    - email:
    - cloud_id: curluser@http://nc-test.local:3680
    - enabled: true
    - groups:
      - 0: curl-only
    - quota: none
    - last_seen: 2023-05-23T11:34:11+00:00
    - user_directory: /var/www/html/data/curluser
    - backend: Database
  - disableduser:
    - user_id: disableduser
    - display_name: disableduser
    - email:
    - cloud_id: disableduser@http://nc-test.local:3680
    - enabled: false
    - groups:
    - quota: none
    - last_seen: 2023-06-04T16:51:57+00:00
    - user_directory: /var/www/html/data/disableduser
    - backend: Database
  - frank:
    - user_id: frank
    - display_name: frank
    - email:
    - cloud_id: frank@http://nc-test.local:3680
    - enabled: true
    - groups:
      - 0: newgroup
    - quota: none
    - last_seen: 1970-01-01T00:00:00+00:00
    - user_directory: /var/www/html/data/frank
    - backend: Database
  - ncadmin:
    - user_id: ncadmin
    - display_name: ncadmin
    - email:
    - cloud_id: ncadmin@http://nc-test.local:3680
    - enabled: true
    - groups:
      - 0: admin
    - quota: none
    - last_seen: 2023-06-07T12:16:27+00:00
    - user_directory: /var/www/html/data/ncadmin
    - backend: Database
  - ncuser:
    - user_id: ncuser
    - display_name: ncuser
    - email:
    - cloud_id: ncuser@http://nc-test.local:3680
    - enabled: true
    - groups:
    - quota: 15.6 MB
    - last_seen: 2023-05-29T17:13:33+00:00
    - user_directory: /var/www/html/data/ncuser
    - backend: Database

There might also be room for some enhancement work to add an option to specify whether to include "all users (the default)" or "enabled users only". I suspect PRs are welcome. :-)

Anth0rx commented 1 year ago

Thanks for the extensive reply. The problem I have is that php occ user:report returns more active users than total users, so:

$ php occ user:list
  - admin: admin
  - 9ceed22e-7d64-4949-b764-dad97ae3ef4e: LDAP_user_1
  - 13af96c8-8e47-43ec-a20f-800443a88cd1: LDAP_user_2
  - 50fc5dec-2bd6-4078-b209-83a67093d511: LDAP_user_3
  - dba687b3-0d69-40c8-9839-bcab6c4a219e: LDAP_user_4
  - a3a736b5-b3f8-4921-8d1f-745be208f341: LDAP_user_5
  - b5a97da9-3802-4915-b325-b5eaa5148537: LDAP_user_6
  - 35038b98-6494-4230-ac5c-98dcf7d00f68: LDAP_user_7

becomes:

+------------------+----+
| User Report      |    |
+------------------+----+
| Database         | 1  |
| LDAP             | 7  |
|                  |    |
| total users      | 8  | <--- correct
|                  |    |
| user directories | 2  |
| active users     | 13 | <--- wrong, includes deleted LDAP test users
| disabled users   | 0  |
+------------------+----+

That is probably an LDAP problem, since the "missing" five users were test users in LDAP, which have been removed in LDAP but are regarded as active users in Nextcloud.

solracsf commented 1 year ago

@Anth0rx can you perform

https://docs.nextcloud.com/server/latest/admin_manual/configuration_user/user_auth_ldap_cleanup.html

occ ldap:show-remnants
occ user:delete [deleted_ldap_user]

and try again?

Anth0rx commented 1 year ago

@solracsf Thank you, that worked like a charm.

Shouldn't that be done automatically? Looking at my user_ldap config via occ config:list user_ldap I cannot see that ldapUserCleanupInterval is set. The documentation states the default is 51 minutes.

solracsf commented 1 year ago

I don't think so; these commands exists because even if the user is deleted from LDAP, data can be held at Nextcloud. So, instead of auto-delete (potentially) important data, user is "hidden" and should be manually managed by an admin, hence these commands. Because you could also move the data, transfer ownsership, etc.

If we're OK here, feel free to close.

solracsf commented 1 year ago

Looking at my user_ldap config via occ config:list user_ldap I cannot see that ldapUserCleanupInterval is set. The documentation states the default is 51 minutes.

It's here https://github.com/nextcloud/server/blob/a719b433a125b2f0e6948180fa0ca35ff7deb0e0/apps/user_ldap/lib/Jobs/CleanUp.php#L48

Anth0rx commented 1 year ago

these commands exists because even if the user is deleted from LDAP, data can be held at Nextcloud.

Okay, that makes sense.

It's here https://github.com/nextcloud/server/blob/a719b433a125b2f0e6948180fa0ca35ff7deb0e0/apps/user_ldap/lib/Jobs/CleanUp.php#L48

But the LDAP cleanup functionality is only activated if I set it in config.php? Or is the cleanup functionality only responsible for marking the respective users and the deletion is up for the admin?

blizzz commented 1 year ago

But the LDAP cleanup functionality is only activated if I set it in config.php? Or is the cleanup functionality only responsible for marking the respective users and the deletion is up for the admin?

The latter, yes. We're looking for LDAP users that are not present on LDAP anymore and flag them. The decision whether, when and how to act is on the admin.

There are reason why we don't swiftly delete a user when it does not appear anymore, and that could be a configuration change mistake, or moving or modifying the user record false-positively so that it does not meet the critera anymore, or you actually want to keep the user for some time, or forever. There might be shares originating from the removed user that still should be available. Deleting users on Nextcloud will then also delete all their data.

Apart of that, there's lot of information and json format offered to the admin, so that if they want, they still can automatize with a custom background job and take into consideration the time since the user was detected as missing, or their state as share source.

nextcloud-command commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity and seems to be missing some essential information. It will be closed if no further activity occurs. Thank you for your contributions.

Anth0rx commented 1 year ago

The latter, yes. We're looking for LDAP users that are not present on LDAP anymore and flag them. The decision whether, when and how to act is on the admin.

Why are remnants counted as active users, then? Also with regard to the fair use policy?

There are reason why we don't swiftly delete a user when it does not appear anymore, and that could be a configuration change mistake, or moving or modifying the user record false-positively so that it does not meet the critera anymore, or you actually want to keep the user for some time, or forever. There might be shares originating from the removed user that still should be available. Deleting users on Nextcloud will then also delete all their data.

That makes sense. Thanks for elaborating.

blizzz commented 1 year ago

Why are remnants counted as active users, then? Also with regard to the fair use policy?

It also counts disabled users. It actually does very much what the method name says: counting seen users, and across backends. The counting also happens at a higher layer and is not exposed to backend implementations. When a user is not needed anymore, delete them? Otherwise it is still a valid user object, e.g. also with outgoing shares.

MPStudyly commented 1 year ago

But the LDAP cleanup functionality is only activated if I set it in config.php? Or is the cleanup functionality only responsible for marking the respective users and the deletion is up for the admin?

The latter, yes. We're looking for LDAP users that are not present on LDAP anymore and flag them. The decision whether, when and how to act is on the admin.

There are reason why we don't swiftly delete a user when it does not appear anymore, and that could be a configuration change mistake, or moving or modifying the user record false-positively so that it does not meet the critera anymore, or you actually want to keep the user for some time, or forever. There might be shares originating from the removed user that still should be available. Deleting users on Nextcloud will then also delete all their data.

Apart of that, there's lot of information and json format offered to the admin, so that if they want, they still can automatize with a custom background job and take into consideration the time since the user was detected as missing, or their state as share source.

While the assumption a user might be of importance, even after deletion from a backend, in itself is fine, the flagging should happen in a way, the Nextcloud UI is able to deal with. In a scenario with limited access, e.g. you have a managed Nextcloud instance with no direct access to the system/occ commands, you are unable to ultimately delete such remnant users, resulting in probably unwanted behavior, like these users continuing to receive notifications (https://github.com/nextcloud/server/issues/40832). This would be no big deal, if these users were properly disabled, just as internal database backed users are, which do not experience such weird issues. Also, disabling them "the classic way" should make them visible as disabled in the users overview page and also allow to delete them from there.