From c4bd2f3987de1b85d0948cb64718fc9f65a9d45d Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Wed, 23 Oct 2024 18:14:41 +0200 Subject: [PATCH 1/2] fix: add missing urls to putRelative --- changelog/unreleased/fix-put-relative.md | 5 +++ .../collaboration/pkg/connector/connector.go | 8 ++-- .../pkg/connector/fileconnector.go | 42 ++++++++++++++----- 3 files changed, 41 insertions(+), 14 deletions(-) create mode 100644 changelog/unreleased/fix-put-relative.md diff --git a/changelog/unreleased/fix-put-relative.md b/changelog/unreleased/fix-put-relative.md new file mode 100644 index 00000000000..7c825b53f6d --- /dev/null +++ b/changelog/unreleased/fix-put-relative.md @@ -0,0 +1,5 @@ +Bugfix: Fix put relative wopi operation for microsoft + +We fixed a bug in the put relative wopi operation for microsoft. The response now contains the correct properties. + +https://github.com/owncloud/ocis/pull/10403 diff --git a/services/collaboration/pkg/connector/connector.go b/services/collaboration/pkg/connector/connector.go index 0e1e821bfb3..27a7ea8e646 100644 --- a/services/collaboration/pkg/connector/connector.go +++ b/services/collaboration/pkg/connector/connector.go @@ -112,12 +112,14 @@ func NewResponseSuccessBodyName(name string) *ConnectorResponse { // contain "Name" and "Url" keys with their respective suplied values // // This is used in the `PutRelativeFile` methods (both suggested and relative). -func NewResponseSuccessBodyNameUrl(name, url string) *ConnectorResponse { +func NewResponseSuccessBodyNameUrl(name, url string, hostEditURL string, hostViewURL string) *ConnectorResponse { return &ConnectorResponse{ Status: 200, Body: map[string]interface{}{ - "Name": name, - "Url": url, + "Name": name, + "Url": url, + "HostEditUrl": hostEditURL, + "HostViewUrl": hostViewURL, }, } } diff --git a/services/collaboration/pkg/connector/fileconnector.go b/services/collaboration/pkg/connector/fileconnector.go index f6156ac4931..cdf7c2840bd 100644 --- a/services/collaboration/pkg/connector/fileconnector.go +++ b/services/collaboration/pkg/connector/fileconnector.go @@ -713,9 +713,9 @@ func (f *FileConnector) PutRelativeFileSuggested(ctx context.Context, ccs Conten return NewResponse(500), nil } } - + var newInfo *providerv1beta1.ResourceInfo // adjust the wopi file reference to use only the resource id without path - if err := f.adjustWopiReference(ctx, &wopiContext, newLogger); err != nil { + if newInfo, err = f.adjustWopiReference(ctx, &wopiContext, newLogger); err != nil { return nil, err } @@ -729,7 +729,16 @@ func (f *FileConnector) PutRelativeFileSuggested(ctx context.Context, ccs Conten Str("FinalReference", wopiContext.FileReference.String()). Msg("PutRelativeFileSuggested: success") - return NewResponseSuccessBodyNameUrl(finalTarget, wopiSrcURL.String()), nil + webURL, err := url.Parse(f.cfg.Commons.OcisURL) + if err != nil { + return nil, err + } + return NewResponseSuccessBodyNameUrl( + finalTarget, + wopiSrcURL.String(), + createHostUrl("write", webURL, strings.ToLower(f.cfg.App.Name), newInfo), + createHostUrl("view", webURL, strings.ToLower(f.cfg.App.Name), newInfo), + ), nil } // PutRelativeFileRelative upload a file using the provided target name @@ -811,10 +820,11 @@ func (f *FileConnector) PutRelativeFileRelative(ctx context.Context, ccs Content lockID = putResponse.Headers[HeaderWopiLock] } + var newInfo *providerv1beta1.ResourceInfo switch putResponse.Status { case 200: // success case, so don't do anything case 409: - if err := f.adjustWopiReference(ctx, &wopiContext, newLogger); err != nil { + if newInfo, err = f.adjustWopiReference(ctx, &wopiContext, newLogger); err != nil { return nil, err } // if conflict generate a different name and retry. @@ -855,7 +865,7 @@ func (f *FileConnector) PutRelativeFileRelative(ctx context.Context, ccs Content return nil, NewConnectorError(putResponse.Status, "put file failed with unhandled status") } - if err := f.adjustWopiReference(ctx, &wopiContext, newLogger); err != nil { + if newInfo, err = f.adjustWopiReference(ctx, &wopiContext, newLogger); err != nil { return nil, err } @@ -867,7 +877,17 @@ func (f *FileConnector) PutRelativeFileRelative(ctx context.Context, ccs Content newLogger.Debug().Msg("PutRelativeFileRelative: success") - return NewResponseSuccessBodyNameUrl(target, wopiSrcURL.String()), nil + webURL, err := url.Parse(f.cfg.Commons.OcisURL) + if err != nil { + return nil, err + } + + return NewResponseSuccessBodyNameUrl( + target, + wopiSrcURL.String(), + createHostUrl("write", webURL, strings.ToLower(f.cfg.App.Name), newInfo), + createHostUrl("view", webURL, strings.ToLower(f.cfg.App.Name), newInfo), + ), nil } // DeleteFile will delete the requested file @@ -1413,10 +1433,10 @@ func (f *FileConnector) generateWOPISrc(wopiContext middleware.WopiContext, logg return wopiSrcURL, nil } -func (f *FileConnector) adjustWopiReference(ctx context.Context, wopiContext *middleware.WopiContext, logger zerolog.Logger) error { +func (f *FileConnector) adjustWopiReference(ctx context.Context, wopiContext *middleware.WopiContext, logger zerolog.Logger) (*providerv1beta1.ResourceInfo, error) { gwc, err := f.gws.Next() if err != nil { - return err + return nil, err } // using resourceid + path won't do for WOPI, we need just the resource if of the new file // the wopicontext has resourceid + path, which is good enough for the stat request @@ -1425,7 +1445,7 @@ func (f *FileConnector) adjustWopiReference(ctx context.Context, wopiContext *mi }) if err != nil { logger.Error().Err(err).Msg("stat failed") - return err + return nil, err } if newStatRes.GetStatus().GetCode() != rpcv1beta1.Code_CODE_OK { @@ -1433,12 +1453,12 @@ func (f *FileConnector) adjustWopiReference(ctx context.Context, wopiContext *mi Str("StatusCode", newStatRes.GetStatus().GetCode().String()). Str("StatusMsg", newStatRes.GetStatus().GetMessage()). Msg("stat failed with unexpected status") - return NewConnectorError(500, newStatRes.GetStatus().GetCode().String()+" "+newStatRes.GetStatus().GetMessage()) + return nil, NewConnectorError(500, newStatRes.GetStatus().GetCode().String()+" "+newStatRes.GetStatus().GetMessage()) } // adjust the reference in the wopi context to use only the resourceid without the path wopiContext.FileReference = &providerv1beta1.Reference{ ResourceId: newStatRes.GetInfo().GetId(), } - return nil + return newStatRes.GetInfo(), nil } From 406c8ca98d17b4bac823d8f740e68392e1df5420 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Thu, 24 Oct 2024 16:30:33 +0200 Subject: [PATCH 2/2] tests: add checks to unit tests --- .../pkg/connector/fileconnector.go | 16 ++++--- .../pkg/connector/fileconnector_test.go | 46 +++++++++++++------ 2 files changed, 40 insertions(+), 22 deletions(-) diff --git a/services/collaboration/pkg/connector/fileconnector.go b/services/collaboration/pkg/connector/fileconnector.go index cdf7c2840bd..b8dc73382d5 100644 --- a/services/collaboration/pkg/connector/fileconnector.go +++ b/services/collaboration/pkg/connector/fileconnector.go @@ -821,6 +821,11 @@ func (f *FileConnector) PutRelativeFileRelative(ctx context.Context, ccs Content } var newInfo *providerv1beta1.ResourceInfo + webURL, err := url.Parse(f.cfg.Commons.OcisURL) + if err != nil { + return nil, err + } + switch putResponse.Status { case 200: // success case, so don't do anything case 409: @@ -854,8 +859,10 @@ func (f *FileConnector) PutRelativeFileRelative(ctx context.Context, ccs Content HeaderWopiLockFailureReason: "Lock Conflict", }, Body: map[string]interface{}{ - "Name": target, - "Url": wopiSrcURL.String(), + "Name": target, + "Url": wopiSrcURL.String(), + "HostViewUrl": createHostUrl("view", webURL, strings.ToLower(f.cfg.App.Name), newInfo), + "HostEditUrl": createHostUrl("write", webURL, strings.ToLower(f.cfg.App.Name), newInfo), }, }, nil default: @@ -877,11 +884,6 @@ func (f *FileConnector) PutRelativeFileRelative(ctx context.Context, ccs Content newLogger.Debug().Msg("PutRelativeFileRelative: success") - webURL, err := url.Parse(f.cfg.Commons.OcisURL) - if err != nil { - return nil, err - } - return NewResponseSuccessBodyNameUrl( target, wopiSrcURL.String(), diff --git a/services/collaboration/pkg/connector/fileconnector_test.go b/services/collaboration/pkg/connector/fileconnector_test.go index 34685ff16ae..f03ab5044b4 100644 --- a/services/collaboration/pkg/connector/fileconnector_test.go +++ b/services/collaboration/pkg/connector/fileconnector_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/hex" "errors" + "net/url" "path" "regexp" "strings" @@ -26,6 +27,7 @@ import ( "github.com/owncloud/ocis/v2/services/collaboration/pkg/middleware" "github.com/owncloud/ocis/v2/services/graph/mocks" "github.com/stretchr/testify/mock" + "google.golang.org/grpc" ) var _ = Describe("FileConnector", func() { @@ -952,6 +954,8 @@ var _ = Describe("FileConnector", func() { rBody := response.Body.(map[string]interface{}) Expect(rBody["Name"]).To(Equal("newDocument.docx")) Expect(rBody["Url"]).To(HavePrefix("https://ocis.server.prv/wopi/files/")) // skip checking the actual reference + Expect(rBody["HostEditUrl"]).To(Equal("https://ocis.example.prv/external-test/personal/path/to/newDocument.docx?fileId=storageid%24spaceid%21opaqueid_newDoc&view_mode=write")) + Expect(rBody["HostViewUrl"]).To(Equal("https://ocis.example.prv/external-test/personal/path/to/newDocument.docx?fileId=storageid%24spaceid%21opaqueid_newDoc&view_mode=view")) }) It("PutRelativeFileSuggested success only extension", func() { @@ -1005,6 +1009,8 @@ var _ = Describe("FileConnector", func() { rBody := response.Body.(map[string]interface{}) Expect(rBody["Name"]).To(Equal("file.pdf")) Expect(rBody["Url"]).To(HavePrefix("https://ocis.server.prv/wopi/files/")) // skip checking the actual reference + Expect(rBody["HostEditUrl"]).To(Equal("https://ocis.example.prv/external-test/personal/path/to/file.pdf?fileId=storageid%24spaceid%21opaqueid_newDoc&view_mode=write")) + Expect(rBody["HostViewUrl"]).To(Equal("https://ocis.example.prv/external-test/personal/path/to/file.pdf?fileId=storageid%24spaceid%21opaqueid_newDoc&view_mode=view")) }) It("PutRelativeFileSuggested success conflict", func() { @@ -1048,21 +1054,25 @@ var _ = Describe("FileConnector", func() { } return false }) - gatewayClient.On("Stat", mock.Anything, stat2ParamMatcher).Times(1).Return(&providerv1beta1.StatResponse{ - Status: status.NewOK(ctx), - Info: &providerv1beta1.ResourceInfo{ - Path: path.Join("/personal/path/to", *newFilePath), - Id: &providerv1beta1.ResourceId{ - StorageId: "storageid", - OpaqueId: "opaqueid_newDoc", - SpaceId: "spaceid", - }, - Lock: &providerv1beta1.Lock{ - LockId: "zzz999", - Type: providerv1beta1.LockType_LOCK_TYPE_WRITE, - }, - }, - }, nil) + + gatewayClient.EXPECT().Stat(mock.Anything, stat2ParamMatcher). + RunAndReturn(func(ctx context.Context, req *providerv1beta1.StatRequest, opts ...grpc.CallOption) (*providerv1beta1.StatResponse, error) { + return &providerv1beta1.StatResponse{ + Status: status.NewOK(ctx), + Info: &providerv1beta1.ResourceInfo{ + Path: path.Join("/personal/path/to", path.Base(req.GetRef().GetPath())), + Id: &providerv1beta1.ResourceId{ + StorageId: "storageid", + OpaqueId: "opaqueid_newDoc", + SpaceId: "spaceid", + }, + Lock: &providerv1beta1.Lock{ + LockId: "zzz999", + Type: providerv1beta1.LockType_LOCK_TYPE_WRITE, + }, + }, + }, nil + }) response, err := fc.PutRelativeFileSuggested(ctx, ccs, stream, int64(stream.Len()), ".pdf") Expect(err).ToNot(HaveOccurred()) @@ -1071,6 +1081,8 @@ var _ = Describe("FileConnector", func() { rBody := response.Body.(map[string]interface{}) Expect(rBody["Name"]).To(MatchRegexp(`[a-zA-Z0-9_-] file\.pdf`)) Expect(rBody["Url"]).To(HavePrefix("https://ocis.server.prv/wopi/files/")) // skip checking the actual reference + Expect(rBody["HostEditUrl"]).To(Equal("https://ocis.example.prv/external-test/personal/path/to/" + url.PathEscape(path.Base(*newFilePath)) + "?fileId=storageid%24spaceid%21opaqueid_newDoc&view_mode=write")) + Expect(rBody["HostViewUrl"]).To(Equal("https://ocis.example.prv/external-test/personal/path/to/" + url.PathEscape(path.Base(*newFilePath)) + "?fileId=storageid%24spaceid%21opaqueid_newDoc&view_mode=view")) }) It("PutRelativeFileSuggested put file fails", func() { @@ -1193,6 +1205,8 @@ var _ = Describe("FileConnector", func() { rBody := response.Body.(map[string]interface{}) Expect(rBody["Name"]).To(Equal("newDocument.docx")) Expect(rBody["Url"]).To(HavePrefix("https://ocis.server.prv/wopi/files/")) // skip checking the actual reference + Expect(rBody["HostEditUrl"]).To(Equal("https://ocis.example.prv/external-test/personal/path/to/newDocument.docx?fileId=storageid%24spaceid%21opaqueid_newDoc&view_mode=write")) + Expect(rBody["HostViewUrl"]).To(Equal("https://ocis.example.prv/external-test/personal/path/to/newDocument.docx?fileId=storageid%24spaceid%21opaqueid_newDoc&view_mode=view")) }) It("PutRelativeFileRelative conflict", func() { @@ -1250,6 +1264,8 @@ var _ = Describe("FileConnector", func() { rBody := response.Body.(map[string]interface{}) Expect(rBody["Name"]).To(Equal("convFile.pdf")) Expect(rBody["Url"]).To(HavePrefix("https://ocis.server.prv/wopi/files/")) // skip checking the actual reference + Expect(rBody["HostEditUrl"]).To(Equal("https://ocis.example.prv/external-test/personal/path/to/convFile.pdf?fileId=storageid%24spaceid%21opaqueid_newDoc&view_mode=write")) + Expect(rBody["HostViewUrl"]).To(Equal("https://ocis.example.prv/external-test/personal/path/to/convFile.pdf?fileId=storageid%24spaceid%21opaqueid_newDoc&view_mode=view")) }) It("PutRelativeFileRelative put file fails", func() {