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.

Sign up for GitHub

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

Jump to bottom

Fix shares read permissions #16761

Merged
merged 7 commits into from
Oct 4, 2019
Merged

Fix shares read permissions #16761

merged 7 commits into from
Oct 4, 2019

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Aug 16, 2019 •
edited
Loading

We need to check if getShare (singular) also have resharing rights :)

@skjnldsv skjnldsv added this to the Nextcloud 17 milestone Aug 16, 2019
@skjnldsv skjnldsv self-assigned this Aug 16, 2019
@skjnldsv skjnldsv force-pushed the fix/sharing/shares-access-sanity-checks branch 2 times, most recently from cee92bd to ec56209 Compare August 16, 2019 14:05
@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Aug 16, 2019
@skjnldsv

This comment has been minimized.

Sign in to view
@skjnldsv skjnldsv force-pushed the fix/sharing/shares-access-sanity-checks branch from ec56209 to 432c411 Compare August 16, 2019 21:44
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 16, 2019
@ArtificialOwl
Copy link
Member

So, with the request to displays the list of sharing to users with resharing rights, I missed an endpoint resulting in users not being able to access information they should be allowed to read.

This PR should allow users with resharing rights to retrieve data from a specific shareId using getShare()

nice catch, @skjnldsv

@rullzer rullzer mentioned this pull request Aug 18, 2019
Merged
@skjnldsv skjnldsv mentioned this pull request Aug 19, 2019
Merged
@skjnldsv skjnldsv force-pushed the fix/sharing/shares-access-sanity-checks branch from 432c411 to 2194a02 Compare August 19, 2019 07:51
danxuliu
danxuliu requested changes Aug 20, 2019
View reviewed changes
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

CI is not happy; it seems that this gives access to a share to users that should not be able to access it.

Similarly several sharing integration tests in Talk fail too with this pull request (which of course could just mean that they need to be adjusted to the new behaviour, but it seems that they are caused by the same aforementioned problem).

@skjnldsv

This comment has been minimized.

Sign in to view
@danxuliu

This comment has been minimized.

Sign in to view
@skjnldsv

This comment has been minimized.

Sign in to view
@rullzer rullzer mentioned this pull request Aug 23, 2019
Merged
@skjnldsv skjnldsv force-pushed the fix/sharing/shares-access-sanity-checks branch from 759ef6e to 9afc6ac Compare August 27, 2019 07:46
skjnldsv
skjnldsv commented Aug 27, 2019
View reviewed changes
apps/files_sharing/lib/Controller/ShareAPIController.php
throw new OCSNotFoundException($this->l->t('Wrong share ID, share doesn\'t exist'));
}

if ($share->getShareOwner() !== $this->currentUser && $share->getSharedBy() !== $this->currentUser) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Already part of the canEditShare

skjnldsv
skjnldsv commented Sep 5, 2019
View reviewed changes
Copy link
Member Author

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

👍

@rullzer rullzer modified the milestones: Nextcloud 17, Nextcloud 18 Sep 5, 2019
@rullzer

This comment has been minimized.

Sign in to view
@skjnldsv

This comment has been minimized.

Sign in to view
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 3, 2019
ChristophWurst
ChristophWurst approved these changes Oct 3, 2019
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

🐘

@skjnldsv skjnldsv force-pushed the fix/sharing/shares-access-sanity-checks branch 4 times, most recently from 717013b to 845ac7f Compare October 4, 2019 08:03
@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 4, 2019

/compile amend /

@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 4, 2019

Weird

modified: apps/oauth2/js/oauth2.js
12 | modified: apps/oauth2/js/oauth2.js.map
13

@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 4, 2019 •
edited
Loading

three acceptance failing, but when running locally:

15 scenarios (15 passed)
245 steps (245 passed)
2m53.09s (5.73Mb)

11 scenarios (11 passed)
91 steps (91 passed)
1m47.76s (5.73Mb)

12 scenarios (12 passed)
225 steps (225 passed)
2m57.98s (5.71Mb)

And the output doesn't even go to the end of test sstatus lines (like above) in drone, I guess it timed out 🤷‍♂️

@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 4, 2019

Weird

modified: apps/oauth2/js/oauth2.js
12 | modified: apps/oauth2/js/oauth2.js.map
13

So, even locally or the bot cannot find changes.
@rullzer @ChristophWurst if you compile this branch locally, do you find uncommited files?

skjnldsv and others added 6 commits October 4, 2019 19:25
@skjnldsv
Add helper script to run integration tests on a Docker container
ba1ba1a 
Copied and adjusted from "tests/integration/run-docker.sh" in Talk; see
its commit history for further reference.

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Move sharing integration tests to their own directory
99404bd 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@danxuliu @skjnldsv
Do not expect a 200 HTTP status code when updating a share
a7526d7 
This will be needed to test scenarios in which updating a share return a
different HTTP status code, like 401.

The assertion for the 200 HTTP status code was added in those scenarios
that tested updating a share (that is, those that were also checking the
OCS status code), but not in those in which updating a share was just a
preparatory step for the actual test (in the same way that the HTTP
status code is not checked in those tests when creating a share).

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu @skjnldsv
Add assorted integration tests for shares
6d4f4c1 
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@skjnldsv
Extract check for whether a user with access to a share can edit it
f02cff1 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Add checks for whether a user with access to a share can delete it
b1069b2 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the fix/sharing/shares-access-sanity-checks branch from 845ac7f to b647341 Compare October 4, 2019 17:25
@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 4, 2019

Okay, the acceptances issues are on master, not on this pr.
Still this weird issue on the oauth

@skjnldsv
Fix shares read permissions
ff895ab 
A user with reshare permissions on a file is now able to get any share
of that file (just like the owner).

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the fix/sharing/shares-access-sanity-checks branch from b647341 to ff895ab Compare October 4, 2019 19:40
@skjnldsv skjnldsv merged commit 2169c26 into master Oct 4, 2019
@skjnldsv skjnldsv deleted the fix/sharing/shares-access-sanity-checks branch October 4, 2019 20:54
@danxuliu danxuliu mentioned this pull request Oct 6, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@ArtificialOwl ArtificialOwl ArtificialOwl approved these changes

@rullzer rullzer rullzer approved these changes

@MorrisJobke MorrisJobke Awaiting requested review from MorrisJobke

@schiessle schiessle Awaiting requested review from schiessle

@blizzz blizzz Awaiting requested review from blizzz

@danxuliu danxuliu Awaiting requested review from danxuliu

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

Assignees

@skjnldsv skjnldsv

Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: sharing
Projects
None yet
Milestone
Nextcloud 18.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@skjnldsv @ArtificialOwl @danxuliu @rullzer @ChristophWurst