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.

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

Only allow removing existing shares that would not be allowed due to reshare restrictions #26571

Merged
merged 4 commits into from
Jun 16, 2021

Conversation

juliusknorr
Copy link
Member

Steps to reproduce:

  • User A creates a file "LinkShare.txt" and shares it by link
  • User B shares a folder "WithoutReshare/" without reshare permissions to User A
  • User A moves a file "LinkShare.txt" into the incoming share "WithoutReshare/"

The share link still exists in the user interface though it is no longer accessible due to the missing reshare permissions on the parent share.

This PR makes sure that the UI doesn't offer to modify the link share and only remove it which is still possible.

However I'm still wondering if we should either indicate that this share is stale in the UI or even remove the share link if the node is moved to a share without reshare permissions, but not sure if dropping those shares automatically would be something feasible.

@juliusknorr
Copy link
Member Author

juliusknorr commented Apr 15, 2021

I've pushed two more commits to get the discussion for my proposal some example here to

  • remove shares when they are moved into a directory where the share owner hasn't allowed resharing
  • downgrade permissions when the owner is changed to not end up with link shares that have higher permissions than the parent share. Even though this might be checked, it feels safer to have the proper permissions set then in the child share as well. Also the share update was failing in that case with the following exception:
  Error    no app in context  OCP\Share\Exceptions\GenericShareException: Can’t increase permissions of  at lib/private/Share20/Manager.php line 353                                                             2021-04-15T10:49:48+00:00 
                                                                                                                                                                                                                                             
                               0. lib/private/Share20/Manager.php line 968                                                                                                                                                                   
                                  OC\Share20\Manager->generalCreateChecks("*** sensitive parameter replaced ***")                                                                                                                            
                               1. apps/files_sharing/lib/Updater.php line 92                                                                                                                                                                 
                                  OC\Share20\Manager->updateShare("*** sensitive parameters replaced ***")                                                                                                                                   
                               2. apps/files_sharing/lib/Updater.php line 40                                                                                                                                                                 
                                  OCA\Files_Sharing\Updater::moveShareToShare("\/ShareNoDelete\/sharedelete3")                                                                                                                               
                               3. lib/private/legacy/OC_Hook.php line 110                                                                                                                                                                    
                                  OCA\Files_Sharing\Updater::renameHook({oldpath:"\/sharedelete3",newpath:"\/ShareNoDelete\/sharedelete3"})                                                                                                  
                               4. lib/private/Files/View.php line 858                                                                                                                                                                        
                                  OC_Hook::emit("OC_Filesystem", "post_rename", {oldpath:"\/sharedelete3",newpath:"\/ShareNoDelete\/sharedelete3"})                                                                                          
                               5. apps/dav/lib/Connector/Sabre/Directory.php line 443                                                                                                                                                        
                                  OC\Files\View->rename("\/sharedelete3", "\/ShareNoDelete\/sharedelete3")                                                                                                                                   
                               6. 3rdparty/sabre/dav/lib/DAV/Tree.php line 160                                                                                                                                                               
                                  OCA\DAV\Connector\Sabre\Directory->moveInto("sharedelete3", "files\/admin\/sharedelete3", OCA\DAV\Connector\Sabre\Directory {})                                                                            
                               7. 3rdparty/sabre/dav/lib/DAV/CorePlugin.php line 612                                                                                                                                                         
                                  Sabre\DAV\Tree->move("files\/admin\/sharedelete3", "files\/admin\/ShareNoDelete\/sharedelete3")                                                                                                            
                               8. 3rdparty/sabre/event/lib/WildcardEmitterTrait.php line 89                                                                                                                                                  
                                  Sabre\DAV\CorePlugin->httpMove(Sabre\HTTP\Request {}, Sabre\HTTP\Response {})                                                                                                                              
                               9. 3rdparty/sabre/dav/lib/DAV/Server.php line 472                                                                                                                                                             
                                  Sabre\DAV\Server->emit("method:MOVE", [Sabre\HTTP\Request {},Sabre\HTTP\Response {}])                                                                                                                      
                              10. 3rdparty/sabre/dav/lib/DAV/Server.php line 253                                                                                                                                                             
                                  Sabre\DAV\Server->invokeMethod(Sabre\HTTP\Request {}, Sabre\HTTP\Response {})                                                                                                                              
                              11. 3rdparty/sabre/dav/lib/DAV/Server.php line 321                                                                                                                                                             
                                  Sabre\DAV\Server->start(                                                                                                                                                                                   
                                                                                                                                                                                                                                             
                                  )                                                                                                                                                                                                          
                              12. apps/dav/lib/Server.php line 332                                                                                                                                                                           
                                  Sabre\DAV\Server->exec(                                                                                                                                                                                    
                                                                                                                                                                                                                                             
                                  )                                                                                                                                                                                                          
                              13. apps/dav/appinfo/v2/remote.php line 35                                                                                                                                                                     
                                  OCA\DAV\Server->exec(                                                                                                                                                                                      
                                                                                                                                                                                                                                             
                                  )                                                                                                                                                                                                          
                              14. remote.php line 167                                                                                                                                                                                        
                                  require_once("\/var\/www\/html\/apps\/dav\/appinfo\/v2\/remote.php") 

