From 08cb22850004d02aa0da2e2b4e8da4dd8b6db326 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Wed, 24 Jul 2024 21:02:54 +0200 Subject: [PATCH 1/3] feat: add new locks parser for microsoft office online server --- services/collaboration/pkg/config/app.go | 2 +- .../pkg/connector/httpadapter.go | 29 +++++--- services/collaboration/pkg/locks/parser.go | 72 +++++++++++++++++++ .../collaboration/pkg/server/http/server.go | 3 +- 4 files changed, 94 insertions(+), 12 deletions(-) create mode 100644 services/collaboration/pkg/locks/parser.go diff --git a/services/collaboration/pkg/config/app.go b/services/collaboration/pkg/config/app.go index f54d058f6..e255f21e8 100644 --- a/services/collaboration/pkg/config/app.go +++ b/services/collaboration/pkg/config/app.go @@ -2,7 +2,7 @@ package config // App defines the available app configuration. type App struct { - Name string `yaml:"name" env:"COLLABORATION_APP_NAME" desc:"The name of the app, either Collabora, OnlyOffice or Microsoft365" introductionVersion:"6.0.0"` + Name string `yaml:"name" env:"COLLABORATION_APP_NAME" desc:"The name of the app, either Collabora, OnlyOffice, Microsoft365 or MicrosoftOfficeOnline" introductionVersion:"6.0.0"` Description string `yaml:"description" env:"COLLABORATION_APP_DESCRIPTION" desc:"App description" introductionVersion:"6.0.0"` Icon string `yaml:"icon" env:"COLLABORATION_APP_ICON" desc:"Icon for the app" introductionVersion:"6.0.0"` LockName string `yaml:"lockname" env:"COLLABORATION_APP_LOCKNAME" desc:"Name for the app lock" introductionVersion:"6.0.0"` diff --git a/services/collaboration/pkg/connector/httpadapter.go b/services/collaboration/pkg/connector/httpadapter.go index 4075722b4..5e7fb809d 100644 --- a/services/collaboration/pkg/connector/httpadapter.go +++ b/services/collaboration/pkg/connector/httpadapter.go @@ -5,9 +5,11 @@ import ( "errors" "net/http" "strconv" + "strings" gatewayv1beta1 "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" "github.com/owncloud/ocis/v2/services/collaboration/pkg/config" + "github.com/owncloud/ocis/v2/services/collaboration/pkg/locks" "github.com/rs/zerolog" ) @@ -25,25 +27,34 @@ const ( // All operations are expected to follow the definitions found in // https://learn.microsoft.com/en-us/microsoft-365/cloud-storage-partner-program/rest/endpoints type HttpAdapter struct { - con ConnectorService + con ConnectorService + config *config.Config + locks locks.LockParser } // NewHttpAdapter will create a new HTTP adapter. A new connector using the // provided gateway API client and configuration will be used in the adapter func NewHttpAdapter(gwc gatewayv1beta1.GatewayAPIClient, cfg *config.Config) *HttpAdapter { - return &HttpAdapter{ + httpAdapter := &HttpAdapter{ con: NewConnector( NewFileConnector(gwc, cfg), NewContentConnector(gwc, cfg), ), } + + httpAdapter.locks = &locks.NoopLockParser{} + if strings.ToLower(cfg.App.Name) == "microsoftofficeonline" { + httpAdapter.locks = &locks.LegacyLockParser{} + } + return httpAdapter } // NewHttpAdapterWithConnector will create a new HTTP adapter that will use // the provided connector service -func NewHttpAdapterWithConnector(con ConnectorService) *HttpAdapter { +func NewHttpAdapterWithConnector(con ConnectorService, l locks.LockParser) *HttpAdapter { return &HttpAdapter{ - con: con, + con: con, + locks: l, } } @@ -75,8 +86,8 @@ func (h *HttpAdapter) GetLock(w http.ResponseWriter, r *http.Request) { // The operation's response will be sent through the response writer and // the headers according to the spec func (h *HttpAdapter) Lock(w http.ResponseWriter, r *http.Request) { - oldLockID := r.Header.Get(HeaderWopiOldLock) - lockID := r.Header.Get(HeaderWopiLock) + oldLockID := h.locks.ParseLock(r.Header.Get(HeaderWopiOldLock)) + lockID := h.locks.ParseLock(r.Header.Get(HeaderWopiLock)) fileCon := h.con.GetFileConnector() newLockID, err := fileCon.Lock(r.Context(), lockID, oldLockID) @@ -103,7 +114,7 @@ func (h *HttpAdapter) Lock(w http.ResponseWriter, r *http.Request) { // The operation's response will be sent through the response writer and // the headers according to the spec func (h *HttpAdapter) RefreshLock(w http.ResponseWriter, r *http.Request) { - lockID := r.Header.Get(HeaderWopiLock) + lockID := h.locks.ParseLock(r.Header.Get(HeaderWopiLock)) fileCon := h.con.GetFileConnector() newLockID, err := fileCon.RefreshLock(r.Context(), lockID) @@ -128,7 +139,7 @@ func (h *HttpAdapter) RefreshLock(w http.ResponseWriter, r *http.Request) { // The operation's response will be sent through the response writer and // the headers according to the spec func (h *HttpAdapter) UnLock(w http.ResponseWriter, r *http.Request) { - lockID := r.Header.Get(HeaderWopiLock) + lockID := h.locks.ParseLock(r.Header.Get(HeaderWopiLock)) fileCon := h.con.GetFileConnector() newLockID, err := fileCon.UnLock(r.Context(), lockID) @@ -211,7 +222,7 @@ func (h *HttpAdapter) GetFile(w http.ResponseWriter, r *http.Request) { // The operation's response will be sent through the response writer and // the headers according to the spec func (h *HttpAdapter) PutFile(w http.ResponseWriter, r *http.Request) { - lockID := r.Header.Get(HeaderWopiLock) + lockID := h.locks.ParseLock(r.Header.Get(HeaderWopiLock)) contentCon := h.con.GetContentConnector() newLockID, err := contentCon.PutFile(r.Context(), r.Body, r.ContentLength, lockID) diff --git a/services/collaboration/pkg/locks/parser.go b/services/collaboration/pkg/locks/parser.go new file mode 100644 index 000000000..022e59adf --- /dev/null +++ b/services/collaboration/pkg/locks/parser.go @@ -0,0 +1,72 @@ +// Package locks provides functionality to parse lockIDs. +// +// It can be used to bridge requests from different clients that send lockIDs in different formats. +// For example, Microsoft Office Online sends the lockID in a JSON string, +// while other clients send the lockID as a plain string. +package locks + +import ( + "encoding/json" + "strings" +) + +// LockParser is the interface that wraps the ParseLock method +type LockParser interface { + ParseLock(id string) string +} + +// LegacyLockParser is a lock parser that can extract the lockID from a JSON string +type LegacyLockParser struct{} + +// NoopLockParser is a lock parser that does not change the lockID +type NoopLockParser struct{} + +// ParseLock will return the lockID as is +func (*NoopLockParser) ParseLock(id string) string { + return id +} + +// ParseLock extracts the lockID from a JSON string. +// For Microsoft Office Online we need to extract the lockID from the JSON string +// that is sent by the WOPI client. +// The JSON string is expected to have the following format: +// +// { +// "L": "12345678", +// "F": 4, +// "E": 2, +// "C": "", +// "P": "3453345345346", +// "M": "12345678" +// } +// +// or +// +// { +// "S": "12345678", +// "F": 4, +// "E": 2, +// "C": "", +// "P": "3453345345346", +// "M": "12345678" +// } +// +// If the JSON string is not in the expected format, the original lockID will be returned. +func (*LegacyLockParser) ParseLock(id string) string { + var decodedValues map[string]interface{} + err := json.NewDecoder(strings.NewReader(id)).Decode(&decodedValues) + if err != nil || len(decodedValues) == 0 { + return id + } + if v, ok := decodedValues["L"]; ok { + if idString, ok := v.(string); ok { + return idString + } + } + if v, ok := decodedValues["S"]; ok { + if idString, ok := v.(string); ok { + return idString + } + } + return id +} diff --git a/services/collaboration/pkg/server/http/server.go b/services/collaboration/pkg/server/http/server.go index 5eab32766..6218d0383 100644 --- a/services/collaboration/pkg/server/http/server.go +++ b/services/collaboration/pkg/server/http/server.go @@ -118,8 +118,7 @@ func prepareRoutes(r *chi.Mux, options Options) { r.Use(func(h stdhttp.Handler) stdhttp.Handler { // authentication and wopi context return colabmiddleware.WopiContextAuthMiddleware(options.Config.Wopi.Secret, h) - }, - ) + }) r.Get("/", func(w stdhttp.ResponseWriter, r *stdhttp.Request) { adapter.CheckFileInfo(w, r) From 4c63ddc4390d92935c5b438a2946c41dac744a1c Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Wed, 24 Jul 2024 21:08:12 +0200 Subject: [PATCH 2/3] tests: add and adapt unit tests --- services/collaboration/.mockery.yaml | 5 ++ .../collaboration/mocks/connector_service.go | 2 +- .../mocks/content_connector_service.go | 2 +- .../mocks/file_connector_service.go | 2 +- services/collaboration/mocks/lock_parser.go | 78 +++++++++++++++++ .../pkg/connector/httpadapter.go | 5 +- .../pkg/connector/httpadapter_test.go | 8 +- .../collaboration/pkg/locks/parser_test.go | 87 +++++++++++++++++++ 8 files changed, 182 insertions(+), 7 deletions(-) create mode 100644 services/collaboration/mocks/lock_parser.go create mode 100644 services/collaboration/pkg/locks/parser_test.go diff --git a/services/collaboration/.mockery.yaml b/services/collaboration/.mockery.yaml index 87425509b..090f21c63 100644 --- a/services/collaboration/.mockery.yaml +++ b/services/collaboration/.mockery.yaml @@ -10,3 +10,8 @@ packages: ConnectorService: ContentConnectorService: FileConnectorService: + github.com/owncloud/ocis/v2/services/collaboration/pkg/locks: + config: + dir: "mocks" + interfaces: + LockParser: diff --git a/services/collaboration/mocks/connector_service.go b/services/collaboration/mocks/connector_service.go index ca93a6f56..4285ec1d8 100644 --- a/services/collaboration/mocks/connector_service.go +++ b/services/collaboration/mocks/connector_service.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.40.2. DO NOT EDIT. +// Code generated by mockery v2.43.2. DO NOT EDIT. package mocks diff --git a/services/collaboration/mocks/content_connector_service.go b/services/collaboration/mocks/content_connector_service.go index 25cd2a2ec..9b034f5da 100644 --- a/services/collaboration/mocks/content_connector_service.go +++ b/services/collaboration/mocks/content_connector_service.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.40.2. DO NOT EDIT. +// Code generated by mockery v2.43.2. DO NOT EDIT. package mocks diff --git a/services/collaboration/mocks/file_connector_service.go b/services/collaboration/mocks/file_connector_service.go index 6a156d04f..e7fa9808e 100644 --- a/services/collaboration/mocks/file_connector_service.go +++ b/services/collaboration/mocks/file_connector_service.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.40.2. DO NOT EDIT. +// Code generated by mockery v2.43.2. DO NOT EDIT. package mocks diff --git a/services/collaboration/mocks/lock_parser.go b/services/collaboration/mocks/lock_parser.go new file mode 100644 index 000000000..8c66ab16e --- /dev/null +++ b/services/collaboration/mocks/lock_parser.go @@ -0,0 +1,78 @@ +// Code generated by mockery v2.43.2. DO NOT EDIT. + +package mocks + +import mock "github.com/stretchr/testify/mock" + +// LockParser is an autogenerated mock type for the LockParser type +type LockParser struct { + mock.Mock +} + +type LockParser_Expecter struct { + mock *mock.Mock +} + +func (_m *LockParser) EXPECT() *LockParser_Expecter { + return &LockParser_Expecter{mock: &_m.Mock} +} + +// ParseLock provides a mock function with given fields: id +func (_m *LockParser) ParseLock(id string) string { + ret := _m.Called(id) + + if len(ret) == 0 { + panic("no return value specified for ParseLock") + } + + var r0 string + if rf, ok := ret.Get(0).(func(string) string); ok { + r0 = rf(id) + } else { + r0 = ret.Get(0).(string) + } + + return r0 +} + +// LockParser_ParseLock_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'ParseLock' +type LockParser_ParseLock_Call struct { + *mock.Call +} + +// ParseLock is a helper method to define mock.On call +// - id string +func (_e *LockParser_Expecter) ParseLock(id interface{}) *LockParser_ParseLock_Call { + return &LockParser_ParseLock_Call{Call: _e.mock.On("ParseLock", id)} +} + +func (_c *LockParser_ParseLock_Call) Run(run func(id string)) *LockParser_ParseLock_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(string)) + }) + return _c +} + +func (_c *LockParser_ParseLock_Call) Return(_a0 string) *LockParser_ParseLock_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *LockParser_ParseLock_Call) RunAndReturn(run func(string) string) *LockParser_ParseLock_Call { + _c.Call.Return(run) + return _c +} + +// NewLockParser creates a new instance of LockParser. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewLockParser(t interface { + mock.TestingT + Cleanup(func()) +}) *LockParser { + mock := &LockParser{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/services/collaboration/pkg/connector/httpadapter.go b/services/collaboration/pkg/connector/httpadapter.go index 5e7fb809d..ea014482a 100644 --- a/services/collaboration/pkg/connector/httpadapter.go +++ b/services/collaboration/pkg/connector/httpadapter.go @@ -27,9 +27,8 @@ const ( // All operations are expected to follow the definitions found in // https://learn.microsoft.com/en-us/microsoft-365/cloud-storage-partner-program/rest/endpoints type HttpAdapter struct { - con ConnectorService - config *config.Config - locks locks.LockParser + con ConnectorService + locks locks.LockParser } // NewHttpAdapter will create a new HTTP adapter. A new connector using the diff --git a/services/collaboration/pkg/connector/httpadapter_test.go b/services/collaboration/pkg/connector/httpadapter_test.go index 52623e736..390740e51 100644 --- a/services/collaboration/pkg/connector/httpadapter_test.go +++ b/services/collaboration/pkg/connector/httpadapter_test.go @@ -20,6 +20,7 @@ var _ = Describe("HttpAdapter", func() { fc *mocks.FileConnectorService cc *mocks.ContentConnectorService con *mocks.ConnectorService + locks *mocks.LockParser httpAdapter *connector.HttpAdapter ) @@ -31,7 +32,12 @@ var _ = Describe("HttpAdapter", func() { con.On("GetContentConnector").Return(cc) con.On("GetFileConnector").Return(fc) - httpAdapter = connector.NewHttpAdapterWithConnector(con) + locks = &mocks.LockParser{} + locks.EXPECT().ParseLock(mock.Anything).RunAndReturn(func(id string) string { + return id + }) + + httpAdapter = connector.NewHttpAdapterWithConnector(con, locks) }) Describe("GetLock", func() { diff --git a/services/collaboration/pkg/locks/parser_test.go b/services/collaboration/pkg/locks/parser_test.go new file mode 100644 index 000000000..3efac6d00 --- /dev/null +++ b/services/collaboration/pkg/locks/parser_test.go @@ -0,0 +1,87 @@ +package locks + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestLegacyLockParser(t *testing.T) { + tests := []struct { + name string + lock string + cleanLock string + }{ + { + name: "JsonStringWithLKey", + lock: createJsonString(map[string]interface{}{"L": "12345678", "F": 4, "E": 2, "C": "", "P": "3453345345346", "M": "12345678"}), + cleanLock: "12345678", + }, + { + name: "JsonStringWithSKey", + lock: createJsonString(map[string]interface{}{"S": "12345678", "F": 4, "E": 2, "C": "", "P": "3453345345346", "M": "12345678"}), + cleanLock: "12345678", + }, + { + name: "PlainString", + lock: "12345678", + cleanLock: "12345678", + }, + { + name: "JsonStringUnknownFormat", + lock: createJsonString(map[string]interface{}{"A": "12345678", "F": 4, "E": 2, "C": "", "P": "3453345345346", "X": "12345678"}), + cleanLock: `{"A":"12345678","C":"","E":2,"F":4,"P":"3453345345346","X":"12345678"}`, + }, + { + name: "InvalidJsonString", + lock: `"A":"12345678","C":"","E":2,"F":4,"P":"3453345345346","X":"12345678"}`, + cleanLock: `"A":"12345678","C":"","E":2,"F":4,"P":"3453345345346","X":"12345678"}`, + }, + { + name: "EmptyString", + lock: "", + cleanLock: "", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + lockParser := &LegacyLockParser{} + lock := lockParser.ParseLock(test.lock) + assert.Equal(t, test.cleanLock, lock) + }) + } +} + +func TestNoopLockParser(t *testing.T) { + tests := []struct { + name string + lock string + }{ + { + name: "PlainString", + lock: "123", + }, + { + name: "EmptyString", + lock: "", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + lockParser := &NoopLockParser{} + lock := lockParser.ParseLock(test.lock) + assert.Equal(t, test.lock, lock) + }) + } +} + +func createJsonString(input map[string]interface{}) string { + rawData, err := json.Marshal(&input) + if err != nil { + return "" + } + return string(rawData) +} From 8b70f6f288164be7697bbfb60c452343f39b46ec Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Wed, 24 Jul 2024 21:20:30 +0200 Subject: [PATCH 3/3] docs: add changelog --- changelog/unreleased/collaboration-msoo-locking.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/unreleased/collaboration-msoo-locking.md diff --git a/changelog/unreleased/collaboration-msoo-locking.md b/changelog/unreleased/collaboration-msoo-locking.md new file mode 100644 index 000000000..cc00bcbd1 --- /dev/null +++ b/changelog/unreleased/collaboration-msoo-locking.md @@ -0,0 +1,6 @@ +Enhancement: Add locking support for MS Office Online Server + +We added support for the special kind of lock tokens that MS Office Online Server uses to lock files via the Wopi protocol. +It will only be active if you set the `COLLABORATION_APP_NAME` environment variable to `MicrosoftOfficeOnline`. + +https://github.com/owncloud/ocis/pull/9685