Locking fixes

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