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

[full-ci] Fix inheritance of share permissions on reshares #7015

Merged
merged 10 commits into from
May 23, 2022

Conversation

JammingBen
Copy link
Collaborator

Description

We've fixed a bug where the permissions of a share were not inherited when trying to reshare a resource. We've also disabled the role-select-dropdown if only one role is available for sharing.

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

@JammingBen JammingBen self-assigned this May 19, 2022
@ownclouders
Copy link
Contributor

ownclouders commented May 19, 2022

Results for oCISSharingInternal3 https://drone.owncloud.com/owncloud/web/25764/62/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUIResharing1-reshareUsers_feature-L219.png

webUIResharing1-reshareUsers_feature-L219.png

webUIResharing1-reshareUsers_feature-L237.png

webUIResharing1-reshareUsers_feature-L237.png

@JammingBen JammingBen changed the title Fix inheritance of share permissions on reshares [full-ci] Fix inheritance of share permissions on reshares May 19, 2022
@JammingBen
Copy link
Collaborator Author

I can't find out why the 2 tests in https://drone.owncloud.com/owncloud/web/25760/62/15 keep failing. I tried to reproduce the steps several times on my local machine, but it always behaves correctly. Unfortunately, I can't get these 2 tests to run locally... @individual-it Maybe your team could have a look if the time permits?

@individual-it individual-it self-assigned this May 23, 2022
Copy link
Collaborator

@fschade fschade left a comment

Choose a reason for hiding this comment

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

tested, works as expected. As soon as the ci approves it, merge it. THX 🚀

Comment on lines -186 to -195
@issue-5461
Scenario: User is not allowed to reshare a file/folder with the higher permissions
Given user "Brian" has shared folder "simple-folder" with user "Alice" with "read, share, delete" permissions in the server
And user "Alice" has accepted the share "Shares/simple-folder" offered by user "Brian" in the server
And user "Alice" has logged in using the webUI
When the user opens folder "Shares" using the webUI
And the user shares folder "simple-folder" with user "Carol King" as "Custom permissions" with permissions "share, delete, update" using the webUI
Then the error message with header "Error while sharing." should be displayed on the webUI
And as "Carol" folder "Shares/simple-folder" should not exist in the server
And user "Carol" should not have received any shares in the server
Copy link
Member

Choose a reason for hiding this comment

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

why deleting this test? is the resharer now allowed to increase the permissions or is the test not applicable at all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We deleted the test because the tested behavior (selecting an invalid role shows an error) changed completely. We want to write a new e2e test to check the new behavior (an invalid role isn't even showing) in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Ah cool, so deleting these tests is not a lost because they never passed and never would.
CC @ScharfViktor is this resharing behavior on your list to test in e2e or do we need a new issue to make sure we don't forget to write the e2e tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

I created issue, not to forget it. #7041

@JammingBen JammingBen marked this pull request as ready for review May 23, 2022 10:48
@sonarcloud
Copy link

sonarcloud bot commented May 23, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

97.4% 97.4% Coverage
0.0% 0.0% Duplication

@JammingBen JammingBen requested a review from kulmann May 23, 2022 11:20
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

LGTM 💪

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

Successfully merging this pull request may close these issues.

Roles select must be limited to disallow permission increases
6 participants