diff --git a/apps/files_sharing/api/share20ocs.php b/apps/files_sharing/api/share20ocs.php index f81eb071a3..c698dccefb 100644 --- a/apps/files_sharing/api/share20ocs.php +++ b/apps/files_sharing/api/share20ocs.php @@ -142,10 +142,20 @@ class Share20OCS { * @return \OC_OCS_Result */ public function getShare($id) { + // Try both our default, and our federated provider.. + $share = null; + + // First check if it is an internal share. try { - $share = $this->shareManager->getShareById($id); + $share = $this->shareManager->getShareById('ocinternal:'.$id); } catch (\OC\Share20\Exception\ShareNotFound $e) { - return new \OC_OCS_Result(null, 404, 'wrong share ID, share doesn\'t exist.'); + // Ignore for now + //return new \OC_OCS_Result(null, 404, 'wrong share ID, share doesn\'t exist.'); + } + + if ($share === null) { + //For now federated shares are handled by the old endpoint. + return \OCA\Files_Sharing\API\Local::getShare(['id' => $id]); } if ($this->canAccessShare($share)) { @@ -163,18 +173,19 @@ class Share20OCS { * @return \OC_OCS_Result */ public function deleteShare($id) { + // Try both our default and our federated provider + $share = null; + try { - $share = $this->shareManager->getShareById($id); + $share = $this->shareManager->getShareById('ocinternal:' . $id); } catch (\OC\Share20\Exception\ShareNotFound $e) { - return new \OC_OCS_Result(null, 404, 'wrong share ID, share doesn\'t exist.'); + //Ignore for now + //return new \OC_OCS_Result(null, 404, 'wrong share ID, share doesn\'t exist.'); } - /* - * FIXME - * User the old code path for remote shares until we have our remoteshareprovider - */ - if ($share->getShareType() === \OCP\Share::SHARE_TYPE_REMOTE) { - \OCA\Files_Sharing\API\Local::deleteShare(['id' => $id]); + // Could not find the share as internal share... maybe it is a federated share + if ($share === null) { + return \OCA\Files_Sharing\API\Local::deleteShare(['id' => $id]); } if (!$this->canAccessShare($share)) { diff --git a/apps/files_sharing/tests/api/share20ocstest.php b/apps/files_sharing/tests/api/share20ocstest.php index b594d253eb..81166b9e3c 100644 --- a/apps/files_sharing/tests/api/share20ocstest.php +++ b/apps/files_sharing/tests/api/share20ocstest.php @@ -82,7 +82,7 @@ class Share20OCSTest extends \Test\TestCase { $this->shareManager ->expects($this->once()) ->method('getShareById') - ->with(42) + ->with('ocinternal:42') ->will($this->throwException(new \OC\Share20\Exception\ShareNotFound())); $expected = new \OC_OCS_Result(null, 404, 'wrong share ID, share doesn\'t exist.'); @@ -95,7 +95,7 @@ class Share20OCSTest extends \Test\TestCase { $this->shareManager ->expects($this->once()) ->method('getShareById') - ->with(42) + ->with('ocinternal:42') ->willReturn($share); $this->shareManager ->expects($this->once()) @@ -114,7 +114,7 @@ class Share20OCSTest extends \Test\TestCase { $this->shareManager ->expects($this->once()) ->method('getShareById') - ->with(42) + ->with('ocinternal:42') ->willReturn($share); $this->shareManager ->expects($this->once()) @@ -125,16 +125,20 @@ class Share20OCSTest extends \Test\TestCase { $this->assertEquals($expected, $this->ocs->deleteShare(42)); } + /* + * FIXME: Enable once we have a federated Share Provider + public function testGetGetShareNotExists() { $this->shareManager ->expects($this->once()) ->method('getShareById') - ->with(42) + ->with('ocinternal:42') ->will($this->throwException(new \OC\Share20\Exception\ShareNotFound())); $expected = new \OC_OCS_Result(null, 404, 'wrong share ID, share doesn\'t exist.'); $this->assertEquals($expected, $this->ocs->getShare(42)); } + */ public function createShare($id, $shareType, $sharedWith, $sharedBy, $shareOwner, $path, $permissions, $shareTime, $expiration, $parent, $target, $mail_send, $token=null, @@ -155,6 +159,12 @@ class Share20OCSTest extends \Test\TestCase { $share->method('getToken')->willReturn($token); $share->method('getPassword')->willReturn($password); + if ($shareType === \OCP\Share::SHARE_TYPE_USER || + $shareType === \OCP\Share::SHARE_TYPE_GROUP || + $shareType === \OCP\Share::SHARE_TYPE_LINK) { + $share->method('getFullId')->willReturn('ocinternal:'.$id); + } + return $share; } @@ -345,7 +355,7 @@ class Share20OCSTest extends \Test\TestCase { $this->shareManager ->expects($this->once()) ->method('getShareById') - ->with($share->getId()) + ->with($share->getFullId()) ->willReturn($share); $userFolder = $this->getMock('OCP\Files\Folder'); diff --git a/lib/private/share20/defaultshareprovider.php b/lib/private/share20/defaultshareprovider.php index f961bc5202..60c3c4390f 100644 --- a/lib/private/share20/defaultshareprovider.php +++ b/lib/private/share20/defaultshareprovider.php @@ -29,6 +29,11 @@ use OCP\Files\IRootFolder; use OCP\IDBConnection; use OCP\Files\Node; +/** + * Class DefaultShareProvider + * + * @package OC\Share20 + */ class DefaultShareProvider implements IShareProvider { /** @var IDBConnection */ @@ -75,6 +80,15 @@ class DefaultShareProvider implements IShareProvider { ]; } + /** + * Return the identifier of this provider. + * + * @return string Containing only [a-zA-Z0-9] + */ + public function identifier() { + return 'ocinternal'; + } + /** * Share a path * @@ -355,6 +369,8 @@ class DefaultShareProvider implements IShareProvider { $share->setExpirationDate($expiration); } + $share->setProviderId($this->identifier()); + return $share; } diff --git a/lib/private/share20/ishare.php b/lib/private/share20/ishare.php index 53cfe76916..34d1dfa4d3 100644 --- a/lib/private/share20/ishare.php +++ b/lib/private/share20/ishare.php @@ -35,11 +35,34 @@ interface IShare { */ public function getId(); + /** + * Set the id of the share + * + * @param string $id + * @return IShare The modified share object + */ + public function setId($id); + + /** + * Get the full share id + * + * @return string + */ + public function getFullId(); + + /** + * Set the provider id + * + * @param string $id + * @return IShare The modified share object + */ + public function setProviderId($id); + /** * Set the path of this share * * @param Node $path - * @return Share The modified object + * @return IShare The modified object */ public function setPath(Node $path); @@ -54,7 +77,7 @@ interface IShare { * Set the shareType * * @param int $shareType - * @return Share The modified object + * @return IShare The modified object */ public function setShareType($shareType); @@ -69,7 +92,7 @@ interface IShare { * Set the receiver of this share * * @param IUser|IGroup|string - * @return Share The modified object + * @return IShare The modified object */ public function setSharedWith($sharedWith); @@ -84,7 +107,7 @@ interface IShare { * Set the permissions * * @param int $permissions - * @return Share The modified object + * @return IShare The modified object */ public function setPermissions($permissions); @@ -99,7 +122,7 @@ interface IShare { * Set the expiration date * * @param \DateTime $expireDate - * @return Share The modified object + * @return IShare The modified object */ public function setExpirationDate($expireDate); @@ -114,7 +137,7 @@ interface IShare { * Set the sharer of the path * * @param IUser|string $sharedBy - * @return Share The modified object + * @return IShare The modified object */ public function setSharedBy($sharedBy); @@ -130,7 +153,7 @@ interface IShare { * * @param IUser|string * - * @return Share The modified object + * @return IShare The modified object */ public function setShareOwner($shareOwner); @@ -146,7 +169,7 @@ interface IShare { * * @param string $password * - * @return Share The modified object + * @return IShare The modified object */ public function setPassword($password); @@ -161,7 +184,7 @@ interface IShare { * Set the token * * @param string $token - * @return Share The modified object + * @return IShare The modified object */ public function setToken($token); @@ -183,7 +206,7 @@ interface IShare { * Set the target of this share * * @param string $target - * @return Share The modified object + * @return IShare The modified object */ public function setTarget($target); diff --git a/lib/private/share20/ishareprovider.php b/lib/private/share20/ishareprovider.php index a7e9c59a97..e3a8e3c55b 100644 --- a/lib/private/share20/ishareprovider.php +++ b/lib/private/share20/ishareprovider.php @@ -33,6 +33,13 @@ interface IShareProvider { */ public function shareTypes(); + /** + * Return the identifier of this provider. + * + * @return string Containing only [a-zA-Z0-9] + */ + public function identifier(); + /** * Share a path * diff --git a/lib/private/share20/manager.php b/lib/private/share20/manager.php index e35b69f003..0784a53ecc 100644 --- a/lib/private/share20/manager.php +++ b/lib/private/share20/manager.php @@ -21,10 +21,12 @@ namespace OC\Share20; +use OC\Share20\Exception\BackendError; use OC\Share20\Exception\ProviderException; use OCP\IConfig; use OCP\IL10N; use OCP\ILogger; +use OCP\Preview\IProvider; use OCP\Security\ISecureRandom; use OCP\Security\IHasher; use OCP\Files\Mount\IMountManager; @@ -112,40 +114,45 @@ class Manager { */ private function addProviders(array $providers) { foreach ($providers as $provider) { - $name = get_class($provider); + $id = $provider->identifier(); + $class = get_class($provider); if (!($provider instanceof IShareProvider)) { - throw new ProviderException($name . ' is not an instance of IShareProvider'); + throw new ProviderException($class . ' is not an instance of IShareProvider'); } - if (isset($this->providers[$name])) { - throw new ProviderException($name . ' is already registered'); + if (isset($this->providers[$id])) { + throw new ProviderException($id . ' is already registered'); } - $this->providers[$name] = $provider; + $this->providers[$id] = $provider; $types = $provider->shareTypes(); if ($types === []) { - throw new ProviderException('Provider ' . $name . ' must supply share types it can handle'); + throw new ProviderException('Provider ' . $id . ' must supply share types it can handle'); } foreach ($types as $type) { if (isset($this->type2provider[$type])) { - throw new ProviderException($name . ' tried to register for share type ' . $type . ' but this type is already handled by ' . $this->type2provider[$type]); + throw new ProviderException($id . ' tried to register for share type ' . $type . ' but this type is already handled by ' . $this->type2provider[$type]); } - $this->type2provider[$type] = $name; + $this->type2provider[$type] = $id; } } } + /** + * Run the factory if this is not done yet + * + * @throws ProviderException + */ private function runFactory() { if (!empty($this->providers)) { return; } $this->addProviders($this->factory->getProviders()); - $this->factoryDone = true; } /** @@ -187,6 +194,16 @@ class Manager { return $this->providers; } + /** + * Convert from a full share id to a tuple (providerId, shareId) + * + * @param string $id + * @return string[] + */ + private function splitFullId($id) { + return explode(':', $id, 2); + } + /** * Verify if a password meets all requirements * @@ -335,7 +352,6 @@ class Manager { return $expireDate; } - /** * Check for pre share requirements for user shares * @@ -549,6 +565,7 @@ class Manager { $provider = $this->getProviderForType($share->getShareType()); $share = $provider->create($share); + $share->setProviderId($provider->identifier()); // Post share hook $postHookData = [ @@ -585,16 +602,14 @@ class Manager { protected function deleteChildren(IShare $share) { $deletedShares = []; - $providers = $this->getProviders(); + $provider = $this->getProviderForType($share->getShareType()); - foreach($providers as $provider) { - foreach ($provider->getChildren($share) as $child) { - $deletedChildren = $this->deleteChildren($child); - $deletedShares = array_merge($deletedShares, $deletedChildren); + foreach ($provider->getChildren($share) as $child) { + $deletedChildren = $this->deleteChildren($child); + $deletedShares = array_merge($deletedShares, $deletedChildren); - $provider->delete($child); - $deletedShares[] = $child; - } + $provider->delete($child); + $deletedShares[] = $child; } return $deletedShares; @@ -605,11 +620,12 @@ class Manager { * * @param IShare $share * @throws ShareNotFound - * @throws \OC\Share20\Exception\BackendError + * @throws BackendError + * @throws ShareNotFound */ public function deleteShare(IShare $share) { // Just to make sure we have all the info - $share = $this->getShareById($share->getId()); + $share = $this->getShareById($share->getFullId()); $formatHookParams = function(IShare $share) { // Prepare hook @@ -686,24 +702,11 @@ class Manager { throw new ShareNotFound(); } - //FIXME ids need to become proper providerid:shareid eventually + list($providerId, $id) = $this->splitFullId($id); + $provider = $this->getProvider($providerId); - $providers = $this->getProviders(); - - $share = null; - foreach ($providers as $provider) { - try { - $share = $provider->getShareById($id); - $found = true; - break; - } catch (ShareNotFound $e) { - // Ignore - } - } - - if ($share === null) { - throw new ShareNotFound(); - } + $share = $provider->getShareById($id); + $share->setProviderId($provider->identifier()); return $share; } diff --git a/lib/private/share20/share.php b/lib/private/share20/share.php index 2cd685b796..ee43725d9b 100644 --- a/lib/private/share20/share.php +++ b/lib/private/share20/share.php @@ -28,6 +28,8 @@ class Share implements IShare { /** @var string */ private $id; + /** @var string */ + private $providerId; /** @var Node */ private $path; /** @var int */ @@ -59,7 +61,7 @@ class Share implements IShare { * Set the id of the share * * @param string $id - * @return Share The modified object + * @return IShare The modified object */ public function setId($id) { $this->id = $id; @@ -75,11 +77,26 @@ class Share implements IShare { return $this->id; } + /** + * @inheritdoc + */ + public function getFullId() { + return $this->providerId . ':' . $this->id; + } + + /** + * @inheritdoc + */ + public function setProviderId($id) { + $this->providerId = $id; + return $this; + } + /** * Set the path of this share * * @param Node $path - * @return Share The modified object + * @return IShare The modified object */ public function setPath(Node $path) { $this->path = $path; @@ -99,7 +116,7 @@ class Share implements IShare { * Set the shareType * * @param int $shareType - * @return Share The modified object + * @return IShare The modified object */ public function setShareType($shareType) { $this->shareType = $shareType; @@ -119,7 +136,7 @@ class Share implements IShare { * Set the receiver of this share * * @param IUser|IGroup|string - * @return Share The modified object + * @return IShare The modified object */ public function setSharedWith($sharedWith) { $this->sharedWith = $sharedWith; @@ -139,7 +156,7 @@ class Share implements IShare { * Set the permissions * * @param int $permissions - * @return Share The modified object + * @return IShare The modified object */ public function setPermissions($permissions) { //TODO checkes @@ -161,7 +178,7 @@ class Share implements IShare { * Set the expiration date * * @param \DateTime $expireDate - * @return Share The modified object + * @return IShare The modified object */ public function setExpirationDate($expireDate) { //TODO checks @@ -183,7 +200,7 @@ class Share implements IShare { * Set the sharer of the path * * @param IUser|string $sharedBy - * @return Share The modified object + * @return IShare The modified object */ public function setSharedBy($sharedBy) { //TODO checks @@ -207,7 +224,7 @@ class Share implements IShare { * * @param IUser|string * - * @return Share The modified object + * @return IShare The modified object */ public function setShareOwner($shareOwner) { //TODO checks @@ -231,7 +248,7 @@ class Share implements IShare { * * @param string $password * - * @return Share The modified object + * @return IShare The modified object */ public function setPassword($password) { //TODO verify @@ -253,7 +270,7 @@ class Share implements IShare { * Set the token * * @param string $token - * @return Share The modified object + * @return IShare The modified object */ public function setToken($token) { $this->token = $token; @@ -273,7 +290,7 @@ class Share implements IShare { * Set the parent id of this share * * @param int $parent - * @return Share The modified object + * @return IShare The modified object */ public function setParent($parent) { $this->parent = $parent; @@ -293,7 +310,7 @@ class Share implements IShare { * Set the target of this share * * @param string $target - * @return Share The modified object + * @return IShare The modified object */ public function setTarget($target) { $this->target = $target; @@ -313,7 +330,7 @@ class Share implements IShare { * Set the time this share was created * * @param int $shareTime - * @return Share The modified object + * @return IShare The modified object */ public function setShareTime($shareTime) { $this->shareTime = $shareTime; @@ -333,7 +350,7 @@ class Share implements IShare { * Set mailSend * * @param bool $mailSend - * @return Share The modified object + * @return IShare The modified object */ public function setMailSend($mailSend) { $this->mailSend = $mailSend; diff --git a/tests/lib/share20/defaultshareprovidertest.php b/tests/lib/share20/defaultshareprovidertest.php index beef4c9ef5..166fbd25f1 100644 --- a/tests/lib/share20/defaultshareprovidertest.php +++ b/tests/lib/share20/defaultshareprovidertest.php @@ -589,6 +589,7 @@ class DefaultShareProviderTest extends \Test\TestCase { $share2 = $this->provider->create($share); $this->assertNotNull($share2->getId()); + $this->assertSame('ocinternal:'.$share2->getId(), $share2->getFullId()); $this->assertSame(\OCP\Share::SHARE_TYPE_USER, $share2->getShareType()); $this->assertSame($sharedWith, $share2->getSharedWith()); $this->assertSame($sharedBy, $share2->getSharedBy()); @@ -651,6 +652,7 @@ class DefaultShareProviderTest extends \Test\TestCase { $share2 = $this->provider->create($share); $this->assertNotNull($share2->getId()); + $this->assertSame('ocinternal:'.$share2->getId(), $share2->getFullId()); $this->assertSame(\OCP\Share::SHARE_TYPE_GROUP, $share2->getShareType()); $this->assertSame($sharedWith, $share2->getSharedWith()); $this->assertSame($sharedBy, $share2->getSharedBy()); @@ -710,6 +712,7 @@ class DefaultShareProviderTest extends \Test\TestCase { $share2 = $this->provider->create($share); $this->assertNotNull($share2->getId()); + $this->assertSame('ocinternal:'.$share2->getId(), $share2->getFullId()); $this->assertSame(\OCP\Share::SHARE_TYPE_LINK, $share2->getShareType()); $this->assertSame($sharedBy, $share2->getSharedBy()); $this->assertSame($shareOwner, $share2->getShareOwner()); diff --git a/tests/lib/share20/managertest.php b/tests/lib/share20/managertest.php index dd1028c78b..93e4bbb2f6 100644 --- a/tests/lib/share20/managertest.php +++ b/tests/lib/share20/managertest.php @@ -109,6 +109,7 @@ class ManagerTest extends \Test\TestCase { \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_LINK, ]); + $this->defaultProvider->method('identifier')->willReturn('default'); } /** @@ -155,6 +156,7 @@ class ManagerTest extends \Test\TestCase { $provider = $provider->getMock(); $provider->method('shareTypes')->willReturn($shareTypes); + $provider->method('identifier')->willReturn(get_class($provider)); return $provider; } @@ -169,7 +171,7 @@ class ManagerTest extends \Test\TestCase { $share ->expects($this->once()) - ->method('getId') + ->method('getFullId') ->with() ->willReturn(null); @@ -204,7 +206,7 @@ class ManagerTest extends \Test\TestCase { \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_LINK, \OCP\Share::SHARE_TYPE_REMOTE, - ]); + ], 'prov'); $this->factoryProviders([$provider]); $sharedBy = $this->getMock('\OCP\IUser'); @@ -215,13 +217,14 @@ class ManagerTest extends \Test\TestCase { $share = $this->getMock('\OC\Share20\IShare'); $share->method('getId')->willReturn(42); + $share->method('getFullId')->willReturn('prov:42'); $share->method('getShareType')->willReturn($shareType); $share->method('getSharedWith')->willReturn($sharedWith); $share->method('getSharedBy')->willReturn($sharedBy); $share->method('getPath')->willReturn($path); $share->method('getTarget')->willReturn('myTarget'); - $manager->expects($this->once())->method('getShareById')->with(42)->willReturn($share); + $manager->expects($this->once())->method('getShareById')->with('prov:42')->willReturn($share); $manager->expects($this->once())->method('deleteChildren')->with($share); $provider @@ -288,7 +291,11 @@ class ManagerTest extends \Test\TestCase { ->setMethods(['getShareById']) ->getMock(); - $provider = $this->createShareProvider([\OCP\Share::SHARE_TYPE_USER]); + $provider = $this->createShareProvider([ + \OCP\Share::SHARE_TYPE_USER, + \OCP\Share::SHARE_TYPE_GROUP, + \OCP\Share::SHARE_TYPE_LINK, + ], 'prov'); $this->factoryProviders([$provider]); $sharedBy1 = $this->getMock('\OCP\IUser'); @@ -308,6 +315,7 @@ class ManagerTest extends \Test\TestCase { $share1 = $this->getMock('\OC\Share20\IShare'); $share1->method('getId')->willReturn(42); + $share1->method('getFullId')->willReturn('prov:42'); $share1->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_USER); $share1->method('getSharedWith')->willReturn($sharedWith1); $share1->method('getSharedBy')->willReturn($sharedBy1); @@ -316,6 +324,7 @@ class ManagerTest extends \Test\TestCase { $share2 = $this->getMock('\OC\Share20\IShare'); $share2->method('getId')->willReturn(43); + $share2->method('getFullId')->willReturn('prov:43'); $share2->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_GROUP); $share2->method('getSharedWith')->willReturn($sharedWith2); $share2->method('getSharedBy')->willReturn($sharedBy2); @@ -325,13 +334,14 @@ class ManagerTest extends \Test\TestCase { $share3 = $this->getMock('\OC\Share20\IShare'); $share3->method('getId')->willReturn(44); + $share3->method('getFullId')->willReturn('prov:44'); $share3->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_LINK); $share3->method('getSharedBy')->willReturn($sharedBy3); $share3->method('getPath')->willReturn($path); $share3->method('getTarget')->willReturn('myTarget3'); $share3->method('getParent')->willReturn(43); - $manager->expects($this->once())->method('getShareById')->with(42)->willReturn($share1); + $manager->expects($this->once())->method('getShareById')->with('prov:42')->willReturn($share1); $provider ->method('getChildren') @@ -408,7 +418,6 @@ class ManagerTest extends \Test\TestCase { ], ]; - $hookListner ->expects($this->exactly(1)) ->method('pre') @@ -430,10 +439,14 @@ class ManagerTest extends \Test\TestCase { $this->factoryProviders([$provider]); $share = $this->getMock('\OC\Share20\IShare'); + $share->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_USER); $child1 = $this->getMock('\OC\Share20\IShare'); + $child1->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_USER); $child2 = $this->getMock('\OC\Share20\IShare'); + $child2->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_USER); $child3 = $this->getMock('\OC\Share20\IShare'); + $child3->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_USER); $shares = [ $child1, @@ -471,7 +484,7 @@ class ManagerTest extends \Test\TestCase { ->with(42) ->willReturn($share); - $this->assertEquals($share, $this->manager->getShareById(42)); + $this->assertEquals($share, $this->manager->getShareById('default:42')); } /**