Delete the correct expired share and handle user/group shares on ExpiredSharesJob by karakayasemi · Pull Request #37729 · owncloud/core

karakayasemi · 2020-07-26T13:01:24Z

Description

Delete the correct expired share by using share id on ExpireSharesJob. ExpireSharesJob now also handles user and group shares.

I created the pr based on @pako81 in https://github.com/owncloud/enterprise/issues/4112#issuecomment-662842454.

Related Issue

Fixes https://github.com/owncloud/enterprise/issues/4112

Motivation and Context

ExpiredSharesJobs currently uses \OCP\Share::unshare method for deleting expired shares.
If multiple links exist for the same node and, non-expired share created before expired share, expiredSharesJob currently deletes non-expired share.

This problem resolved with deleting correct expired shares by using share id.

Also, ExpireSharesJob now handles user and group shares. If we prefer, I can separate this part of the change to another pr.

How Has This Been Tested?

The class has an integration test. Also, tried manually.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Database schema changes (next release will require increase of minor version instead of patch)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests only (no source changes)

Checklist:

update-docs · 2020-07-26T13:01:28Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

codecov · 2020-07-26T14:42:30Z

Codecov Report

Merging #37729 into master will decrease coverage by 0.00%.
The diff coverage is 87.50%.

@@             Coverage Diff              @@
##             master   #37729      +/-   ##
============================================
- Coverage     64.71%   64.71%   -0.01%     
- Complexity    19386    19387       +1     
============================================
  Files          1283     1283              
  Lines         75730    75731       +1     
  Branches       1333     1333              
============================================
  Hits          49008    49008              
- Misses        26330    26331       +1     
  Partials        392      392

