diff --git a/changelog/unreleased/fix-share-mount-unmount.md b/changelog/unreleased/fix-share-mount-unmount.md new file mode 100644 index 0000000000..da89cdffec --- /dev/null +++ b/changelog/unreleased/fix-share-mount-unmount.md @@ -0,0 +1,6 @@ +Bugfix: Fix the status code for multiple mount and unmount share + +We fixed the status code for multiple mount and unmount share. + +https://github.com/owncloud/ocis/pull/9193 +https://github.com/owncloud/ocis/issues/8876 diff --git a/services/graph/pkg/service/v0/api_drives_drive_item.go b/services/graph/pkg/service/v0/api_drives_drive_item.go index 04d0af046f..962d427769 100644 --- a/services/graph/pkg/service/v0/api_drives_drive_item.go +++ b/services/graph/pkg/service/v0/api_drives_drive_item.go @@ -64,6 +64,12 @@ var ( // ErrNoShares is returned when no shares are found ErrNoShares = errorcode.New(errorcode.ItemNotFound, "no shares found") + + // ErrAlreadyMounted is returned when all shares are already mounted + ErrAlreadyMounted = errorcode.New(errorcode.NameAlreadyExists, "shares already mounted") + + // ErrAlreadyUnmounted is returned when all shares are already unmounted + ErrAlreadyUnmounted = errorcode.New(errorcode.NameAlreadyExists, "shares already unmounted") ) type ( @@ -214,17 +220,39 @@ func (s DrivesDriveItemService) UnmountShare(ctx context.Context, shareID *colla return err } - availableShares, err := s.GetSharesForResource(ctx, share.GetShare().GetResourceId(), []*collaboration.Filter{ + shares, err := s.GetSharesForResource(ctx, share.GetShare().GetResourceId(), []*collaboration.Filter{ { Type: collaboration.Filter_TYPE_STATE, Term: &collaboration.Filter_State{ State: collaboration.ShareState_SHARE_STATE_ACCEPTED, }, }, + { + Type: collaboration.Filter_TYPE_STATE, + Term: &collaboration.Filter_State{ + State: collaboration.ShareState_SHARE_STATE_REJECTED, + }, + }, }) if err != nil { return err } + availableShares := make([]*collaboration.ReceivedShare, 0, 1) + rejectedShares := make([]*collaboration.ReceivedShare, 0, 1) + for _, v := range shares { + switch v.GetState() { + case collaboration.ShareState_SHARE_STATE_ACCEPTED: + availableShares = append(availableShares, v) + case collaboration.ShareState_SHARE_STATE_REJECTED: + rejectedShares = append(rejectedShares, v) + } + } + if len(availableShares) == 0 { + if len(rejectedShares) > 0 { + return ErrAlreadyUnmounted + } + return ErrNoShares + } _, err = s.UpdateShares(ctx, availableShares, func(_ *collaboration.ReceivedShare, request *collaboration.UpdateReceivedShareRequest) { request.Share.State = collaboration.ShareState_SHARE_STATE_REJECTED @@ -246,24 +274,28 @@ func (s DrivesDriveItemService) MountShare(ctx context.Context, resourceID *stor name = filepath.Clean(name) } - availableShares, err := s.GetSharesForResource(ctx, resourceID, []*collaboration.Filter{ - { - Type: collaboration.Filter_TYPE_STATE, - Term: &collaboration.Filter_State{ - State: collaboration.ShareState_SHARE_STATE_PENDING, - }, - }, - { - Type: collaboration.Filter_TYPE_STATE, - Term: &collaboration.Filter_State{ - State: collaboration.ShareState_SHARE_STATE_REJECTED, - }, - }, - }) + shares, err := s.GetSharesForResource(ctx, resourceID, nil) if err != nil { return nil, err } + availableShares := make([]*collaboration.ReceivedShare, 0, len(shares)) + mountedShares := make([]*collaboration.ReceivedShare, 0, 1) + for _, v := range shares { + switch v.GetState() { + case collaboration.ShareState_SHARE_STATE_ACCEPTED: + mountedShares = append(mountedShares, v) + case collaboration.ShareState_SHARE_STATE_PENDING, collaboration.ShareState_SHARE_STATE_REJECTED: + availableShares = append(availableShares, v) + } + } + if len(availableShares) == 0 { + if len(mountedShares) > 0 { + return nil, ErrAlreadyMounted + } + return nil, ErrNoShares + } + updatedShares, err := s.UpdateShares(ctx, availableShares, func(share *collaboration.ReceivedShare, request *collaboration.UpdateReceivedShareRequest) { request.Share.State = collaboration.ShareState_SHARE_STATE_ACCEPTED request.UpdateMask.Paths = append(request.UpdateMask.Paths, _fieldMaskPathState) @@ -328,8 +360,8 @@ func (api DrivesDriveItemApi) DeleteDriveItem(w http.ResponseWriter, r *http.Req shareID := ExtractShareIdFromResourceId(itemID) if err := api.drivesDriveItemService.UnmountShare(ctx, shareID); err != nil { - api.logger.Debug().Err(err).Msg(ErrUnmountShare.Error()) - ErrUnmountShare.Render(w, r) + api.logger.Debug().Err(err).Msg(err.Error()) + errorcode.RenderError(w, r, err) return } @@ -484,11 +516,10 @@ func (api DrivesDriveItemApi) CreateDriveItem(w http.ResponseWriter, r *http.Req return } - mountedShares, err := api.drivesDriveItemService. - MountShare(ctx, &resourceId, requestDriveItem.GetName()) + mountedShares, err := api.drivesDriveItemService.MountShare(ctx, &resourceId, requestDriveItem.GetName()) if err != nil { - api.logger.Debug().Err(err).Msg(ErrMountShare.Error()) - ErrMountShare.Render(w, r) + api.logger.Debug().Err(err).Msg(err.Error()) + errorcode.RenderError(w, r, err) return } diff --git a/services/graph/pkg/service/v0/api_drives_drive_item_test.go b/services/graph/pkg/service/v0/api_drives_drive_item_test.go index e0542a1985..1d2a3ae34e 100644 --- a/services/graph/pkg/service/v0/api_drives_drive_item_test.go +++ b/services/graph/pkg/service/v0/api_drives_drive_item_test.go @@ -297,14 +297,33 @@ var _ = Describe("DrivesDriveItemService", func() { EXPECT(). ListReceivedShares(context.Background(), mock.Anything, mock.Anything, mock.Anything). RunAndReturn(func(ctx context.Context, request *collaborationv1beta1.ListReceivedSharesRequest, option ...grpc.CallOption) (*collaborationv1beta1.ListReceivedSharesResponse, error) { - Expect(request.Filters).To(HaveLen(2)) + Expect(request.Filters).To(HaveLen(3)) Expect(request.Filters[0].Type).To(Equal(collaborationv1beta1.Filter_TYPE_RESOURCE_ID)) Expect(request.Filters[1].Term.(*collaborationv1beta1.Filter_State).State).To(Equal(collaborationv1beta1.ShareState_SHARE_STATE_ACCEPTED)) - return nil, nil + Expect(request.Filters[2].Term.(*collaborationv1beta1.Filter_State).State).To(Equal(collaborationv1beta1.ShareState_SHARE_STATE_REJECTED)) + return &collaborationv1beta1.ListReceivedSharesResponse{ + Status: status.NewOK(context.Background()), + Shares: []*collaborationv1beta1.ReceivedShare{ + {Share: &collaborationv1beta1.Share{Id: &collaborationv1beta1.ShareId{}}, State: collaborationv1beta1.ShareState_SHARE_STATE_REJECTED}, + {Share: &collaborationv1beta1.Share{Id: &collaborationv1beta1.ShareId{}}, State: collaborationv1beta1.ShareState_SHARE_STATE_ACCEPTED}, + {Share: &collaborationv1beta1.Share{Id: &collaborationv1beta1.ShareId{}}, State: collaborationv1beta1.ShareState_SHARE_STATE_ACCEPTED}, + }, + }, nil }). Once() - _ = drivesDriveItemService.UnmountShare(context.Background(), &collaborationv1beta1.ShareId{}) + gatewayClient. + EXPECT(). + UpdateReceivedShare(context.Background(), mock.Anything, mock.Anything). + RunAndReturn(func(ctx context.Context, request *collaborationv1beta1.UpdateReceivedShareRequest, option ...grpc.CallOption) (*collaborationv1beta1.UpdateReceivedShareResponse, error) { + return &collaborationv1beta1.UpdateReceivedShareResponse{ + Status: status.NewOK(ctx), + }, nil + }). + Times(2) + + err := drivesDriveItemService.UnmountShare(context.Background(), &collaborationv1beta1.ShareId{}) + Expect(err).To(BeNil()) }) It("reports some error if one or multiple shares could not be unmounted", func() { @@ -323,7 +342,7 @@ var _ = Describe("DrivesDriveItemService", func() { Status: status.NewOK(context.Background()), Shares: []*collaborationv1beta1.ReceivedShare{ {}, - {Share: &collaborationv1beta1.Share{Id: &collaborationv1beta1.ShareId{}}}, + {Share: &collaborationv1beta1.Share{Id: &collaborationv1beta1.ShareId{}}, State: collaborationv1beta1.ShareState_SHARE_STATE_ACCEPTED}, {}, }, }, nil). @@ -334,19 +353,13 @@ var _ = Describe("DrivesDriveItemService", func() { EXPECT(). UpdateReceivedShare(context.Background(), mock.Anything, mock.Anything). RunAndReturn(func(ctx context.Context, request *collaborationv1beta1.UpdateReceivedShareRequest, option ...grpc.CallOption) (*collaborationv1beta1.UpdateReceivedShareResponse, error) { - if request.GetShare().GetShare().GetId() != nil { - return &collaborationv1beta1.UpdateReceivedShareResponse{ - Status: status.NewOK(ctx), - }, nil - } - return nil, someErr }). - Times(3) + Times(1) err := drivesDriveItemService.UnmountShare(context.Background(), &collaborationv1beta1.ShareId{}) Expect(err).To(MatchError(errorcode.New(errorcode.GeneralException, someErr.Error()))) - Expect(err.(interface{ Unwrap() []error }).Unwrap()).To(HaveLen(2)) + Expect(err.(interface{ Unwrap() []error }).Unwrap()).To(HaveLen(1)) }) }) @@ -362,10 +375,8 @@ var _ = Describe("DrivesDriveItemService", func() { EXPECT(). ListReceivedShares(context.Background(), mock.Anything, mock.Anything, mock.Anything). RunAndReturn(func(ctx context.Context, request *collaborationv1beta1.ListReceivedSharesRequest, option ...grpc.CallOption) (*collaborationv1beta1.ListReceivedSharesResponse, error) { - Expect(request.Filters).To(HaveLen(3)) + Expect(request.Filters).To(HaveLen(1)) Expect(request.Filters[0].Type).To(Equal(collaborationv1beta1.Filter_TYPE_RESOURCE_ID)) - Expect(request.Filters[1].Term.(*collaborationv1beta1.Filter_State).State).To(Equal(collaborationv1beta1.ShareState_SHARE_STATE_PENDING)) - Expect(request.Filters[2].Term.(*collaborationv1beta1.Filter_State).State).To(Equal(collaborationv1beta1.ShareState_SHARE_STATE_REJECTED)) return nil, nil }). Once() @@ -373,6 +384,36 @@ var _ = Describe("DrivesDriveItemService", func() { _, _ = drivesDriveItemService.MountShare(context.Background(), nil, "some") }) + It("reports no errors when shares have mounted", func() { + gatewayClient. + EXPECT(). + ListReceivedShares(context.Background(), mock.Anything, mock.Anything, mock.Anything). + RunAndReturn(func(ctx context.Context, request *collaborationv1beta1.ListReceivedSharesRequest, option ...grpc.CallOption) (*collaborationv1beta1.ListReceivedSharesResponse, error) { + return &collaborationv1beta1.ListReceivedSharesResponse{ + Status: status.NewOK(context.Background()), + Shares: []*collaborationv1beta1.ReceivedShare{ + {Share: &collaborationv1beta1.Share{Id: &collaborationv1beta1.ShareId{}}, State: collaborationv1beta1.ShareState_SHARE_STATE_REJECTED}, + {Share: &collaborationv1beta1.Share{Id: &collaborationv1beta1.ShareId{}}, State: collaborationv1beta1.ShareState_SHARE_STATE_ACCEPTED}, + {Share: &collaborationv1beta1.Share{Id: &collaborationv1beta1.ShareId{}}, State: collaborationv1beta1.ShareState_SHARE_STATE_PENDING}, + }, + }, nil + }). + Once() + gatewayClient. + EXPECT(). + UpdateReceivedShare(context.Background(), mock.Anything, mock.Anything). + RunAndReturn(func(ctx context.Context, request *collaborationv1beta1.UpdateReceivedShareRequest, option ...grpc.CallOption) (*collaborationv1beta1.UpdateReceivedShareResponse, error) { + return &collaborationv1beta1.UpdateReceivedShareResponse{ + Status: status.NewOK(ctx), + }, nil + }). + Times(2) + + shares, err := drivesDriveItemService.MountShare(context.Background(), nil, "some") + Expect(err).To(BeNil()) + Expect(shares).To(HaveLen(2)) + }) + It("fails if the update instructions produce as many errors as there are shares", func() { gatewayClient. EXPECT(). @@ -380,9 +421,9 @@ var _ = Describe("DrivesDriveItemService", func() { Return(&collaborationv1beta1.ListReceivedSharesResponse{ Status: status.NewOK(context.Background()), Shares: []*collaborationv1beta1.ReceivedShare{ - {}, - {}, - {}, + {Share: &collaborationv1beta1.Share{Id: &collaborationv1beta1.ShareId{}}, State: collaborationv1beta1.ShareState_SHARE_STATE_PENDING}, + {Share: &collaborationv1beta1.Share{Id: &collaborationv1beta1.ShareId{}}, State: collaborationv1beta1.ShareState_SHARE_STATE_PENDING}, + {Share: &collaborationv1beta1.Share{Id: &collaborationv1beta1.ShareId{}}, State: collaborationv1beta1.ShareState_SHARE_STATE_PENDING}, }, }, nil). Once() @@ -408,7 +449,7 @@ var _ = Describe("DrivesDriveItemService", func() { Status: status.NewOK(context.Background()), Shares: []*collaborationv1beta1.ReceivedShare{ {}, - {Share: &collaborationv1beta1.Share{Id: &collaborationv1beta1.ShareId{}}}, + {Share: &collaborationv1beta1.Share{Id: &collaborationv1beta1.ShareId{}}, State: collaborationv1beta1.ShareState_SHARE_STATE_PENDING}, {}, }, }, nil). @@ -430,7 +471,7 @@ var _ = Describe("DrivesDriveItemService", func() { Status: status.NewOK(ctx), }, nil }). - Times(3) + Once() shares, err := drivesDriveItemService.MountShare(context.Background(), nil, "some") Expect(err).To(BeNil()) @@ -518,7 +559,7 @@ var _ = Describe("DrivesDriveItemApi", func() { failOnNonShareJailDriveID(drivesDriveItemApi.DeleteDriveItem) - It("fails if unmounting the share fails", func() { + It("fails if unmounting the share fails with the general error", func() { rCTX.URLParams.Add("driveID", "a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668") rCTX.URLParams.Add("itemID", "a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668!1") w := httptest.NewRecorder() @@ -534,10 +575,10 @@ var _ = Describe("DrivesDriveItemApi", func() { Once() drivesDriveItemApi.DeleteDriveItem(w, r) - Expect(w.Code).To(Equal(http.StatusBadRequest)) + Expect(w.Code).To(Equal(http.StatusInternalServerError)) jsonData := gjson.Get(w.Body.String(), "error") - Expect(jsonData.Get("code").String() + ": " + jsonData.Get("message").String()).To(Equal(svc.ErrUnmountShare.Error())) + Expect(jsonData.Get("code").String() + ": " + jsonData.Get("message").String()).To(Equal("generalException: some error")) }) It("successfully unmounts the share", func() { @@ -1021,7 +1062,7 @@ var _ = Describe("DrivesDriveItemApi", func() { Expect(jsonData.Get("code").String() + ": " + jsonData.Get("message").String()).To(Equal(svc.ErrInvalidID.Error())) }) - It("fails if mounting the share fails", func() { + It("fails if mounting the share fails with the general error", func() { rCTX.URLParams.Add("driveID", "a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668") rCTX.URLParams.Add("itemID", "a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668!1") @@ -1046,10 +1087,10 @@ var _ = Describe("DrivesDriveItemApi", func() { ) drivesDriveItemApi.CreateDriveItem(w, r) - Expect(w.Code).To(Equal(http.StatusBadRequest)) + Expect(w.Code).To(Equal(http.StatusInternalServerError)) jsonData := gjson.Get(w.Body.String(), "error") - Expect(jsonData.Get("code").String() + ": " + jsonData.Get("message").String()).To(Equal(svc.ErrMountShare.Error())) + Expect(jsonData.Get("code").String() + ": " + jsonData.Get("message").String()).To(Equal("generalException: some error")) }) It("fails if drive item conversion fails", func() { diff --git a/tests/acceptance/features/apiSharingNg/enableDisableShareSync.feature b/tests/acceptance/features/apiSharingNg/enableDisableShareSync.feature index 5aed5dcf33..27f9cd37db 100644 --- a/tests/acceptance/features/apiSharingNg/enableDisableShareSync.feature +++ b/tests/acceptance/features/apiSharingNg/enableDisableShareSync.feature @@ -298,11 +298,10 @@ Feature: enable or disable sync of incoming shares | textfile0.txt | | FolderToShare | - @issue-8724 Scenario: try to enable share sync of a non-existent resource Given user "Brian" has disabled the auto-sync share When user "Brian" tries to enable share sync of a resource "nonexistent" using the Graph API - Then the HTTP status code should be "400" + Then the HTTP status code should be "404" And the JSON data of the response should match """ { @@ -318,7 +317,7 @@ Feature: enable or disable sync of incoming shares ], "properties": { "code" : { - "const": "invalidRequest" + "const": "itemNotFound" }, "innererror" : { "type": "object", @@ -328,7 +327,7 @@ Feature: enable or disable sync of incoming shares ] }, "message" : { - "const": "mounting share failed" + "const": "no shares found" } } } @@ -380,7 +379,7 @@ Feature: enable or disable sync of incoming shares And user "Alice" has uploaded file with content "some data" to "/fileNotShared.txt" And we save it into "FILEID" When user "Brian" tries to enable share sync of a resource "<>" using the Graph API - Then the HTTP status code should be "400" + Then the HTTP status code should be "404" And the JSON data of the response should match """ { @@ -396,7 +395,7 @@ Feature: enable or disable sync of incoming shares ], "properties": { "code" : { - "const": "invalidRequest" + "const": "itemNotFound" }, "innererror" : { "type": "object", @@ -406,7 +405,7 @@ Feature: enable or disable sync of incoming shares ] }, "message" : { - "const": "mounting share failed" + "const": "no shares found" } } } @@ -474,7 +473,7 @@ Feature: enable or disable sync of incoming shares | permissionsRole | Viewer | And the user "Admin" has deleted a user "Alice" When user "Brian" tries to disable sync of share "textfile0.txt" using the Graph API - Then the HTTP status code should be "400" + Then the HTTP status code should be "404" And the JSON data of the response should match """ { @@ -490,7 +489,7 @@ Feature: enable or disable sync of incoming shares ], "properties": { "code" : { - "const": "invalidRequest" + "const": "itemNotFound" }, "innererror" : { "type": "object", @@ -500,7 +499,7 @@ Feature: enable or disable sync of incoming shares ] }, "message" : { - "const": "unmounting share failed" + "const": "no shares found" } } } @@ -559,7 +558,7 @@ Feature: enable or disable sync of incoming shares | permissionsRole | Viewer | And group "grp1" has been deleted When user "Brian" tries to enable share sync of a resource "<>" using the Graph API - Then the HTTP status code should be "400" + Then the HTTP status code should be "404" And the JSON data of the response should match """ { @@ -575,7 +574,7 @@ Feature: enable or disable sync of incoming shares ], "properties": { "code" : { - "const": "invalidRequest" + "const": "itemNotFound" }, "innererror" : { "type": "object", @@ -585,7 +584,7 @@ Feature: enable or disable sync of incoming shares ] }, "message" : { - "const": "mounting share failed" + "const": "no shares found" } } } @@ -609,7 +608,7 @@ Feature: enable or disable sync of incoming shares | permissionsRole | Viewer | And group "grp1" has been deleted When user "Brian" tries to disable sync of share "textfile0.txt" using the Graph API - Then the HTTP status code should be "400" + Then the HTTP status code should be "404" And the JSON data of the response should match """ { @@ -625,7 +624,7 @@ Feature: enable or disable sync of incoming shares ], "properties": { "code" : { - "const": "invalidRequest" + "const": "itemNotFound" }, "innererror" : { "type": "object", @@ -635,7 +634,7 @@ Feature: enable or disable sync of incoming shares ] }, "message" : { - "const": "unmounting share failed" + "const": "error getting received share" } } }