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

LDAP Mapping Collision of 1 user prevents all new LDAP users from being synced into ownCloud #751

Closed
s3lph opened this issue Aug 17, 2022 · 19 comments

Comments

@s3lph
Copy link

s3lph commented Aug 17, 2022

ownCloud appears to handle users that are renamed and/or moved to a different position in the LDAP tree quite well.

However, when a user (bob.example in the example below) is deleted and then re-created at a different position in the LDAP tree (leading to a "mapping collision" due to having a different LDAP entryUUID in addition to the changed DN), the LDAP sync starts showing some issues:

  • Already existing users (alice.example in the example below) are synced without any issues.
  • The sync fails for all new users (charlie.example in the example below) once even a single recreated user exists, and the new users do not show up in occ user:list.
  • Nevertheless, occ user:sync finishes without reporting any of these errors:
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
Analysing known accounts ...
 27340 [============================]

Disabling accounts:
[...]

Inserting new and updating all known users from OCA\User_LDAP\User_Proxy ...
 25660 [============================]
 
Sync of users finished, encountered 0 errors.
  • However, an error is reported in owncloud.log (see at the bottom)

Steps to reproduce

  1. Create 3 users in LDAP, in this example we're using
  • uid=alice.example,cn=users,ou=deptA,dc=example,dc=org
  • uid=bob.example,cn=users,ou=deptA,dc=example,dc=org
  • uid=charlie.example,cn=users,ou=deptA,dc=example,dc=org
  1. occ user:sync "OCA\User_LDAP\User_Proxy" -u alice.example
  2. occ user:sync "OCA\User_LDAP\User_Proxy" -u bob.example
  3. occ user:list .example
  1. Change Alice's displayName (to test wheter syncing of existing users still works)
  2. Delete the LDAP user uid=bob.example,cn=users,ou=deptA,dc=example,dc=org
  3. Create the LDAP user uid=bob.example,cn=users,ou=deptB,dc=example,dc=org (same name, different OU)
  4. occ user:sync "OCA\User_LDAP\User_Proxy"
  5. occ user:list .example

Expected behaviour

  - alice.example: Alice Changed ([email protected])
  - bob.example: Bob Example ([email protected])
  - charlie.example: Charlie Example ([email protected])
  • The sync updates existing users (alice.example)
  • The sync adds new users (charlie.example)

Actual behaviour

  - alice.example: Alice Changed ([email protected])
  - bob.example: Bob Example ([email protected])
  • The sync updates existing users (alice.example)
  • The sync does not add new users (charlie.example)

Server configuration

Operating system: Debian 10

Web server: Apache2 2.4.38-3+deb10u7

Database: MariaDB (Galera cluster)

PHP version: 7.3.31-1~deb10u1

ownCloud version: ownCloud Enterprise 10.10.0.3 (clustered)

Updated from an older ownCloud or fresh install: first_install_version: 10.4.1.3

Signing status (ownCloud 9.0 and above):

