rework the user ldap logic to match the account table by butonic · Pull Request #125 · owncloud/user_ldap

butonic · 2017-08-17T21:14:59Z

I am sick of our ldap problems. The app is heavily used, yet undertested and quirky at best. It grew over time but its purpose changes with the introduction of the Account table in OC10.

getUsers() fetches and caches all necessary ldap attributes in a single paginated ldap search
- no subsequent readAttribute that aborts the running paged search ...
- most of the user backend interface calls only work correctly after a getUsers call. That is fine for now, because that is the way the sync works. checkPassword also correctly caches the attributes of the logged in user
- only caches the UserEntries (which wrap ldap results) in the current process. no longer uses the distributed cache for users ... wasted effort on sync and for user search the account table is used.
groups code is unchanged for now.
- could use some refactoring

requires owncloud/core#28729 to make the user sync correctly update quota and email

move some functions from Access to a helper class
review commented tests, some are already covered by UserEntryTest, others are better located in the Manager
add more tests
add Class structure / chain to php doc
test with user shibboleth
test users as group members if getUsers did not yet fetch the entry ... might there be a problem when groups have > 512 Users (the default size of the capped cache?)

yes, I am ripping this apart and putting it together anew. I don't care about the ui. I need the underlying sync logic to work reliably AND the code to be readable. That means not trying to be smart and use less reflection ...

butonic · 2017-08-18T21:01:52Z

fixes #121
obsoletes #119

butonic · 2017-09-01T20:43:26Z

fix and test GUID binary mapping

mrow4a · 2017-09-02T22:43:34Z

To me, it is cleaner version of the code + adjustment to accounts thing. Looks good in general, just difficult to review 👍

tomneedham · 2017-09-08T22:03:04Z

Tested with master:

user sync
extended search terms
multi-value search terms
mail attribute sync
group sync
ui basic setup

mrow4a · 2017-09-11T21:11:11Z

 	 * @return string|false with with the name to use in ownCloud or false if unsuccessful
 	 */
