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

Move more share permission checks to usershareprovider #4421

Merged
merged 4 commits into from
Dec 20, 2023
Merged

Move more share permission checks to usershareprovider #4421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
712164a
usershareprovider: Check for AddGrant permission before creating a share
rhafer Dec 19, 2023
0b13cb2
usershareprovider: Additional permission checks on share deletion
rhafer Dec 19, 2023
23939ae
usershareprovider: Additional permission checks on share update
rhafer Dec 19, 2023
0431ec9
Update changelog
rhafer Dec 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions changelog/unreleased/add-update-share-permission-check.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
Enhancement: Check permissions before updating shares
Enhancement: Check permissions before adding, deleting or updating shares

The user share provider now checks if the user has sufficient permissions to update a share.
The user share provider now checks if the user has sufficient permissions to
add, delete or update a share.

https://github.com/cs3org/reva/pull/4421
https://github.com/cs3org/reva/pull/4405
53 changes: 52 additions & 1 deletion internal/grpc/services/usershareprovider/usershareprovider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ func (s *service) isPathAllowed(path string) bool {
}

func (s *service) CreateShare(ctx context.Context, req *collaboration.CreateShareRequest) (*collaboration.CreateShareResponse, error) {
log := appctx.GetLogger(ctx)
user := ctxpkg.ContextMustGetUser(ctx)

gatewayClient, err := s.gatewaySelector.Next()
Expand Down Expand Up @@ -184,9 +185,22 @@ func (s *service) CreateShare(ctx context.Context, req *collaboration.CreateShar
}
}

