From 355c5e2e946fd7ea4816661ba55c6676f7eb8324 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Thu, 30 Jun 2022 14:11:56 +0200 Subject: [PATCH 1/2] improve sharing validators and comparisons --- changelog/unreleased/sharing-fixes.md | 5 +++++ .../owncloud/ocs/conversions/permissions.go | 12 ++++++++---- .../owncloud/ocs/conversions/permissions_test.go | 4 ++-- pkg/storage/utils/grants/grants.go | 15 ++++++++++++--- 4 files changed, 27 insertions(+), 9 deletions(-) create mode 100644 changelog/unreleased/sharing-fixes.md diff --git a/changelog/unreleased/sharing-fixes.md b/changelog/unreleased/sharing-fixes.md new file mode 100644 index 0000000000..d56cd12618 --- /dev/null +++ b/changelog/unreleased/sharing-fixes.md @@ -0,0 +1,5 @@ +Bugfix: Improve the sharing internals + +We cleaned up the sharing code validation and comparisons. + +https://github.com/cs3org/reva/pull/3022 diff --git a/internal/http/services/owncloud/ocs/conversions/permissions.go b/internal/http/services/owncloud/ocs/conversions/permissions.go index bc2f86e0e5..86a6d644ff 100644 --- a/internal/http/services/owncloud/ocs/conversions/permissions.go +++ b/internal/http/services/owncloud/ocs/conversions/permissions.go @@ -27,7 +27,7 @@ import ( type Permissions uint const ( - // PermissionInvalid grants no permissions on a resource + // PermissionInvalid represents an invalid permission PermissionInvalid Permissions = 0 // PermissionRead grants read permissions on a resource PermissionRead Permissions = 1 << (iota - 1) @@ -41,19 +41,23 @@ const ( PermissionShare // PermissionAll grants all permissions on a resource PermissionAll Permissions = (1 << (iota - 1)) - 1 + // PermissionMaxInput is to be used within value range checks + PermissionMaxInput = PermissionAll + // PermissionMinInput is to be used within value range checks + PermissionMinInput = PermissionRead ) var ( // ErrPermissionNotInRange defines a permission specific error. - ErrPermissionNotInRange = fmt.Errorf("The provided permission is not between %d and %d", PermissionInvalid, PermissionAll) + ErrPermissionNotInRange = fmt.Errorf("The provided permission is not between %d and %d", PermissionMinInput, PermissionMaxInput) ) // NewPermissions creates a new Permissions instance. // The value must be in the valid range. func NewPermissions(val int) (Permissions, error) { if val == int(PermissionInvalid) { - return PermissionInvalid, nil - } else if val < int(PermissionInvalid) || int(PermissionAll) < val { + return PermissionInvalid, fmt.Errorf("permissions %d out of range %d - %d", val, PermissionMinInput, PermissionMaxInput) + } else if val < int(PermissionInvalid) || int(PermissionMaxInput) < val { return PermissionInvalid, ErrPermissionNotInRange } return Permissions(val), nil diff --git a/internal/http/services/owncloud/ocs/conversions/permissions_test.go b/internal/http/services/owncloud/ocs/conversions/permissions_test.go index 77963f16a6..de41ce4770 100644 --- a/internal/http/services/owncloud/ocs/conversions/permissions_test.go +++ b/internal/http/services/owncloud/ocs/conversions/permissions_test.go @@ -23,7 +23,7 @@ import ( ) func TestNewPermissions(t *testing.T) { - for val := int(PermissionRead); val <= int(PermissionAll); val++ { + for val := int(PermissionMinInput); val <= int(PermissionMaxInput); val++ { _, err := NewPermissions(val) if err != nil { t.Errorf("value %d should be a valid permissions", val) @@ -34,7 +34,7 @@ func TestNewPermissions(t *testing.T) { func TestNewPermissionsWithInvalidValueShouldFail(t *testing.T) { vals := []int{ -1, - int(PermissionAll) + 1, + int(PermissionMaxInput) + 1, } for _, v := range vals { _, err := NewPermissions(v) diff --git a/pkg/storage/utils/grants/grants.go b/pkg/storage/utils/grants/grants.go index 1d32550cbd..e49b3d56ea 100644 --- a/pkg/storage/utils/grants/grants.go +++ b/pkg/storage/utils/grants/grants.go @@ -31,7 +31,7 @@ import ( // TODO(labkode): fine grained permission controls. func GetACLPerm(set *provider.ResourcePermissions) (string, error) { // resource permission is denied - if cmp.Equal(provider.ResourcePermissions{}, *set) { + if cmp.Equal(provider.ResourcePermissions{}, *set, ignoreProtobufXxx()) { return "!r!w!x!m!u!d", nil } @@ -135,10 +135,19 @@ func GetGranteeType(aclType string) provider.GranteeType { // PermissionsEqual returns true if the permissions are equal func PermissionsEqual(p1, p2 *provider.ResourcePermissions) bool { - return p1 != nil && p2 != nil && cmp.Equal(*p1, *p2) + return p1 != nil && p2 != nil && cmp.Equal(*p1, *p2, ignoreProtobufXxx()) } // GranteeEqual returns true if the grantee are equal func GranteeEqual(g1, g2 *provider.Grantee) bool { - return g1 != nil && g2 != nil && cmp.Equal(*g1, *g2) + return g1 != nil && g2 != nil && cmp.Equal(*g1, *g2, ignoreProtobufXxx()) +} + +func ignoreProtobufXxx() cmp.Option { + return cmp.FilterPath( + func(path cmp.Path) bool { + return strings.HasPrefix(path.String(), "XXX") + }, + cmp.Ignore(), + ) } From 8347d4d9fd551cb7aee1275ef97c5ec0c5fcf863 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Thu, 30 Jun 2022 16:10:42 +0200 Subject: [PATCH 2/2] unskip passing tetsts --- .../acceptance/expected-failures-on-OCIS-storage.md | 13 ------------- .../acceptance/expected-failures-on-S3NG-storage.md | 13 ------------- 2 files changed, 26 deletions(-) diff --git a/tests/acceptance/expected-failures-on-OCIS-storage.md b/tests/acceptance/expected-failures-on-OCIS-storage.md index 51fafeab95..59cb42466d 100644 --- a/tests/acceptance/expected-failures-on-OCIS-storage.md +++ b/tests/acceptance/expected-failures-on-OCIS-storage.md @@ -1441,19 +1441,6 @@ moving outside of the Shares folder gives 501 Not Implemented. - [apiMain/checksums.feature:233](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L233) -#### empty permissions on a link is now allowed - -- [apiShareUpdateToShares/updateShare.feature:113](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareUpdateToShares/updateShare.feature#L113) -- [apiShareUpdateToShares/updateShare.feature:114](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareUpdateToShares/updateShare.feature#L114) -- [apiShareManagementBasicToShares/createShareToSharesFolder.feature:117](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareManagementBasicToShares/createShareToSharesFolder.feature#L117) -- [apiShareManagementBasicToShares/createShareToSharesFolder.feature:118](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareManagementBasicToShares/createShareToSharesFolder.feature#L118) -- [apiShareManagementBasicToShares/createShareToSharesFolder.feature:132](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareManagementBasicToShares/createShareToSharesFolder.feature#L132) -- [apiShareManagementBasicToShares/createShareToSharesFolder.feature:133](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareManagementBasicToShares/createShareToSharesFolder.feature#L133) -- [apiShareCreateSpecialToShares2/createShareWithInvalidPermissions.feature:26](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareCreateSpecialToShares2/createShareWithInvalidPermissions.feature#L26) -- [apiShareCreateSpecialToShares2/createShareWithInvalidPermissions.feature:27](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareCreateSpecialToShares2/createShareWithInvalidPermissions.feature#L27) -- [apiShareCreateSpecialToShares2/createShareWithInvalidPermissions.feature:28](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareCreateSpecialToShares2/createShareWithInvalidPermissions.feature#L28) -- [apiShareCreateSpecialToShares2/createShareWithInvalidPermissions.feature:29](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareCreateSpecialToShares2/createShareWithInvalidPermissions.feature#L29) - #### [OCS status code zero](https://github.com/owncloud/ocis/issues/3621) - [apiShareManagementToShares/moveReceivedShare.feature:32](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareManagementToShares/moveReceivedShare.feature#L32) diff --git a/tests/acceptance/expected-failures-on-S3NG-storage.md b/tests/acceptance/expected-failures-on-S3NG-storage.md index 9fa1946e7b..408f81139e 100644 --- a/tests/acceptance/expected-failures-on-S3NG-storage.md +++ b/tests/acceptance/expected-failures-on-S3NG-storage.md @@ -1439,19 +1439,6 @@ _ocs: api compatibility, return correct status code_ - [apiMain/checksums.feature:233](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/checksums.feature#L233) -#### empty permissions on a link is now allowed - -- [apiShareUpdateToShares/updateShare.feature:113](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareUpdateToShares/updateShare.feature#L113) -- [apiShareUpdateToShares/updateShare.feature:114](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareUpdateToShares/updateShare.feature#L114) -- [apiShareManagementBasicToShares/createShareToSharesFolder.feature:117](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareManagementBasicToShares/createShareToSharesFolder.feature#L117) -- [apiShareManagementBasicToShares/createShareToSharesFolder.feature:118](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareManagementBasicToShares/createShareToSharesFolder.feature#L118) -- [apiShareManagementBasicToShares/createShareToSharesFolder.feature:132](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareManagementBasicToShares/createShareToSharesFolder.feature#L132) -- [apiShareManagementBasicToShares/createShareToSharesFolder.feature:133](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareManagementBasicToShares/createShareToSharesFolder.feature#L133) -- [apiShareCreateSpecialToShares2/createShareWithInvalidPermissions.feature:26](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareCreateSpecialToShares2/createShareWithInvalidPermissions.feature#L26) -- [apiShareCreateSpecialToShares2/createShareWithInvalidPermissions.feature:27](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareCreateSpecialToShares2/createShareWithInvalidPermissions.feature#L27) -- [apiShareCreateSpecialToShares2/createShareWithInvalidPermissions.feature:28](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareCreateSpecialToShares2/createShareWithInvalidPermissions.feature#L28) -- [apiShareCreateSpecialToShares2/createShareWithInvalidPermissions.feature:29](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareCreateSpecialToShares2/createShareWithInvalidPermissions.feature#L29) - #### [OCS status code zero](https://github.com/owncloud/ocis/issues/3621) - [apiShareManagementToShares/moveReceivedShare.feature:32](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareManagementToShares/moveReceivedShare.feature#L32)