Flag	Coverage Δ	Complexity Δ
#javascript	`54.03% <ø> (ø)`	`0.00 <ø> (ø)`
#phpunit	`65.89% <87.50%> (-0.01%)`	`19387.00 <1.00> (+1.00)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Share20/Manager.php	`96.19% <ø> (ø)`	`273.00 <0.00> (ø)`
apps/files_sharing/lib/ExpireSharesJob.php	`95.45% <87.50%> (-4.55%)`	`4.00 <1.00> (+1.00)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f447f0...5f9cdc9. Read the comment docs.

apps/files_sharing/lib/ExpireSharesJob.php

jvillafanez · 2020-07-27T07:21:38Z

apps/files_sharing/lib/ExpireSharesJob.php

 		while ($share = $shares->fetch()) {
-			\OCP\Share::unshare($share['item_type'], $share['file_source'], \OCP\Share::SHARE_TYPE_LINK, null, $share['uid_owner']);
+			try {
+				$share = $this->shareManager->getShareById('ocinternal:' . $share['id']);


Any alternative to avoid using "ocinternal:"? It shouldn't be known from the outside.
If it isn't possible, we should include a comment so we could find and adjust this piece of code in case the identifier changes.

Ideally, we need to inject \OCP\Share\IProviderFactory and get the correct share provider for share_type from it. However, IProviderFactory can not auto-injectable and manually creating it a bit tricky. See

core/lib/private/Server.php

Lines 877 to 880 in 6038c8e

$config = $c->getConfig();

$factoryClass = $config->getSystemValue('sharing.managerFactory', '\OC\Share20\ProviderFactory');

/** @var \OCP\Share\IProviderFactory $factory */

$factory = new $factoryClass($this);

IMHO, for now, using the default share provider is the best option even it has \OC namespace.

micbar · 2020-07-27T08:42:31Z

@karakayasemi Can you add a changelog plse?

karakayasemi · 2020-07-27T18:03:32Z

Changelog has been added. ~~@jvillafanez ready to review again.~~ Some tests are failed in Postgres. I will look at it.

jvillafanez · 2020-07-28T06:49:43Z

Code looks good 👍

karakayasemi · 2020-07-28T21:32:10Z

share id is coming as integer in a pgsql database, but, it is coming as string in all the other supported databases. Because of that, returnValueMap did not work in Postgres and Postgres tests failed.

The failing tests opened a can of worms about share id inconsistency. Share id is supposed to be a string according to its interface description. However, IShareProvider expects share id as integer, in fact, its implementations also cast share id to integer when they setting id to any IShare instance. In contrast, setId method of Share implementation casts incoming integer argument to string.

core/lib/private/Share20/Share.php

Lines 95 to 97 in 378d38a

    
           if (\is_int($id)) { 
        
           	$id = (string)$id; 
        
           }

So, I corrected IShareProvider's getShareById method description and removed the unnecessary casting operation from IShareProvider implementations.

codecov · 2020-07-28T22:29:20Z

Codecov Report

Merging #37729 into master will decrease coverage by 0.00%.
The diff coverage is 90.90%.

@@             Coverage Diff              @@
##             master   #37729      +/-   ##
============================================
- Coverage     64.71%   64.71%   -0.01%     
- Complexity    19386    19387       +1     
============================================
  Files          1283     1283              
  Lines         75732    75734       +2     
  Branches       1333     1333              
============================================
+ Hits          49009    49010       +1     
- Misses        26331    26332       +1     
  Partials        392      392

Flag	Coverage Δ	Complexity Δ
#javascript	`54.03% <ø> (ø)`	`0.00 <ø> (ø)`
#phpunit	`65.89% <90.90%> (-0.01%)`	`19387.00 <1.00> (+1.00)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Share20/Manager.php	`96.19% <ø> (ø)`	`273.00 <0.00> (ø)`
apps/files_sharing/lib/ExpireSharesJob.php	`95.65% <88.88%> (-4.35%)`	`4.00 <1.00> (+1.00)`	⬇️
...ederatedfilesharing/lib/FederatedShareProvider.php	`63.36% <100.00%> (ø)`	`94.00 <0.00> (ø)`
lib/private/Share20/DefaultShareProvider.php	`97.74% <100.00%> (ø)`	`120.00 <0.00> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c19d083...8afe84d. Read the comment docs.

codecov · 2020-07-28T22:37:47Z

Codecov Report

Merging #37729 into master will decrease coverage by 0.00%.
The diff coverage is 90.90%.

@@             Coverage Diff              @@
##             master   #37729      +/-   ##
============================================
- Coverage     64.71%   64.71%   -0.01%     
- Complexity    19386    19387       +1     
============================================
  Files          1283     1283              
  Lines         75732    75734       +2     
  Branches       1333     1333              
============================================
+ Hits          49009    49010       +1     
- Misses        26331    26332       +1     
  Partials        392      392

Flag	Coverage Δ	Complexity Δ
#javascript	`54.03% <ø> (ø)`	`0.00 <ø> (ø)`
#phpunit	`65.89% <90.90%> (-0.01%)`	`19387.00 <1.00> (+1.00)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Share20/Manager.php	`96.19% <ø> (ø)`	`273.00 <0.00> (ø)`
apps/files_sharing/lib/ExpireSharesJob.php	`95.65% <88.88%> (-4.35%)`	`4.00 <1.00> (+1.00)`	⬇️
...ederatedfilesharing/lib/FederatedShareProvider.php	`63.36% <100.00%> (ø)`	`94.00 <0.00> (ø)`
lib/private/Share20/DefaultShareProvider.php	`97.74% <100.00%> (ø)`	`120.00 <0.00> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c19d083...8afe84d. Read the comment docs.

codecov · 2020-07-28T22:43:34Z

Codecov Report

Merging #37729 into master will decrease coverage by 0.00%.
The diff coverage is 90.90%.

@@             Coverage Diff              @@
##             master   #37729      +/-   ##
============================================
- Coverage     64.71%   64.71%   -0.01%     
- Complexity    19386    19387       +1     
============================================
  Files          1283     1283              
  Lines         75735    75737       +2     
  Branches       1333     1333              
============================================
+ Hits          49011    49012       +1     
- Misses        26332    26333       +1     
  Partials        392      392

Flag	Coverage Δ	Complexity Δ
#javascript	`54.03% <ø> (ø)`	`0.00 <ø> (ø)`
#phpunit	`65.89% <90.90%> (-0.01%)`	`19387.00 <1.00> (+1.00)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Share20/Manager.php	`96.19% <ø> (ø)`	`273.00 <0.00> (ø)`
apps/files_sharing/lib/ExpireSharesJob.php	`95.65% <88.88%> (-4.35%)`	`4.00 <1.00> (+1.00)`	⬇️
...ederatedfilesharing/lib/FederatedShareProvider.php	`63.36% <100.00%> (ø)`	`94.00 <0.00> (ø)`
lib/private/Share20/DefaultShareProvider.php	`97.74% <100.00%> (ø)`	`120.00 <0.00> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24b305a...c58fe67. Read the comment docs.

jvillafanez · 2020-07-29T08:40:13Z

Could you double-check the type of the DB column in postgresql? At least in mysql, I see the "id" as int column.

I guess this works due to how we're using it, not due to it's properly implemented:

Doctrine is likely fetching an integer, which is stored as string in the object because of the type checking done in the setId method of the share.
We aren't actively setting any id to any share. We're only set the id when we create the object from the DB.
There are checks in place to prevent changing the id after being set.

My worry is the interaction being the share object and the DB. If the object has a string but the DB requires an integer, it's either Doctrine or the actual DB the one making an automatic type conversion.

karakayasemi · 2020-07-29T18:49:11Z

apps/federatedfilesharing/lib/FederatedShareProvider.php

 	 */
 	private function createShareObject($data) {
 		$share = new Share($this->rootFolder, $this->userManager);
-		$share->setId((int)$data['id'])


setId expects string. Casting the parameter to integer does not make sense in any case here.

karakayasemi · 2020-07-29T18:52:44Z

lib/public/Share/IShareProvider.php

 	 * Get share by id
 	 *
-	 * @param int $id
+	 * @param string $id


I can revert this change and cast incoming share id to an integer when I call this method in ExpireSharesJob to ensure consistency between databases.

karakayasemi · 2020-07-29T18:54:10Z

Share id is integer in Postgres like Mysql also. But, when we query this column, DBAL converts integer id's to string on all the supported databases except Postgres, probably due to possible overflow reasons. I checked one more time. Since IShare getId method always returns string, Phpunit's returnValueMap method configuration did not work for Postgres.

jvillafanez · 2020-07-30T08:09:27Z

I think this only happens here. I mean, we're using the ids from other tables in other places and it works fine I guess.
I'm fine with the fix, but include a comment explaining the situation, specially mentioning mysql and postgresql, so we can remember this problem in the future and prevent breaking the same thing again. In addition, it would make easier to know what we need to check if we touch something around this.

karakayasemi · 2020-07-30T08:36:40Z

This id string/integer inconcistency more likely happen in other tables but doctrine/dbal handling these cases without problem. I guess, there is no other test case that configures a stub method with id parameter comes from database. That's why we did not notice before.

@jvillafanez, thank you for review. I added a comment that explain why we used type casting.

…. ExpireSharesJob now also handles user and group shares.

karakayasemi · 2020-07-30T11:35:19Z

Project coverage was not enough. Added a test to increase coverage.

karakayasemi added the 2 - Developing label Jul 26, 2020

karakayasemi added this to the development milestone Jul 26, 2020

karakayasemi self-assigned this Jul 26, 2020

karakayasemi marked this pull request as ready for review July 26, 2020 13:54

karakayasemi requested review from jvillafanez, mrow4a and pako81 July 26, 2020 13:54

karakayasemi added 3 - To Review and removed 2 - Developing labels Jul 26, 2020

karakayasemi commented Jul 26, 2020

View reviewed changes

apps/files_sharing/lib/ExpireSharesJob.php Outdated Show resolved Hide resolved

owncloud deleted a comment from codecov bot Jul 26, 2020

jvillafanez reviewed Jul 27, 2020

View reviewed changes

karakayasemi force-pushed the expire-share-job branch 3 times, most recently from 56e993f to 442929f Compare July 27, 2020 18:02

karakayasemi force-pushed the expire-share-job branch 3 times, most recently from 0e205ea to 08389d9 Compare July 27, 2020 18:54

owncloud deleted a comment from codecov bot Jul 27, 2020

karakayasemi force-pushed the expire-share-job branch from 08389d9 to 726357e Compare July 27, 2020 20:45

karakayasemi force-pushed the expire-share-job branch from 5642c23 to 8afe84d Compare July 28, 2020 21:17

karakayasemi commented Jul 29, 2020

View reviewed changes

karakayasemi force-pushed the expire-share-job branch from 8afe84d to a8ea321 Compare July 30, 2020 08:36

jvillafanez approved these changes Jul 30, 2020

View reviewed changes

karakayasemi added 2 commits July 30, 2020 04:33

Delete the correct expired share by using share id on ExpireSharesJob…

03926f8

…. ExpireSharesJob now also handles user and group shares.

share_id should be string according to \OCP\Share\IShare interface

c58fe67

karakayasemi force-pushed the expire-share-job branch from a8ea321 to c58fe67 Compare July 30, 2020 11:33

karakayasemi merged commit 38ab9fb into master Jul 30, 2020

delete-merged-branch bot deleted the expire-share-job branch July 30, 2020 13:21

	$config = $c->getConfig();
	$factoryClass = $config->getSystemValue('sharing.managerFactory', '\OC\Share20\ProviderFactory');
	/** @var \OCP\Share\IProviderFactory $factory */
	$factory = new $factoryClass($this);

Conversation

karakayasemi commented Jul 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

update-docs bot commented Jul 26, 2020

Uh oh!

codecov bot commented Jul 26, 2020

Codecov Report

Uh oh!

Uh oh!

Uh oh!

jvillafanez Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

karakayasemi Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

micbar commented Jul 27, 2020

Uh oh!

karakayasemi commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jvillafanez commented Jul 28, 2020

Uh oh!

karakayasemi commented Jul 28, 2020

Uh oh!

codecov bot commented Jul 28, 2020

Codecov Report

Uh oh!

codecov bot commented Jul 28, 2020

Codecov Report

Uh oh!

codecov bot commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jvillafanez commented Jul 29, 2020

Uh oh!

karakayasemi Jul 29, 2020

Choose a reason for hiding this comment

Uh oh!

karakayasemi Jul 29, 2020

Choose a reason for hiding this comment

Uh oh!

karakayasemi commented Jul 29, 2020

Uh oh!

jvillafanez commented Jul 30, 2020

Uh oh!

karakayasemi commented Jul 30, 2020

Uh oh!

karakayasemi commented Jul 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

karakayasemi commented Jul 26, 2020 •

edited

Loading

karakayasemi commented Jul 27, 2020 •

edited

Loading

codecov bot commented Jul 28, 2020 •

edited

Loading