-	private function createAltInternalOwnCloudName($name, $isUser) {
+	public function createAltInternalOwnCloudName($name, $isUser) {


some more context here?

I'll also need context here.

If this needs to be public I wonder if this function should be here. It would make more sense to move this function to a specialized object.
Although we should move this function to another place, I think it's fine if we can make it private for now.

separate PR, see lib\User\Manager.php:292:

// FIXME move to a better place, naming related. eg DistinguishedNameUtils

jvillafanez

Still need to review the big diffs

jvillafanez · 2017-09-19T13:06:40Z

 			$this->isAllowed($input->getOption('force'));
 			$this->confirmUserIsMapped($uid);
-			$exists = $this->backend->userExistsOnLDAP($uid);
+			$exists = $this->backend->userExists($uid);


Reason for this change? I think originally this was intended to bypass the cache and check directly against the LDAP server.

The implementation now always talks to LDAP but caches the result per request.

Actually userExistsOnLdap has been moved to User_LDAP. userExists tries to determine the answer based on the cache in the LDAP UserManager. It will fall back to LDAP just as before.

Ok, so the redis cache has been removed and we cache the information per request... fair enough 👍

jvillafanez · 2017-09-19T13:28:03Z

+			$username = $this->getAttributeValue($attr, null);
+		}
+		if ($username === null) {
+			$json = @json_encode($this->ldapEntry, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE | JSON_NUMERIC_CHECK | JSON_PARTIAL_OUTPUT_ON_ERROR );


I'd rather have a comment here for the @ since error supression isn't common.

jvillafanez · 2017-09-19T13:37:31Z

+	/**
+	 * @param string $ownCloudUID
+	 */
+	public function setOwnCloudUID($ownCloudUID) {


I'm worried this is the only "set" method here. Is it expected to be changed? Could it be set during the object creation?

We need to store an already mapped uid somewhere.

My point is if we can set the value only in the constructor in order to prevent modifications, or we need to modify the value once the object is already constructor.

jvillafanez · 2017-09-19T13:48:01Z

+			}
+		}
+		if ($quota === null) {
+			$quota = 'default';


I think this changed recently to not set the "default" quota in order to avoid being overwritten

fix incoming

jvillafanez · 2017-09-19T13:52:11Z

+	}
+
+	/**
+	 * @return string


I'd comment here what is the expected behavior. I don't expect this method to return the username unless explicitly told.
My expectation is that this method returns null if the displayname can't be fetched, so anyone can fetch the username later if they want. Better to add a comment if this isn't the case

jvillafanez · 2017-09-19T13:55:24Z

+	/**
+	 * returns the home directory of the user if specified by LDAP settings
+	 * @return string|null
+	 * @throws \Exception


Comment here when this exception is expected to be thrown.
In addition, we should narrow the exception.

jvillafanez · 2017-09-19T14:00:00Z

+		if (isset($this->ldapEntry['memberof'])) {
+			return $this->ldapEntry['memberof'];
+		}
+		return [];


I'd rather return something different because empty result is also a valid result, it would be difficult to distinguish this case and an error case (or if there isn't such entry)

jvillafanez · 2017-09-19T14:08:33Z

+	 */
+	private function getAttributeValue($attributeName, $default = null, $trim = true) {
+		$attributeName = strtolower($attributeName); // all ldap keys are lowercase
+		if (isset($this->ldapEntry[$attributeName][0])) {


Should we check for multivalued attributes? We should have a comment just in case.

no. only search attributes may be multi value. updating phpdoc as well

jvillafanez · 2017-09-19T14:09:13Z

+	 * @param $attributeName string
+	 * @param $default string
+	 * @param $trim bool don't trim value, eg for binary data
+	 * @return string|null|mixed


Specify when we should expect each data type.

jvillafanez · 2017-09-19T14:13:16Z

 use OCP\UserInterface;

-class User_Proxy extends Proxy implements IUserBackend, UserInterface, IProvidesExtendedSearchBackend {
+class User_Proxy extends Proxy implements IUserBackend, UserInterface, IProvidesQuotaBackend, IProvidesExtendedSearchBackend, IProvidesEMailBackend {


I'm forced to complain about the massive interface implementation here, but no change needed for now because it will require quite huge changes, and not just here.

The app used to update core itself, which is not the way it shoud be. This PR just corrects that by removing the related code and implementing the interfaces to get the correct behavior.

jvillafanez · 2017-09-20T12:07:17Z

@@ -25,6 +26,9 @@
 namespace OCA\User_LDAP;

 abstract class LDAPUtility {


We should review the usage if this class because right now I think it's quite pointless. Probably we can remove it.

separate PR

jvillafanez · 2017-09-20T12:13:28Z

+	/**
+	 * returns the Connection instance
+	 * @return \OCA\User_LDAP\ILDAPWrapper
+	 */


Is this function used outside of the class? I'm fine to use this function as a shortcut but also private. Making it public would open new dependencies between components which doesn't seem a good idea.

jvillafanez · 2017-09-20T12:18:15Z

-		$hex_guid_to_guid_str .= '-' . substr($hex_guid, 20);
-
-		return strtoupper($hex_guid_to_guid_str);
+	public static function binGUID2str($binGuid) {


Any reason to use static methods here? I'd rather move this function to a helper object and inject it in the constructor. This way we have this function outside if this class (I don't think this is needed to be here), and also make the function testeable.

separate PR

jvillafanez · 2017-09-20T12:18:36Z

 	 * @return string
 	 */
-	public function convertSID2Str($sid) {
+	public static function sid2str($sid) {


Same as above

separate PR

jvillafanez · 2017-09-20T12:24:06Z

+	 * The nsuniqueid values are generated based on the entryuuid value by moving the "-" to comply with the format of the ODSEE Nsuniqueid Virtual Attribute attribute.
+	 *
+	 * ## RedHat FreeIPA is defined as utf string
+	 * {@link https://github.com/freeipa/freeipa/blob/master/install/share/uuid.ldif ipaUniqueID schema}


👏
Really greatful for this references. Just this one should be an official redhat link. No need to do this now anyway.

jvillafanez · 2017-09-20T13:52:25Z

+ *
+ * @package OCA\User_LDAP
+ */
+class User_LDAP implements IUserBackend, UserInterface {


I wonder if this class should implement these interfaces. It should be oriented to implement them so the User_Proxy class is easier to implement but other than that, the interfaces will be implemented by the User_Proxy.

jvillafanez · 2017-09-20T13:56:04Z

-		$user = $this->access->userManager->get($uid);
-		if(!$user instanceof User) {
+		$user = $this->userManager->getByOwnCloudUID($uid);
+		if(!$user instanceof UserEntry) {


According to the PHPDocs, the method returns a UserEntry or null. I'd change this to check against null because otherwise it seems that other type of objects can be returned by that function.

getByOwnCloudUID now always returns a UserEntry

or throws an exception

so we can remove that check (or use a try-catch for the exception instead)

jvillafanez · 2017-09-20T14:02:57Z

+	 * Get a users email address
+	 *
+	 * @param string $uid The username
+	 * @return string|null|false false if user was not found, null if no email is set


copy & paste 👀

jvillafanez · 2017-09-20T14:07:17Z

+	 * FIXME This is an expensive operation and takes roughly half a second to parse the data and create the image. This might be too slow for sync jobs.
+	 *
+	 * @param string $uid The username
+	 * @return IImage|null|false


Need to specify each possible cases

jvillafanez · 2017-09-20T14:10:42Z

+	/**
+	 * Get avatar for a users account for core powered user search
+	 *
+	 * FIXME This is an expensive operation and takes roughly half a second to parse the data and create the image. This might be too slow for sync jobs.


Side question for the future: is it possible to store the raw image data (I think it's base64 encoded) and compare the data we have and the data we receive, and then create the image if the data is different? I'm not sure if it will improve the performance, but it might be worthy to have a try.

jvillafanez · 2017-09-20T14:15:09Z

Review finished from my part. I think most of them are easy to solve, and the rest might be moved to tech debt if they're too complex to manage in this PR.

tomneedham · 2017-09-27T11:08:48Z

bug here with user sync, suggested arribute is not found when it is present: https://github.com/owncloud/enterprise/issues/2275

tomneedham · 2017-10-03T07:23:38Z

Rebased to include some other fixes and the tests should now run

tomneedham · 2017-10-11T12:26:34Z

💥 after running a user sync twice

tomneedham · 2017-10-11T13:04:42Z

"0" quota is recorded in the accounts table as NULL => correct or not?

tomneedham · 2017-10-12T08:47:27Z

-		if($userEntry === null) {
+		try {
+			$this->userManager->getCachedEntry($uid);
+		} catch (\Exception $e) {


tomneedham · 2017-10-12T09:08:21Z

This mornings commit fixes the sync issue I found.

…to getCachedEntry

…ption to trigger the fetching from ldap

…etrieve a results which was not cached and return null.

…w takes array of attributes

tomneedham · 2017-10-25T08:01:16Z

Rebased + pushed a test. Will wait for CI. Then build test tarball which we can share round as a beta release 🎉

tomneedham · 2017-10-25T08:04:31Z

Ideas for performance comparisons for release:

cold login time
user sync + single user sync
call for all users

butonic · 2017-10-25T08:13:20Z

Merging, next up testing with large installations before release

butonic changed the title ~~rework the user ldap logic to match the account table~~ 🚧 rework the user ldap logic to match the account table Aug 17, 2017

butonic self-assigned this Aug 17, 2017

butonic added the 2 - Developing label Aug 17, 2017

butonic added this to the development milestone Aug 17, 2017

butonic force-pushed the fix_user_access branch from 1b62dad to fe52c0d Compare August 17, 2017 21:51

butonic added the p1-urgent label Aug 18, 2017

butonic mentioned this pull request Aug 18, 2017

Fix handle request to correctly call backend when cache is empty #119

Closed

butonic added the blue-ticket label Aug 24, 2017

butonic mentioned this pull request Aug 25, 2017

skip to next user on OutOfBoundsException owncloud/core#28805

Closed

butonic force-pushed the fix_user_access branch from 0d6ff75 to 8d2057e Compare August 28, 2017 14:21

mrow4a reviewed Sep 11, 2017

View reviewed changes

butonic mentioned this pull request Sep 14, 2017

Email should be set for users when log in for the 1st time #128

Closed

butonic changed the title ~~🚧 rework the user ldap logic to match the account table~~ rework the user ldap logic to match the account table Sep 18, 2017

butonic added 3 - To Review and removed 2 - Developing labels Sep 18, 2017

jvillafanez reviewed Sep 19, 2017

View reviewed changes

jvillafanez reviewed Sep 20, 2017

View reviewed changes

tomneedham force-pushed the fix_user_access branch from 76dccbe to 602c16e Compare October 3, 2017 07:23

tomneedham reviewed Oct 12, 2017

View reviewed changes

tomneedham approved these changes Oct 12, 2017

View reviewed changes

butonic and others added 18 commits October 25, 2017 09:59

add to ldap interface, log cookies for debugging

3005d98

fix-wizard-tests

5a64054

add tests for uuid detection ond binary to string conversion

d5eb667

query expertUsernameAttr

692b957

log faulty ldap entry

7821125

minor-cleanup

46abadc

fix-tests

df4024a

Log LDAP calls as events for diagnostics (#126)

3ebbd76

address review

6bab10b

fix tests

fb9ba32

rename for redability

4819a27

fix typo

a61e45c

Fix failing tests for method GetBNyOwnCloudUID - it was refactored in…

ac6e306

…to getCachedEntry

we need a cached entry that can be created when we have a valid login

3451574

since we now throw an exception, our mock also needs to throw an exce…

42054e6

…ption to trigger the fetching from ldap

If we couldn't find a cached result, log a warning that we tried to r…

8649162

…etrieve a results which was not cached and return null.

Fix user exists to check LDAP and cache user response. executeRead no…

09ed4ce

…w takes array of attributes

Lowercase entry keys

c9ed34f

tomneedham force-pushed the fix_user_access branch from 3ddbc28 to c9ed34f Compare October 25, 2017 07:59

butonic merged commit a91ed2b into master Oct 25, 2017

butonic deleted the fix_user_access branch October 25, 2017 08:14

patrickjahns mentioned this pull request Nov 13, 2017

0.10.0 release checklist #140

Closed

20 tasks

butonic mentioned this pull request Nov 29, 2017

Recursion in ldap search owncloud/core#19400

Closed

		@@ -25,6 +26,9 @@
		namespace OCA\User_LDAP;

		abstract class LDAPUtility {

Uh oh!

Conversation

butonic commented Aug 17, 2017 • edited by tomneedham Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

butonic commented Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

butonic commented Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrow4a commented Sep 2, 2017

Uh oh!

tomneedham commented Sep 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvillafanez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

butonic Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

butonic commented Aug 17, 2017 •

edited by tomneedham

Loading

butonic commented Aug 18, 2017 •

edited

Loading

butonic commented Sep 1, 2017 •

edited

Loading

butonic Oct 11, 2017 •

edited

Loading