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

[stable17] Use the correct mountpoint to calculate #21774

Merged
merged 5 commits into from
Jul 9, 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions apps/files_sharing/tests/EtagPropagationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ protected function setUpShares() {
$view2 = new View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files');
$view2->mkdir('/sub1/sub2');
$view2->rename('/folder', '/sub1/sub2/folder');
$this->loginAsUser(self::TEST_FILES_SHARING_API_USER2);

$insideInfo = $view2->getFileInfo('/sub1/sub2/folder/inside');
$this->assertInstanceOf('\OC\Files\FileInfo', $insideInfo);

Expand Down
8 changes: 8 additions & 0 deletions apps/files_sharing/tests/TestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,14 @@ protected function tearDown() {
$qb->delete('share');
$qb->execute();

$qb = \OC::$server->getDatabaseConnection()->getQueryBuilder();
$qb->delete('mounts');
$qb->execute();

$qb = \OC::$server->getDatabaseConnection()->getQueryBuilder();
$qb->delete('filecache');
$qb->execute();

parent::tearDown();
}

Expand Down
228 changes: 228 additions & 0 deletions build/integration/features/sharing-v1-part2.feature
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,34 @@ Feature: sharing
Then the OCS status code should be "404"
And the HTTP status code should be "200"

Scenario: User is allowed to reshare file with more permissions if shares of same file to same user have them
Given user "user0" exists
And user "user1" exists
And user "user2" exists
And group "group1" exists
And user "user1" belongs to group "group1"
And As an "user0"
And creating a share with
| path | /textfile0.txt |
| shareType | 1 |
| shareWith | group1 |
| permissions | 15 |
And As an "user1"
And As an "user0"
And creating a share with
| path | /textfile0.txt |
| shareType | 0 |
| shareWith | user1 |
| permissions | 17 |
And As an "user1"
When creating a share with
| path | /textfile0 (2).txt |
| shareType | 0 |
| shareWith | user2 |
| permissions | 19 |
Then the OCS status code should be "100"
And the HTTP status code should be "200"

Scenario: User is not allowed to reshare file with more permissions
As an "admin"
Given user "user0" exists
Expand All @@ -251,6 +279,86 @@ Feature: sharing
Then the OCS status code should be "404"
And the HTTP status code should be "200"

Scenario: User is not allowed to reshare file with more permissions even if shares of same file to other users have them
Given user "user0" exists
And user "user1" exists
And user "user2" exists
And user "user3" exists
And As an "user0"
And creating a share with
| path | /textfile0.txt |
| shareType | 0 |
| shareWith | user3 |
| permissions | 15 |
And As an "user3"
And As an "user0"
And creating a share with
| path | /textfile0.txt |
| shareType | 0 |
| shareWith | user1 |
| permissions | 17 |
And As an "user1"
When creating a share with
| path | /textfile0 (2).txt |
| shareType | 0 |
| shareWith | user2 |
| permissions | 19 |
Then the OCS status code should be "404"
And the HTTP status code should be "200"

Scenario: User is not allowed to reshare file with more permissions even if shares of other files from same user have them
Given user "user0" exists
And user "user1" exists
And user "user2" exists
And As an "user0"
And creating a share with
| path | /textfile0.txt |
| shareType | 0 |
| shareWith | user1 |
| permissions | 15 |
And As an "user1"
And As an "user0"
And creating a share with
| path | /textfile1.txt |
| shareType | 0 |
| shareWith | user1 |
| permissions | 17 |
And As an "user1"
When creating a share with
| path | /textfile1 (2).txt |
| shareType | 0 |
| shareWith | user2 |
| permissions | 19 |
Then the OCS status code should be "404"
And the HTTP status code should be "200"

Scenario: User is not allowed to reshare file with more permissions even if shares of other files from other users have them
Given user "user0" exists
And user "user1" exists
And user "user2" exists
And user "user3" exists
And As an "user3"
And creating a share with
| path | /textfile0.txt |
| shareType | 0 |
| shareWith | user1 |
| permissions | 15 |
And As an "user1"
And As an "user0"
And creating a share with
| path | /textfile1.txt |
| shareType | 0 |
| shareWith | user1 |
| permissions | 17 |
And As an "user1"
When creating a share with
| path | /textfile1 (2).txt |
| shareType | 0 |
| shareWith | user2 |
| permissions | 19 |
Then the OCS status code should be "404"
And the HTTP status code should be "200"

Scenario: User is not allowed to reshare file with additional delete permissions
As an "admin"
Given user "user0" exists
Expand Down Expand Up @@ -456,6 +564,37 @@ Feature: sharing
| permissions | 1 |
Then the OCS status code should be "100"

Scenario: Allow reshare to exceed permissions if shares of same file to same user have them
Given user "user0" exists
And user "user1" exists
And user "user2" exists
And group "group1" exists
And user "user1" belongs to group "group1"
And user "user0" created a folder "/TMP"
And As an "user0"
And creating a share with
| path | /TMP |
| shareType | 1 |
| shareWith | group1 |
| permissions | 15 |
And As an "user1"
And As an "user0"
And creating a share with
| path | /TMP |
| shareType | 0 |
| shareWith | user1 |
| permissions | 17 |
And As an "user1"
And creating a share with
| path | /TMP |
| shareType | 0 |
| shareWith | user2 |
| permissions | 17 |
When Updating last share with
| permissions | 31 |
Then the OCS status code should be "100"
And the HTTP status code should be "200"

Scenario: Do not allow reshare to exceed permissions
Given user "user0" exists
And user "user1" exists
Expand All @@ -477,6 +616,95 @@ Feature: sharing
| permissions | 31 |
Then the OCS status code should be "404"

Scenario: Do not allow reshare to exceed permissions even if shares of same file to other users have them
Given user "user0" exists
And user "user1" exists
And user "user2" exists
And user "user3" exists
And user "user0" created a folder "/TMP"
And As an "user0"
And creating a share with
| path | /TMP |
| shareType | 0 |
| shareWith | user3 |
| permissions | 15 |
And As an "user3"
And As an "user0"
And creating a share with
| path | /TMP |
| shareType | 0 |
| shareWith | user1 |
| permissions | 21 |
And As an "user1"
And creating a share with
| path | /TMP |
| shareType | 0 |
| shareWith | user2 |
| permissions | 21 |
When Updating last share with
| permissions | 31 |
Then the OCS status code should be "404"
And the HTTP status code should be "200"

Scenario: Do not allow reshare to exceed permissions even if shares of other files from same user have them
Given user "user0" exists
And user "user1" exists
And user "user2" exists
And As an "user0"
And creating a share with
| path | /FOLDER |
| shareType | 0 |
| shareWith | user1 |
| permissions | 15 |
And As an "user1"
And user "user0" created a folder "/TMP"
And As an "user0"
And creating a share with
| path | /TMP |
| shareType | 0 |
| shareWith | user1 |
| permissions | 21 |
And As an "user1"
And creating a share with
| path | /TMP |
| shareType | 0 |
| shareWith | user2 |
| permissions | 21 |
When Updating last share with
| permissions | 31 |
Then the OCS status code should be "404"
And the HTTP status code should be "200"

Scenario: Do not allow reshare to exceed permissions even if shares of other files from other users have them
Given user "user0" exists
And user "user1" exists
And user "user2" exists
And user "user3" exists
And As an "user3"
And creating a share with
| path | /FOLDER |
| shareType | 0 |
| shareWith | user1 |
| permissions | 15 |
And As an "user1"
And user "user0" created a folder "/TMP"
And As an "user0"
And creating a share with
| path | /TMP |
| shareType | 0 |
| shareWith | user1 |
| permissions | 21 |
And As an "user1"
And creating a share with
| path | /TMP |
| shareType | 0 |
| shareWith | user2 |
| permissions | 21 |
When Updating last share with
| permissions | 31 |
Then the OCS status code should be "404"
And the HTTP status code should be "200"

Scenario: Do not allow sub reshare to exceed permissions
Given user "user0" exists
And user "user1" exists
Expand Down
4 changes: 4 additions & 0 deletions lib/private/Files/Filesystem.php
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,10 @@ public static function initMountPoints($user = '') {

// home mounts are handled seperate since we need to ensure this is mounted before we call the other mount providers
$homeMount = $mountConfigManager->getHomeMountForUser($userObject);
if ($homeMount->getStorageRootId() === -1) {
$homeMount->getStorage()->mkdir('');
$homeMount->getStorage()->getScanner()->scan('');
}

self::getMountManager()->addMount($homeMount);

Expand Down
2 changes: 1 addition & 1 deletion lib/private/Files/Mount/LocalHomeMountProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class LocalHomeMountProvider implements IHomeMountProvider {
*
* @param IUser $user
* @param IStorageFactory $loader
* @return \OCP\Files\Mount\IMountPoint[]
* @return \OCP\Files\Mount\IMountPoint|null
*/
public function getHomeMountForUser(IUser $user, IStorageFactory $loader) {
$arguments = ['user' => $user];
Expand Down
2 changes: 1 addition & 1 deletion lib/private/Files/Mount/MountPoint.php
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ public function getOptions() {
* @return int
*/
public function getStorageRootId() {
if (is_null($this->rootId)) {
if (is_null($this->rootId) || $this->rootId === -1) {
$this->rootId = (int)$this->getStorage()->getCache()->getId('');
}
return $this->rootId;
Expand Down
9 changes: 8 additions & 1 deletion lib/private/Share20/Manager.php
Original file line number Diff line number Diff line change
Expand Up @@ -291,13 +291,20 @@ protected function generalCreateChecks(\OCP\Share\IShare $share) {

$isFederatedShare = $share->getNode()->getStorage()->instanceOfStorage('\OCA\Files_Sharing\External\Storage');
$permissions = 0;
$mount = $share->getNode()->getMountPoint();

$userMounts = $userFolder->getById($share->getNode()->getId());
$userMount = array_shift($userMounts);
$mount = $userMount->getMountPoint();
if (!$isFederatedShare && $share->getNode()->getOwner() && $share->getNode()->getOwner()->getUID() !== $share->getSharedBy()) {
// When it's a reshare use the parent share permissions as maximum
$userMountPointId = $mount->getStorageRootId();
$userMountPoints = $userFolder->getById($userMountPointId);
$userMountPoint = array_shift($userMountPoints);

if ($userMountPoint === null) {
throw new GenericShareException('Could not get proper user mount for ' . $userMountPointId . '. Failing since else the next calls are called with null');
}

/* Check if this is an incoming share */
$incomingShares = $this->getSharedWith($share->getSharedBy(), Share::SHARE_TYPE_USER, $userMountPoint, -1, 0);
$incomingShares = array_merge($incomingShares, $this->getSharedWith($share->getSharedBy(), Share::SHARE_TYPE_GROUP, $userMountPoint, -1, 0));
Expand Down
9 changes: 9 additions & 0 deletions tests/lib/Share20/ManagerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,7 @@ public function dataGeneralChecks() {
$limitedPermssions = $this->createMock(File::class);
$limitedPermssions->method('isShareable')->willReturn(true);
$limitedPermssions->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_READ);
$limitedPermssions->method('getId')->willReturn(108);
$limitedPermssions->method('getPath')->willReturn('path');
$limitedPermssions->method('getOwner')
->willReturn($owner);
Expand All @@ -623,6 +624,7 @@ public function dataGeneralChecks() {
$nonMoveableMountPermssions = $this->createMock(Folder::class);
$nonMoveableMountPermssions->method('isShareable')->willReturn(true);
$nonMoveableMountPermssions->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_READ);
$nonMoveableMountPermssions->method('getId')->willReturn(108);
$nonMoveableMountPermssions->method('getPath')->willReturn('path');
$nonMoveableMountPermssions->method('getOwner')
->willReturn($owner);
Expand All @@ -644,6 +646,7 @@ public function dataGeneralChecks() {
$allPermssions = $this->createMock(Folder::class);
$allPermssions->method('isShareable')->willReturn(true);
$allPermssions->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_ALL);
$allPermssions->method('getId')->willReturn(108);
$allPermssions->method('getOwner')
->willReturn($owner);
$allPermssions->method('getStorage')
Expand All @@ -664,6 +667,7 @@ public function dataGeneralChecks() {
$remoteFile = $this->createMock(Folder::class);
$remoteFile->method('isShareable')->willReturn(true);
$remoteFile->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_READ ^ \OCP\Constants::PERMISSION_UPDATE);
$remoteFile->method('getId')->willReturn(108);
$remoteFile->method('getOwner')
->willReturn($owner);
$remoteFile->method('getStorage')
Expand Down Expand Up @@ -698,6 +702,11 @@ public function testGeneralChecks($share, $exceptionMessage, $exception) {
$userFolder->expects($this->any())
->method('getId')
->willReturn(42);
// Id 108 is used in the data to refer to the node of the share.
$userFolder->expects($this->any())
->method('getById')
->with(108)
->willReturn([$share->getNode()]);
$userFolder->expects($this->any())
->method('getRelativePath')
->willReturnArgument(0);
Expand Down
2 changes: 1 addition & 1 deletion tests/lib/TestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ protected function isFileLocked($view, $path, $type, $onMountPoint = false) {
}
}

private function IsDatabaseAccessAllowed() {
protected function IsDatabaseAccessAllowed() {
// on travis-ci.org we allow database access in any case - otherwise
// this will break all apps right away
if (true == getenv('TRAVIS')) {
Expand Down
6 changes: 6 additions & 0 deletions tests/lib/Traits/MountProviderTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ protected function registerMount($userId, $storage, $mountPoint, $arguments = nu
$this->mounts[$userId] = [];
}
$this->mounts[$userId][] = ['storage' => $storage, 'mountPoint' => $mountPoint, 'arguments' => $arguments];

if ($this->IsDatabaseAccessAllowed()) {
$mount = new MountPoint($storage, $mountPoint, $arguments, $this->storageFactory);
$storage = $mount->getStorage();
$storage->getScanner()->scan('');
}
}

protected function registerStorageWrapper($name, $wrapper) {
Expand Down