Failed integrity check due to changes files provided by ownCloud, (see ownCloud Support Case #00018427). However, the same issue appears on our PROD environment (10.7.0.4, integrity check passes)

- core
	- INVALID_HASH
		- lib/private/Group/MetaData.php
		- settings/Controller/UsersController.php
		- settings/Panels/Personal/Profile.php
		- settings/js/users/users.js
		- settings/templates/panels/personal/profile.php
		- settings/templates/users/part.grouplist.php
- user_ldap
	- INVALID_HASH
		- appinfo/info.xml
		- js/wizard/wizardTabExpert.js
		- lib/Access.php
		- lib/Configuration.php
		- lib/Group_LDAP.php
		- lib/Group_Proxy.php
		- lib/User/IUserTools.php
		- templates/settings.php
	- EXTRA_FILE
		- appinfo/Migrations/Version20220725070804.php

The content of config/config.php:

{
    "system": {
        "instanceid": "ocdgh07npzvq",
        "passwordsalt": "***REMOVED SENSITIVE VALUE***",
        "secret": "***REMOVED SENSITIVE VALUE***",
        "trusted_domains": [
            "owncloud.example.org",
        ],
        "datadirectory": "\/mnt\/owncloud_filestore",
        "crashdirectory": "\/var\/log\/owncloud\/",
        "overwrite.cli.url": "https:\/\/owncloud.example.org",
        "htaccess.RewriteBase": "\/",
        "dbtype": "mysql",
        "version": "10.10.0.3",
        "dbname": "owncloud",
        "dbhost": "galera.example.org",
        "dbtableprefix": "oc_",
        "mysql.utf8mb4": true,
        "dbuser": "***REMOVED SENSITIVE VALUE***",
        "dbpassword": "***REMOVED SENSITIVE VALUE***",
        "logtimezone": "UTC",
        "apps_paths": [
            {
                "path": "\/var\/www\/owncloud\/apps",
                "url": "\/apps",
                "writable": false
            },
            {
                "path": "\/var\/www\/owncloud\/apps-external\/",
                "url": "\/apps-external",
                "writable": true
            }
        ],
        "installed": true,
        "maintenance": false,
        "ldapIgnoreNamingRules": false,
        "accounts.enable_medial_search": false,
        "user.search_min_length": 3,
        "mail_domain": "***REMOVED SENSITIVE VALUE***",
        "mail_from_address": "***REMOVED SENSITIVE VALUE***",
        "mail_smtpmode": "smtp",
        "mail_smtphost": "***REMOVED SENSITIVE VALUE***",
        "mail_smtpport": "25",
        "mail_smtpsecure": "tls",
        "log_type": "owncloud",
        "logfile": "\/var\/log\/owncloud\/owncloud.log",
        "loglevel": 2,
        "cron_log": true,
        "operation.mode": "clustered-instance",
        "filelocking.enabled": true,
        "memcache.locking": "\\OC\\Memcache\\Redis",
        "memcache.distributed": "\\OC\\Memcache\\Redis",
        "memcache.local": "\\OC\\Memcache\\APCu",
        "redis": {
            "host": "redis.example.org",
            "port": 6379,
            "password": "***REMOVED SENSITIVE VALUE***"
        },
        "http.cookie.samesite": "None",
        "openid-connect": {
            "provider-url": "https:\/\/keycloak.example.org\/auth\/realms\/example\/",
            "client-id": "owncloud.example.org",
            "client-secret": "***REMOVED SENSITIVE VALUE***",
            "mode": "userid",
            "search-attribute": "preferred_username",
            "loginButtonName": "OIDC",
            "autoRedirectOnLoginPage": true,
            "redirect-url": "https:\/\/owncloud.example.org\/index.php\/apps\/openidconnect\/redirect",
            "post_logout_redirect_uri": "https:\/\/owncloud.example.org\/",
            "scopes": [
                "openid",
                "profile"
            ]
        },
        "firewall.debug": 2,
        "firewall.rules": "[]"
    }
}

List of activated apps:

  - activity:
    - Version: 2.7.0
    - Path: /var/www/owncloud/apps/activity
  - admin_audit:
    - Version: 2.1.3
    - Path: /var/www/owncloud/apps/admin_audit
  - comments:
    - Version: 0.3.0
    - Path: /var/www/owncloud/apps/comments
  - configreport:
    - Version: 0.2.1
    - Path: /var/www/owncloud/apps/configreport
  - customgroups:
    - Version: 0.6.2
    - Path: /var/www/owncloud/apps/customgroups
  - dav:
    - Version: 0.7.0
    - Path: /var/www/owncloud/apps/dav
  - federatedfilesharing:
    - Version: 0.5.0
    - Path: /var/www/owncloud/apps/federatedfilesharing
  - federation:
    - Version: 0.1.0
    - Path: /var/www/owncloud/apps/federation
  - files:
    - Version: 1.5.2
    - Path: /var/www/owncloud/apps/files
  - files_external:
    - Version: 0.9.0
    - Path: /var/www/owncloud/apps/files_external
  - files_lifecycle:
    - Version: 1.3.2
    - Path: /var/www/owncloud/apps/files_lifecycle
  - files_mediaviewer:
    - Version: 1.0.5
    - Path: /var/www/owncloud/apps/files_mediaviewer
  - files_sharing:
    - Version: 0.14.0
    - Path: /var/www/owncloud/apps/files_sharing
  - files_trashbin:
    - Version: 0.9.1
    - Path: /var/www/owncloud/apps/files_trashbin
  - files_versions:
    - Version: 1.3.0
    - Path: /var/www/owncloud/apps/files_versions
  - firewall:
    - Version: 2.10.3
    - Path: /var/www/owncloud/apps/firewall
  - firstrunwizard:
    - Version: 1.2.0
    - Path: /var/www/owncloud/apps/firstrunwizard
  - impersonate:
    - Version: 0.5.0
    - Path: /var/www/owncloud/apps-external/impersonate
  - market:
    - Version: 0.6.3
    - Path: /var/www/owncloud/apps/market
  - msteamsbridge:
    - Version: 1.0.0
    - Path: /var/www/owncloud/apps-external/msteamsbridge
  - notifications:
    - Version: 0.5.4
    - Path: /var/www/owncloud/apps/notifications
  - openidconnect:
    - Version: 2.1.1
    - Path: /var/www/owncloud/apps/openidconnect
  - provisioning_api:
    - Version: 0.5.0
    - Path: /var/www/owncloud/apps/provisioning_api
  - ransomware_protection:
    - Version: 1.4.0
    - Path: /var/www/owncloud/apps/ransomware_protection
  - richdocuments:
    - Version: 2.5.0
    - Path: /var/www/owncloud/apps-external/richdocuments
  - systemtags:
    - Version: 0.3.0
    - Path: /var/www/owncloud/apps/systemtags
  - user_ldap:
    - Version: 0.17.0
    - Path: /var/www/owncloud/apps/user_ldap

LDAP configuration (delete this part if not used)

+-------------------------------+----------------------------------------------------------------------+
| Configuration                 | s01                                                                  |
+-------------------------------+----------------------------------------------------------------------+
| hasMemberOfFilterSupport      | 1                                                                    |
| hasPagedResultSupport         |                                                                      |
| homeFolderNamingRule          | attr:uid                                                             |
| lastJpegPhotoLookup           | 0                                                                    |
| ldapAgentName                 | uid=owncloud.ldap,cn=users,dc=example,dc=ch                          |
| ldapAgentPassword             | ***                                                                  |
| ldapAttributesForGroupSearch  |                                                                      |
| ldapAttributesForUserSearch   | uid                                                                  |
| ldapBackupHost                |                                                                      |
| ldapBackupPort                |                                                                      |
| ldapBase                      | dc=example,dc=org                                                    |
| ldapBaseGroups                | dc=example,dc=org                                                    |
| ldapBaseUsers                 | dc=example,dc=org                                                    |
| ldapCacheTTL                  | 6000                                                                 |
| ldapConfigurationActive       | 1                                                                    |
| ldapDynamicGroupMemberURL     |                                                                      |
| ldapEmailAttribute            | mailPrimaryAddress                                                   |
| ldapExperiencedAdmin          | 1                                                                    |
| ldapExpertGroupnameAttr       | cn                                                                   |
| ldapExpertUUIDGroupAttr       | redacted                                                             |
| ldapExpertUUIDUserAttr        | entryuuid                                                            |
| ldapExpertUsernameAttr        | uid                                                                  |
| ldapGroupDisplayName          | cn                                                                   |
| ldapGroupFilter               | (& (univentionRedactedOwnCloudGroup=1)(objectClass=univentionGroup)) |
| ldapGroupFilterGroups         |                                                                      |
| ldapGroupFilterMode           | 0                                                                    |
| ldapGroupFilterObjectclass    |                                                                      |
| ldapGroupMemberAlgo           | groupScan                                                            |
| ldapGroupMemberAssocAttr      | uniqueMember                                                         |
| ldapHost                      | ldaps://ucs.example.org                                              |
| ldapIgnoreNamingRules         |                                                                      |
| ldapLoginFilter               | (& (!(shadowExpire=1)) (univentionRedactededactedOwnCloud=1) (uid=%uid) )   |
| ldapLoginFilterAttributes     |                                                                      |
| ldapLoginFilterEmail          | 0                                                                    |
| ldapLoginFilterMode           | 1                                                                    |
| ldapLoginFilterUsername       | 1                                                                    |
| ldapNestedGroups              | 1                                                                    |
| ldapNetworkTimeout            | 60                                                                   |
| ldapOverrideMainServer        |                                                                      |
| ldapPagingSize                | 500                                                                  |
| ldapPort                      | 636                                                                  |
| ldapQuotaAttribute            | univentionRedactedOwnCloudQuota                                      |
| ldapQuotaDefault              |                                                                      |
| ldapTLS                       |                                                                      |
| ldapUserDisplayName           | displayname                                                          |
| ldapUserDisplayName2          | mailprimaryaddress                                                   |
| ldapUserFilter                | (& (!(shadowExpire=1)) (univentionRedactedOwnCloud=1))               |
| ldapUserFilterGroups          |                                                                      |
| ldapUserFilterMode            | 1                                                                    |
| ldapUserFilterObjectclass     |                                                                      |
| ldapUserName                  | uid                                                                  |
| ldapUuidGroupAttribute        | auto                                                                 |
| ldapUuidUserAttribute         | auto                                                                 |
| turnOffCertCheck              | 0                                                                    |
| useMemberOfToDetectMembership | 1                                                                    |
+-------------------------------+----------------------------------------------------------------------+

Logs

ownCloud log (data/owncloud.log)

{"reqId":"Aa9WhhKKCClU1JuosRBg","level":3,"time":"2022-08-16T14:38:17+00:00","remoteAddr":"","user":"--","app":"OCA\\User_LDAP\\User\\Manager","method":"--","url":"--","message":"Exception: {\"Exception\":\"OutOfBoundsException\",\"Message\":\"Mapping collision for DN uid=bob.example,cn=users,ou=deptB,dc=example,dc=org and UUID b59b74e4-b1bc-103c-91c2-5bdeec75c58c. Couldnt map to: bob.example\",\"Code\":0,\"Trace\":\"#0 \\\/var\\\/www\\\/owncloud\\\/apps\\\/user_ldap\\\/lib\\\/User\\\/Manager.php(238): OCA\\\\User_LDAP\\\\User\\\\Manager->resolveUID(Object(OCA\\\\User_LDAP\\\\User\\\\UserEntry))\\n#1 \\\/var\\\/www\\\/owncloud\\\/apps\\\/user_ldap\\\/lib\\\/User\\\/Manager.php(525): OCA\\\\User_LDAP\\\\User\\\\Manager->getFromEntry(Array)\\n#2 \\\/var\\\/www\\\/owncloud\\\/apps\\\/user_ldap\\\/lib\\\/User_LDAP.php(178): OCA\\\\User_LDAP\\\\User\\\\Manager->getUsers('bob.example', 500, 0)\\n#3 \\\/var\\\/www\\\/owncloud\\\/apps\\\/user_ldap\\\/lib\\\/User_Proxy.php(170): OCA\\\\User_LDAP\\\\User_LDAP->getUsers('bob.example2', 500, 0)\\n#4 \\\/var\\\/www\\\/owncloud\\\/lib\\\/private\\\/User\\\/Sync\\\/BackendUsersIterator.php(54): OCA\\\\User_LDAP\\\\User_Proxy->getUsers('bob.example', 500, 0)\\n#5 \\\/var\\\/www\\\/owncloud\\\/core\\\/Command\\\/User\\\/SyncBackend.php(285): OC\\\\User\\\\Sync\\\\BackendUsersIterator->rewind()\\n#6 \\\/var\\\/www\\\/owncloud\\\/core\\\/Command\\\/User\\\/SyncBackend.php(174): OC\\\\Core\\\\Command\\\\User\\\\SyncBackend->syncSingleUser(Object(Symfony\\\\Component\\\\Console\\\\Input\\\\ArgvInput), Object(Symfony\\\\Component\\\\Console\\\\Output\\\\ConsoleOutput), Object(OC\\\\User\\\\SyncService), Object(OCA\\\\User_LDAP\\\\User_Proxy), 'bob.example', 'disable')\\n#7 \\\/var\\\/www\\\/owncloud\\\/lib\\\/composer\\\/symfony\\\/console\\\/Command\\\/Command.php(255): OC\\\\Core\\\\Command\\\\User\\\\SyncBackend->execute(Object(Symfony\\\\Component\\\\Console\\\\Input\\\\ArgvInput), Object(Symfony\\\\Component\\\\Console\\\\Output\\\\ConsoleOutput))\\n#8 \\\/var\\\/www\\\/owncloud\\\/lib\\\/composer\\\/symfony\\\/console\\\/Application.php(1009): Symfony\\\\Component\\\\Console\\\\Command\\\\Command->run(Object(Symfony\\\\Component\\\\Console\\\\Input\\\\ArgvInput), Object(Symfony\\\\Component\\\\Console\\\\Output\\\\ConsoleOutput))\\n#9 \\\/var\\\/www\\\/owncloud\\\/lib\\\/composer\\\/symfony\\\/console\\\/Application.php(273): Symfony\\\\Component\\\\Console\\\\Application->doRunCommand(Object(OC\\\\Core\\\\Command\\\\User\\\\SyncBackend), Object(Symfony\\\\Component\\\\Console\\\\Input\\\\ArgvInput), Object(Symfony\\\\Component\\\\Console\\\\Output\\\\ConsoleOutput))\\n#10 \\\/var\\\/www\\\/owncloud\\\/lib\\\/composer\\\/symfony\\\/console\\\/Application.php(149): Symfony\\\\Component\\\\Console\\\\Application->doRun(Object(Symfony\\\\Component\\\\Console\\\\Input\\\\ArgvInput), Object(Symfony\\\\Component\\\\Console\\\\Output\\\\ConsoleOutput))\\n#11 \\\/var\\\/www\\\/owncloud\\\/lib\\\/private\\\/Console\\\/Application.php(165): Symfony\\\\Component\\\\Console\\\\Application->run(Object(Symfony\\\\Component\\\\Console\\\\Input\\\\ArgvInput), Object(Symfony\\\\Component\\\\Console\\\\Output\\\\ConsoleOutput))\\n#12 \\\/var\\\/www\\\/owncloud\\\/console.php(116): OC\\\\Console\\\\Application->run()\\n#13 \\\/var\\\/www\\\/owncloud\\\/occ(11): require_once('\\\/var\\\/www\\\/ownclo...')\\n#14 {main}\",\"File\":\"\\\/var\\\/www\\\/owncloud\\\/apps\\\/user_ldap\\\/lib\\\/User\\\/Manager.php\",\"Line\":373}"}
@dsobon
Copy link

dsobon commented Sep 6, 2022

Apparently it can be solved by setting config "reuse_accounts" to "yes"

@s3lph
Copy link
Author

s3lph commented Sep 13, 2022

Hi @dsobon, thanks for the hint. Setting reuse_accounts to yes indeed seems to workaround the mapping collision issue.

I'm saying workaround instead of solution because:

  • I'd still expect an error message on mapping collisions
  • The mapping collision actually still occurs, but seems to be ignored; the oc_ldap_user_mapping still contains the old ldap_dn/directory_uuids. Not entirely sure how this even works, but it's what we're observing.
  • I can't tell from the top of my head whether it's actually possible for us to always reuse accounts, or whether we'd need to evaluate this on a case-by-case basis (is it acutally the same user which was recreated, or is it an entirely new user who'd then have access to another (deleted in LDAP) user's data).

@dsobon
Copy link

dsobon commented Sep 13, 2022

Yes. the collision will then happen on directory_uuid. I only managed to have it triggered once a few days ago... if it happens again, I will file a bug report with support as we have a support contract. I am not sure how the problem is happening for us, as we are also in the transition of migrating all AD users from using username-based SPN to email-based. I do know it happens when a user is deleted, and then recreated with the same username (directory_uuid), but different DN - that should be covered by reuse_accounts.

@dsobon
Copy link

dsobon commented Oct 11, 2022

Here's a fix for when ObjectGUID (directory_uuid) & DN (ldap_dn) changes, but the UPN (owncloud_name) does not change.

--- ./user_ldap/lib/Mapping/AbstractMapping.php.orig      2022-10-11 11:57:02.384148647 +0200
+++ ./user_ldap/lib/Mapping/AbstractMapping.php   2022-10-11 12:04:15.267980049 +0200
@@ -236,6 +236,10 @@
                        // insertIfNotExist returns values as int
                        return (bool)$result;
                } catch (\Exception $e) {
+                       if (\OC::$server->getConfig()->getAppValue('user_ldap', 'reuse_accounts', 'no') === 'yes') {
+                              \OC::$server->getLogger()->warn("unmapping $name due to reuse_accounts; will be fixed on next sync.", ['app' => 'user_ldap']);
+                               $this->unmap($name);
+                       }
                        return false;
                }
        }

