Skip to content

Commit

Permalink
Migrate files sharing to PSR LoggerInterface
Browse files Browse the repository at this point in the history
Signed-off-by: Benjamin Gaussorgues <[email protected]>
  • Loading branch information
Altahrim committed Jul 6, 2023
1 parent 79d24bf commit 3aab8c3
Show file tree
Hide file tree
Showing 7 changed files with 62 additions and 144 deletions.
22 changes: 7 additions & 15 deletions apps/files_sharing/lib/Controller/RemoteController.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,10 @@
use OCP\AppFramework\OCS\OCSForbiddenException;
use OCP\AppFramework\OCS\OCSNotFoundException;
use OCP\AppFramework\OCSController;
use OCP\ILogger;
use OCP\IRequest;
use Psr\Log\LoggerInterface;

class RemoteController extends OCSController {

/** @var Manager */
private $externalManager;

/** @var ILogger */
private $logger;

/**
* @NoAdminRequired
*
Expand All @@ -48,14 +41,13 @@ class RemoteController extends OCSController {
* @param IRequest $request
* @param Manager $externalManager
*/
public function __construct($appName,
IRequest $request,
Manager $externalManager,
ILogger $logger) {
public function __construct(
$appName,
IRequest $request,
private Manager $externalManager,
private LoggerInterface $logger,
) {
parent::__construct($appName, $request);

$this->externalManager = $externalManager;
$this->logger = $logger;
}

/**
Expand Down
73 changes: 17 additions & 56 deletions apps/files_sharing/lib/Controller/ShareController.php
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,9 @@
use OCA\Files_Sharing\Activity\Providers\Downloads;
use OCA\Files_Sharing\Event\BeforeTemplateRenderedEvent;
use OCA\Files_Sharing\Event\ShareLinkAccessedEvent;
use OCA\Viewer\Event\LoadViewer;
use OCP\Accounts\IAccountManager;
use OCP\AppFramework\AuthPublicShareController;
use OCP\AppFramework\Http\NotFoundResponse;
use OCP\AppFramework\Http\Template\ExternalShareMenuAction;
use OCP\AppFramework\Http\Template\LinkMenuAction;
use OCP\AppFramework\Http\Template\PublicTemplateResponse;
use OCP\AppFramework\Http\Template\SimpleMenuAction;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\Defaults;
use OCP\EventDispatcher\IEventDispatcher;
Expand All @@ -65,12 +60,10 @@
use OCP\Files\NotFoundException;
use OCP\IConfig;
use OCP\IL10N;
use OCP\ILogger;
use OCP\IPreview;
use OCP\IRequest;
use OCP\ISession;
use OCP\IURLGenerator;
use OCP\IUser;
use OCP\IUserManager;
use OCP\Security\ISecureRandom;
use OCP\Share;
Expand All @@ -86,64 +79,34 @@
* @package OCA\Files_Sharing\Controllers
*/
class ShareController extends AuthPublicShareController {
protected IConfig $config;
protected IUserManager $userManager;
protected ILogger $logger;
protected \OCP\Activity\IManager $activityManager;
protected IPreview $previewManager;
protected IRootFolder $rootFolder;
protected FederatedShareProvider $federatedShareProvider;
protected IAccountManager $accountManager;
protected IEventDispatcher $eventDispatcher;
protected IL10N $l10n;
protected Defaults $defaults;
protected ShareManager $shareManager;
protected ISecureRandom $secureRandom;
protected ?Share\IShare $share = null;
private IPublicShareTemplateFactory $publicShareTemplateFactory;

public const SHARE_ACCESS = 'access';
public const SHARE_AUTH = 'auth';
public const SHARE_DOWNLOAD = 'download';

public function __construct(
string $appName,
IRequest $request,
IConfig $config,
protected IConfig $config,
IURLGenerator $urlGenerator,
IUserManager $userManager,
ILogger $logger,
\OCP\Activity\IManager $activityManager,
ShareManager $shareManager,
protected IUserManager $userManager,
protected \OCP\Activity\IManager $activityManager,
protected ShareManager $shareManager,
ISession $session,
IPreview $previewManager,
IRootFolder $rootFolder,
FederatedShareProvider $federatedShareProvider,
IAccountManager $accountManager,
IEventDispatcher $eventDispatcher,
IL10N $l10n,
ISecureRandom $secureRandom,
Defaults $defaults,
IPublicShareTemplateFactory $publicShareTemplateFactory
protected IPreview $previewManager,
protected IRootFolder $rootFolder,
protected FederatedShareProvider $federatedShareProvider,
protected IAccountManager $accountManager,
protected IEventDispatcher $eventDispatcher,
protected IL10N $l10n,
protected ISecureRandom $secureRandom,
protected Defaults $defaults,
private IPublicShareTemplateFactory $publicShareTemplateFactory,
) {
parent::__construct($appName, $request, $session, $urlGenerator);

$this->config = $config;
$this->userManager = $userManager;
$this->logger = $logger;
$this->activityManager = $activityManager;
$this->previewManager = $previewManager;
$this->rootFolder = $rootFolder;
$this->federatedShareProvider = $federatedShareProvider;
$this->accountManager = $accountManager;
$this->eventDispatcher = $eventDispatcher;
$this->l10n = $l10n;
$this->secureRandom = $secureRandom;
$this->defaults = $defaults;
$this->shareManager = $shareManager;
$this->publicShareTemplateFactory = $publicShareTemplateFactory;
}

public const SHARE_ACCESS = 'access';
public const SHARE_AUTH = 'auth';
public const SHARE_DOWNLOAD = 'download';

/**
* @PublicPage
* @NoCSRFRequired
Expand Down Expand Up @@ -212,7 +175,6 @@ protected function showIdentificationResult(bool $success = false): TemplateResp
* @return bool
*/
protected function validateIdentity(?string $identityToken = null): bool {

if ($this->share->getShareType() !== IShare::TYPE_EMAIL) {
return false;
}
Expand Down Expand Up @@ -496,7 +458,6 @@ public function downloadShare($token, $files = null, $path = '', $downloadStartS
if (!empty($downloadStartSecret)
&& !isset($downloadStartSecret[32])
&& preg_match('!^[a-zA-Z0-9]+$!', $downloadStartSecret) === 1) {

// FIXME: set on the response once we use an actual app framework response
setcookie('ocDownloadStarted', $downloadStartSecret, time() + 20, '/');

Check failure on line 462 in apps/files_sharing/lib/Controller/ShareController.php

View workflow job for this annotation

GitHub Actions / static-code-analysis-security

TaintedCookie

apps/files_sharing/lib/Controller/ShareController.php:462:35: TaintedCookie: Detected tainted cookie (see https://psalm.dev/257)
}
Expand Down
22 changes: 6 additions & 16 deletions apps/files_sharing/lib/Listener/ShareInteractionListener.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@
use OCP\EventDispatcher\Event;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\EventDispatcher\IEventListener;
use OCP\ILogger;
use OCP\IUserManager;
use OCP\Share\Events\ShareCreatedEvent;
use OCP\Share\IShare;
use Psr\Log\LoggerInterface;
use function in_array;

class ShareInteractionListener implements IEventListener {
Expand All @@ -43,21 +43,11 @@ class ShareInteractionListener implements IEventListener {
IShare::TYPE_REMOTE,
];

/** @var IEventDispatcher */
private $dispatcher;

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

/** @var ILogger */
private $logger;

public function __construct(IEventDispatcher $dispatcher,
IUserManager $userManager,
ILogger $logger) {
$this->dispatcher = $dispatcher;
$this->userManager = $userManager;
$this->logger = $logger;
public function __construct(
private IEventDispatcher $dispatcher,
private IUserManager $userManager,
private LoggerInterface $logger,
) {
}

public function handle(Event $event): void {
Expand Down
55 changes: 17 additions & 38 deletions apps/files_sharing/lib/MountProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,54 +36,26 @@
use OCP\Files\Storage\IStorageFactory;
use OCP\ICacheFactory;
use OCP\IConfig;
use OCP\ILogger;
use OCP\IUser;
use OCP\Share\IAttributes;
use OCP\Share\IManager;
use OCP\Share\IShare;
use Psr\Log\LoggerInterface;

class MountProvider implements IMountProvider {
/**
* @var \OCP\IConfig
*/
protected $config;

/**
* @var IManager
*/
protected $shareManager;

/**
* @var ILogger
*/
protected $logger;

/** @var IEventDispatcher */
protected $eventDispatcher;

/** @var ICacheFactory */
protected $cacheFactory;

/**
* @param \OCP\IConfig $config
* @param IManager $shareManager
* @param ILogger $logger
* @param LoggerInterface $logger
*/
public function __construct(
IConfig $config,
IManager $shareManager,
ILogger $logger,
IEventDispatcher $eventDispatcher,
ICacheFactory $cacheFactory
protected IConfig $config,
protected IManager $shareManager,
protected LoggerInterface $logger,
protected IEventDispatcher $eventDispatcher,
protected ICacheFactory $cacheFactory
) {
$this->config = $config;
$this->shareManager = $shareManager;
$this->logger = $logger;
$this->eventDispatcher = $eventDispatcher;
$this->cacheFactory = $cacheFactory;
}


/**
* Get all mountpoints applicable for the user and check for shares where we need to update the etags
*
Expand Down Expand Up @@ -157,8 +129,13 @@ public function getMountsForUser(IUser $user, IStorageFactory $loader) {
$mounts[$additionalMount->getMountPoint()] = $additionalMount;
}
} catch (\Exception $e) {
$this->logger->logException($e);
$this->logger->error('Error while trying to create shared mount');
$this->logger->error(
'Error while trying to create shared mount',
[
'app' => 'files_sharing',
'exception' => $e,
],
);
}
}

Expand Down Expand Up @@ -234,7 +211,9 @@ private function buildSuperShares(array $allShares, \OCP\IUser $user) {
// Since these are readly available here, storing them
// enables the DAV FilesPlugin to avoid executing many
// DB queries to retrieve the same information.
$allNotes = implode("\n", array_map(function ($sh) { return $sh->getNote(); }, $shares));
$allNotes = implode("\n", array_map(function ($sh) {
return $sh->getNote();
}, $shares));
$superShare->setNote($allNotes);

// use most permissive permissions
Expand Down
14 changes: 6 additions & 8 deletions apps/files_sharing/lib/SharedStorage.php
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
use OCP\Files\Storage\IStorage;
use OCP\Lock\ILockingProvider;
use OCP\Share\IShare;
use Psr\Log\LoggerInterface;

/**
* Convert target path to source path and pass the function call to the correct storage provider
Expand All @@ -80,11 +81,6 @@ class SharedStorage extends \OC\Files\Storage\Wrapper\Jail implements ISharedSto
/** @var string */
private $user;

/**
* @var \OCP\ILogger
*/
private $logger;

/** @var IStorage */
private $nonMaskedStorage;

Expand All @@ -98,9 +94,11 @@ class SharedStorage extends \OC\Files\Storage\Wrapper\Jail implements ISharedSto

private string $sourcePath = '';

public function __construct($arguments) {
public function __construct(
array $arguments,

Check failure on line 98 in apps/files_sharing/lib/SharedStorage.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

ConstructorSignatureMismatch

apps/files_sharing/lib/SharedStorage.php:98:9: ConstructorSignatureMismatch: Argument 1 of OCA\Files_Sharing\SharedStorage::__construct has wrong type 'array<array-key, mixed>', expecting '' as defined by OCP\Files\Storage\IStorage::__construct (see https://psalm.dev/231)

Check failure on line 98 in apps/files_sharing/lib/SharedStorage.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

ConstructorSignatureMismatch

apps/files_sharing/lib/SharedStorage.php:98:9: ConstructorSignatureMismatch: Argument 1 of OCA\Files_Sharing\SharedStorage::__construct has wrong type 'array<array-key, mixed>', expecting '' as defined by OCP\Files\Storage::__construct (see https://psalm.dev/231)

Check failure

Code scanning / Psalm

ConstructorSignatureMismatch Error

Argument 1 of OCA\Files_Sharing\SharedStorage::__construct has wrong type 'array<array-key, mixed>', expecting '' as defined by OCP\Files\Storage\IStorage::__construct

Check failure

Code scanning / Psalm

ConstructorSignatureMismatch Error

Argument 1 of OCA\Files_Sharing\SharedStorage::__construct has wrong type 'array<array-key, mixed>', expecting '' as defined by OCP\Files\Storage::__construct
private LoggerInterface $logger,
) {
$this->ownerView = $arguments['ownerView'];
$this->logger = \OC::$server->getLogger();

$this->superShare = $arguments['superShare'];
$this->groupedShares = $arguments['groupedShares'];
Expand Down Expand Up @@ -173,7 +171,7 @@ private function init() {
$this->storage = new FailedStorage(['exception' => $e]);
$this->cache = new FailedCache();
$this->rootPath = '';
$this->logger->logException($e);
$this->logger->error($e->getMessage(), ['exception' => $e]);
}

if (!$this->nonMaskedStorage) {
Expand Down
5 changes: 2 additions & 3 deletions apps/files_sharing/tests/Controller/ShareControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@
use OCP\Files\Storage;
use OCP\IConfig;
use OCP\IL10N;
use OCP\ILogger;
use OCP\IPreview;
use OCP\IRequest;
use OCP\ISession;
Expand All @@ -71,14 +70,14 @@
use OC\Share20\Manager;
use OCA\Files_Sharing\DefaultPublicShareTemplateProvider;
use OCP\Share\IPublicShareTemplateFactory;
use Psr\Log\LoggerInterface;

/**
* @group DB
*
* @package OCA\Files_Sharing\Controllers
*/
class ShareControllerTest extends \Test\TestCase {

/** @var string */
private $user;
/** @var string */
Expand Down Expand Up @@ -160,7 +159,7 @@ protected function setUp(): void {
$this->config,
$this->urlGenerator,
$this->userManager,
$this->createMock(ILogger::class),
$this->createMock(LoggerInterface::class),
$this->createMock(IManager::class),
$this->shareManager,
$this->session,
Expand Down
Loading

0 comments on commit 3aab8c3

Please sign in to comment.