Access list to share manager #2834

rullzer · 2016-12-22T20:49:26Z

Revived this PR to get rid of the old code

What we do now is offload the accesslist creation to the share providers. Which allows for way more efficient queries.

The manager function now can handle recursion and directly filter out users the currently have no access to the Node (delete group shares for example).

The helper class should be able to assist for notifications.

@nickvergessen @schiessle lets see if we can get this moving!

TODO:

add accesslist function to circles provider
- Issue at Update ShareProvider circles#46

mention-bot · 2016-12-22T20:49:28Z

@rullzer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @schiessle, @LukasReschke and @icewind1991 to be potential reviewers.

schiessle · 2016-12-23T10:21:55Z

apps/sharebymail/lib/ShareByMailProvider.php

@@ -767,4 +767,8 @@ public function getSharesInFolder($userId, Folder $node, $reshares) {
 		return $shares;
 	}

+	public function getAccessList($nodes, $currentAccess) {
+		return ['users' => [], 'remote' => false, 'public' => false];


Shouldn't we check here if the file or a file in the folder is shared by mail and set 'public' to true (this would be enough for encryption)? To make it more generic we could also add another key 'mail' to the array, which would probably be the better solution.

Yeah didn't think about it yet. Just had to implement something that did not break. I'm fine with both so whatever you prefer.

codecov-io · 2017-01-04T09:11:45Z

Codecov Report

Merging #2834 into master will increase coverage by 0.36%.
The diff coverage is 90.3%.

@@             Coverage Diff              @@
##             master    #2834      +/-   ##
============================================
+ Coverage     54.08%   54.44%   +0.36%     
- Complexity    21428    21690     +262     
============================================
  Files          1317     1324       +7     
  Lines         81663    82896    +1233     
  Branches       1305     1305              
============================================
+ Hits          44167    45134     +967     
- Misses        37496    37762     +266

Impacted Files	Coverage Δ	Complexity Δ
...e/AppFramework/DependencyInjection/DIContainer.php	`82.75% <ø> (-0.69%)`	`42 <0> (+17)`
apps/sharebymail/tests/ShareByMailProviderTest.php	`99.76% <100%> (+0.02%)`	`25 <1> (+1)`	⬆️
lib/private/Server.php	`92.73% <100%> (-0.33%)`	`188 <1> (+68)`
...ederatedfilesharing/lib/FederatedShareProvider.php	`66.25% <100%> (+2.1%)`	`80 <4> (+4)`	⬆️
lib/private/Encryption/File.php	`83.33% <100%> (+4.76%)`	`12 <1> (+2)`	⬆️
lib/private/Share20/ShareHelper.php	`74.35% <74.35%> (ø)`	`28 <28> (?)`
lib/private/Share20/DefaultShareProvider.php	`91.5% <90.66%> (-0.16%)`	`114 <22> (+21)`
apps/sharebymail/lib/ShareByMailProvider.php	`62.46% <94.44%> (+1.51%)`	`64 <2> (+2)`	⬆️
lib/private/Share20/Manager.php	`94.35% <96.96%> (+0.33%)`	`221 <12> (+11)`	⬆️
lib/public/Share.php	`59.64% <0%> (-7.02%)`	`26% <0%> (ø)`
... and 24 more

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 51377f3...cab4111. Read the comment docs.

rullzer · 2017-01-10T15:17:45Z

@nickvergessen @schiessle please have a look

nickvergessen · 2017-01-10T16:09:26Z

lib/private/Encryption/File.php

-		$public = $resultForFile['public'] || $resultForFile['remote'] || $public;
+		if ($file !== null) {
+			$resultForFile = $this->shareManager->getAccessList($file, false);
+			$userIds = \array_merge($userIds, $resultForFile['users']);


remove leading slash?

nickvergessen · 2017-01-10T16:21:09Z

lib/public/Share/IManager.php

+	 * @return array
+	 * @since 9.2.0
+	 */
+	public function getAccessList(\OCP\Files\Node $path, $recursive = true);


Missing , $currentAccess = false ?

nickvergessen · 2017-01-10T16:21:21Z

lib/private/Share20/ShareHelper.php

+	 * @param array $users Array of userIds
+	 * @return array Mapping $uid to an array of nodes
+	 */
+	public function getPathsForAccessList(Node $node, $users) {


No public API?

MorrisJobke

@rullzer See comments by @nickvergessen

Signed-off-by: Joas Schilling <coding@schilljs.com>

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

fixed

rullzer · 2017-04-13T11:40:05Z

👍 for me now. Seems to do all the tricks!

@schiessle can you have a look as well?

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

schiessle · 2017-04-13T12:57:15Z

'public' need to be true as soon as one remote-share, email-share or public link exists. I don't see that this is the case here, but maybe I missed something.

rullzer · 2017-04-13T12:58:17Z

don't we ave remote for remote?

rullzer · 2017-04-13T13:08:16Z

@schiessle also see #2834 (comment) ;)

schiessle · 2017-04-13T13:12:32Z

Yes you are right, remote shares are separate, only the encryption code merges them: https://github.com/nextcloud/server/blob/master/lib/private/Encryption/File.php#L79

But mail-shares and public links where handled by the share api as the same: https://github.com/nextcloud/server/blob/master/lib/private/Share/Share.php#L269

So we either need to keep this behaviour for mail shares and public links or extend the logic on the encryption code to merge public, remote and mail.

rullzer · 2017-04-13T13:14:08Z

Let me just replace the public key. Because the code doesn't care so that seems the easy way.

schiessle · 2017-04-13T13:14:19Z

also see #2834 (comment) ;)

Yes, makes probably sense to show mail shares as mail shares but then we need to update it here https://github.com/nextcloud/server/blob/master/lib/private/Encryption/File.php#L79 and here https://github.com/nextcloud/server/blob/master/lib/private/Encryption/File.php#L85

rullzer · 2017-04-13T13:15:43Z

@schiessle I just made it public now. As that keep the behavior as is for now.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer · 2017-04-13T20:34:53Z

CI is happy (well 1 timed out).
Also activities and encryption seem to work properly for me still!

@schiessle @nickvergessen what do you say? Lets get this in!

nickvergessen · 2017-04-14T05:05:17Z

👍

ArtificialOwl · 2017-04-26T15:09:46Z

Circles complies with the new ShareProvider

rullzer added 3. to review Waiting for reviews enhancement feature: encryption (server-side) feature: sharing technical debt labels Dec 22, 2016

rullzer added this to the Nextcloud 12.0 milestone Dec 22, 2016

rullzer requested review from nickvergessen and schiessle December 22, 2016 20:49

rullzer added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Dec 23, 2016

rullzer force-pushed the accesListToShareManager branch 2 times, most recently from fa99f97 to 2309e25 Compare December 23, 2016 07:44

schiessle reviewed Dec 23, 2016

View reviewed changes

LukasReschke changed the title ~~Acces list to share manager~~ Access list to share manager Dec 27, 2016

rullzer force-pushed the accesListToShareManager branch 2 times, most recently from a5051d9 to 76a0ddb Compare January 4, 2017 08:33

rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 4, 2017

nickvergessen reviewed Jan 10, 2017

View reviewed changes

nickvergessen mentioned this pull request Jan 10, 2017

Use accesslist from the new share manager nextcloud/activity#101

Closed

MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jan 26, 2017

MorrisJobke previously requested changes Jan 26, 2017

View reviewed changes

rullzer mentioned this pull request Mar 14, 2017

take share by mail into consideration if we calculate the access list #3843

Merged

rullzer force-pushed the accesListToShareManager branch from 76a0ddb to 9e12f9c Compare March 20, 2017 13:22

nickvergessen and others added 4 commits April 13, 2017 12:58

Catch exceptions and use as many results as possible

29f2088

Signed-off-by: Joas Schilling <coding@schilljs.com>

Add more tests for the share helper

e1d54e3

Signed-off-by: Joas Schilling <coding@schilljs.com>

Add samples to the docs

f57ef55

Signed-off-by: Joas Schilling <coding@schilljs.com>

Do not set full path if not currentAccess

b96297e

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer force-pushed the accesListToShareManager branch from 112e3c4 to b96297e Compare April 13, 2017 10:59

Not needed in the DIContainer anymore

aef95b9

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 13, 2017

rullzer added 2 commits April 13, 2017 14:52

Fix server container registration

0f56823

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Update autoloader

6a519ab

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Mail shares trigger the public key

cab4111

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer force-pushed the accesListToShareManager branch from 3e86cb5 to cab4111 Compare April 13, 2017 13:19

rullzer mentioned this pull request Apr 13, 2017

Update ShareProvider nextcloud/circles#46

Closed

LukasReschke approved these changes Apr 14, 2017

View reviewed changes

MorrisJobke merged commit 10290eb into master Apr 15, 2017

MorrisJobke deleted the accesListToShareManager branch April 15, 2017 18:06

kesselb mentioned this pull request Jan 25, 2023

Bugfix/32630/get access list use correct array key #36305

Closed

kesselb mentioned this pull request Jun 24, 2023

Get access list share by email recipients #32631

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Access list to share manager #2834

Access list to share manager #2834

rullzer commented Dec 22, 2016 •

edited

Loading

mention-bot commented Dec 22, 2016

schiessle Dec 23, 2016 •

edited

Loading

rullzer Dec 23, 2016

rullzer Jan 4, 2017

codecov-io commented Jan 4, 2017 •

edited

Loading

rullzer commented Jan 10, 2017

nickvergessen Jan 10, 2017

nickvergessen Jan 10, 2017

nickvergessen Jan 10, 2017

MorrisJobke left a comment

rullzer commented Apr 13, 2017

schiessle commented Apr 13, 2017

rullzer commented Apr 13, 2017

rullzer commented Apr 13, 2017

schiessle commented Apr 13, 2017

rullzer commented Apr 13, 2017

schiessle commented Apr 13, 2017

rullzer commented Apr 13, 2017

rullzer commented Apr 13, 2017

nickvergessen commented Apr 14, 2017

ArtificialOwl commented Apr 26, 2017

Access list to share manager #2834

Access list to share manager #2834

Conversation

rullzer commented Dec 22, 2016 • edited Loading

mention-bot commented Dec 22, 2016

schiessle Dec 23, 2016 • edited Loading

Choose a reason for hiding this comment

rullzer Dec 23, 2016

Choose a reason for hiding this comment

rullzer Jan 4, 2017

Choose a reason for hiding this comment

codecov-io commented Jan 4, 2017 • edited Loading

Codecov Report

rullzer commented Jan 10, 2017

nickvergessen Jan 10, 2017

Choose a reason for hiding this comment

nickvergessen Jan 10, 2017

Choose a reason for hiding this comment

nickvergessen Jan 10, 2017

Choose a reason for hiding this comment

MorrisJobke left a comment

Choose a reason for hiding this comment

rullzer commented Apr 13, 2017

schiessle commented Apr 13, 2017

rullzer commented Apr 13, 2017

rullzer commented Apr 13, 2017

schiessle commented Apr 13, 2017

rullzer commented Apr 13, 2017

schiessle commented Apr 13, 2017

rullzer commented Apr 13, 2017

rullzer commented Apr 13, 2017

nickvergessen commented Apr 14, 2017

ArtificialOwl commented Apr 26, 2017

rullzer commented Dec 22, 2016 •

edited

Loading

schiessle Dec 23, 2016 •

edited

Loading

codecov-io commented Jan 4, 2017 •

edited

Loading