Repository: owncloud/ocis
Pull request #9685 (2024-07-24 19:13:22)

[webOffice] Microsoft Office Online Server Lock support
<!--
Thanks for submitting a change to ownCloud!

This is the bug tracker for the oCIS component. Find other components at https://github.com/owncloud/core/blob/master/.github/CONTRIBUTING.md#guidelines

For fixing potential security issues please see https://owncloud.org/security/

To make it possible for us to get your change reviewed and merged please carefully fill out the requested information below.

Please note that any kind of change needs first be submitted to the master branch which holds the next version of oCIS.

Please set the following labels:

- Set label "Status:Needs-Review" for review or "Status:In-Progress" if the PR still has open tasks.
- Assignment: assign to self
- Reviewers: pick at least one
-->

## Description

Add a lock parser to be ready for Microsoft Office Online Server.

It will only try to parse the locks out of a json string when `COLLABORATION_APP_NAME=MicrosoftOfficeOnline`.

## Related Issue
<!--- This project only accepts pull requests related to open issues -->
<!--- If suggesting a new feature or change, please discuss it in an issue first -->
<!--- If fixing a bug, there should be an issue describing it with steps to reproduce -->
<!--- Please link to the issue here: -->
- Relates to https://github.com/owncloud/enterprise/issues/6790

## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->

## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->
- test environment:
- test case 1:
- test case 2:
- ...

## Screenshots (if appropriate):

## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] Technical debt
- [ ] Tests only (no source changes)

## Checklist:
<!-- Tick the checkboxes when done. -->
<!-- Raise documentation ticket in owncloud.github.io/ -->
- [x] Code changes
- [x] Unit tests added
- [ ] Acceptance tests added
- [ ] Documentation ticket raised: <link> 


update-docs[bot]: Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a [changelog](https://github.com/owncloud/ocis/blob/master/changelog/README.md) item based on your changes.


Patch:
diff --git a/changelog/unreleased/collaboration-msoo-locking.md b/changelog/unreleased/collaboration-msoo-locking.md
new file mode 100644
index 00000000000..cc00bcbd163
--- /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
diff --git a/services/collaboration/.mockery.yaml b/services/collaboration/.mockery.yaml
index 87425509bcd..090f21c63a1 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 ca93a6f562f..4285ec1d8d0 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 25cd2a2ec7d..9b034f5da2d 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 6a156d04f1a..e7fa9808ec4 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 00000000000..8c66ab16e2e
--- /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/config/app.go b/services/collaboration/pkg/config/app.go
index f54d058f63f..e255f21e8ca 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 4075722b48f..ea014482a4c 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,33 @@ 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
+	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 +85,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 +113,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 +138,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 +221,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 00000000000..022e59adff7
--- /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 5eab327664e..6218d038392 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)


Test patch:
diff --git a/services/collaboration/pkg/connector/httpadapter_test.go b/services/collaboration/pkg/connector/httpadapter_test.go
index 52623e73682..390740e51fd 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 00000000000..3efac6d001a
--- /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)
+}