diff --git a/changelog/unreleased/add-update-share-permission-check.md b/changelog/unreleased/add-update-share-permission-check.md index 4af4d52f0e..dd9bf58999 100644 --- a/changelog/unreleased/add-update-share-permission-check.md +++ b/changelog/unreleased/add-update-share-permission-check.md @@ -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 diff --git a/internal/grpc/services/usershareprovider/usershareprovider.go b/internal/grpc/services/usershareprovider/usershareprovider.go index 5e09a084e1..d18024d6f7 100644 --- a/internal/grpc/services/usershareprovider/usershareprovider.go +++ b/internal/grpc/services/usershareprovider/usershareprovider.go @@ -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() @@ -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{ @@ -214,6 +228,8 @@ 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{ @@ -221,6 +237,29 @@ func (s *service) RemoveShare(ctx context.Context, req *collaboration.RemoveShar }, 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{ @@ -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 @@ -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 diff --git a/internal/grpc/services/usershareprovider/usershareprovider_test.go b/internal/grpc/services/usershareprovider/usershareprovider_test.go index a7202c7a3d..7e9b4347a1 100644 --- a/internal/grpc/services/usershareprovider/usershareprovider_test.go +++ b/internal/grpc/services/usershareprovider/usershareprovider_test.go @@ -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{} @@ -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", }, @@ -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) @@ -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() { @@ -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, @@ -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(), @@ -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, } @@ -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{