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

Fix sharing with uids in numerical format #37336

Merged
merged 3 commits into from May 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 6 additions & 3 deletions apps/files_sharing/lib/Controller/Share20OcsController.php
Expand Up @@ -404,7 +404,7 @@ public function createShare() {
$userAutoAccept = $this->config->getUserValue($shareWith, 'files_sharing', 'auto_accept_share', 'yes') === 'yes';
}
// Valid user is required to share
if ($shareWith === null || !$this->userManager->userExists($shareWith)) {
if (!\is_string($shareWith) || !$this->userManager->userExists($shareWith)) {
$share->getNode()->unlock(ILockingProvider::LOCK_SHARED);
return new Result(null, 404, $this->l->t('Please specify a valid user'));
}
Expand All @@ -422,7 +422,7 @@ public function createShare() {
}

// Valid group is required to share
if ($shareWith === null || !$this->groupManager->groupExists($shareWith)) {
if (!\is_string($shareWith) || !$this->groupManager->groupExists($shareWith)) {
$share->getNode()->unlock(ILockingProvider::LOCK_SHARED);
return new Result(null, 404, $this->l->t('Please specify a valid group'));
}
Expand Down Expand Up @@ -494,7 +494,10 @@ public function createShare() {
$share->getNode()->unlock(ILockingProvider::LOCK_SHARED);
return new Result(null, 403, $this->l->t('Sharing %s failed because the back end does not allow shares from type %s', [$path->getPath(), $shareType]));
}

if (!\is_string($shareWith)) {
$share->getNode()->unlock(ILockingProvider::LOCK_SHARED);
return new Result(null, 404, $this->l->t('shareWith parameter must be a string'));
}
$share->setSharedWith($shareWith);
$share->setPermissions($permissions);
} else {
Expand Down
7 changes: 6 additions & 1 deletion apps/files_sharing/lib/Controller/ShareesController.php
Expand Up @@ -189,7 +189,12 @@ protected function getUsers($search) {

$foundUserById = false;
$lowerSearch = \strtolower($search);
foreach ($users as $uid => $user) {
foreach ($users as $user) {
/**
* Php parses numeric UID strings as integer in array key,
* because of that, we need to learn uid from User object
*/
$uid = $user->getUID();
/* @var $user IUser */
$entry = [
'label' => $user->getDisplayName(),
Expand Down
4 changes: 2 additions & 2 deletions apps/files_sharing/tests/API/ShareesTest.php
Expand Up @@ -424,7 +424,7 @@ public function dataGetUsers() {
['abc', 'xyz'],
[
['abc', 'test', 2, 0, [
'test' => $this->getUserMock('test1', 'Test One'),
'test' => $this->getUserMock('test', 'Test One'),
]],
['xyz', 'test', 2, 0, [
'test2' => $this->getUserMock('test2', 'Test Two'),
Expand All @@ -446,7 +446,7 @@ public function dataGetUsers() {
['abc', 'xyz'],
[
['abc', 'test', 2, 0, [
'test' => $this->getUserMock('test1', 'Test One'),
'test' => $this->getUserMock('test', 'Test One'),
]],
['xyz', 'test', 2, 0, [
'test2' => $this->getUserMock('test2', 'Test Two'),
Expand Down
70 changes: 66 additions & 4 deletions apps/files_sharing/tests/Controller/Share20OcsControllerTest.php
Expand Up @@ -758,7 +758,19 @@ public function testCreateShareUserNoShareWith() {
$this->assertEquals($expected->getData(), $result->getData());
}

public function testCreateShareUserNoValidShareWith() {
public function InvalidShareWithProvider() {
return [
['invaliduser'],
[123456],
[null]
];
}

/**
* @dataProvider InvalidShareWithProvider
* @param mixed $shareWith
*/
public function testCreateShareUserNoValidShareWith($shareWith) {
$share = $this->newShare();
$this->shareManager->method('newShare')->willReturn($share);

Expand All @@ -768,7 +780,7 @@ public function testCreateShareUserNoValidShareWith() {
['path', null, 'valid-path'],
['permissions', null, \OCP\Constants::PERMISSION_ALL],
['shareType', $this->any(), Share::SHARE_TYPE_USER],
['shareWith', $this->any(), 'invalidUser'],
['shareWith', $this->any(), $shareWith],
]));

$userFolder = $this->createMock('\OCP\Files\Folder');
Expand Down Expand Up @@ -862,7 +874,11 @@ public function testCreateShareUser() {
$this->assertEquals($expected->getData(), $result->getData());
}

public function testCreateShareGroupNoValidShareWith() {
/**
* @dataProvider InvalidShareWithProvider
* @param mixed $shareWith
*/
public function testCreateShareGroupNoValidShareWith($shareWith) {
$share = $this->newShare();
$this->shareManager->method('newShare')->willReturn($share);
$this->shareManager->method('createShare')->will($this->returnArgument(0));
Expand All @@ -873,7 +889,7 @@ public function testCreateShareGroupNoValidShareWith() {
['path', null, 'valid-path'],
['permissions', null, \OCP\Constants::PERMISSION_ALL],
['shareType', $this->any(), Share::SHARE_TYPE_GROUP],
['shareWith', $this->any(), 'invalidGroup'],
['shareWith', $this->any(), $shareWith],
]));

$userFolder = $this->createMock('\OCP\Files\Folder');
Expand Down Expand Up @@ -1399,6 +1415,52 @@ public function testCreateShareLinkPassword() {
$this->assertEquals($expected->getData(), $result->getData());
}

/**
* @dataProvider InvalidShareWithProvider
* @param mixed $shareWith
*/
public function testCreateShareRemoteNoValidShareWith($shareWith) {
$share = $this->newShare();
$this->shareManager->method('newShare')->willReturn($share);
$this->shareManager->method('outgoingServer2ServerSharesAllowed')->willReturn(true);

$this->request
->method('getParam')
->will($this->returnValueMap([
['path', null, 'valid-path'],
['permissions', null, \OCP\Constants::PERMISSION_ALL],
['shareType', '-1', Share::SHARE_TYPE_REMOTE],
['shareWith', $this->any(), $shareWith]
]));

$userFolder = $this->createMock('\OCP\Files\Folder');
$this->rootFolder->expects($this->once())
->method('getUserFolder')
->with('currentUser')
->willReturn($userFolder);

$path = $this->createMock('\OCP\Files\File');
$storage = $this->createMock('OCP\Files\Storage');
$storage->method('instanceOfStorage')
->with('OCA\Files_Sharing\External\Storage')
->willReturn(false);
$path->method('getStorage')->willReturn($storage);
$userFolder->expects($this->once())
->method('get')
->with('valid-path')
->willReturn($path);

$path->expects($this->once())
->method('lock')
->with(ILockingProvider::LOCK_SHARED);

$expected = new Result(null, 404, 'shareWith parameter must be a string');
$result = $this->ocs->createShare();

$this->assertEquals($expected->getMeta(), $result->getMeta());
$this->assertEquals($expected->getData(), $result->getData());
}

public function testCreateShareValidExpireDate() {
$ocs = $this->mockFormatShare();

Expand Down
7 changes: 7 additions & 0 deletions changelog/unreleased/37324
@@ -0,0 +1,7 @@
Bugfix: Cannot share with user name that has only numbers in the UI

A regression in 10.4.0 meant that new shares with user names that were numbers
could not be created in the UI. This regression has been fixed.

https://github.com/owncloud/core/issues/37324
https://github.com/owncloud/core/pull/37336
Expand Up @@ -56,16 +56,26 @@ Feature: Sharing files and folders with internal users
And file "lorem.txt" should not be listed in shared-with-others page on the webUI
And as "user2" file "lorem (2).txt" should not exist

Scenario: user shares a file with another user with uppercase username
@skipOnOcV10.3 @skipOnOcV10.4
Scenario Outline: user shares a file with another user with unusual usernames
Given user "user1" has been created with default attributes and skeleton files
And these users have been created without skeleton files:
| username |
| SomeUser |
| username |
| <username> |
And user "user1" has logged in using the webUI
When the user shares file "lorem.txt" with user "SomeUser" using the webUI
And the user re-logs in as "SomeUser" using the webUI
When the user shares file "lorem.txt" with user "<username>" using the webUI
And the user re-logs in as "<username>" using the webUI
And the user browses to the shared-with-you page
Then file "lorem.txt" should be listed on the webUI
Examples:
| username |
| 123456 |
| -12 |
| +12 |
| 1.2 |
| 1.2E3 |
| 0x10 |
| Some-User |

Scenario: multiple users share a file with the same name to a user
Given these users have been created with default attributes and without skeleton files:
Expand Down