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

#1 move jailing code to core #27286

Merged
merged 10 commits into from
Mar 23, 2017
2 changes: 1 addition & 1 deletion core/js/share.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ OC.Share = _.extend(OC.Share || {}, {
SHARE_TYPE_USER:0,
SHARE_TYPE_GROUP:1,
SHARE_TYPE_LINK:3,
SHARE_TYPE_EMAIL:4,
SHARE_TYPE_GUEST:4,
SHARE_TYPE_REMOTE:6,

/**
Expand Down
2 changes: 1 addition & 1 deletion core/js/sharedialogview.js
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@
'onShareWithFieldChanged'
);

OC.Plugins.attach('OCA.Share.ShareDialogView', this);
OC.Plugins.attach('OC.Share.ShareDialogView', this);
},

onShareWithFieldChanged: function() {
Expand Down
177 changes: 177 additions & 0 deletions lib/private/Files/Storage/Wrapper/DirMask.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
<?php
/**
* @author Jörn Friedrich Dreyer <jfd@butonic.de>
*
* @copyright Copyright (c) 2017, ownCloud GmbH
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/

namespace OC\Files\Storage\Wrapper;


/**
* While PermissionMask can mask a whole storage this can
* mask a certain directory inside a storage
*
* @package OC\Files\Storage\Wrapper
*/
class DirMask extends PermissionsMask {

/**
* @var string the dir that should be masked
*/
private $path;
/**
* @var int remember length
*/
private $pathLength;
/**
* @param array $arguments ['storage' => $storage, 'mask' => $mask, 'path' => $path]
*
* $storage: The storage the permissions mask should be applied on
* $mask: The permission bits that should be kept, a combination of the \OCP\Constant::PERMISSION_ constants
* $path: The path relative to the storage root that should be masked
*/
public function __construct($arguments) {
parent::__construct($arguments);
$this->path = $arguments['path'];
$this->pathLength = strlen($arguments['path']);
}

protected function checkPath($path) {
return substr($path, 0, $this->pathLength) === $this->path;
}

public function isUpdatable($path) {
if ($this->checkPath($path)) {
return parent::isUpdatable($path);
} else {
return $this->storage->isUpdatable($path);
}
}

public function isCreatable($path) {
if ($this->checkPath($path)) {
return parent::isCreatable($path);
} else {
return $this->storage->isCreatable($path);
}
}

public function isDeletable($path) {
if ($this->checkPath($path)) {
return parent::isDeletable($path);
} else {
return $this->storage->isDeletable($path);
}
}

public function isSharable($path) {
if ($this->checkPath($path)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do decide to prevent sharing globally for guests, this is the place where we could always return false.

Let's leave this as is for now.

return parent::isSharable($path);
} else {
return $this->storage->isSharable($path);
}
}

public function getPermissions($path) {
if ($this->checkPath($path)) {
return parent::getPermissions($path);
} else {
return $this->storage->getPermissions($path);
}
}

public function rename($path1, $path2) {
if (!$this->isReadable($path1)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need special handling for when $path1 is a part file and $path2 isn't ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw later on that you added the special handling in the subclass, ok

return false;
}
if ($this->file_exists($path2)) {
if ($this->isUpdatable($path2)) {
return $this->storage->rename($path1, $path2);
}
} else {
if ($this->isCreatable($path2)) {
return $this->storage->rename($path1, $path2);
}
}
return false;
}

public function copy($path1, $path2) {
if (!$this->isReadable($path1)) {
return false;
}
if ($this->file_exists($path2)) {
if ($this->isUpdatable($path2)) {
return $this->storage->copy($path1, $path2);
}
} else {
if ($this->isCreatable($path2)) {
return $this->storage->copy($path1, $path2);
}
}
return false;
}

public function touch($path, $mtime = null) {
if ($this->checkPath($path)) {
return parent::touch($path);
} else {
return $this->storage->touch($path);
}
}

public function mkdir($path) {
if ($this->checkPath($path)) {
return parent::mkdir($path);
} else {
return $this->storage->mkdir($path);
}
}

public function rmdir($path) {
if ($this->checkPath($path)) {
return parent::rmdir($path);
} else {
return $this->storage->rmdir($path);
}
}

public function unlink($path) {
if ($this->checkPath($path)) {
return parent::unlink($path);
} else {
return $this->storage->unlink($path);
}
}

public function file_put_contents($path, $data) {
if ($this->checkPath($path)) {
return parent::file_put_contents($path, $data);
} else {
return $this->storage->file_put_contents($path, $data);
}
}

public function fopen($path, $mode) {
if ($this->checkPath($path)) {
return parent::fopen($path, $mode);
} else {
return $this->storage->fopen($path, $mode);
}
}
}
2 changes: 1 addition & 1 deletion lib/private/Files/Storage/Wrapper/PermissionsMask.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class PermissionsMask extends Wrapper {
/**
* @var int the permissions bits we want to keep
*/
private $mask;
protected $mask;

/**
* @param array $arguments ['storage' => $storage, 'mask' => $mask]
Expand Down
69 changes: 69 additions & 0 deletions lib/private/Files/Storage/Wrapper/ReadOnlyJail.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
<?php

/**
* @author Ilja Neumann <ineumann@owncloud.com>
*
* @copyright Copyright (c) 2017, ownCloud GmbH
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/
namespace OC\Files\Storage\Wrapper;

use OCP\Constants;

class ReadOnlyJail extends DirMask {

/**
* @param $path
* @return bool
*/
protected function checkPath($path) {
if ($path === 'files') {
return true;
}

return parent::checkPath($path);
}


/**
* @param string $path
* @return bool
*/
public function isDeletable($path) {
if (pathinfo($path, PATHINFO_EXTENSION) === 'part') {
return true;
}

return $this->getWrapperStorage()->isDeletable($path);
}

/**
* @param string $path
* @return bool
*/
public function mkdir($path) {
// Lift restrictions if files dir is created (at first login)
if ($path === 'files') {
$this->mask = Constants::PERMISSION_CREATE;
};

$result = parent::mkdir($path);
$this->mask = Constants::PERMISSION_READ;

return $result;
}

}
2 changes: 1 addition & 1 deletion lib/private/Share/Constants.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class Constants {
const SHARE_TYPE_USER = 0;
const SHARE_TYPE_GROUP = 1;
const SHARE_TYPE_LINK = 3;
const SHARE_TYPE_EMAIL = 4; // ToDo Check if it is still in use otherwise remove it
const SHARE_TYPE_GUEST = 4;
const SHARE_TYPE_CONTACT = 5; // ToDo Check if it is still in use otherwise remove it
const SHARE_TYPE_REMOTE = 6; // ToDo Check if it is still in use otherwise remove it

Expand Down
12 changes: 10 additions & 2 deletions lib/private/User/Session.php
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
use OCP\AppFramework\QueryException;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\Authentication\IAuthModule;
use OCP\Files\NotPermittedException;
use OCP\IConfig;
use OCP\IRequest;
use OCP\ISession;
Expand Down Expand Up @@ -395,8 +396,15 @@ protected function prepareUserLogin($firstTimeLogin) {
//trigger creation of user home and /files folder
$userFolder = \OC::$server->getUserFolder($user);

// copy skeleton
\OC_Util::copySkeleton($user, $userFolder);
try {
// copy skeleton
\OC_Util::copySkeleton($user, $userFolder);
} catch (NotPermittedException $ex) {
// possible if files directory is in an readonly jail
\OC::$server->getLogger()->warning(
'Skeleton not created due to missing write permission'
);
}

// trigger any other initialization
\OC::$server->getEventDispatcher()->dispatch(IUser::class . '::firstLogin', new GenericEvent($this->getUser()));
Expand Down
51 changes: 51 additions & 0 deletions lib/private/legacy/util.php
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,57 @@ public static function setupFS($user = '') {
OC_Hook::emit('OC_Filesystem', 'preSetup', ['user' => $user]);
\OC\Files\Filesystem::logWarningWhenAddingStorageWrapper(true);

// Make users storage readonly if he is a guest or in a read_only group

$isGuest = \OC::$server->getConfig()->getUserValue(
$user,
'owncloud',
'isGuest',
false
);

if (!$isGuest) {
$readOnlyGroups = json_decode(\OC::$server->getConfig()->getAppValue(
'core',
'read_only_groups',
'[]'
), true);

if (!is_array($readOnlyGroups)) {
$readOnlyGroups = [];
}


$userGroups = array_keys(
\OC::$server->getGroupManager()->getUserIdGroups($user)
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are aware, that this introduced additional query - for each request going to oc, even most trivial? #29380 @PVince81 @DeepDiver1975

Copy link
Contributor

@mrow4a mrow4a Oct 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, because isAdmin, and in turn isInGroup is calling exactly this function, it is not a problem (since it will cache result earlier), but with central group table it might change (isInGroup should not fetch all groups of the users and then filter on app level).

I will try to optimize it later

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that this introduced additional query

Are you saying this was added recently and is a regression ? Or is it part of the years old code ? Can you git blame and link to the PR that introduced it ?


$readOnlyGroupMemberships = array_intersect(
$readOnlyGroups,
$userGroups
);
}


if ($isGuest === '1' || !empty($readOnlyGroupMemberships)) {
\OC\Files\Filesystem::addStorageWrapper(
'oc_readonly',
function ($mountPoint, $storage) use ($user) {
if ($mountPoint === '/' || $mountPoint === "/$user/") {
return new \OC\Files\Storage\Wrapper\ReadOnlyJail(
[
'storage' => $storage,
'mask' => \OCP\Constants::PERMISSION_READ,
'path' => 'files'
]
);
}

return $storage;
}
);
}

//check if we are using an object storage
$objectStore = \OC::$server->getSystemConfig()->getValue('objectstore', null);
if (isset($objectStore)) {
Expand Down
Loading