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.

Sign up for GitHub

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

Jump to bottom

[sub-PR] Move common logic to share manager #29561

Closed
wants to merge 2 commits into from
Closed

[sub-PR] Move common logic to share manager #29561

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f18664
Respect user enumeration settings on profile
Pytal Nov 5, 2021
f8d9c12
Move common logic to share manager
nickvergessen Nov 5, 2021
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
4 changes: 3 additions & 1 deletion apps/files_sharing/tests/CapabilitiesTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
*/
namespace OCA\Files_Sharing\Tests;

use OC\KnownUser\KnownUserService;
use OC\Share20\Manager;
use OCA\Files_Sharing\Capabilities;
use OCP\EventDispatcher\IEventDispatcher;
Expand Down Expand Up @@ -94,7 +95,8 @@ private function getResults(array $map) {
$this->createMock(IURLGenerator::class),
$this->createMock(\OC_Defaults::class),
$this->createMock(IEventDispatcher::class),
$this->createMock(IUserSession::class)
$this->createMock(IUserSession::class),
$this->createMock(KnownUserService::class)
);
$cap = new Capabilities($config, $shareManager);
$result = $this->getFilesSharingPart($cap->getCapabilities());
Expand Down
48 changes: 35 additions & 13 deletions core/Controller/ProfilePageController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,17 @@

namespace OC\Core\Controller;

use OC\KnownUser\KnownUserService;
use OC\Profile\ProfileManager;
use OCP\Accounts\IAccountManager;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\AppFramework\Services\IInitialState;
use OCP\IGroupManager;
use OCP\IRequest;
use OCP\IUserManager;
use OCP\IUserSession;
use OC\Profile\ProfileManager;
use OCP\Share\IManager as IShareManager;
use OCP\UserStatus\IManager as IUserStatusManager;

