fix: fix deny access to prevent a regression #10627

micbar · 2024-11-22T08:54:33Z

Regression in ocis 7.0.0-rc.3

After updating an ocis 5.0.9 to ocis 7.0.0-rc.3, existing denials are breaking the web client.

Related Issue

Fixes <issue_link>

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 only (no source changes)

Checklist:

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

update-docs · 2024-11-22T08:54:38Z

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.

ScharfViktor · 2024-11-22T11:06:45Z

TODO check deny feature during the upgrade test between 5.0.0 and 7.0.0
put FRONTEND_OCS_ENABLE_DENIALS to deprecated envs
enable e2e test https://github.com/owncloud/web/blob/master/tests/e2e/cucumber/features/shares/denyShareAccess.feature

mmattel · 2024-11-22T14:31:11Z

If this gets merged, I will do the needed translation on transifex for the new strings.

Note, if TX needs to be added quickly befor final releasing, one needs to initiate a manualy sync, I will do the TX and another sync needs to be done. Then all is up to date.

ScharfViktor · 2024-11-22T16:34:14Z

tested:

creating/changing/deleting share with role cannot access - works correct
user can see/change/delete cannot access share which was created in 5.0 version

remarks:

500 error when deleting deny share (was created in version 5.0) but looks like works -> user recipient can see resource
steps:
- switch ocis version 5
- admin creates folder and shares folder to Users group
- admin shares folder to einstein with denied role
- upgrade ocis to version 7
- admin deletes denied role
  log:
  {"level":"error","service":"sharing","pkg":"rgrpc","traceid":"56df2dfcf09102480861bcf5efb79e17","hostname":"","userID":"9955764e-d4f7-4191-96ea-7d559e8f7955","shareID":"b1b4a413-4987-45ed-8cee-986911d246cd:9955764e-d4f7-4191-96ea-7d559e8f7955:fa3ce1ec-9cd8-475e-80f9-053e86c1c836","error":"error: already exists: already exists","time":"2024-11-22T17:23:14+01:00","message":"persisting removed share failed"}

@micbar today I saw that you had similar issue and fixed it. it was case with project space - it works

no german translation

user can see activity of the denied resource
sse events in this case work correct but confuse users. If we deny access -> user gets notification that resource was shared but user doesn't see resource. If we delete cannot access - user see notification that share was deleted and user can see resource.
user can search by denied resource and cannot create resource with same name - existing bugs User doesn't get correct message if he creates resource with same of denied resource name web#9029 Denial of access to a resource. User can find denied resource #6288

micbar · 2024-11-22T16:59:44Z

@micbar today I saw that you had similar issue and fixed it. it was case with project space - it works

Correct. That was a general issue with sharing and has been fixed in reva cs3org/reva#4968.

Needs a reva bump to arrive in ocis master.

micbar · 2024-11-22T17:11:28Z

services/graph/pkg/service/v0/base.go

