From 6a554e02b21ab5e7dc29d2482118c093e6293a00 Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Wed, 9 Mar 2022 11:14:54 +0100 Subject: [PATCH 01/19] add X-Lock-Id to the datagatway to enable upload to a locked file --- pkg/rhttp/datatx/manager/simple/simple.go | 6 ++++++ pkg/rhttp/datatx/manager/spaces/spaces.go | 5 +++++ 2 files changed, 11 insertions(+) diff --git a/pkg/rhttp/datatx/manager/simple/simple.go b/pkg/rhttp/datatx/manager/simple/simple.go index a7acc5dba2..e732f9be0c 100644 --- a/pkg/rhttp/datatx/manager/simple/simple.go +++ b/pkg/rhttp/datatx/manager/simple/simple.go @@ -23,6 +23,7 @@ import ( provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" "github.com/cs3org/reva/v2/pkg/appctx" + ctxpkg "github.com/cs3org/reva/v2/pkg/ctx" "github.com/cs3org/reva/v2/pkg/errtypes" "github.com/cs3org/reva/v2/pkg/rhttp/datatx" "github.com/cs3org/reva/v2/pkg/rhttp/datatx/manager/registry" @@ -64,6 +65,11 @@ func New(m map[string]interface{}) (datatx.DataTX, error) { func (m *manager) Handler(fs storage.FS) (http.Handler, error) { h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() + + if lockID := r.Header.Get("X-Lock-Id"); lockID != "" { + ctx = ctxpkg.ContextSetLockID(ctx, lockID) + } + sublog := appctx.GetLogger(ctx).With().Str("datatx", "simple").Logger() switch r.Method { diff --git a/pkg/rhttp/datatx/manager/spaces/spaces.go b/pkg/rhttp/datatx/manager/spaces/spaces.go index 6fd9bfb9ef..e0a5aff11c 100644 --- a/pkg/rhttp/datatx/manager/spaces/spaces.go +++ b/pkg/rhttp/datatx/manager/spaces/spaces.go @@ -25,6 +25,7 @@ import ( provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" "github.com/cs3org/reva/v2/pkg/appctx" + ctxpkg "github.com/cs3org/reva/v2/pkg/ctx" "github.com/cs3org/reva/v2/pkg/errtypes" "github.com/cs3org/reva/v2/pkg/rhttp/datatx" "github.com/cs3org/reva/v2/pkg/rhttp/datatx/manager/registry" @@ -69,6 +70,10 @@ func (m *manager) Handler(fs storage.FS) (http.Handler, error) { h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() + if lockID := r.Header.Get("X-Lock-Id"); lockID != "" { + ctx = ctxpkg.ContextSetLockID(ctx, lockID) + } + var spaceID string spaceID, r.URL.Path = router.ShiftPath(r.URL.Path) From 422e74942ebe45629a1fe2e7b32adfef0bea4692 Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Wed, 9 Mar 2022 11:15:25 +0100 Subject: [PATCH 02/19] expire locks and enable unlock without current user check --- pkg/storage/utils/decomposedfs/node/locks.go | 25 ++++++++------------ 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/node/locks.go b/pkg/storage/utils/decomposedfs/node/locks.go index 9caadc508d..150009c939 100644 --- a/pkg/storage/utils/decomposedfs/node/locks.go +++ b/pkg/storage/utils/decomposedfs/node/locks.go @@ -24,6 +24,7 @@ import ( "io/fs" "os" "path/filepath" + "time" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" types "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" @@ -31,7 +32,6 @@ import ( ctxpkg "github.com/cs3org/reva/v2/pkg/ctx" "github.com/cs3org/reva/v2/pkg/errtypes" "github.com/cs3org/reva/v2/pkg/storage/utils/filelocks" - "github.com/cs3org/reva/v2/pkg/utils" "github.com/pkg/errors" ) @@ -121,6 +121,15 @@ func (n Node) ReadLock(ctx context.Context) (*provider.Lock, error) { appctx.GetLogger(ctx).Error().Err(err).Msg("Decomposedfs: could not decode lock file, ignoring") return nil, errors.Wrap(err, "Decomposedfs: could not read lock file") } + + // lock already expired + if time.Now().After(time.Unix(int64(lock.Expiration.Seconds), int64(lock.Expiration.Nanos))) { + if err = os.Remove(f.Name()); err != nil { + return nil, errors.Wrap(err, "Decomposedfs: could not remove expired lock file") + } + return nil, nil + } + return lock, err } @@ -165,15 +174,6 @@ func (n *Node) RefreshLock(ctx context.Context, lock *provider.Lock) error { return errtypes.PreconditionFailed("mismatching lock") } - u := ctxpkg.ContextMustGetUser(ctx) - if !utils.UserEqual(oldLock.User, u.Id) { - return errtypes.PermissionDenied("cannot refresh lock of another holder") - } - - if !utils.UserEqual(oldLock.User, lock.GetUser()) { - return errtypes.PermissionDenied("cannot change holder when refreshing a lock") - } - if err := json.NewEncoder(f).Encode(lock); err != nil { return errors.Wrap(err, "Decomposedfs: could not write lock file") } @@ -222,11 +222,6 @@ func (n *Node) Unlock(ctx context.Context, lock *provider.Lock) error { return errtypes.Locked(oldLock.LockId) } - u := ctxpkg.ContextMustGetUser(ctx) - if !utils.UserEqual(oldLock.User, u.Id) { - return errtypes.PermissionDenied("mismatching holder") - } - if err = os.Remove(f.Name()); err != nil { return errors.Wrap(err, "Decomposedfs: could not remove lock file") } From 9aca212ea7bb3dbf8a2f0dd25d8d2c316ac5a157 Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Fri, 14 Jan 2022 15:05:52 +0100 Subject: [PATCH 03/19] fix paths and check additional scopes --- internal/grpc/interceptors/auth/scope.go | 19 +++++++++++++++++-- .../http/services/appprovider/appprovider.go | 1 + 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/internal/grpc/interceptors/auth/scope.go b/internal/grpc/interceptors/auth/scope.go index 6a250c46a8..da7518fec5 100644 --- a/internal/grpc/interceptors/auth/scope.go +++ b/internal/grpc/interceptors/auth/scope.go @@ -164,6 +164,8 @@ func expandAndVerifyScope(ctx context.Context, req interface{}, tokenScope map[s } } } + } else if _, ok := listStorageSpaces(req); ok { + return nil } return errtypes.PermissionDenied("access to resource not allowed within the assigned scope") @@ -245,13 +247,13 @@ func extractRef(req interface{}, hasEditorRole bool) (*provider.Reference, bool) case *provider.InitiateFileDownloadRequest: return v.GetRef(), true case *appprovider.OpenInAppRequest: - return &provider.Reference{ResourceId: v.ResourceInfo.Id}, true + return &provider.Reference{ResourceId: v.ResourceInfo.Id, Path: "."}, true case *gateway.OpenInAppRequest: return v.GetRef(), true // App provider requests case *appregistry.GetAppProvidersRequest: - return &provider.Reference{ResourceId: v.ResourceInfo.Id}, true + return &provider.Reference{ResourceId: v.ResourceInfo.Id, Path: "."}, true } if !hasEditorRole { @@ -288,3 +290,16 @@ func extractShareRef(req interface{}) (*collaboration.ShareReference, bool) { } return nil, false } + +func listStorageSpaces(req interface{}) (*provider.ListStorageSpacesRequest, bool) { + switch req.(type) { + case *provider.ListStorageSpacesRequest: + // TODO: checks + return nil, true + case *registry.ListStorageProvidersRequest: + // TODO: checks + return nil, true + default: + return nil, false + } +} diff --git a/internal/http/services/appprovider/appprovider.go b/internal/http/services/appprovider/appprovider.go index 92b30ba271..f0527abf64 100644 --- a/internal/http/services/appprovider/appprovider.go +++ b/internal/http/services/appprovider/appprovider.go @@ -349,6 +349,7 @@ func (s *svc) handleOpen(w http.ResponseWriter, r *http.Request) { fileRef := &provider.Reference{ ResourceId: resourceID, + Path: ".", } statRes, err := client.Stat(ctx, &provider.StatRequest{Ref: fileRef}) From ac2418f62f9cfccb78a4e487406f4b6f14ba9e45 Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Thu, 10 Mar 2022 15:15:51 +0100 Subject: [PATCH 04/19] implement locking on public storage provider --- .../publicstorageprovider.go | 44 +++++++++++++++++-- pkg/auth/scope/publicshare.go | 14 ++++++ 2 files changed, 54 insertions(+), 4 deletions(-) diff --git a/internal/grpc/services/publicstorageprovider/publicstorageprovider.go b/internal/grpc/services/publicstorageprovider/publicstorageprovider.go index 8665bdf84d..4bbed7f24f 100644 --- a/internal/grpc/services/publicstorageprovider/publicstorageprovider.go +++ b/internal/grpc/services/publicstorageprovider/publicstorageprovider.go @@ -120,22 +120,58 @@ func (s *service) UnsetArbitraryMetadata(ctx context.Context, req *provider.Unse // SetLock puts a lock on the given reference func (s *service) SetLock(ctx context.Context, req *provider.SetLockRequest) (*provider.SetLockResponse, error) { - return nil, gstatus.Errorf(codes.Unimplemented, "method not implemented") + ref, _, _, st, err := s.translatePublicRefToCS3Ref(ctx, req.Ref) + switch { + case err != nil: + return nil, err + case st != nil: + return &provider.SetLockResponse{ + Status: st, + }, nil + } + return s.gateway.SetLock(ctx, &provider.SetLockRequest{Opaque: req.Opaque, Ref: ref, Lock: req.Lock}) } // GetLock returns an existing lock on the given reference func (s *service) GetLock(ctx context.Context, req *provider.GetLockRequest) (*provider.GetLockResponse, error) { - return nil, gstatus.Errorf(codes.Unimplemented, "method not implemented") + ref, _, _, st, err := s.translatePublicRefToCS3Ref(ctx, req.Ref) + switch { + case err != nil: + return nil, err + case st != nil: + return &provider.GetLockResponse{ + Status: st, + }, nil + } + return s.gateway.GetLock(ctx, &provider.GetLockRequest{Opaque: req.Opaque, Ref: ref}) } // RefreshLock refreshes an existing lock on the given reference func (s *service) RefreshLock(ctx context.Context, req *provider.RefreshLockRequest) (*provider.RefreshLockResponse, error) { - return nil, gstatus.Errorf(codes.Unimplemented, "method not implemented") + ref, _, _, st, err := s.translatePublicRefToCS3Ref(ctx, req.Ref) + switch { + case err != nil: + return nil, err + case st != nil: + return &provider.RefreshLockResponse{ + Status: st, + }, nil + } + return s.gateway.RefreshLock(ctx, &provider.RefreshLockRequest{Opaque: req.Opaque, Ref: ref, Lock: req.Lock}) } // Unlock removes an existing lock from the given reference func (s *service) Unlock(ctx context.Context, req *provider.UnlockRequest) (*provider.UnlockResponse, error) { - return nil, gstatus.Errorf(codes.Unimplemented, "method not implemented") + ref, _, _, st, err := s.translatePublicRefToCS3Ref(ctx, req.Ref) + switch { + case err != nil: + return nil, err + case st != nil: + return &provider.UnlockResponse{ + Status: st, + }, nil + } + return s.gateway.Unlock(ctx, &provider.UnlockRequest{Opaque: req.Opaque, Ref: ref, Lock: req.Lock}) } func (s *service) InitiateFileDownload(ctx context.Context, req *provider.InitiateFileDownloadRequest) (*provider.InitiateFileDownloadResponse, error) { diff --git a/pkg/auth/scope/publicshare.go b/pkg/auth/scope/publicshare.go index 207cd7e2f9..0c5e26a502 100644 --- a/pkg/auth/scope/publicshare.go +++ b/pkg/auth/scope/publicshare.go @@ -77,6 +77,18 @@ func publicshareScope(ctx context.Context, scope *authpb.Scope, resource interfa return checkStorageRef(ctx, &share, &provider.Reference{ResourceId: v.GetResourceId()}), nil case *provider.StatRequest: return checkStorageRef(ctx, &share, v.GetRef()), nil + case *provider.GetLockRequest: + return true, nil + //return checkStorageRef(ctx, &share, v.GetRef()), nil + case *provider.UnlockRequest: + return true, nil + //return checkStorageRef(ctx, &share, v.GetRef()), nil + case *provider.RefreshLockRequest: + return true, nil + //return checkStorageRef(ctx, &share, v.GetRef()), nil + case *provider.SetLockRequest: + return true, nil + //return checkStorageRef(ctx, &share, v.GetRef()), nil case *provider.ListContainerRequest: return checkStorageRef(ctx, &share, v.GetRef()), nil case *provider.InitiateFileDownloadRequest: @@ -109,6 +121,8 @@ func publicshareScope(ctx context.Context, scope *authpb.Scope, resource interfa case *appregistry.GetDefaultAppProviderForMimeTypeRequest: return true, nil + case *appregistry.GetAppProvidersRequest: + return true, nil case *userv1beta1.GetUserByClaimRequest: return true, nil case *userv1beta1.GetUserRequest: From 94aa4ddbbbd6b81770ea4412c9638e11750736f5 Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Thu, 10 Mar 2022 15:23:05 +0100 Subject: [PATCH 05/19] remove superflous scope checks --- internal/grpc/interceptors/auth/scope.go | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/internal/grpc/interceptors/auth/scope.go b/internal/grpc/interceptors/auth/scope.go index da7518fec5..bf55f20d78 100644 --- a/internal/grpc/interceptors/auth/scope.go +++ b/internal/grpc/interceptors/auth/scope.go @@ -164,8 +164,6 @@ func expandAndVerifyScope(ctx context.Context, req interface{}, tokenScope map[s } } } - } else if _, ok := listStorageSpaces(req); ok { - return nil } return errtypes.PermissionDenied("access to resource not allowed within the assigned scope") @@ -290,16 +288,3 @@ func extractShareRef(req interface{}) (*collaboration.ShareReference, bool) { } return nil, false } - -func listStorageSpaces(req interface{}) (*provider.ListStorageSpacesRequest, bool) { - switch req.(type) { - case *provider.ListStorageSpacesRequest: - // TODO: checks - return nil, true - case *registry.ListStorageProvidersRequest: - // TODO: checks - return nil, true - default: - return nil, false - } -} From d454f6ba22c2f03596788fe0d29826410f5101a9 Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Thu, 10 Mar 2022 15:30:34 +0100 Subject: [PATCH 06/19] use reference from info --- internal/grpc/interceptors/auth/scope.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/grpc/interceptors/auth/scope.go b/internal/grpc/interceptors/auth/scope.go index bf55f20d78..844e89b409 100644 --- a/internal/grpc/interceptors/auth/scope.go +++ b/internal/grpc/interceptors/auth/scope.go @@ -244,14 +244,14 @@ func extractRef(req interface{}, hasEditorRole bool) (*provider.Reference, bool) return v.GetRef(), true case *provider.InitiateFileDownloadRequest: return v.GetRef(), true + + // App provider requests + case *appregistry.GetAppProvidersRequest: + return &provider.Reference{ResourceId: v.ResourceInfo.Id, Path: v.ResourceInfo.Path}, true case *appprovider.OpenInAppRequest: - return &provider.Reference{ResourceId: v.ResourceInfo.Id, Path: "."}, true + return &provider.Reference{ResourceId: v.ResourceInfo.Id, Path: v.ResourceInfo.Path}, true case *gateway.OpenInAppRequest: return v.GetRef(), true - - // App provider requests - case *appregistry.GetAppProvidersRequest: - return &provider.Reference{ResourceId: v.ResourceInfo.Id, Path: "."}, true } if !hasEditorRole { From d15bfd98edfa7cb1eb6aa5ee3a78f58dd20a008b Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Thu, 10 Mar 2022 15:58:09 +0100 Subject: [PATCH 07/19] pass lockid on InitiateUploadRequest --- pkg/rhttp/datatx/manager/simple/simple.go | 6 ------ pkg/rhttp/datatx/manager/spaces/spaces.go | 6 ------ pkg/storage/utils/decomposedfs/upload.go | 9 +++++++++ 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/pkg/rhttp/datatx/manager/simple/simple.go b/pkg/rhttp/datatx/manager/simple/simple.go index e732f9be0c..a7acc5dba2 100644 --- a/pkg/rhttp/datatx/manager/simple/simple.go +++ b/pkg/rhttp/datatx/manager/simple/simple.go @@ -23,7 +23,6 @@ import ( provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" "github.com/cs3org/reva/v2/pkg/appctx" - ctxpkg "github.com/cs3org/reva/v2/pkg/ctx" "github.com/cs3org/reva/v2/pkg/errtypes" "github.com/cs3org/reva/v2/pkg/rhttp/datatx" "github.com/cs3org/reva/v2/pkg/rhttp/datatx/manager/registry" @@ -65,11 +64,6 @@ func New(m map[string]interface{}) (datatx.DataTX, error) { func (m *manager) Handler(fs storage.FS) (http.Handler, error) { h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - - if lockID := r.Header.Get("X-Lock-Id"); lockID != "" { - ctx = ctxpkg.ContextSetLockID(ctx, lockID) - } - sublog := appctx.GetLogger(ctx).With().Str("datatx", "simple").Logger() switch r.Method { diff --git a/pkg/rhttp/datatx/manager/spaces/spaces.go b/pkg/rhttp/datatx/manager/spaces/spaces.go index e0a5aff11c..1ff0788b02 100644 --- a/pkg/rhttp/datatx/manager/spaces/spaces.go +++ b/pkg/rhttp/datatx/manager/spaces/spaces.go @@ -25,7 +25,6 @@ import ( provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" "github.com/cs3org/reva/v2/pkg/appctx" - ctxpkg "github.com/cs3org/reva/v2/pkg/ctx" "github.com/cs3org/reva/v2/pkg/errtypes" "github.com/cs3org/reva/v2/pkg/rhttp/datatx" "github.com/cs3org/reva/v2/pkg/rhttp/datatx/manager/registry" @@ -69,11 +68,6 @@ func New(m map[string]interface{}) (datatx.DataTX, error) { func (m *manager) Handler(fs storage.FS) (http.Handler, error) { h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - - if lockID := r.Header.Get("X-Lock-Id"); lockID != "" { - ctx = ctxpkg.ContextSetLockID(ctx, lockID) - } - var spaceID string spaceID, r.URL.Path = router.ShiftPath(r.URL.Path) diff --git a/pkg/storage/utils/decomposedfs/upload.go b/pkg/storage/utils/decomposedfs/upload.go index a35cf67097..1addd50158 100644 --- a/pkg/storage/utils/decomposedfs/upload.go +++ b/pkg/storage/utils/decomposedfs/upload.go @@ -114,10 +114,13 @@ func (fs *Decomposedfs) InitiateUpload(ctx context.Context, ref *provider.Refere return nil, err } + lockID, _ := ctxpkg.ContextGetLockID(ctx) + info := tusd.FileInfo{ MetaData: tusd.MetaData{ "filename": filepath.Base(relative), "dir": filepath.Dir(relative), + "lockid": lockID, }, Size: uploadLength, Storage: map[string]string{ @@ -235,6 +238,9 @@ func (fs *Decomposedfs) NewUpload(ctx context.Context, info tusd.FileInfo) (uplo } // check lock + if info.MetaData["lockid"] != "" { + ctx = ctxpkg.ContextSetLockID(ctx, info.MetaData["lockid"]) + } if err := n.CheckLock(ctx); err != nil { return nil, err } @@ -468,6 +474,9 @@ func (upload *fileUpload) FinishUpload(ctx context.Context) (err error) { n.SpaceRoot = node.New(spaceID, spaceID, "", "", 0, "", nil, upload.fs.lu) // check lock + if upload.info.MetaData["lockid"] != "" { + ctx = ctxpkg.ContextSetLockID(ctx, upload.info.MetaData["lockid"]) + } if err := n.CheckLock(ctx); err != nil { return err } From 7ccb3067e7255795b19fb6b276245e1473c45286 Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Mon, 14 Mar 2022 11:44:17 +0100 Subject: [PATCH 08/19] fix locking scope check for public links --- internal/grpc/interceptors/auth/scope.go | 10 ++++++++++ pkg/auth/scope/publicshare.go | 12 ++++-------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/internal/grpc/interceptors/auth/scope.go b/internal/grpc/interceptors/auth/scope.go index 844e89b409..75fbcab4d0 100644 --- a/internal/grpc/interceptors/auth/scope.go +++ b/internal/grpc/interceptors/auth/scope.go @@ -245,6 +245,16 @@ func extractRef(req interface{}, hasEditorRole bool) (*provider.Reference, bool) case *provider.InitiateFileDownloadRequest: return v.GetRef(), true + // Locking + case *provider.GetLockRequest: + return v.GetRef(), true + case *provider.SetLockRequest: + return v.GetRef(), true + case *provider.RefreshLockRequest: + return v.GetRef(), true + case *provider.UnlockRequest: + return v.GetRef(), true + // App provider requests case *appregistry.GetAppProvidersRequest: return &provider.Reference{ResourceId: v.ResourceInfo.Id, Path: v.ResourceInfo.Path}, true diff --git a/pkg/auth/scope/publicshare.go b/pkg/auth/scope/publicshare.go index 0c5e26a502..b303e07f78 100644 --- a/pkg/auth/scope/publicshare.go +++ b/pkg/auth/scope/publicshare.go @@ -78,17 +78,13 @@ func publicshareScope(ctx context.Context, scope *authpb.Scope, resource interfa case *provider.StatRequest: return checkStorageRef(ctx, &share, v.GetRef()), nil case *provider.GetLockRequest: - return true, nil - //return checkStorageRef(ctx, &share, v.GetRef()), nil + return checkStorageRef(ctx, &share, v.GetRef()), nil case *provider.UnlockRequest: - return true, nil - //return checkStorageRef(ctx, &share, v.GetRef()), nil + return checkStorageRef(ctx, &share, v.GetRef()), nil case *provider.RefreshLockRequest: - return true, nil - //return checkStorageRef(ctx, &share, v.GetRef()), nil + return checkStorageRef(ctx, &share, v.GetRef()), nil case *provider.SetLockRequest: - return true, nil - //return checkStorageRef(ctx, &share, v.GetRef()), nil + return checkStorageRef(ctx, &share, v.GetRef()), nil case *provider.ListContainerRequest: return checkStorageRef(ctx, &share, v.GetRef()), nil case *provider.InitiateFileDownloadRequest: From b94f64aa8f9a5b033a10aa5c2b643a7d05069856 Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Mon, 14 Mar 2022 11:50:08 +0100 Subject: [PATCH 09/19] add changelog --- changelog/unreleased/fix-locking.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 changelog/unreleased/fix-locking.md diff --git a/changelog/unreleased/fix-locking.md b/changelog/unreleased/fix-locking.md new file mode 100644 index 0000000000..6ad4c68351 --- /dev/null +++ b/changelog/unreleased/fix-locking.md @@ -0,0 +1,12 @@ +Bugfix: Fix locking on publik links and the decomposed filesystem + +We've fixed the behavior of locking on the decomposed filesystem, so that now +users can remove a lock which was initially created by another user (needed for WOPI integration). +Also we added a check, if a lock is already expired and if so, we lazily delete the lock. +The InitiateUploadRequest now adds the Lock to the upload metadata so that an upload to an +locked file is possible. + +We'v added the locking api requests to the public link scope checks, so that locking +also can be used on public links (needed for WOPI integration). + +https://github.com/cs3org/reva/pull/2625 From cc3d7408004312cefb0ff2f595892c61799e898e Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Mon, 14 Mar 2022 13:04:47 +0100 Subject: [PATCH 10/19] adapt tests to new logic --- pkg/storage/utils/decomposedfs/node/locks.go | 5 +++-- .../utils/decomposedfs/node/locks_test.go | 17 +++++++---------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/node/locks.go b/pkg/storage/utils/decomposedfs/node/locks.go index 150009c939..bc16eb40c3 100644 --- a/pkg/storage/utils/decomposedfs/node/locks.go +++ b/pkg/storage/utils/decomposedfs/node/locks.go @@ -123,14 +123,15 @@ func (n Node) ReadLock(ctx context.Context) (*provider.Lock, error) { } // lock already expired - if time.Now().After(time.Unix(int64(lock.Expiration.Seconds), int64(lock.Expiration.Nanos))) { + if lock.Expiration != nil && time.Now().After(time.Unix(int64(lock.Expiration.Seconds), int64(lock.Expiration.Nanos))) { if err = os.Remove(f.Name()); err != nil { return nil, errors.Wrap(err, "Decomposedfs: could not remove expired lock file") } + // we successfully deleted the expired lock return nil, nil } - return lock, err + return lock, nil } // RefreshLock refreshes the node's lock diff --git a/pkg/storage/utils/decomposedfs/node/locks_test.go b/pkg/storage/utils/decomposedfs/node/locks_test.go index f11878953e..841c3be744 100644 --- a/pkg/storage/utils/decomposedfs/node/locks_test.go +++ b/pkg/storage/utils/decomposedfs/node/locks_test.go @@ -158,20 +158,18 @@ var _ = Describe("Node locks", func() { Expect(err.Error()).To(ContainSubstring("mismatching")) }) - It("refuses to refresh the lock for other users than the lock holder", func() { + It("refreshes the lock for other users than the lock holder", func() { err := n.RefreshLock(otherCtx, newLock) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("permission denied")) + Expect(err).NotTo(HaveOccurred()) }) - It("refuses to change the lock holder", func() { + It("changes the lock holder", func() { newLock.User = otherUser.Id err := n.RefreshLock(env.Ctx, newLock) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("permission denied")) + Expect(err).NotTo(HaveOccurred()) }) - It("refreshes the lock", func() { + It("refreshes the lock for the original user", func() { err := n.RefreshLock(env.Ctx, newLock) Expect(err).ToNot(HaveOccurred()) }) @@ -191,10 +189,9 @@ var _ = Describe("Node locks", func() { Expect(err.Error()).To(ContainSubstring(lock.LockId)) }) - It("refuses to unlock for others even if they have the lock", func() { + It("unlocks when other another user uses the lock", func() { err := n.Unlock(otherCtx, lock) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("mismatching")) + Expect(err).NotTo(HaveOccurred()) }) It("unlocks when the owner uses the lock", func() { From 541a9d20d7210a8b200ba18f57e7f9a5036ab113 Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Mon, 21 Mar 2022 13:56:37 +0100 Subject: [PATCH 11/19] reintroduce lock modification checks --- pkg/storage/utils/decomposedfs/node/locks.go | 30 ++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/pkg/storage/utils/decomposedfs/node/locks.go b/pkg/storage/utils/decomposedfs/node/locks.go index bc16eb40c3..20205afcd3 100644 --- a/pkg/storage/utils/decomposedfs/node/locks.go +++ b/pkg/storage/utils/decomposedfs/node/locks.go @@ -32,6 +32,7 @@ import ( ctxpkg "github.com/cs3org/reva/v2/pkg/ctx" "github.com/cs3org/reva/v2/pkg/errtypes" "github.com/cs3org/reva/v2/pkg/storage/utils/filelocks" + "github.com/cs3org/reva/v2/pkg/utils" "github.com/pkg/errors" ) @@ -175,6 +176,10 @@ func (n *Node) RefreshLock(ctx context.Context, lock *provider.Lock) error { return errtypes.PreconditionFailed("mismatching lock") } + if ok, err := isLockModificationAllowed(oldLock, lock); !ok { + return err + } + if err := json.NewEncoder(f).Encode(lock); err != nil { return errors.Wrap(err, "Decomposedfs: could not write lock file") } @@ -223,6 +228,10 @@ func (n *Node) Unlock(ctx context.Context, lock *provider.Lock) error { return errtypes.Locked(oldLock.LockId) } + if ok, err := isLockModificationAllowed(oldLock, lock); !ok { + return err + } + if err = os.Remove(f.Name()); err != nil { return errors.Wrap(err, "Decomposedfs: could not remove lock file") } @@ -303,3 +312,24 @@ func (n *Node) hasLocks(ctx context.Context) bool { _, err := os.Stat(n.LockFilePath()) // FIXME better error checking return err == nil } + +func isLockModificationAllowed(oldLock *provider.Lock, newLock *provider.Lock) (bool, error) { + if oldLock.Type == provider.LockType_LOCK_TYPE_SHARED { + return true, nil + } + + var allowed bool = true + var err error + + if oldLock.AppName != "" && oldLock.AppName != newLock.AppName { + allowed = false + err = errtypes.PermissionDenied("cannot change holder (app name) when refreshing a lock") + } + + if oldLock.User != nil && !utils.UserEqual(oldLock.User, newLock.GetUser()) { + allowed = false + err = errtypes.PermissionDenied("cannot change holder (user) when refreshing a lock") + } + + return allowed, err +} From 704f5e819530f4db9f5176cec18b54ed0efb7edc Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Mon, 21 Mar 2022 14:29:02 +0100 Subject: [PATCH 12/19] fix return code on expired lock --- pkg/storage/utils/decomposedfs/node/locks.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/storage/utils/decomposedfs/node/locks.go b/pkg/storage/utils/decomposedfs/node/locks.go index 20205afcd3..9230fdf846 100644 --- a/pkg/storage/utils/decomposedfs/node/locks.go +++ b/pkg/storage/utils/decomposedfs/node/locks.go @@ -129,7 +129,7 @@ func (n Node) ReadLock(ctx context.Context) (*provider.Lock, error) { return nil, errors.Wrap(err, "Decomposedfs: could not remove expired lock file") } // we successfully deleted the expired lock - return nil, nil + return nil, errtypes.NotFound("no lock found") } return lock, nil From 1f31c34923a53464e9cc3a4465d32fe382d89f64 Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Mon, 21 Mar 2022 15:24:12 +0100 Subject: [PATCH 13/19] add and adapt tests --- pkg/storage/utils/decomposedfs/node/locks.go | 12 +- .../utils/decomposedfs/node/locks_test.go | 223 +++++++++++++++--- 2 files changed, 196 insertions(+), 39 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/node/locks.go b/pkg/storage/utils/decomposedfs/node/locks.go index 9230fdf846..5d82df6170 100644 --- a/pkg/storage/utils/decomposedfs/node/locks.go +++ b/pkg/storage/utils/decomposedfs/node/locks.go @@ -176,7 +176,7 @@ func (n *Node) RefreshLock(ctx context.Context, lock *provider.Lock) error { return errtypes.PreconditionFailed("mismatching lock") } - if ok, err := isLockModificationAllowed(oldLock, lock); !ok { + if ok, err := isLockModificationAllowed(ctx, oldLock, lock); !ok { return err } @@ -228,7 +228,7 @@ func (n *Node) Unlock(ctx context.Context, lock *provider.Lock) error { return errtypes.Locked(oldLock.LockId) } - if ok, err := isLockModificationAllowed(oldLock, lock); !ok { + if ok, err := isLockModificationAllowed(ctx, oldLock, lock); !ok { return err } @@ -313,7 +313,7 @@ func (n *Node) hasLocks(ctx context.Context) bool { return err == nil } -func isLockModificationAllowed(oldLock *provider.Lock, newLock *provider.Lock) (bool, error) { +func isLockModificationAllowed(ctx context.Context, oldLock *provider.Lock, newLock *provider.Lock) (bool, error) { if oldLock.Type == provider.LockType_LOCK_TYPE_SHARED { return true, nil } @@ -331,5 +331,11 @@ func isLockModificationAllowed(oldLock *provider.Lock, newLock *provider.Lock) ( err = errtypes.PermissionDenied("cannot change holder (user) when refreshing a lock") } + u := ctxpkg.ContextMustGetUser(ctx) + if oldLock.User != nil && !utils.UserEqual(oldLock.User, u.Id) { + allowed = false + err = errtypes.PermissionDenied("mismatching holder") + } + return allowed, err } diff --git a/pkg/storage/utils/decomposedfs/node/locks_test.go b/pkg/storage/utils/decomposedfs/node/locks_test.go index 841c3be744..3e785a6367 100644 --- a/pkg/storage/utils/decomposedfs/node/locks_test.go +++ b/pkg/storage/utils/decomposedfs/node/locks_test.go @@ -37,10 +37,12 @@ var _ = Describe("Node locks", func() { var ( env *helpers.TestEnv - lock *provider.Lock - wrongLock *provider.Lock - n *node.Node - n2 *node.Node + lockByUser *provider.Lock + wrongLockByUser *provider.Lock + lockByApp *provider.Lock + wrongLockByApp *provider.Lock + n *node.Node + n2 *node.Node otherUser = &userpb.User{ Id: &userpb.UserId{ @@ -58,16 +60,26 @@ var _ = Describe("Node locks", func() { env, err = helpers.NewTestEnv(nil) Expect(err).ToNot(HaveOccurred()) - lock = &provider.Lock{ + lockByUser = &provider.Lock{ Type: provider.LockType_LOCK_TYPE_EXCL, User: env.Owner.Id, LockId: uuid.New().String(), } - wrongLock = &provider.Lock{ + wrongLockByUser = &provider.Lock{ Type: provider.LockType_LOCK_TYPE_EXCL, User: env.Owner.Id, LockId: uuid.New().String(), } + lockByApp = &provider.Lock{ + Type: provider.LockType_LOCK_TYPE_WRITE, + AppName: "app1", + LockId: uuid.New().String(), + } + wrongLockByApp = &provider.Lock{ + Type: provider.LockType_LOCK_TYPE_WRITE, + AppName: "app2", + LockId: uuid.New().String(), + } n = node.New("u-s-e-r-id", "tobelockedid", "", "tobelocked", 10, "", env.Owner.Id, env.Lookup) n2 = node.New("u-s-e-r-id", "neverlockedid", "", "neverlocked", 10, "", env.Owner.Id, env.Lookup) }) @@ -78,12 +90,46 @@ var _ = Describe("Node locks", func() { } }) - Describe("SetLock", func() { + Describe("SetLock for a user", func() { + It("sets the lock", func() { + _, err := os.Stat(n.LockFilePath()) + Expect(err).To(HaveOccurred()) + + err = n.SetLock(env.Ctx, lockByUser) + Expect(err).ToNot(HaveOccurred()) + + _, err = os.Stat(n.LockFilePath()) + Expect(err).ToNot(HaveOccurred()) + }) + + It("refuses to lock if already locked an existing lock was not provided", func() { + err := n.SetLock(env.Ctx, lockByUser) + Expect(err).ToNot(HaveOccurred()) + + err = n.SetLock(env.Ctx, lockByUser) + Expect(err).To(HaveOccurred()) + + env.Ctx = ctxpkg.ContextSetLockID(env.Ctx, wrongLockByUser.LockId) + err = n.SetLock(env.Ctx, lockByUser) + Expect(err).To(HaveOccurred()) + }) + + It("relocks if the existing lock was provided", func() { + err := n.SetLock(env.Ctx, lockByUser) + Expect(err).ToNot(HaveOccurred()) + + env.Ctx = ctxpkg.ContextSetLockID(env.Ctx, lockByUser.LockId) + err = n.SetLock(env.Ctx, lockByUser) + Expect(err).ToNot(HaveOccurred()) + }) + }) + + Describe("SetLock for an app", func() { It("sets the lock", func() { _, err := os.Stat(n.LockFilePath()) Expect(err).To(HaveOccurred()) - err = n.SetLock(env.Ctx, lock) + err = n.SetLock(env.Ctx, lockByApp) Expect(err).ToNot(HaveOccurred()) _, err = os.Stat(n.LockFilePath()) @@ -91,30 +137,30 @@ var _ = Describe("Node locks", func() { }) It("refuses to lock if already locked an existing lock was not provided", func() { - err := n.SetLock(env.Ctx, lock) + err := n.SetLock(env.Ctx, lockByApp) Expect(err).ToNot(HaveOccurred()) - err = n.SetLock(env.Ctx, lock) + err = n.SetLock(env.Ctx, lockByApp) Expect(err).To(HaveOccurred()) - env.Ctx = ctxpkg.ContextSetLockID(env.Ctx, wrongLock.LockId) - err = n.SetLock(env.Ctx, lock) + env.Ctx = ctxpkg.ContextSetLockID(env.Ctx, wrongLockByApp.LockId) + err = n.SetLock(env.Ctx, lockByApp) Expect(err).To(HaveOccurred()) }) It("relocks if the existing lock was provided", func() { - err := n.SetLock(env.Ctx, lock) + err := n.SetLock(env.Ctx, lockByApp) Expect(err).ToNot(HaveOccurred()) - env.Ctx = ctxpkg.ContextSetLockID(env.Ctx, lock.LockId) - err = n.SetLock(env.Ctx, lock) + env.Ctx = ctxpkg.ContextSetLockID(env.Ctx, lockByApp.LockId) + err = n.SetLock(env.Ctx, lockByApp) Expect(err).ToNot(HaveOccurred()) }) }) - Context("with an existing lock", func() { + Context("with an existing lock for a user", func() { BeforeEach(func() { - err := n.SetLock(env.Ctx, lock) + err := n.SetLock(env.Ctx, lockByUser) Expect(err).ToNot(HaveOccurred()) }) @@ -122,10 +168,10 @@ var _ = Describe("Node locks", func() { It("returns the lock", func() { l, err := n.ReadLock(env.Ctx) Expect(err).ToNot(HaveOccurred()) - Expect(l).To(Equal(lock)) + Expect(l).To(Equal(lockByUser)) }) - It("reporst an error when the node wasn't locked", func() { + It("reports an error when the node wasn't locked", func() { _, err := n2.ReadLock(env.Ctx) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("no lock found")) @@ -141,12 +187,12 @@ var _ = Describe("Node locks", func() { newLock = &provider.Lock{ Type: provider.LockType_LOCK_TYPE_EXCL, User: env.Owner.Id, - LockId: lock.LockId, + LockId: lockByUser.LockId, } }) It("fails when the node is unlocked", func() { - err := n2.RefreshLock(env.Ctx, lock) + err := n2.RefreshLock(env.Ctx, lockByUser) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("precondition failed")) }) @@ -158,18 +204,123 @@ var _ = Describe("Node locks", func() { Expect(err.Error()).To(ContainSubstring("mismatching")) }) - It("refreshes the lock for other users than the lock holder", func() { + It("refuses to refresh the lock for other users than the lock holder", func() { err := n.RefreshLock(otherCtx, newLock) - Expect(err).NotTo(HaveOccurred()) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("permission denied")) }) - It("changes the lock holder", func() { + It("refuses to change the lock holder", func() { newLock.User = otherUser.Id err := n.RefreshLock(env.Ctx, newLock) - Expect(err).NotTo(HaveOccurred()) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("permission denied")) + }) + + It("refreshes the lock", func() { + err := n.RefreshLock(env.Ctx, newLock) + Expect(err).ToNot(HaveOccurred()) + }) + }) + + Describe("Unlock", func() { + It("refuses to unlock without having a lock", func() { + err := n.Unlock(env.Ctx, nil) + Expect(err.Error()).To(ContainSubstring(lockByUser.LockId)) + }) + + It("refuses to unlock without having the proper lock", func() { + err := n.Unlock(env.Ctx, nil) + Expect(err.Error()).To(ContainSubstring(lockByUser.LockId)) + + err = n.Unlock(env.Ctx, wrongLockByUser) + Expect(err.Error()).To(ContainSubstring(lockByUser.LockId)) + }) + + It("refuses to unlock for others even if they have the lock", func() { + err := n.Unlock(otherCtx, lockByUser) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("mismatching")) + }) + + It("unlocks when the owner uses the lock", func() { + err := n.Unlock(env.Ctx, lockByUser) + Expect(err).ToNot(HaveOccurred()) + + _, err = os.Stat(n.LockFilePath()) + Expect(err).To(HaveOccurred()) + }) + + It("fails to unlock an unlocked node", func() { + err := n.Unlock(env.Ctx, lockByUser) + Expect(err).ToNot(HaveOccurred()) + + err = n.Unlock(env.Ctx, lockByUser) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("lock does not exist")) + }) + }) + }) + + Context("with an existing lock for an app", func() { + BeforeEach(func() { + err := n.SetLock(env.Ctx, lockByApp) + Expect(err).ToNot(HaveOccurred()) + }) + + Describe("ReadLock", func() { + It("returns the lock", func() { + l, err := n.ReadLock(env.Ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(l).To(Equal(lockByApp)) + }) + + It("reports an error when the node wasn't locked", func() { + _, err := n2.ReadLock(env.Ctx) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("no lock found")) + }) + }) + + Describe("RefreshLock", func() { + var ( + newLock *provider.Lock + ) + + JustBeforeEach(func() { + newLock = &provider.Lock{ + Type: provider.LockType_LOCK_TYPE_EXCL, + AppName: lockByApp.AppName, + LockId: lockByApp.LockId, + } + }) + + It("fails when the node is unlocked", func() { + err := n2.RefreshLock(env.Ctx, lockByApp) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("precondition failed")) + }) + + It("refuses to refresh the lock without holding the lock", func() { + newLock.LockId = "somethingsomething" + err := n.RefreshLock(env.Ctx, newLock) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("mismatching")) }) - It("refreshes the lock for the original user", func() { + It("refreshes the lock for other users", func() { + err := n.RefreshLock(otherCtx, lockByApp) + Expect(err).ToNot(HaveOccurred()) + }) + + It("refuses to change the lock holder", func() { + newLock.AppName = wrongLockByApp.AppName + err := n.RefreshLock(env.Ctx, newLock) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("permission denied")) + }) + + It("refreshes the lock", func() { err := n.RefreshLock(env.Ctx, newLock) Expect(err).ToNot(HaveOccurred()) }) @@ -178,24 +329,24 @@ var _ = Describe("Node locks", func() { Describe("Unlock", func() { It("refuses to unlock without having a lock", func() { err := n.Unlock(env.Ctx, nil) - Expect(err.Error()).To(ContainSubstring(lock.LockId)) + Expect(err.Error()).To(ContainSubstring(lockByApp.LockId)) }) It("refuses to unlock without having the proper lock", func() { err := n.Unlock(env.Ctx, nil) - Expect(err.Error()).To(ContainSubstring(lock.LockId)) + Expect(err.Error()).To(ContainSubstring(lockByApp.LockId)) - err = n.Unlock(env.Ctx, wrongLock) - Expect(err.Error()).To(ContainSubstring(lock.LockId)) + err = n.Unlock(env.Ctx, wrongLockByUser) + Expect(err.Error()).To(ContainSubstring(lockByApp.LockId)) }) - It("unlocks when other another user uses the lock", func() { - err := n.Unlock(otherCtx, lock) - Expect(err).NotTo(HaveOccurred()) + It("accepts to unlock for others if they have the lock", func() { + err := n.Unlock(otherCtx, lockByApp) + Expect(err).ToNot(HaveOccurred()) }) It("unlocks when the owner uses the lock", func() { - err := n.Unlock(env.Ctx, lock) + err := n.Unlock(env.Ctx, lockByApp) Expect(err).ToNot(HaveOccurred()) _, err = os.Stat(n.LockFilePath()) @@ -203,10 +354,10 @@ var _ = Describe("Node locks", func() { }) It("fails to unlock an unlocked node", func() { - err := n.Unlock(env.Ctx, lock) + err := n.Unlock(env.Ctx, lockByApp) Expect(err).ToNot(HaveOccurred()) - err = n.Unlock(env.Ctx, lock) + err = n.Unlock(env.Ctx, lockByApp) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("lock does not exist")) }) From 9bb6c169e4c020ba4c8bbe108298daf32d1fff00 Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Wed, 23 Mar 2022 09:53:16 +0100 Subject: [PATCH 14/19] switch to forbidden --- pkg/storage/utils/decomposedfs/node/locks.go | 33 ++++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/node/locks.go b/pkg/storage/utils/decomposedfs/node/locks.go index 5d82df6170..d7924002a3 100644 --- a/pkg/storage/utils/decomposedfs/node/locks.go +++ b/pkg/storage/utils/decomposedfs/node/locks.go @@ -318,24 +318,31 @@ func isLockModificationAllowed(ctx context.Context, oldLock *provider.Lock, newL return true, nil } - var allowed bool = true + var forbidden bool = true var err error - if oldLock.AppName != "" && oldLock.AppName != newLock.AppName { - allowed = false - err = errtypes.PermissionDenied("cannot change holder (app name) when refreshing a lock") + if oldLock.AppName != "" || newLock.AppName != "" { + if oldLock.AppName != newLock.AppName { + err = errtypes.PermissionDenied("cannot change holder (app name) when refreshing a lock") + } else { + forbidden = forbidden && true + } } - if oldLock.User != nil && !utils.UserEqual(oldLock.User, newLock.GetUser()) { - allowed = false - err = errtypes.PermissionDenied("cannot change holder (user) when refreshing a lock") - } + if oldLock.User != nil || newLock.GetUser() != nil { + if !utils.UserEqual(oldLock.User, newLock.GetUser()) { + err = errtypes.PermissionDenied("cannot change holder (user) when refreshing a lock") + } else { + forbidden = forbidden && true + } - u := ctxpkg.ContextMustGetUser(ctx) - if oldLock.User != nil && !utils.UserEqual(oldLock.User, u.Id) { - allowed = false - err = errtypes.PermissionDenied("mismatching holder") + u := ctxpkg.ContextMustGetUser(ctx) + if !utils.UserEqual(oldLock.User, u.Id) { + err = errtypes.PermissionDenied("mismatching holder") + } else { + forbidden = forbidden && true + } } - return allowed, err + return !forbidden, err } From f8d29f0087a5c6700ba66c9c39d54b1374907f41 Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Fri, 25 Mar 2022 09:07:56 +0100 Subject: [PATCH 15/19] rework lock modification permission logic --- pkg/storage/utils/decomposedfs/node/locks.go | 54 ++++++++++++++------ 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/node/locks.go b/pkg/storage/utils/decomposedfs/node/locks.go index d7924002a3..dbcc9d853d 100644 --- a/pkg/storage/utils/decomposedfs/node/locks.go +++ b/pkg/storage/utils/decomposedfs/node/locks.go @@ -318,31 +318,51 @@ func isLockModificationAllowed(ctx context.Context, oldLock *provider.Lock, newL return true, nil } - var forbidden bool = true - var err error + var appNameEquals, lockUserEquals, contextUserEquals bool if oldLock.AppName != "" || newLock.AppName != "" { - if oldLock.AppName != newLock.AppName { - err = errtypes.PermissionDenied("cannot change holder (app name) when refreshing a lock") - } else { - forbidden = forbidden && true - } + appNameEquals = oldLock.AppName == newLock.AppName + } else { + // no app lock set + appNameEquals = true } if oldLock.User != nil || newLock.GetUser() != nil { - if !utils.UserEqual(oldLock.User, newLock.GetUser()) { - err = errtypes.PermissionDenied("cannot change holder (user) when refreshing a lock") - } else { - forbidden = forbidden && true - } + lockUserEquals = utils.UserEqual(oldLock.User, newLock.GetUser()) + //err = errtypes.PermissionDenied("cannot change holder (user) when refreshing a lock") u := ctxpkg.ContextMustGetUser(ctx) - if !utils.UserEqual(oldLock.User, u.Id) { - err = errtypes.PermissionDenied("mismatching holder") - } else { - forbidden = forbidden && true + contextUserEquals = utils.UserEqual(oldLock.User, u.Id) + } else { + // no user lock set + lockUserEquals = true + contextUserEquals = true + } + + if appNameEquals && lockUserEquals && contextUserEquals { + return true, nil + } + + denialReasons := []string{} + + if !appNameEquals { + denialReasons = append(denialReasons, "app names of the locks are mismatching") + } + if !lockUserEquals { + denialReasons = append(denialReasons, "users of the locks are mismatching") + } + if !contextUserEquals { + denialReasons = append(denialReasons, "lock holder and current user are mismatching") + } + + errMsg := "cannot modify the lock because: " + + for i, reason := range denialReasons { + if i != 0 && i != len(denialReasons)-1 { + errMsg = errMsg + ", " } + errMsg = errMsg + reason } - return !forbidden, err + return false, errtypes.PermissionDenied(errMsg) } From 4b21e7a5e4ecc82eaef14ba291a01b59fcf43a62 Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Fri, 25 Mar 2022 09:49:36 +0100 Subject: [PATCH 16/19] fix lint issues --- pkg/storage/utils/decomposedfs/node/locks.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/node/locks.go b/pkg/storage/utils/decomposedfs/node/locks.go index dbcc9d853d..035cf09b97 100644 --- a/pkg/storage/utils/decomposedfs/node/locks.go +++ b/pkg/storage/utils/decomposedfs/node/locks.go @@ -329,7 +329,6 @@ func isLockModificationAllowed(ctx context.Context, oldLock *provider.Lock, newL if oldLock.User != nil || newLock.GetUser() != nil { lockUserEquals = utils.UserEqual(oldLock.User, newLock.GetUser()) - //err = errtypes.PermissionDenied("cannot change holder (user) when refreshing a lock") u := ctxpkg.ContextMustGetUser(ctx) contextUserEquals = utils.UserEqual(oldLock.User, u.Id) @@ -359,9 +358,9 @@ func isLockModificationAllowed(ctx context.Context, oldLock *provider.Lock, newL for i, reason := range denialReasons { if i != 0 && i != len(denialReasons)-1 { - errMsg = errMsg + ", " + errMsg += ", " } - errMsg = errMsg + reason + errMsg += reason } return false, errtypes.PermissionDenied(errMsg) From 01cd8b2f9bf6de561093a7f6a65c23fbc0dd8885 Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Fri, 25 Mar 2022 10:21:40 +0100 Subject: [PATCH 17/19] simplify locking logic --- pkg/storage/utils/decomposedfs/node/locks.go | 53 ++++++-------------- 1 file changed, 16 insertions(+), 37 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/node/locks.go b/pkg/storage/utils/decomposedfs/node/locks.go index 035cf09b97..455c4eb8d0 100644 --- a/pkg/storage/utils/decomposedfs/node/locks.go +++ b/pkg/storage/utils/decomposedfs/node/locks.go @@ -318,50 +318,29 @@ func isLockModificationAllowed(ctx context.Context, oldLock *provider.Lock, newL return true, nil } - var appNameEquals, lockUserEquals, contextUserEquals bool - - if oldLock.AppName != "" || newLock.AppName != "" { - appNameEquals = oldLock.AppName == newLock.AppName - } else { - // no app lock set - appNameEquals = true + appNameEquals := oldLock.AppName == newLock.AppName + if !appNameEquals { + return false, errtypes.PermissionDenied("app names of the locks are mismatching") } - if oldLock.User != nil || newLock.GetUser() != nil { - lockUserEquals = utils.UserEqual(oldLock.User, newLock.GetUser()) - - u := ctxpkg.ContextMustGetUser(ctx) - contextUserEquals = utils.UserEqual(oldLock.User, u.Id) - } else { + var lockUserEquals, contextUserEquals bool + if oldLock.User == nil && newLock.GetUser() == nil { // no user lock set lockUserEquals = true contextUserEquals = true - } - - if appNameEquals && lockUserEquals && contextUserEquals { - return true, nil - } - - denialReasons := []string{} - - if !appNameEquals { - denialReasons = append(denialReasons, "app names of the locks are mismatching") - } - if !lockUserEquals { - denialReasons = append(denialReasons, "users of the locks are mismatching") - } - if !contextUserEquals { - denialReasons = append(denialReasons, "lock holder and current user are mismatching") - } - - errMsg := "cannot modify the lock because: " + } else { + lockUserEquals = utils.UserEqual(oldLock.User, newLock.GetUser()) + if !lockUserEquals { + return false, errtypes.PermissionDenied("users of the locks are mismatching") + } - for i, reason := range denialReasons { - if i != 0 && i != len(denialReasons)-1 { - errMsg += ", " + u := ctxpkg.ContextMustGetUser(ctx) + contextUserEquals = utils.UserEqual(oldLock.User, u.Id) + if !contextUserEquals { + return false, errtypes.PermissionDenied("lock holder and current user are mismatching") } - errMsg += reason } - return false, errtypes.PermissionDenied(errMsg) + return appNameEquals && lockUserEquals && contextUserEquals, nil + } From dfca59a384eb1dd9bd5461a41812887399b179be Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Fri, 25 Mar 2022 10:30:07 +0100 Subject: [PATCH 18/19] update changelog --- changelog/unreleased/fix-locking.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/unreleased/fix-locking.md b/changelog/unreleased/fix-locking.md index 6ad4c68351..ad672e1f44 100644 --- a/changelog/unreleased/fix-locking.md +++ b/changelog/unreleased/fix-locking.md @@ -1,7 +1,7 @@ Bugfix: Fix locking on publik links and the decomposed filesystem We've fixed the behavior of locking on the decomposed filesystem, so that now -users can remove a lock which was initially created by another user (needed for WOPI integration). +app based locks can be modified user independently (needed for WOPI integration). Also we added a check, if a lock is already expired and if so, we lazily delete the lock. The InitiateUploadRequest now adds the Lock to the upload metadata so that an upload to an locked file is possible. From 520f58b4f53dde011c5b25331ee6a5133cb87865 Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Mon, 28 Mar 2022 09:54:18 +0200 Subject: [PATCH 19/19] use user from context for webdav unlock request --- internal/http/services/owncloud/ocdav/locks.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/http/services/owncloud/ocdav/locks.go b/internal/http/services/owncloud/ocdav/locks.go index d88bd1d986..ba02badf09 100644 --- a/internal/http/services/owncloud/ocdav/locks.go +++ b/internal/http/services/owncloud/ocdav/locks.go @@ -220,10 +220,13 @@ func (cls *cs3LS) Refresh(ctx context.Context, now time.Time, token string, dura return LockDetails{}, errors.ErrNotImplemented } func (cls *cs3LS) Unlock(ctx context.Context, now time.Time, ref *provider.Reference, token string) error { + u := ctxpkg.ContextMustGetUser(ctx) + r := &provider.UnlockRequest{ Ref: ref, Lock: &provider.Lock{ LockId: token, // can be a token or a Coded-URL + User: u.Id, }, } res, err := cls.client.Unlock(ctx, r)