sRes, err := gatewayClient.Stat(ctx, &provider.StatRequest{Ref: &provider.Reference{ResourceId: req.GetResourceInfo().GetId()}})
if err != nil {
log.Err(err).Interface("resource_id", req.GetResourceInfo().GetId()).Msg("failed to stat resource to share")
return &collaboration.CreateShareResponse{
Status: status.NewInternal(ctx, "failed to stat shared resource"),
}, err
}
// the user needs to have the AddGrant permissions on the Resource to be able to create a share
if !sRes.GetInfo().GetPermissionSet().AddGrant {
return &collaboration.CreateShareResponse{
Status: status.NewPermissionDenied(ctx, nil, "no permission to add grants on shared resource"),
}, err
}
// check if the requested share creation has sufficient permissions to do so.
if shareCreationAllowed := conversions.SufficientCS3Permissions(
req.GetResourceInfo().GetPermissionSet(),
sRes.GetInfo().GetPermissionSet(),
req.GetGrant().GetPermissions().GetPermissions(),
); !shareCreationAllowed {
return &collaboration.CreateShareResponse{
Expand Down Expand Up @@ -214,13 +228,38 @@ func (s *service) CreateShare(ctx context.Context, req *collaboration.CreateShar
}

func (s *service) RemoveShare(ctx context.Context, req *collaboration.RemoveShareRequest) (*collaboration.RemoveShareResponse, error) {
log := appctx.GetLogger(ctx)
user := ctxpkg.ContextMustGetUser(ctx)
share, err := s.sm.GetShare(ctx, req.Ref)
if err != nil {
return &collaboration.RemoveShareResponse{
Status: status.NewInternal(ctx, "error getting share"),
}, nil
}

gatewayClient, err := s.gatewaySelector.Next()
if err != nil {
return nil, err
}
sRes, err := gatewayClient.Stat(ctx, &provider.StatRequest{Ref: &provider.Reference{ResourceId: share.GetResourceId()}})
if err != nil {
log.Err(err).Interface("resource_id", share.GetResourceId()).Msg("failed to stat shared resource")
return &collaboration.RemoveShareResponse{
Status: status.NewInternal(ctx, "failed to stat shared resource"),
}, err
}
// the requesting user needs to be either the Owner/Creator of the share or have the RemoveGrant permissions on the Resource
switch {
case utils.UserEqual(user.GetId(), share.GetCreator()) || utils.UserEqual(user.GetId(), share.GetOwner()):
fallthrough
case sRes.GetInfo().GetPermissionSet().RemoveGrant:
break
default:
return &collaboration.RemoveShareResponse{
Status: status.NewPermissionDenied(ctx, nil, "no permission to remove grants on shared resource"),
}, err
}

err = s.sm.Unshare(ctx, req.Ref)
if err != nil {
return &collaboration.RemoveShareResponse{
Expand Down Expand Up @@ -279,6 +318,7 @@ func (s *service) ListShares(ctx context.Context, req *collaboration.ListSharesR

func (s *service) UpdateShare(ctx context.Context, req *collaboration.UpdateShareRequest) (*collaboration.UpdateShareResponse, error) {
log := appctx.GetLogger(ctx)
user := ctxpkg.ContextMustGetUser(ctx)
gatewayClient, err := s.gatewaySelector.Next()
if err != nil {
return nil, err
Expand Down Expand Up @@ -326,6 +366,17 @@ func (s *service) UpdateShare(ctx context.Context, req *collaboration.UpdateShar
Status: status.NewInternal(ctx, "failed to stat shared resource"),
}, err
}
// the requesting user needs to be either the Owner/Creator of the share or have the UpdateGrant permissions on the Resource
switch {
case utils.UserEqual(user.GetId(), currentShare.GetCreator()) || utils.UserEqual(user.GetId(), currentShare.GetOwner()):
fallthrough
case sRes.GetInfo().GetPermissionSet().UpdateGrant:
break
default:
return &collaboration.UpdateShareResponse{
Status: status.NewPermissionDenied(ctx, nil, "no permission to remove grants on shared resource"),
}, err
}

// If this is a permissions update, check if user's permissions on the resource are sufficient to set the desired permissions
var newPermissions *provider.ResourcePermissions
Expand Down
122 changes: 106 additions & 16 deletions internal/grpc/services/usershareprovider/usershareprovider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,18 @@ import (

var _ = Describe("user share provider service", func() {
var (
ctx context.Context
provider collaborationpb.CollaborationAPIServer
manager *mocks.Manager
gatewayClient *cs3mocks.GatewayAPIClient
gatewaySelector pool.Selectable[gateway.GatewayAPIClient]
checkPermissionResponse *permissions.CheckPermissionResponse
statResourceResponse *providerpb.StatResponse
ctx context.Context
provider collaborationpb.CollaborationAPIServer
manager *mocks.Manager
gatewayClient *cs3mocks.GatewayAPIClient
gatewaySelector pool.Selectable[gateway.GatewayAPIClient]
checkPermissionResponse *permissions.CheckPermissionResponse
statResourceResponse *providerpb.StatResponse
cs3permissionsNoAddGrant *providerpb.ResourcePermissions
getShareResponse *collaborationpb.Share
)
cs3permissionsNoAddGrant = conversions.RoleFromName("manager", true).CS3ResourcePermissions()
cs3permissionsNoAddGrant.AddGrant = false

BeforeEach(func() {
manager = &mocks.Manager{}
Expand Down Expand Up @@ -87,8 +91,14 @@ var _ = Describe("user share provider service", func() {
},
}
gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResourceResponse, nil)
alice := &userpb.User{
Id: &userpb.UserId{
OpaqueId: "alice",
},
Username: "alice",
}

getShareResponse := &collaborationpb.Share{
getShareResponse = &collaborationpb.Share{
Id: &collaborationpb.ShareId{
OpaqueId: "shareid",
},
Expand All @@ -97,6 +107,8 @@ var _ = Describe("user share provider service", func() {
SpaceId: "spaceid",
OpaqueId: "opaqueid",
},
Owner: alice.Id,
Creator: alice.Id,
}
manager.On("GetShare", mock.Anything, mock.Anything).Return(getShareResponse, nil)

Expand All @@ -105,12 +117,7 @@ var _ = Describe("user share provider service", func() {
provider = rgrpcService.(collaborationpb.CollaborationAPIServer)
Expect(provider).ToNot(BeNil())

ctx = ctxpkg.ContextSetUser(context.Background(), &userpb.User{
Id: &userpb.UserId{
OpaqueId: "alice",
},
Username: "alice",
})
ctx = ctxpkg.ContextSetUser(context.Background(), alice)
})

Describe("CreateShare", func() {
Expand All @@ -125,6 +132,8 @@ var _ = Describe("user share provider service", func() {
manager.On("Share", mock.Anything, mock.Anything, mock.Anything).Return(&collaborationpb.Share{}, nil)
checkPermissionResponse.Status.Code = checkPermissionStatusCode

statResourceResponse.Info.PermissionSet = resourceInfoPermissions

createShareResponse, err := provider.CreateShare(ctx, &collaborationpb.CreateShareRequest{
ResourceInfo: &providerpb.ResourceInfo{
PermissionSet: resourceInfoPermissions,
Expand Down Expand Up @@ -157,6 +166,14 @@ var _ = Describe("user share provider service", func() {
rpcpb.Code_CODE_OK,
1,
),
Entry(
"no AddGrant permission on resource",
cs3permissionsNoAddGrant,
conversions.RoleFromName("spaceeditor", true).CS3ResourcePermissions(),
rpcpb.Code_CODE_OK,
rpcpb.Code_CODE_PERMISSION_DENIED,
0,
),
Entry(
"no WriteShare permission on user role",
conversions.RoleFromName("manager", true).CS3ResourcePermissions(),
Expand Down Expand Up @@ -193,7 +210,6 @@ var _ = Describe("user share provider service", func() {
It("fails when the user tries to share with elevated permissions", func() {
// user has only read access
statResourceResponse.Info.PermissionSet = &providerpb.ResourcePermissions{
UpdateGrant: true,
InitiateFileDownload: true,
Stat: true,
}
Expand Down Expand Up @@ -226,13 +242,87 @@ var _ = Describe("user share provider service", func() {

manager.AssertNumberOfCalls(GinkgoT(), "UpdateShare", 0)
})
It("succeeds when the user has sufficient permissions", func() {
It("succeeds when the user is not the owner/creator and does not have the UpdateGrant permissions", func() {
// user has only read access
statResourceResponse.Info.PermissionSet = &providerpb.ResourcePermissions{
InitiateFileDownload: true,
Stat: true,
}
bobId := &userpb.UserId{OpaqueId: "bob"}
getShareResponse.Owner = bobId
getShareResponse.Creator = bobId

// user tries to update a share to give write permissions
updateShareResponse, err := provider.UpdateShare(ctx, &collaborationpb.UpdateShareRequest{
Ref: &collaborationpb.ShareReference{
Spec: &collaborationpb.ShareReference_Id{
Id: &collaborationpb.ShareId{
OpaqueId: "shareid",
},
},
},
Share: &collaborationpb.Share{
Permissions: &collaborationpb.SharePermissions{
Permissions: &providerpb.ResourcePermissions{
Stat: true,
InitiateFileDownload: true,
},
},
},
UpdateMask: &fieldmaskpb.FieldMask{
Paths: []string{"permissions"},
},
})

Expect(err).ToNot(HaveOccurred())
Expect(updateShareResponse.Status.Code).To(Equal(rpcpb.Code_CODE_PERMISSION_DENIED))

manager.AssertNumberOfCalls(GinkgoT(), "UpdateShare", 0)
})
It("succeeds when the user is the owner/creator", func() {
// user has only read access
statResourceResponse.Info.PermissionSet = &providerpb.ResourcePermissions{
InitiateFileDownload: true,
Stat: true,
}

// user tries to update a share to give write permissions
updateShareResponse, err := provider.UpdateShare(ctx, &collaborationpb.UpdateShareRequest{
Ref: &collaborationpb.ShareReference{
Spec: &collaborationpb.ShareReference_Id{
Id: &collaborationpb.ShareId{
OpaqueId: "shareid",
},
},
},
Share: &collaborationpb.Share{
Permissions: &collaborationpb.SharePermissions{
Permissions: &providerpb.ResourcePermissions{
Stat: true,
InitiateFileDownload: true,
},
},
},
UpdateMask: &fieldmaskpb.FieldMask{
Paths: []string{"permissions"},
},
})

Expect(err).ToNot(HaveOccurred())
Expect(updateShareResponse.Status.Code).To(Equal(rpcpb.Code_CODE_OK))

manager.AssertNumberOfCalls(GinkgoT(), "UpdateShare", 1)
})
It("succeeds when the user is not the owner/creator but has the UpdateGrant permissions", func() {
// user has only read access
statResourceResponse.Info.PermissionSet = &providerpb.ResourcePermissions{
UpdateGrant: true,
InitiateFileDownload: true,
Stat: true,
}
bobId := &userpb.UserId{OpaqueId: "bob"}
getShareResponse.Owner = bobId
getShareResponse.Creator = bobId

// user tries to update a share to give write permissions
updateShareResponse, err := provider.UpdateShare(ctx, &collaborationpb.UpdateShareRequest{
Expand Down
Loading