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] feature-deny-access #8983

Merged
merged 12 commits into from
May 10, 2023
Merged

[full-ci] feature-deny-access #8983

merged 12 commits into from
May 10, 2023

Conversation

AlexAndBear
Copy link
Contributor

@AlexAndBear AlexAndBear commented May 4, 2023

Description

Enhancement: deny share access

We've added a way to deny the share access in sub folders in a share or space.
This allows the share editors to restrict access to certain resources within a share for certain share receivers.

Related Issue

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

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

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • ...

@update-docs
Copy link

update-docs bot commented May 4, 2023

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.

Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

I'm totally not into the feature scope here, but is it intended that you can deny access only on folders, but not files?

Also, the PR increased some of the context actions height:
image

@ownclouders
Copy link
Contributor

ownclouders commented May 5, 2023

Results for acceptance oCIS https://drone.owncloud.com/owncloud/web/35474/60/1

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

webUISharingInternalUsers-shareWithUsers_feature-L156.png

webUISharingInternalUsers-shareWithUsers_feature-L156.png

@AlexAndBear
Copy link
Contributor Author

AlexAndBear commented May 5, 2023

TBD:

  • Add e2e tests
  • Implement logic for spaces

@AlexAndBear AlexAndBear requested review from lookacat and JammingBen May 5, 2023 13:40
@AlexAndBear AlexAndBear changed the title feature-deny-access [full-ci] feature-deny-access May 5, 2023
@AlexAndBear
Copy link
Contributor Author

AlexAndBear commented May 5, 2023

I'm totally not into the feature scope here, but is it intended that you can deny access only on folders, but not files?

Also, the PR increased some of the context actions height: image

Fixed =)

but is it intended that you can deny access only on folders, but not files?

Unfortantly only works on folders, due to backend limitations

@AlexAndBear AlexAndBear force-pushed the feature-deny-access branch 2 times, most recently from 4075c50 to 4807f12 Compare May 8, 2023 12:08
@AlexAndBear AlexAndBear force-pushed the feature-deny-access branch from 4807f12 to 47cc260 Compare May 8, 2023 12:11
@AlexAndBear AlexAndBear requested a review from kulmann May 8, 2023 13:35
@AlexAndBear AlexAndBear marked this pull request as ready for review May 8, 2023 13:35
@AlexAndBear AlexAndBear removed the request for review from lookacat May 8, 2023 13:36
@AlexAndBear AlexAndBear requested review from lookacat and JammingBen and removed request for JammingBen May 8, 2023 13:44
@AlexAndBear
Copy link
Contributor Author

This PR killed my last brain cell, please review carefully

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.

Puh. Big and tough one... I love it so far!

Things that need to be solved as well, but can be a followup:

  • subfolder/files inside an already denied folder should be shown as Access denied as well (we already discussed this, just for completeness)
  • the Shared with others page should not include the denials in the listing. They can just completely disappear from that page (remember that we want to hide the technical detail of a denial being a share from the user)
  • Space member listing shows the logged in user in the list. But the Deny access option should not be listed in the context menu for the logged in user.

@AlexAndBear
Copy link
Contributor Author

AlexAndBear commented May 9, 2023

subfolder/files inside an already denied folder should be shown as Access denied as well (we already discussed this, just for completeness)

Implemented

the Shared with others page should not include the denials in the listing. They can just completely disappear from that page (remember that we want to hide the technical detail of a denial being a share from the user)

Fixed

Space member listing shows the logged in user in the list. But the Deny access option should not be listed in the context menu for the logged in user.

Fixed

@AlexAndBear AlexAndBear requested a review from kulmann May 9, 2023 08:41
@AlexAndBear AlexAndBear requested a review from ScharfViktor May 9, 2023 09:26
@ScharfViktor
Copy link
Contributor

LGTM on the testing side

@AlexAndBear AlexAndBear requested a review from JammingBen May 9, 2023 12:39
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.

Awesome 💪

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 8 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

52.7% 52.7% Coverage
0.0% 0.0% Duplication

@kulmann kulmann merged commit 2bfcaeb into master May 10, 2023
@delete-merged-branch delete-merged-branch bot deleted the feature-deny-access branch May 10, 2023 12:29
@micbar micbar mentioned this pull request Jul 24, 2023
68 tasks
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.

[web] Deny access
6 participants