@juliusknorr
Copy link
Member Author

@rullzer Maybe you can share your opinion on this :)

@juliusknorr juliusknorr force-pushed the bugfix/noid/existing-link-no-reshare branch from 44d5e22 to 2824cce Compare May 14, 2021 06:43
@juliusknorr juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 14, 2021
@juliusknorr juliusknorr added this to the Nextcloud 22 milestone May 14, 2021
@juliusknorr juliusknorr marked this pull request as ready for review May 14, 2021 06:43
@juliusknorr juliusknorr force-pushed the bugfix/noid/existing-link-no-reshare branch from 2824cce to 4810ec9 Compare May 14, 2021 06:43
This was referenced May 20, 2021
@juliusknorr juliusknorr force-pushed the bugfix/noid/existing-link-no-reshare branch 2 times, most recently from b872c2f to 140dd42 Compare May 27, 2021 11:41
@skjnldsv
Copy link
Member

Is it still "to review" ?
Or still developing?

@juliusknorr
Copy link
Member Author

Would be ready for review, though after my rebase today i guess i need to have another look at the one failing acceptance test which seems related, which would cover exactly that case where now a share would be cleaned up:

Scenario: sharee can unshare a reshare by link if resharing is disabled in the settings after the reshare is created # /drone/src/tests/acceptance/features/app-files-sharing-link.feature:195

@blizzz blizzz mentioned this pull request Jun 2, 2021
57 tasks
@juliusknorr juliusknorr force-pushed the bugfix/noid/existing-link-no-reshare branch from 140dd42 to 64d6655 Compare June 7, 2021 15:27
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
…reshare restrictions

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr force-pushed the bugfix/noid/existing-link-no-reshare branch from 64d6655 to 964fd27 Compare June 9, 2021 08:44
@MorrisJobke MorrisJobke mentioned this pull request Jun 10, 2021
59 tasks
@juliusknorr juliusknorr requested a review from artonge June 15, 2021 19:48
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

I probably don't have the full picture, but the code looks fine :).

@blizzz blizzz mentioned this pull request Jun 16, 2021
45 tasks
@blizzz blizzz merged commit 818fc95 into master Jun 16, 2021
@blizzz blizzz deleted the bugfix/noid/existing-link-no-reshare branch June 16, 2021 13:26
@blizzz
Copy link
Member

blizzz commented Jun 16, 2021

do we need backports?

@juliusknorr
Copy link
Member Author

/backport to stable21

@juliusknorr
Copy link
Member Author

/backport to stable20

@juliusknorr
Copy link
Member Author

/backport to stable19

@backportbot-nextcloud
Copy link

The backport to stable19 failed. Please do this backport manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants