Locking fixes (#2625)

* add X-Lock-Id to the datagatway to enable upload to a locked file

* expire locks and enable unlock without current user check

* fix paths and check additional scopes

* implement locking on public storage provider

* remove superflous scope checks

* use reference from info

* pass lockid on InitiateUploadRequest

* fix locking scope check for public links

* add changelog

* adapt tests to new logic

* reintroduce lock modification checks

* fix return code on expired lock

* add and adapt tests

* switch to forbidden

* rework lock modification permission logic

* fix lint issues

* simplify locking logic

* update changelog

* use user from context for webdav unlock request

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
+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.
+
+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

internal/grpc/interceptors/auth/scope.go

@@ -244,14 +244,24 @@ func extractRef(req interface{}, hasEditorRole bool) (*provider.Reference, bool)
  return v.GetRef(), true
  case *provider.InitiateFileDownloadRequest:
  return v.GetRef(), true
- case *appprovider.OpenInAppRequest:
- return &provider.Reference{ResourceId: v.ResourceInfo.Id}, true
- case *gateway.OpenInAppRequest:
+
+ // 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
+ // App provider requests
  case *appregistry.GetAppProvidersRequest:
- return &provider.Reference{ResourceId: v.ResourceInfo.Id}, true
+ return &provider.Reference{ResourceId: v.ResourceInfo.Id, Path: v.ResourceInfo.Path}, true
+ case *appprovider.OpenInAppRequest:
+ return &provider.Reference{ResourceId: v.ResourceInfo.Id, Path: v.ResourceInfo.Path}, true
+ case *gateway.OpenInAppRequest:
+ return v.GetRef(), true
  }
 
  if !hasEditorRole {

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) {

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})

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)

pkg/auth/scope/publicshare.go

@@ -77,6 +77,14 @@ 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 checkStorageRef(ctx, &share, v.GetRef()), nil
+ case *provider.UnlockRequest:
+ return checkStorageRef(ctx, &share, v.GetRef()), nil
+ case *provider.RefreshLockRequest:
+ return checkStorageRef(ctx, &share, v.GetRef()), nil
+ case *provider.SetLockRequest:
+ return checkStorageRef(ctx, &share, v.GetRef()), nil
  case *provider.ListContainerRequest:
  return checkStorageRef(ctx, &share, v.GetRef()), nil
  case *provider.InitiateFileDownloadRequest:
@@ -109,6 +117,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:

pkg/rhttp/datatx/manager/spaces/spaces.go

@@ -68,7 +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()
-
  var spaceID string
  spaceID, r.URL.Path = router.ShiftPath(r.URL.Path)
 

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"
@@ -121,7 +122,17 @@ 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")
  }
- return lock, err
+
+ // lock already expired
+ 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, errtypes.NotFound("no lock found")
+ }
+
+ return lock, nil
 }
 
 // RefreshLock refreshes the node's lock
@@ -165,13 +176,8 @@ 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 ok, err := isLockModificationAllowed(ctx, oldLock, lock); !ok {
+ return err
  }
 
  if err := json.NewEncoder(f).Encode(lock); err != nil {
@@ -222,9 +228,8 @@ 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 ok, err := isLockModificationAllowed(ctx, oldLock, lock); !ok {
+ return err
  }
 
  if err = os.Remove(f.Name()); err != nil {
@@ -307,3 +312,35 @@ func (n *Node) hasLocks(ctx context.Context) bool {
  _, err := os.Stat(n.LockFilePath()) // FIXME better error checking
  return err == nil
 }
+
+func isLockModificationAllowed(ctx context.Context, oldLock *provider.Lock, newLock *provider.Lock) (bool, error) {
+ if oldLock.Type == provider.LockType_LOCK_TYPE_SHARED {
+ return true, nil
+ }
+
+ appNameEquals := oldLock.AppName == newLock.AppName
+ if !appNameEquals {
+ return false, errtypes.PermissionDenied("app names of the locks are mismatching")
+ }
+
+ var lockUserEquals, contextUserEquals bool
+ if oldLock.User == nil && newLock.GetUser() == nil {
+ // no user lock set
+ lockUserEquals = true
+ contextUserEquals = true
+ } else {
+ lockUserEquals = utils.UserEqual(oldLock.User, newLock.GetUser())
+ if !lockUserEquals {
+ return false, errtypes.PermissionDenied("users of the locks are mismatching")
+ }
+
+ u := ctxpkg.ContextMustGetUser(ctx)
+ contextUserEquals = utils.UserEqual(oldLock.User, u.Id)
+ if !contextUserEquals {
+ return false, errtypes.PermissionDenied("lock holder and current user are mismatching")
+ }
+ }
+
+ return appNameEquals && lockUserEquals && contextUserEquals, nil
+
+}