@@ -482,6 +482,10 @@ func (g BaseGraphService) cs3UserShareToPermission(ctx context.Context, share *c
 		perm.SetRoles([]string{role.GetId()})
 	} else {
 		actions := unifiedrole.CS3ResourcePermissionsToLibregraphActions(share.GetPermissions().GetPermissions())
+		// neither a role nor actions are set, that is a full denial
+		if len(actions) == 0 {
+			actions = []string{"access denied"}


@rhafer Not the "nicest" solution, I admit.

We have the contract with the clients that we

either set a value on roles

or set a value on @libre.graph.permissions.actions

That means, if we disable the role "RoleDenied" and have existing shares, we need to return at least a hint to the user.

Fine for me, if you want to solve it differently.

After discussion with @butonic and @kobergj we decided to return none in that case.

micbar · 2024-11-22T17:13:18Z

services/graph/pkg/service/v0/base.go

@@ -482,6 +482,10 @@ func (g BaseGraphService) cs3UserShareToPermission(ctx context.Context, share *c
 		perm.SetRoles([]string{role.GetId()})
 	} else {
 		actions := unifiedrole.CS3ResourcePermissionsToLibregraphActions(share.GetPermissions().GetPermissions())
+		// neither a role nor actions are set, that is a full denial


Important Info: We decided long ago, that we will only go with a "full denial", we do not intend to think about negative permissions in the future. So my assumption here is valid.

services/graph/pkg/unifiedrole/roles.go

mmattel · 2024-11-22T17:32:42Z

put FRONTEND_OCS_ENABLE_DENIALS to deprecated envs

Before merging, if the var has been deprecated, please run make -C docs docs-generate to add the changed env_vars.yaml to the PR. I need to do afterwards with a separate PR the changed envvar magic... 😅

micbar · 2024-11-22T17:38:44Z

Before merging, if the var has been deprecated, please run make -C docs docs-generate to add the changed env_vars.yaml to the PR. I need to do afterwards with a separate PR the changed envvar magic... 😅

Needs to be discussed with @kobergj

We deprecated the whole OCS Api with 5.0.0.

I would have removed it already but the clients were not able to move to the LibreGraph API in time.

butonic · 2024-11-25T08:38:30Z

We deprecated the whole OCS Api with 5.0.0.

7.0.0?

micbar · 2024-11-25T08:39:56Z

We deprecated the whole OCS Api with 5.0.0.

7.0.0?

5.0.0

https://doc.owncloud.com/ocis_release_notes.html#deprecation-announcements

services/graph/pkg/unifiedrole/roles.go

sonarqubecloud · 2024-11-25T11:31:55Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

fix: fix deny access to prevent a regression

micbar force-pushed the fix-denials-graph branch 2 times, most recently from d4b6566 to 28d8b7d Compare November 22, 2024 10:47

micbar force-pushed the fix-denials-graph branch from 28d8b7d to fdc10b1 Compare November 22, 2024 11:45

micbar requested review from ScharfViktor, 2403905 and kobergj November 22, 2024 11:45

micbar marked this pull request as ready for review November 22, 2024 11:55

micbar requested a review from kulmann as a code owner November 22, 2024 11:55

2403905 approved these changes Nov 22, 2024

View reviewed changes

micbar force-pushed the fix-denials-graph branch from fdc10b1 to 028fab2 Compare November 22, 2024 17:04

micbar mentioned this pull request Nov 22, 2024

Release 7.0.0 #10373

Closed

85 tasks

micbar requested a review from rhafer November 22, 2024 17:07

micbar commented Nov 22, 2024

View reviewed changes

services/graph/pkg/unifiedrole/roles.go Outdated Show resolved Hide resolved

micbar force-pushed the fix-denials-graph branch from 028fab2 to 31a80fa Compare November 22, 2024 17:31

micbar force-pushed the fix-denials-graph branch from 31a80fa to cb1b07e Compare November 22, 2024 18:12

micbar force-pushed the fix-denials-graph branch 2 times, most recently from f93a001 to c5d0cc2 Compare November 25, 2024 09:54

ScharfViktor reviewed Nov 25, 2024

View reviewed changes

services/graph/pkg/unifiedrole/roles.go Outdated Show resolved Hide resolved

fix: fix deny access to prevent a regression

bac2256

micbar force-pushed the fix-denials-graph branch from c5d0cc2 to bac2256 Compare November 25, 2024 10:32

kobergj approved these changes Nov 25, 2024

View reviewed changes

mmattel mentioned this pull request Nov 25, 2024

[docs-only] Deprecate OCS related frontend envvars because the OCS API is deprecated #10653

Merged

micbar merged commit b5decc6 into master Nov 25, 2024
4 checks passed

micbar deleted the fix-denials-graph branch November 25, 2024 12:30

ownclouders pushed a commit that referenced this pull request Nov 25, 2024

Merge pull request #10627 from owncloud/fix-denials-graph

1d83da0

fix: fix deny access to prevent a regression

ScharfViktor mentioned this pull request Nov 25, 2024

[test-only] enable again denyAccess tests owncloud/web#11959

Merged

saw-jan mentioned this pull request Nov 26, 2024

[QA] tests for new sharing role: Denied #10656

Open

6 tasks

nirajacharya2 mentioned this pull request Dec 6, 2024

[tests-only][full-ci] adding test for listing permission of a folder in personal and project space with denied role enabled #10722

Merged

9 tasks

micbar mentioned this pull request Dec 9, 2024

[full-ci][k6-tests] chore: prepare release, bump version #10749

Merged

12 tasks

anon-pradip added a commit that referenced this pull request Dec 10, 2024

test(refactor): refactor scenarios after fix #10627

0598b25

anon-pradip added a commit that referenced this pull request Dec 12, 2024

test(refactor): refactor scenarios after fix #10627

fe63f58

anon-pradip added a commit that referenced this pull request Dec 12, 2024

test(refactor): refactor scenarios after fix #10627

77fbcb5

anon-pradip added a commit that referenced this pull request Dec 12, 2024

test(refactor): refactor scenarios after fix #10627

4fb6d85

anon-pradip added a commit that referenced this pull request Dec 13, 2024

test(refactor): refactor scenarios after fix #10627

c71fb6d

anon-pradip added a commit that referenced this pull request Dec 13, 2024

test(refactor): refactor scenarios after fix #10627

d395df7

anon-pradip added a commit that referenced this pull request Dec 17, 2024

test(refactor): refactor scenarios after fix #10627

b690313

micbar mentioned this pull request Dec 17, 2024

[full-ci] [k6-tests] Release 7.0.0 #10778

Merged

12 tasks

PrajwolAmatya mentioned this pull request Dec 19, 2024

[tests-only][full-ci]added test to list drives after the share role has been disabled #10756

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix deny access to prevent a regression #10627

fix: fix deny access to prevent a regression #10627

micbar commented Nov 22, 2024 •

edited

Loading

update-docs bot commented Nov 22, 2024

ScharfViktor commented Nov 22, 2024 •

edited

Loading

mmattel commented Nov 22, 2024

ScharfViktor commented Nov 22, 2024 •

edited

Loading

micbar commented Nov 22, 2024

micbar Nov 22, 2024

micbar Nov 25, 2024

micbar Nov 22, 2024

mmattel commented Nov 22, 2024 •

edited

Loading

micbar commented Nov 22, 2024

butonic commented Nov 25, 2024 •

edited

Loading

micbar commented Nov 25, 2024 •

edited

Loading

sonarqubecloud bot commented Nov 25, 2024

fix: fix deny access to prevent a regression #10627

fix: fix deny access to prevent a regression #10627

Conversation

micbar commented Nov 22, 2024 • edited Loading

Regression in ocis 7.0.0-rc.3

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

update-docs bot commented Nov 22, 2024

ScharfViktor commented Nov 22, 2024 • edited Loading

mmattel commented Nov 22, 2024

ScharfViktor commented Nov 22, 2024 • edited Loading

micbar commented Nov 22, 2024

micbar Nov 22, 2024

Choose a reason for hiding this comment

micbar Nov 25, 2024

Choose a reason for hiding this comment

micbar Nov 22, 2024

Choose a reason for hiding this comment

mmattel commented Nov 22, 2024 • edited Loading

micbar commented Nov 22, 2024

butonic commented Nov 25, 2024 • edited Loading

micbar commented Nov 25, 2024 • edited Loading

sonarqubecloud bot commented Nov 25, 2024

Quality Gate passed

micbar commented Nov 22, 2024 •

edited

Loading

ScharfViktor commented Nov 22, 2024 •

edited

Loading

ScharfViktor commented Nov 22, 2024 •

edited

Loading

mmattel commented Nov 22, 2024 •

edited

Loading

butonic commented Nov 25, 2024 •

edited

Loading

micbar commented Nov 25, 2024 •

edited

Loading