From 903b513d9cc9e8b5e4f8d52f07f285f5c4d54265 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Fri, 30 Aug 2024 16:19:42 +0200 Subject: [PATCH] fix: implement review --- .../collaboration/pkg/connector/connector.go | 30 ++++++-------- .../pkg/connector/contentconnector.go | 5 +-- .../pkg/connector/fileconnector.go | 12 +++--- .../pkg/connector/fileconnector_test.go | 6 +-- .../pkg/connector/httpadapter.go | 1 + .../pkg/connector/httpadapter_test.go | 4 +- services/collaboration/pkg/helpers/path.go | 41 ------------------- services/collaboration/pkg/helpers/version.go | 20 --------- .../pkg/middleware/wopicontext.go | 36 +++++++++++++++- 9 files changed, 59 insertions(+), 96 deletions(-) delete mode 100644 services/collaboration/pkg/helpers/path.go delete mode 100644 services/collaboration/pkg/helpers/version.go diff --git a/services/collaboration/pkg/connector/connector.go b/services/collaboration/pkg/connector/connector.go index 96d196748..0e1e821bf 100644 --- a/services/collaboration/pkg/connector/connector.go +++ b/services/collaboration/pkg/connector/connector.go @@ -1,8 +1,9 @@ package connector import ( + "strconv" + types "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" - "github.com/owncloud/ocis/v2/services/collaboration/pkg/helpers" ) // ConnectorResponse represent a response from the FileConnectorService. @@ -57,24 +58,11 @@ func NewResponseLockConflict(lockID string, lockFailureReason string) *Connector // NewResponseWithVersion creates a new ConnectorResponse with the specified status // and the "X-WOPI-ItemVersion" header having the value in the mtime parameter. -func NewResponseWithVersion(status int, mtime *types.Timestamp) *ConnectorResponse { +func NewResponseWithVersion(mtime *types.Timestamp) *ConnectorResponse { return &ConnectorResponse{ - Status: status, + Status: 200, Headers: map[string]string{ - HeaderWopiVersion: helpers.GetVersion(mtime), - }, - } -} - -// NewResponseConflictWithVersion creates a new ConnectorResponse with the status 409 -// and the "X-WOPI-ItemVersion" header having the value in the mtime parameter. -// The lockFailureReason parameter will be included in the "X-WOPI-LockFailureReason". -func NewResponseConflictWithVersion(mtime *types.Timestamp, lockFailureReason string) *ConnectorResponse { - return &ConnectorResponse{ - Status: 409, - Headers: map[string]string{ - HeaderWopiVersion: helpers.GetVersion(mtime), - HeaderWopiLockFailureReason: lockFailureReason, + HeaderWopiVersion: getVersion(mtime), }, } } @@ -86,7 +74,7 @@ func NewResponseWithVersionAndLock(status int, mtime *types.Timestamp, lockID st r := &ConnectorResponse{ Status: status, Headers: map[string]string{ - HeaderWopiVersion: helpers.GetVersion(mtime), + HeaderWopiVersion: getVersion(mtime), HeaderWopiLock: lockID, }, } @@ -196,3 +184,9 @@ func (c *Connector) GetFileConnector() FileConnectorService { func (c *Connector) GetContentConnector() ContentConnectorService { return c.contentConnector } + +// getVersion returns a string representation of the timestamp +func getVersion(timestamp *types.Timestamp) string { + return "v" + strconv.FormatUint(timestamp.GetSeconds(), 10) + + strconv.FormatUint(uint64(timestamp.GetNanos()), 10) +} diff --git a/services/collaboration/pkg/connector/contentconnector.go b/services/collaboration/pkg/connector/contentconnector.go index fab2f1b33..3f4fbf026 100644 --- a/services/collaboration/pkg/connector/contentconnector.go +++ b/services/collaboration/pkg/connector/contentconnector.go @@ -17,7 +17,6 @@ import ( revactx "github.com/cs3org/reva/v2/pkg/ctx" "github.com/owncloud/ocis/v2/ocis-pkg/tracing" "github.com/owncloud/ocis/v2/services/collaboration/pkg/config" - "github.com/owncloud/ocis/v2/services/collaboration/pkg/helpers" "github.com/owncloud/ocis/v2/services/collaboration/pkg/middleware" "github.com/rs/zerolog" "go.opentelemetry.io/otel/propagation" @@ -181,7 +180,7 @@ func (c *ContentConnector) GetFile(ctx context.Context, w http.ResponseWriter) e return NewConnectorError(500, "GetFile: Downloading the file failed") } - helpers.SetVersionHeader(w, sResp.GetInfo().GetMtime()) + w.Header().Set(HeaderWopiVersion, getVersion(sResp.GetInfo().GetMtime())) // Copy the download into the writer _, err = io.Copy(w, httpResp.Body) @@ -404,5 +403,5 @@ func (c *ContentConnector) PutFile(ctx context.Context, stream io.Reader, stream } logger.Debug().Msg("PutFile: success") - return NewResponseWithVersion(200, mtime), nil + return NewResponseWithVersion(mtime), nil } diff --git a/services/collaboration/pkg/connector/fileconnector.go b/services/collaboration/pkg/connector/fileconnector.go index 64712a901..a934fb5f2 100644 --- a/services/collaboration/pkg/connector/fileconnector.go +++ b/services/collaboration/pkg/connector/fileconnector.go @@ -259,7 +259,7 @@ func (f *FileConnector) Lock(ctx context.Context, lockID, oldLockID string) (*Co switch setOrRefreshStatus.GetCode() { case rpcv1beta1.Code_CODE_OK: logger.Debug().Msg("SetLock successful") - return NewResponseWithVersion(200, statResp.GetInfo().GetMtime()), nil + return NewResponseWithVersion(statResp.GetInfo().GetMtime()), nil case rpcv1beta1.Code_CODE_FAILED_PRECONDITION, rpcv1beta1.Code_CODE_ABORTED: // Code_CODE_FAILED_PRECONDITION -> Lock operation mismatched lock @@ -300,7 +300,7 @@ func (f *FileConnector) Lock(ctx context.Context, lockID, oldLockID string) (*Co logger.Warn(). Str("LockID", resp.GetLock().GetLockId()). Msg("SetLock lock refreshed instead") - return NewResponseWithVersionAndLock(200, statResp.GetInfo().GetMtime(), resp.GetLock().GetLockId()), nil + return NewResponseWithVersion(statResp.GetInfo().GetMtime()), nil } logger.Error().Msg("SetLock failed and could not refresh") @@ -388,7 +388,7 @@ func (f *FileConnector) RefreshLock(ctx context.Context, lockID string) (*Connec logger.Debug().Msg("RefreshLock successful") // The current lock should not be returned in the headers on success // https://learn.microsoft.com/en-us/microsoft-365/cloud-storage-partner-program/rest/files/refreshlock#response-headers - return NewResponseWithVersion(200, statResp.GetInfo().GetMtime()), nil + return NewResponseWithVersion(statResp.GetInfo().GetMtime()), nil case rpcv1beta1.Code_CODE_NOT_FOUND: logger.Error(). @@ -428,7 +428,7 @@ func (f *FileConnector) RefreshLock(ctx context.Context, lockID string) (*Connec Str("StatusCode", resp.GetStatus().GetCode().String()). Str("StatusMsg", resp.GetStatus().GetMessage()). Msg("RefreshLock failed, no lock on file") - return NewResponseConflictWithVersion(statResp.GetInfo().GetMtime(), "No lock on file"), nil + return NewResponseLockConflict("", "No lock on file"), nil } else { // lock is different than the one requested, otherwise we wouldn't reached this point logger.Error(). @@ -510,7 +510,7 @@ func (f *FileConnector) UnLock(ctx context.Context, lockID string) (*ConnectorRe switch resp.GetStatus().GetCode() { case rpcv1beta1.Code_CODE_OK: logger.Debug().Msg("Unlock successful") - return NewResponseWithVersion(200, statResp.GetInfo().GetMtime()), nil + return NewResponseWithVersion(statResp.GetInfo().GetMtime()), nil case rpcv1beta1.Code_CODE_ABORTED: // File isn't locked. Need to return 409 with empty lock logger.Error().Err(err).Msg("Unlock failed, file isn't locked") @@ -1114,7 +1114,7 @@ func (f *FileConnector) CheckFileInfo(ctx context.Context) (*ConnectorResponse, infoMap := map[string]interface{}{ fileinfo.KeyOwnerID: hexEncodedOwnerId, fileinfo.KeySize: int64(statRes.GetInfo().GetSize()), - fileinfo.KeyVersion: helpers.GetVersion(statRes.GetInfo().GetMtime()), + fileinfo.KeyVersion: getVersion(statRes.GetInfo().GetMtime()), fileinfo.KeyBaseFileName: path.Base(statRes.GetInfo().GetPath()), fileinfo.KeyBreadcrumbDocName: path.Base(statRes.GetInfo().GetPath()), // to get the folder we actually need to do a GetPath() request diff --git a/services/collaboration/pkg/connector/fileconnector_test.go b/services/collaboration/pkg/connector/fileconnector_test.go index 9afd01482..8dfa4961f 100644 --- a/services/collaboration/pkg/connector/fileconnector_test.go +++ b/services/collaboration/pkg/connector/fileconnector_test.go @@ -255,8 +255,7 @@ var _ = Describe("FileConnector", func() { response, err := fc.Lock(ctx, "abcdef123", "") Expect(err).ToNot(HaveOccurred()) Expect(response.Status).To(Equal(200)) - Expect(response.Headers).To(HaveLen(2)) - Expect(response.Headers[connector.HeaderWopiLock]).To(Equal("abcdef123")) + Expect(response.Headers).To(HaveLen(1)) Expect(response.Headers[connector.HeaderWopiVersion]).To(Equal("v123456789")) }) @@ -437,8 +436,7 @@ var _ = Describe("FileConnector", func() { response, err := fc.Lock(ctx, "abcdef123", "112233") Expect(err).ToNot(HaveOccurred()) Expect(response.Status).To(Equal(200)) - Expect(response.Headers).To(HaveLen(2)) - Expect(response.Headers[connector.HeaderWopiLock]).To(Equal("abcdef123")) + Expect(response.Headers).To(HaveLen(1)) Expect(response.Headers[connector.HeaderWopiVersion]).To(Equal("v123456789")) }) diff --git a/services/collaboration/pkg/connector/httpadapter.go b/services/collaboration/pkg/connector/httpadapter.go index 04046c5b6..b0a427945 100644 --- a/services/collaboration/pkg/connector/httpadapter.go +++ b/services/collaboration/pkg/connector/httpadapter.go @@ -51,6 +51,7 @@ func NewHttpAdapter(gwc gatewayv1beta1.GatewayAPIClient, cfg *config.Config) *Ht ), } + // TODO: check if we can get rid of custom log parsing completely httpAdapter.locks = &locks.NoopLockParser{} return httpAdapter } diff --git a/services/collaboration/pkg/connector/httpadapter_test.go b/services/collaboration/pkg/connector/httpadapter_test.go index 4d4c9db05..3d3d97c7a 100644 --- a/services/collaboration/pkg/connector/httpadapter_test.go +++ b/services/collaboration/pkg/connector/httpadapter_test.go @@ -357,9 +357,7 @@ var _ = Describe("HttpAdapter", func() { w := httptest.NewRecorder() fc.On("UnLock", mock.Anything, "abc123").Times(1).Return( - connector.NewResponseWithVersion(200, - &typesv1beta1.Timestamp{Seconds: uint64(1234), Nanos: uint32(567)}, - ), nil) + connector.NewResponseWithVersion(&typesv1beta1.Timestamp{Seconds: uint64(1234), Nanos: uint32(567)}), nil) httpAdapter.UnLock(w, req) resp := w.Result() diff --git a/services/collaboration/pkg/helpers/path.go b/services/collaboration/pkg/helpers/path.go deleted file mode 100644 index 290b85b3b..000000000 --- a/services/collaboration/pkg/helpers/path.go +++ /dev/null @@ -1,41 +0,0 @@ -package helpers - -import ( - "strings" - - "github.com/golang-jwt/jwt/v5" - "github.com/owncloud/ocis/v2/services/collaboration/pkg/config" -) - -// ParseWopiFileID extracts the file id from a wopi path -// -// If the file id is a jwt, it will be decoded and the file id will be extracted from the jwt claims. -// If the file id is not a jwt, it will be returned as is. -func ParseWopiFileID(cfg *config.Config, path string) string { - s := strings.Split(path, "/") - if len(s) < 4 || (s[1] != "wopi" && s[2] != "files") { - return path - } - // check if the fileid is a jwt - if strings.Contains(s[3], ".") { - token, err := jwt.Parse(s[3], func(_ *jwt.Token) (interface{}, error) { - return []byte(cfg.Wopi.ProxySecret), nil - }) - if err != nil { - return s[3] - } - - claims, ok := token.Claims.(jwt.MapClaims) - if !ok { - return s[3] - } - - f, ok := claims["f"].(string) - if !ok { - return s[3] - } - return f - } - // fileid is not a jwt - return s[3] -} diff --git a/services/collaboration/pkg/helpers/version.go b/services/collaboration/pkg/helpers/version.go deleted file mode 100644 index a9196da06..000000000 --- a/services/collaboration/pkg/helpers/version.go +++ /dev/null @@ -1,20 +0,0 @@ -package helpers - -import ( - "net/http" - "strconv" - - typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" -) - -// SetVersionHeader sets a WOPI version header on the response writer -func SetVersionHeader(w http.ResponseWriter, t *typesv1beta1.Timestamp) { - // non-canonical headers can only be set directly on the header map - w.Header().Set("X-WOPI-ItemVersion", GetVersion(t)) -} - -// GetVersion returns a string representation of the timestamp -func GetVersion(timestamp *typesv1beta1.Timestamp) string { - return "v" + strconv.FormatUint(timestamp.GetSeconds(), 10) + - strconv.FormatUint(uint64(timestamp.GetNanos()), 10) -} diff --git a/services/collaboration/pkg/middleware/wopicontext.go b/services/collaboration/pkg/middleware/wopicontext.go index b02e1c7c3..c69308a4c 100644 --- a/services/collaboration/pkg/middleware/wopicontext.go +++ b/services/collaboration/pkg/middleware/wopicontext.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net/http" + "strings" appproviderv1beta1 "github.com/cs3org/go-cs3apis/cs3/app/provider/v1beta1" providerv1beta1 "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" @@ -109,7 +110,7 @@ func WopiContextAuthMiddleware(cfg *config.Config, next http.Handler) http.Handl ctx = wopiLogger.WithContext(ctx) hashedRef := helpers.HashResourceId(claims.WopiContext.FileReference.GetResourceId()) - fileID := helpers.ParseWopiFileID(cfg, r.URL.Path) + fileID := parseWopiFileID(cfg, r.URL.Path) if fileID != hashedRef { wopiLogger.Error().Msg("file reference in the URL doesn't match the one inside the access token") http.Error(w, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized) @@ -167,3 +168,36 @@ func GenerateWopiToken(wopiContext WopiContext, cfg *config.Config) (string, int return accessToken, claims.ExpiresAt.UnixMilli(), err } + +// parseWopiFileID extracts the file id from a wopi path +// +// If the file id is a jwt, it will be decoded and the file id will be extracted from the jwt claims. +// If the file id is not a jwt, it will be returned as is. +func parseWopiFileID(cfg *config.Config, path string) string { + s := strings.Split(path, "/") + if len(s) < 4 || (s[1] != "wopi" && s[2] != "files") { + return path + } + // check if the fileid is a jwt + if strings.Contains(s[3], ".") { + token, err := jwt.Parse(s[3], func(_ *jwt.Token) (interface{}, error) { + return []byte(cfg.Wopi.ProxySecret), nil + }) + if err != nil { + return s[3] + } + + claims, ok := token.Claims.(jwt.MapClaims) + if !ok { + return s[3] + } + + f, ok := claims["f"].(string) + if !ok { + return s[3] + } + return f + } + // fileid is not a jwt + return s[3] +}