Allow hidden smb shares

[kesselb]

Fix #15567

A hidden smb share ends with $. This patch changes the placeholder
detection to allow shares with $ at the end.

[kesselb]

@blizzz i tried to add some test cases for the array as $option but not sure which format is expected.

server/apps/files_external/lib/config.php

Lines 288 to 294 in ae7f89f

	if(is_array($option)) {
	foreach ($option as $optionItem) {
	if(is_array($optionItem)) {
	$result = $result && self::arePlaceholdersSubstituted($option);
	}
	}
	} else if (is_string($option)) {

I guess we should pass $optionItem instead of $option to arePlaceholdersSubstituted?

[blizzz]

@blizzz i tried to add some test cases for the array as $option but not sure which format is expected.

Anything that is entered in the external storage configuration (but the mount name) will be passed there (just for the password there should be some exception somewhere). We pass only strings, and if it is an array we go a level deeper.

[kesselb]

@blizzz i tried to add some test cases for the array as $option but not sure which format is expected.

Anything that is entered in the external storage configuration (but the mount name) will be passed there (just for the password there should be some exception somewhere). We pass only strings, and if it is an array we go a level deeper.

Thank you ;) Mind to check the code? I think we call arePlaceholdersSubstituted with the wrong variable here.

[kesselb]

As reference: #15654

I guess $ is a valid character for an smb share name: https://docs.microsoft.com/en-us/rest/api/storageservices/naming-and-referencing-shares--directories--files--and-metadata Do you think we could use another character for placeholders?

[kesselb]

As reference: #15934

Looks like $ is also a valid character for an smb username.

[wiswedel]

Fix #15934

[Trujo-Dilar]

Like wiswedel I can confirm this patch resolve hidden share problem, with this you can use //server/share$.
but $user substitution fail (with and without patch).

[skjnldsv]

To merge?

[wiswedel]

To merge?

@skjnldsv At least @icewind1991 already approved.
@schiessle or/and @blizzz would make good 2nd reviewers.

[kesselb]

server/apps/files_external/lib/config.php

Lines 288 to 294 in ae7f89f

	if(is_array($option)) {
	foreach ($option as $optionItem) {
	if(is_array($optionItem)) {
	$result = $result && self::arePlaceholdersSubstituted($option);
	}
	}
	} else if (is_string($option)) {

I guess we should pass $optionItem instead of $option to arePlaceholdersSubstituted?

@schiessle / @blizzz If you do a second review please look at the comment. I still think there is something broken. Thank you 👍

[icewind1991]

@kesselb you are correct I think, also the 2nd is_array check seems wrong and should be removed.

[hessercan]

16.0.3 breaks this patch

[blizzz]

@kesselb yes, in hindsight, i agree with @icewind1991

[kesselb]

Pushed a fix for the invalid recursion. I'm not sure if the feature now works as expected. Could not find any information under which conditions $option should be an array.

[skjnldsv]

Failure unrelated :)

[blizzz]

/backport to stable16

[backportbot-nextcloud]

backport to stable16 in #16527