LDAP variables can be integers #35701

alpapan · 2022-12-09T07:08:42Z

Signed-off-by: Alexie Papanicolaou alpapan@gmail.com

Resolves: [Bug]: LDAP usernames full-numeric (1234) gives Exception : Trying to access array|string on value of type int #35642

Some LDAP variables (such as cn / uids, group names) can be only integers. The is_string(int) returns false.
This commit adds an || is_numeric or !is_array() check for these use cases.
added // comments above most changes. I was not sure what UUID was and whether it could ever be just a number.

TODO

Please review.

Backports should be needed

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

Signed-off-by: Alexie Papanicolaou <alpapan@gmail.com>

alpapan · 2022-12-14T03:32:53Z

apps/user_ldap/lib/Mapping/AbstractMapping.php

@@ -192,7 +192,8 @@
 	 */
 	protected function getDNHash(string $fdn): string {
 		$hash = hash('sha256', $fdn, false);
-		if (is_string($hash)) {
+		// very rare but a hash could just be numbers? is_string(int) would return false
+		if (is_string($hash) || is_numeric($hash)) {


in theory a sha256 can return just numbers. PHP will parse that as numeric and not a string AFAIK.

alpapan · 2022-12-14T03:31:54Z

apps/user_ldap/lib/Configuration.php

@@ -332,7 +332,7 @@
 		} else {
 			$finalValue = [];
 			foreach ($value as $key => $val) {
-				if (is_string($val)) {
+				if (is_string($val) || is_numeric($int)) {


oops, that's my mistake!

apps/user_ldap/lib/Group_LDAP.php

@@ -703,7 +703,8 @@
 					if ($userMatch !== false) {
 						// match found so this user is in this group
 						$groupName = $this->access->dn2groupname($dynamicGroup['dn'][0]);
-						if (is_string($groupName)) {
+						// in case it is just an integer (is_string(int) returns false
+						if (is_string($groupName) || is_numeric($groupName)) {


apps/user_ldap/lib/Group_LDAP.php

@@ -1191,7 +1194,8 @@
 			if ($dn = $this->groupPluginManager->createGroup($gid)) {
 				//updates group mapping
 				$uuid = $this->access->getUUID($dn, false);
-				if (is_string($uuid)) {
+                                // in case it is just an integer, not sure if this UUID could ever be an int though
+				if (is_string($uuid) || is_numeric($uuid)) {


alpapan · 2022-12-14T03:31:37Z

apps/user_ldap/lib/User_LDAP.php

@@ -304,7 +304,8 @@
 	 * @throws \OC\ServerNotAvailableException
 	 */
 	public function userExistsOnLDAP($user, bool $ignoreCache = false): bool {
-		if (is_string($user)) {
+                // in case it is just an integer (is_string(int) returns false
+		if (is_string($user) || is_numeric($user)) {


Why can't the username be just numbers?

alpapan · 2022-12-14T03:33:53Z

apps/user_ldap/lib/User_LDAP.php

@@ -645,7 +646,8 @@
 				if (is_string($dn)) {
 					// the NC user creation work flow requires a know user id up front
 					$uuid = $this->access->getUUID($dn, true);
-					if (is_string($uuid)) {
+					// not sure if UUID could ever be just a number
+					if (is_string($uuid) || is_numeric($uuid)) {


IDK what UUID is but if it can ever be a string of integers, PHP parses that as an integer not a string (no typecasting)

alpapan · 2022-12-14T03:31:21Z

apps/user_ldap/lib/Group_LDAP.php

@@ -730,7 +731,8 @@
 			$groupDNs = $this->_getGroupDNsFromMemberOf($userDN);
 			foreach ($groupDNs as $dn) {
 				$groupName = $this->access->dn2groupname($dn);
-				if (is_string($groupName)) {
+                                // in case it is just an integer (is_string(int) returns false
+				if (is_string($groupName) || is_numeric($groupName)) {


why can't the name of a group be just numbers?

come-nc

I agree with psalm that all these is_numeric checks are redundant. I will look into the bug and backtrace to try to understand what happens, ldap_* methods should always return values as strings to my knowledge, I need to look into where this int comes from.

sorbaugh · 2024-08-16T09:25:22Z

Hello @alpapan, looks like the original issue was solved with a different approach! Wanted to still thank you for your contribution and hope to seeing more from you! 🚀

Addresses nextcloud#35642: LDAP variables can be integers

3efaf50

Signed-off-by: Alexie Papanicolaou <alpapan@gmail.com>

solracsf changed the title ~~Addresses #35642: LDAP variables can be integers~~ LDAP variables can be integers Dec 9, 2022

szaimen added the 3. to review Waiting for reviews label Dec 9, 2022

szaimen added this to the Nextcloud 26 milestone Dec 9, 2022

szaimen requested review from come-nc, CarlSchwan, ChristophWurst and blizzz and removed request for come-nc December 9, 2022 07:52

github-advanced-security bot found potential problems Dec 9, 2022

View reviewed changes

come-nc requested changes Dec 12, 2022

View reviewed changes

Changes for nextcloud#35701

25ad28c

blizzz mentioned this pull request Feb 1, 2023

26.0.0 beta 2 #36484

Merged

skjnldsv mentioned this pull request Feb 23, 2023

26.0.0 beta 5 #36834

Merged

blizzz mentioned this pull request Mar 7, 2023

26.0.0 RC2 #37078

Merged

blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023

This was referenced May 3, 2023

27.0.0 beta 1 #38038

Merged

27.0.0 beta 2 #38137

Closed

27.0.0 beta 2 #38139

Merged

blizzz mentioned this pull request May 17, 2023

27.0.0 RC1 #38341

Merged

blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023

joshtrichards added the feature: ldap label Aug 29, 2023

skjnldsv mentioned this pull request Nov 1, 2023

chore: 28.0.0 beta 1 #41228

Merged

This was referenced Nov 6, 2023

28.0.0 beta 2 #41314

Merged

28.0.0 beta 3 #41379

Merged

This was referenced Nov 14, 2023

28.0.0 beta 4 #41477

Merged

28.0.0 RC1 #41621

Merged

blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023

solracsf mentioned this pull request Feb 7, 2024

fix(user_ldap): usernames can be numeric #43415

Closed

solracsf added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Feb 7, 2024

This was referenced Mar 12, 2024

29.0.0 beta 2 #44152

Merged

29.0.0 beta 3 #44186

Merged

29.0.0 beta 4 #44275

Merged

This was referenced Mar 20, 2024

29.0.0 beta 5 #44361

Merged

29.0.0 beta 6 #44449

Merged

skjnldsv mentioned this pull request Mar 28, 2024

29.0.0 RC1 #44552

Merged

81 tasks

skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024

This was referenced Jul 30, 2024

30.0.0 beta 2 #46870

Merged

30.0.0 beta 3 #46946

Merged

This was referenced Aug 5, 2024

30.0.0 beta 4 #47028

Merged

30.0.0 beta 5 #47100

Merged

skjnldsv mentioned this pull request Aug 13, 2024

30.0.0 RC1 #47190

Merged

skjnldsv closed this Aug 14, 2024

skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LDAP variables can be integers #35701

LDAP variables can be integers #35701

alpapan commented Dec 9, 2022 •

edited by solracsf

Loading

alpapan Dec 14, 2022

alpapan Dec 14, 2022

alpapan Dec 14, 2022

alpapan Dec 14, 2022

alpapan Dec 14, 2022

come-nc left a comment

sorbaugh commented Aug 16, 2024

LDAP variables can be integers #35701

LDAP variables can be integers #35701

Conversation

alpapan commented Dec 9, 2022 • edited by solracsf Loading

TODO

Checklist

alpapan Dec 14, 2022

Choose a reason for hiding this comment

alpapan Dec 14, 2022

Choose a reason for hiding this comment

alpapan Dec 14, 2022

Choose a reason for hiding this comment

alpapan Dec 14, 2022

Choose a reason for hiding this comment

alpapan Dec 14, 2022

Choose a reason for hiding this comment

come-nc left a comment

Choose a reason for hiding this comment

sorbaugh commented Aug 16, 2024

alpapan commented Dec 9, 2022 •

edited by solracsf

Loading