usershareprovider: Additional permission checks on share update

We need to make sure the requesting user is either the owner/creator of the share or has the UpdateGrant permission on the desired resource before deleting a share.
cs3org · Dec 19, 2023 · 23939ae · 23939ae
1 parent 0b13cb2
commit 23939ae
Show file tree

Hide file tree

Showing 2 changed files with 98 additions and 9 deletions.
diff --git a/internal/grpc/services/usershareprovider/usershareprovider.go b/internal/grpc/services/usershareprovider/usershareprovider.go
@@ -318,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
@@ -365,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
@@ -56,6 +56,7 @@ var _ = Describe("user share provider service", func() {
 		checkPermissionResponse  *permissions.CheckPermissionResponse
 		statResourceResponse     *providerpb.StatResponse
 		cs3permissionsNoAddGrant *providerpb.ResourcePermissions
+		getShareResponse         *collaborationpb.Share
 	)
 	cs3permissionsNoAddGrant = conversions.RoleFromName("manager", true).CS3ResourcePermissions()
 	cs3permissionsNoAddGrant.AddGrant = false
@@ -90,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",
 			},
@@ -100,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)
 
@@ -108,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() {
@@ -206,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,
 			}
@@ -239,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{