Honestly... the mapping table is lacking when the link to the accounts table is lost.

Ideally it should have - separate from owncloud_name - the upn, maybe samaccountname, and email as columns... and for cleaning up purposes, timestamp column when it was last validated.

@dsobon
Copy link

dsobon commented Oct 11, 2022

Or a more reliable patch... also requires one less (3->2) sync step.

--- ./user_ldap/lib/Mapping/AbstractMapping.php.orig	2022-10-11 11:57:02.384148647 +0200
+++ ./user_ldap/lib/Mapping/AbstractMapping.php	2022-10-11 16:23:52.273826088 +0200
@@ -90,6 +90,36 @@
 	}
 
 	/**
+	 * Check if there is duplicate entry by matching $match column.
+	 * @param array $row
+	 * @param array $match
+	 * @return true|false
+	 */
+	protected function checkDupe($row, $match) {
+		$sql = 'SELECT * FROM `'. $this->getTableName() .'` WHERE ';
+		$vals = [];
+		$where = [];
+		foreach ($row as $k => $v) {
+			if (in_array($k,$match)) {
+				array_push($where, '`' . $k . '` = ?');
+			} else {
+				array_push($where, '`' . $k . '` != ?');
+			}
+			array_push($vals, $v);
+		}
+		$sql .= join(" AND ", $where);
+
+		$query = $this->dbc->prepare($sql);
+		$res = $query->execute($vals);
+
+		if ($res !== false) {
+			return true;
+		}
+
+		return false;
+	}
+
+	/**
 	 * Performs a DELETE or UPDATE query to the database.
 	 * @param \Doctrine\DBAL\Driver\Statement $query
 	 * @param array $parameters
@@ -231,11 +261,20 @@
 			'directory_uuid' => $uuid
 		];
 
+		// check if same UPN exist, but different UUID and different DN.
+		if (\OC::$server->getConfig()->getAppValue('user_ldap', 'reuse_accounts', 'no') === 'yes') {
+			if ($this->checkDupe($row, ['owncloud_name'])) {
+				\OC::$server->getLogger()->warning("unmapping $name due to reuse_accounts.", ['app' => 'user_ldap']);
+				$this->unmap($name);
+			}
+		}
+
 		try {
 			$result = $this->dbc->insertIfNotExist($this->getTableName(), $row);
 			// insertIfNotExist returns values as int
 			return (bool)$result;
 		} catch (\Exception $e) {
+			\OC::$server->getLogger()->warning("map($fdn, $name, $uuid): Insert FAILED.", ['app' => 'user_ldap']);
 			return false;
 		}
 	}

@jvillafanez
Copy link
Member

Just some clarifications:

  • If the objectguid (or whatever attribute is used as uuid) is different, it's a different account for ownCloud.
  • Usually, when the dn attribute changes (the ldap object moved), the objectguid also changes. As said in the previous point, this means that, for ownCloud, the account is different.

What happens when you move a user in ldap is that, usually you remove the old user and create a new one, at least from ownCloud's perspective.
For ownCloud, the old user still exists because the old user ownCloud's account is still present. When ownCloud wants to map the new ldap user, it fails because the username is already taken by the old user.

Note that we're just talking about one specific case, which is moving a user in ldap. We also need to consider cases such as a collision with a local user (local user "user001" exists, so ldap user "user001" can't be mapped), or a collision with another user in a different part of the tree.

As far as I know, the "reuse_accounts" option covers the case movements in the ldap directory. However, it could cause problems if multiple ldap users (from different parts of the ldap tree) could be mapped to the same account. It's very likely that the second account, which should cause the collision, would be mapped over the previous account.

@dsobon
Copy link

dsobon commented Oct 13, 2022

Yes, the problem is "from owncloud's perspective", because it does not have the (correct) logic to handle lost/disconnected/stale mappings.

Currently, the code does not handle the collision, so what is the solution for the end-user admin?

@jvillafanez
Copy link
Member

I don't think we can remap a ldap user automatically. If a remap is needed, I think there should be admin intervention somehow.

As said, the entryuuid (or whatever attribute is used as uuid) must remain the same in order to detect a movement in the LDAP. If the entryuuid is different, the user is is different.
You could change the attribute to a better one such as the upn (assuming uniqueness is guaranteed) with the same rules: if the upn is different, the user is different. Note that changing the uuid attribute after syncing the users is NOT recommended because you might need to clear the mappings, and could cause data loss since you might need to remove the ownCloud accounts to create the new ones.

So, for your case, if you moved the user in LDAP and the entryuuid changed, you should check why that happened and how you can prevent that.
Otherwise, we should consider that the old account can be removed because the old user isn't in LDAP any longer. If this is the case, removing the ownCloud account and syncing the new user should fix this issue. Note that removing the account will also remove all the files.

If the entryuuid has changed while moving the user, a remap might be needed. This is currently only possible with the "reuse_accounts" options, but it could be problematic because it will affect all the ownCloud accounts. At the moment, there is no other option, so we might need to include an occ command for the admin to remap a user

@dsobon
Copy link

dsobon commented Oct 13, 2022

yes, we are changing to upn (which on AD has also been migrated from sam@domain to email address) on owncloud, which is a pending task for me to do on owncloud production.

It can legitimately happen for these scenarios (there is a reason - but I do not know the technical justification - why they do delete/re-create rather than update the attributes):

  • user changes surname (after marriage). Thus email, DN, and now UPN is updated. Most of the time, samaccountname is NOT updated.
  • user moves to another office. Email may change, DN will change, UPN may now change (as it is now the email address). samaccountname most likely will be different, but cannot be guaranteed it will not be the same.
  • user leaves company. same person comes back to company 1+ years later to the same office, or different office.

The above scenarios is what may (and will) happen in a large enterprise environment.

So it is difficult for me to prevent that.

Also, other internal web application have not suffered any of these type of issues (every app has a different way of treating what is the primary key from the source, and then treating what is the linking key, and if it's a single key, or if all but one key changes, then the link is lost).

In the end, I do not want to touch the database to manually delete entries as that is way more risky than having optional logic implemented in user_ldap.

@s3lph
Copy link
Author

s3lph commented Oct 13, 2022

I don't think we can remap a ldap user automatically. If a remap is needed, I think there should be admin intervention somehow.

I share @jvillafanez point of view here. The LDAP server in question serves some ~30k user accounts. While in this instance it actually was the same user being recreated at a different location, it could very well be an entirely different user (think of common names like John Smith). This other user would then automatically see all the ownCloud data from the previous user. So we cannot simply set reuse_accounts: true without admin review.

But this is not really the thing that's the issue here. The main issue (for us) is not that mapping collisions can appear, but that a single mapping collision blocks all other new LDAP accounts from being synced.

@NannaBarz
Copy link

@jvillafanez Please elaborate if we have options for the migration

Please summarize the case and then we have to discuss this on the product board. @cdamken

@jvillafanez
Copy link
Member

Mapping collision stops syncing

I think this will need a redesign of the syncing mechanism. I'm not finding a good way to fix this issue otherwise.

  • It isn't clear who is responsible of what. Core should handle the mapping collisions because it's the one managing the accounts and it knows all available users from all available sources. This means that ldap should be able to return duplicated uids because core will handle those, but this leads us to the next point.
    The alternative could be for ldap to ask core if it's possible to map an specific user. I think this is what is being doing at the moment
  • Reusing code for a different purpose. We're using the getUsers method for syncing, but it's also used for other things. There are a couple of important things to notice:
    • The getUsers method shouldn't return duplicated users (it doesn't make sense). This is conflicting to the alternative of returning duplicate users, and could cause problems in other areas where it's assumed the method won't return duplicated users.
    • The method could return less users than asked for. It's fair to assume that if you ask for 50 users but 45 are returned, then there aren't any more users. In case of a mapping collision, ldap will return less users than requested.

Basically, what happens is: mapping collision happens -> ldap returns less users than what have been asked for -> syncing stops thinking there are no more users.

We could change the condition, so if there are less users, we could request the next bunch and stop if there are 0, but this is still fragile because there could be enough collisions for the next bunch to return 0 results and there could still be users to be synced. So changing the condition still have room for error.

In addition, note that core doesn't really know about the mapping collision since it's handled by the ldap app. This means that core will ask for users 0-49, 50-99, etc; if there is a mapping collision in the first bunch, the ldap app will need to track that collision because core will ask for user 50-99, but ldap will need to get users from 54-103. I'd expected that if I ask again out of the blue for users 50-99, ldap will return the same users, but ldap will have to somehow track what users couldn't be mapped in order to skip them.
Moreover, the performance will drop since there are more requests to be done: we could request for the first 50 users, but then will have to make another request to fill the gap caused by the collisions.

Manually remapping ldap users

This doesn't seem a good idea. All the user syncing is done automatically via occ command, and this command is usually setup in a cron job.
Changing the map manually could cause the sync job to overwrite those changes, or to try to reinsert the old user, both cases don't look good.

We have 3 fields we could touch in the DB: the owncloud_user, ldap_dn and directory_uuid.

  • Changing the owncloud_user is impossible because it's attached to the ownCloud's uid. If it changes, it will be linked to a new ownCloud account, and the old account will be lost. It might still be possible to run a occ user:transfer-ownership to move the files to the new account (assuming the old account hasn't been removed yet), but beware of additional data that could be attached to the old ownCloud account.
    It's a requirement that the value of the chosen attribute to be the username mustn't change, otherwise the user is different.
  • Changing the ldap_dn is possible only if the other 2 fields remain the same. This should be the case when the user is moved in the ldap tree. This should happen automatically already.
  • Changing the directory_uuid isn't possible at the moment. This identifies the user in the ldap server, so if it changes in means it's a different user. Even if the other 2 fields are the same, it could be a coincidence: old user left the company, and a new user came in with a matching username in the same position.

Regardless of the outcome, most of these cases should go through the sync job, and as such, should be fixed without intervention, but most of them can't be done automatically without having a security risk.

The only special case would be if the user is moved in ldap and somehow the directory_uuid also changes. Assuming that this is the only collision that happens, the reuse_accounts: true should fix it. This is probably the best compromise. I need to check how we can put the option available in an occ command so we don't need to change the configuration and potentially affect more things.

Anyway, I think the priority should be the syncing mechanism, and we probably need to redesign it.

@NannaBarz
Copy link

@hodyroff @jnweiger @pako81 please have a look here and decide what we should do

@IljaN
Copy link
Member

IljaN commented Jan 2, 2023

Discuss in product board

@jnweiger
Copy link
Contributor

jnweiger commented Jan 9, 2023

In my interpretation,

  • they came up with an example scenario of a user collision, to demonstrate, that syncing stops at the first error.
  • we look into that root cause. But not into the symptoms (what they actually asked)
  • we should also consider that there may be other (yet unknown) ways how a user can fail to sync.
    • Is it possible to continue the sync loop instead of aborting on first error?
    • Can we report sync errors with occ user:sync?

@hodyroff
Copy link

We will continue to look at each customer. It is part of our Enterprise support offering. Errors go into the logfile.

@jvillafanez
Copy link
Member

We should check whether #757 could be helpful for the customers or not. The feature is technically ready, but it's currently in draft because it isn't clear if we want it in the app or not.

@santanair
Copy link

@cdamken

@IljaN
Copy link
Member

IljaN commented May 15, 2023

Customer applied custom patch. Is happy now.

@IljaN IljaN closed this as completed May 15, 2023
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

No branches or pull requests

8 participants