class ProfilePageController extends Controller {
Expand All @@ -48,6 +51,15 @@ class ProfilePageController extends Controller {
/** @var ProfileManager */
private $profileManager;

/** @var IShareManager */
private $shareManager;

/** @var IGroupManager */
private $groupManager;

/** @var KnownUserService */
private $knownUserService;

/** @var IUserManager */
private $userManager;

Expand All @@ -63,6 +75,9 @@ public function __construct(
IInitialState $initialStateService,
IAccountManager $accountManager,
ProfileManager $profileManager,
IShareManager $shareManager,
IGroupManager $groupManager,
KnownUserService $knownUserService,
IUserManager $userManager,
IUserSession $userSession,
IUserStatusManager $userStatusManager
Expand All @@ -71,6 +86,9 @@ public function __construct(
$this->initialStateService = $initialStateService;
$this->accountManager = $accountManager;
$this->profileManager = $profileManager;
$this->shareManager = $shareManager;
$this->groupManager = $groupManager;
$this->knownUserService = $knownUserService;
$this->userManager = $userManager;
$this->userSession = $userSession;
$this->userStatusManager = $userStatusManager;
Expand All @@ -83,26 +101,30 @@ public function __construct(
* @NoSubAdminRequired
*/
public function index(string $targetUserId): TemplateResponse {
$profileNotFoundTemplate = new TemplateResponse(
'core',
'404-profile',
[],
TemplateResponse::RENDER_AS_GUEST,
);

if (!$this->userManager->userExists($targetUserId)) {
return new TemplateResponse(
'core',
'404-profile',
[],
TemplateResponse::RENDER_AS_GUEST,
);
return $profileNotFoundTemplate;
}

$visitingUser = $this->userSession->getUser();
$targetUser = $this->userManager->get($targetUserId);
$targetAccount = $this->accountManager->getAccount($targetUser);

if (!$this->isProfileEnabled($targetAccount)) {
return new TemplateResponse(
'core',
'404-profile',
[],
TemplateResponse::RENDER_AS_GUEST,
);
return $profileNotFoundTemplate;
}

// Run user enumeration checks only if viewing another user's profile
if ($targetUser !== $visitingUser) {
if (!$this->shareManager->currentUserCanEnumerateTargetUser($visitingUser, $targetUser)) {
return $profileNotFoundTemplate;
}
}

$userStatuses = $this->userStatusManager->getUserStatuses([$targetUserId]);
Expand Down
44 changes: 43 additions & 1 deletion lib/private/Share20/Manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@

use OC\Cache\CappedMemoryCache;
use OC\Files\Mount\MoveableMount;
use OC\KnownUser\KnownUserService;
use OC\Share20\Exception\ProviderException;
use OCA\Files_Sharing\AppInfo\Application;
use OCA\Files_Sharing\ISharedStorage;
Expand Down Expand Up @@ -118,7 +119,10 @@ class Manager implements IManager {
private $defaults;
/** @var IEventDispatcher */
private $dispatcher;
/** @var IUserSession */
private $userSession;
/** @var KnownUserService */
private $knownUserService;

public function __construct(
ILogger $logger,
Expand All @@ -137,7 +141,8 @@ public function __construct(
IURLGenerator $urlGenerator,
\OC_Defaults $defaults,
IEventDispatcher $dispatcher,
IUserSession $userSession
IUserSession $userSession,
KnownUserService $knownUserService
) {
$this->logger = $logger;
$this->config = $config;
Expand All @@ -160,6 +165,7 @@ public function __construct(
$this->defaults = $defaults;
$this->dispatcher = $dispatcher;
$this->userSession = $userSession;
$this->knownUserService = $knownUserService;
}

/**
Expand Down Expand Up @@ -1909,6 +1915,42 @@ public function allowEnumerationFullMatch(): bool {
return $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match', 'yes') === 'yes';
}

public function currentUserCanEnumerateTargetUser(?IUser $currentUser, IUser $targetUser): bool {
if ($this->allowEnumerationFullMatch()) {
return true;
}

if (!$this->allowEnumeration()) {
return false;
}

if (!$this->limitEnumerationToPhone() && !$this->limitEnumerationToGroups()) {
// Enumeration is enabled and not restricted: OK
return true;
}

if (!$currentUser instanceof IUser) {
// Enumeration restrictions require an account
return false;
}

// Enumeration is limited to phone match
if ($this->limitEnumerationToPhone() && $this->knownUserService->isKnownToUser($currentUser->getUID(), $targetUser->getUID())) {
return true;
}

// Enumeration is limited to groups
if ($this->limitEnumerationToGroups()) {
$currentUserGroupIds = $this->groupManager->getUserGroupIds($currentUser);
$targetUserGroupIds = $this->groupManager->getUserGroupIds($targetUser);
if (!empty(array_intersect($currentUserGroupIds, $targetUserGroupIds))) {
return true;
}
}

return false;
}

/**
* Copied from \OC_Util::isSharingDisabledForUser
*
Expand Down
11 changes: 11 additions & 0 deletions lib/public/Share/IManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
use OCP\Files\Folder;
use OCP\Files\Node;

use OCP\IUser;
use OCP\Share\Exceptions\GenericShareException;
use OCP\Share\Exceptions\ShareNotFound;

Expand Down Expand Up @@ -447,6 +448,16 @@ public function limitEnumerationToPhone(): bool;
*/
public function allowEnumerationFullMatch(): bool;

/**
* Check if a user A can enumerate another user B
Copy link
Member

Choose a reason for hiding this comment

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

Should reflect function parameter names :)

*
* @param IUser|null $currentUser
* @param IUser $targetUser
* @return bool
* @since 23.0.0
*/
public function currentUserCanEnumerateTargetUser(?IUser $currentUser, IUser $targetUser): bool;

/**
* Check if sharing is disabled for the given user
*
Expand Down
32 changes: 23 additions & 9 deletions tests/lib/Share20/ManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
namespace Test\Share20;

use OC\Files\Mount\MoveableMount;
use OC\KnownUser\KnownUserService;
use OC\Share20\DefaultShareProvider;
use OC\Share20\Exception;
use OC\Share20\Manager;
Expand Down Expand Up @@ -105,7 +106,10 @@ class ManagerTest extends \Test\TestCase {
protected $urlGenerator;
/** @var \OC_Defaults|MockObject */
protected $defaults;
/** @var IUserSession|MockObject */
protected $userSession;
/** @var KnownUserService|MockObject */
protected $knownUserService;

protected function setUp(): void {
$this->logger = $this->createMock(ILogger::class);
Expand All @@ -122,6 +126,7 @@ protected function setUp(): void {
$this->defaults = $this->createMock(\OC_Defaults::class);
$this->dispatcher = $this->createMock(IEventDispatcher::class);
$this->userSession = $this->createMock(IUserSession::class);
$this->knownUserService = $this->createMock(KnownUserService::class);

$this->l10nFactory = $this->createMock(IFactory::class);
$this->l = $this->createMock(IL10N::class);
Expand Down Expand Up @@ -153,7 +158,8 @@ protected function setUp(): void {
$this->urlGenerator,
$this->defaults,
$this->dispatcher,
$this->userSession
$this->userSession,
$this->knownUserService
);

$this->defaultProvider = $this->createMock(DefaultShareProvider::class);
Expand Down Expand Up @@ -2696,7 +2702,8 @@ public function testGetShareByToken() {
$this->urlGenerator,
$this->defaults,
$this->dispatcher,
$this->userSession
$this->userSession,
$this->knownUserService
);

$share = $this->createMock(IShare::class);
Expand Down Expand Up @@ -2741,7 +2748,8 @@ public function testGetShareByTokenRoom() {
$this->urlGenerator,
$this->defaults,
$this->dispatcher,
$this->userSession
$this->userSession,
$this->knownUserService
);

$share = $this->createMock(IShare::class);
Expand Down Expand Up @@ -2793,7 +2801,8 @@ public function testGetShareByTokenWithException() {
$this->urlGenerator,
$this->defaults,
$this->dispatcher,
$this->userSession
$this->userSession,
$this->knownUserService
);

$share = $this->createMock(IShare::class);
Expand Down Expand Up @@ -4132,7 +4141,8 @@ public function testShareProviderExists($shareType, $expected) {
$this->urlGenerator,
$this->defaults,
$this->dispatcher,
$this->userSession
$this->userSession,
$this->knownUserService
);
$this->assertSame($expected,
$manager->shareProviderExists($shareType)
Expand Down Expand Up @@ -4166,7 +4176,8 @@ public function testGetSharesInFolder() {
$this->urlGenerator,
$this->defaults,
$this->dispatcher,
$this->userSession
$this->userSession,
$this->knownUserService
);

$factory->setProvider($this->defaultProvider);
Expand Down Expand Up @@ -4231,7 +4242,8 @@ public function testGetAccessList() {
$this->urlGenerator,
$this->defaults,
$this->dispatcher,
$this->userSession
$this->userSession,
$this->knownUserService
);

$factory->setProvider($this->defaultProvider);
Expand Down Expand Up @@ -4348,7 +4360,8 @@ public function testGetAccessListWithCurrentAccess() {
$this->urlGenerator,
$this->defaults,
$this->dispatcher,
$this->userSession
$this->userSession,
$this->knownUserService
);

$factory->setProvider($this->defaultProvider);
Expand Down Expand Up @@ -4474,7 +4487,8 @@ public function testGetAllShares() {
$this->urlGenerator,
$this->defaults,
$this->dispatcher,
$this->userSession
$this->userSession,
$this->knownUserService
);

$factory->setProvider($this->defaultProvider);
Expand Down