Skip to content

Commit

Permalink
Merge pull request #4421 from rhafer/update-share-permission-check
Browse files Browse the repository at this point in the history 
Move more share permission checks to usershareprovider
  • Loading branch information
@kobergj
kobergj authored Dec 20, 2023
2 parents 073f9b9 + 0431ec9 commit 726f76e
Show file tree
Hide file tree
Showing 3 changed files with 162 additions and 19 deletions.
6 changes: 4 additions & 2 deletions changelog/unreleased/add-update-share-permission-check.md
View file
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
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
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

0 comments on commit 726f76e

Please sign in to comment.