[full-ci] fix an error when lock/unlock a file (#4518)

* [full-ci] fix an error when lock/unlock a public shared file

* added errtype to http code mapping

* reworked the error handling

---------

Co-authored-by: Roman Perekhod <rperekhod@owncloud.com>

changelog/unreleased/fix-public-link-lock.md

@@ -0,0 +1,6 @@
+Bugfix: Fix an error when lock/unlock a file
+
+We fixed a bug when anonymous user with viewer role in public link of a folder can lock/unlock a file inside it
+
+https://github.com/cs3org/reva/pull/4518  
+https://github.com/owncloud/ocis/issues/7785  

internal/grpc/services/storageprovider/storageprovider.go

@@ -33,6 +33,7 @@ import (
 	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
 	typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1"
 	"github.com/cs3org/reva/v2/pkg/appctx"
+	"github.com/cs3org/reva/v2/pkg/conversions"
 	ctxpkg "github.com/cs3org/reva/v2/pkg/ctx"
 	"github.com/cs3org/reva/v2/pkg/errtypes"
 	"github.com/cs3org/reva/v2/pkg/events"
@@ -230,6 +231,11 @@ 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) {
+	if !canLockPublicShare(ctx) {
+		return &provider.SetLockResponse{
+			Status: status.NewPermissionDenied(ctx, nil, "no permission to lock the share"),
+		}, nil
+	}
 	err := s.storage.SetLock(ctx, req.Ref, req.Lock)
 
 	return &provider.SetLockResponse{
@@ -249,6 +255,12 @@ func (s *service) GetLock(ctx context.Context, req *provider.GetLockRequest) (*p
 
 // RefreshLock refreshes an existing lock on the given reference
 func (s *service) RefreshLock(ctx context.Context, req *provider.RefreshLockRequest) (*provider.RefreshLockResponse, error) {
+	if !canLockPublicShare(ctx) {
+		return &provider.RefreshLockResponse{
+			Status: status.NewPermissionDenied(ctx, nil, "no permission to refresh the share lock"),
+		}, nil
+	}
+
 	err := s.storage.RefreshLock(ctx, req.Ref, req.Lock, req.ExistingLockId)
 
 	return &provider.RefreshLockResponse{
@@ -258,6 +270,12 @@ func (s *service) RefreshLock(ctx context.Context, req *provider.RefreshLockRequ
 
 // Unlock removes an existing lock from the given reference
 func (s *service) Unlock(ctx context.Context, req *provider.UnlockRequest) (*provider.UnlockResponse, error) {
+	if !canLockPublicShare(ctx) {
+		return &provider.UnlockResponse{
+			Status: status.NewPermissionDenied(ctx, nil, "no permission to unlock the share"),
+		}, nil
+	}
+
 	err := s.storage.Unlock(ctx, req.Ref, req.Lock)
 
 	return &provider.UnlockResponse{
@@ -1285,3 +1303,9 @@ func estreamFromConfig(c eventconfig) (events.Stream, error) {
 
 	return stream.NatsFromConfig("storageprovider", false, stream.NatsConfig(c))
 }
+
+func canLockPublicShare(ctx context.Context) bool {
+	u := ctxpkg.ContextMustGetUser(ctx)
+	psr := utils.ReadPlainFromOpaque(u.Opaque, "public-share-role")
+	return psr == "" || psr == conversions.RoleEditor
+}

internal/http/services/owncloud/ocdav/errors/error.go

@@ -21,6 +21,7 @@ package errors
 import (
 	"bytes"
 	"encoding/xml"
+	"fmt"
 	"net/http"
 
 	rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
@@ -193,3 +194,20 @@ func HandleWebdavError(log *zerolog.Logger, w http.ResponseWriter, b []byte, err
 		log.Err(err).Msg("error writing response")
 	}
 }
+
+func NewErrFromStatus(s *rpc.Status) error {
+	switch s.GetCode() {
+	case rpc.Code_CODE_OK:
+		return nil
+	case rpc.Code_CODE_DEADLINE_EXCEEDED:
+		return ErrInvalidTimeout
+	case rpc.Code_CODE_PERMISSION_DENIED:
+		return ErrForbidden
+	case rpc.Code_CODE_LOCKED, rpc.Code_CODE_FAILED_PRECONDITION:
+		return ErrLocked
+	case rpc.Code_CODE_UNIMPLEMENTED:
+		return ErrNotImplemented
+	default:
+		return fmt.Errorf(s.GetMessage())
+	}
+}

internal/http/services/owncloud/ocdav/locks.go

@@ -21,6 +21,7 @@ package ocdav
 import (
 	"context"
 	"encoding/xml"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -34,13 +35,12 @@ import (
 	rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
 	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
 	types "github.com/cs3org/go-cs3apis/cs3/types/v1beta1"
-	"github.com/cs3org/reva/v2/internal/http/services/owncloud/ocdav/errors"
+	ocdavErrors "github.com/cs3org/reva/v2/internal/http/services/owncloud/ocdav/errors"
 	"github.com/cs3org/reva/v2/internal/http/services/owncloud/ocdav/net"
 	"github.com/cs3org/reva/v2/internal/http/services/owncloud/ocdav/prop"
 	"github.com/cs3org/reva/v2/internal/http/services/owncloud/ocdav/spacelookup"
 	"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/rgrpc/todo/pool"
 	"github.com/cs3org/reva/v2/pkg/utils"
 	"github.com/google/uuid"
@@ -172,15 +172,15 @@ type cs3LS struct {
 }
 
 func (cls *cs3LS) Confirm(ctx context.Context, now time.Time, name0, name1 string, conditions ...Condition) (func(), error) {
-	return nil, errors.ErrNotImplemented
+	return nil, ocdavErrors.ErrNotImplemented
 }
 
 func (cls *cs3LS) Create(ctx context.Context, now time.Time, details LockDetails) (string, error) {
 	// always assume depth infinity?
 	/*
 		if !details.ZeroDepth {
 		 The CS3 Lock api currently has no depth property, it only locks single resources
-			return "", errors.ErrUnsupportedLockInfo
+			return "", ocdavErrors.ErrUnsupportedLockInfo
 		}
 	*/
 
@@ -225,20 +225,19 @@ func (cls *cs3LS) Create(ctx context.Context, now time.Time, details LockDetails
 	if err != nil {
 		return "", err
 	}
-	switch res.Status.Code {
+	switch res.GetStatus().GetCode() {
 	case rpc.Code_CODE_OK:
 		return lockTokenPrefix + token.String(), nil
-	case rpc.Code_CODE_FAILED_PRECONDITION:
-		return "", errtypes.Aborted("file is already locked")
 	default:
-		return "", errtypes.NewErrtypeFromStatus(res.Status)
+		return "", ocdavErrors.NewErrFromStatus(res.GetStatus())
 	}
 
 }
 
 func (cls *cs3LS) Refresh(ctx context.Context, now time.Time, token string, duration time.Duration) (LockDetails, error) {
-	return LockDetails{}, errors.ErrNotImplemented
+	return LockDetails{}, ocdavErrors.ErrNotImplemented
 }
+
 func (cls *cs3LS) Unlock(ctx context.Context, now time.Time, ref *provider.Reference, token string) error {
 	u := ctxpkg.ContextMustGetUser(ctx)
 
@@ -260,14 +259,11 @@ func (cls *cs3LS) Unlock(ctx context.Context, now time.Time, ref *provider.Refer
 		return err
 	}
 
-	switch res.Status.Code {
-	case rpc.Code_CODE_OK:
-		return nil
-	case rpc.Code_CODE_FAILED_PRECONDITION:
-		return errtypes.Aborted("file is not locked")
-	default:
-		return errtypes.NewErrtypeFromStatus(res.Status)
+	newErr := ocdavErrors.NewErrFromStatus(res.GetStatus())
+	if newErr != nil {
+		appctx.GetLogger(ctx).Error().Str("token", token).Interface("unlock", ref).Msg("could not unlock " + res.GetStatus().GetMessage())
 	}
+	return newErr
 }
 
 // LockDetails are a lock's metadata.
@@ -302,7 +298,7 @@ func readLockInfo(r io.Reader) (li lockInfo, status int, err error) {
 				// http://www.webdav.org/specs/rfc4918.html#refreshing-locks
 				return lockInfo{}, 0, nil
 			}
-			err = errors.ErrInvalidLockInfo
+			err = ocdavErrors.ErrInvalidLockInfo
 		}
 		return lockInfo{}, http.StatusBadRequest, err
 	}
@@ -349,15 +345,15 @@ func parseTimeout(s string) (time.Duration, error) {
 	}
 	const pre = "Second-"
 	if !strings.HasPrefix(s, pre) {
-		return 0, errors.ErrInvalidTimeout
+		return 0, ocdavErrors.ErrInvalidTimeout
 	}
 	s = s[len(pre):]
 	if s == "" || s[0] < '0' || '9' < s[0] {
-		return 0, errors.ErrInvalidTimeout
+		return 0, ocdavErrors.ErrInvalidTimeout
 	}
 	n, err := strconv.ParseInt(s, 10, 64)
 	if err != nil || 1<<32-1 < n {
-		return 0, errors.ErrInvalidTimeout
+		return 0, ocdavErrors.ErrInvalidTimeout
 	}
 	return time.Duration(n) * time.Second, nil
 }
@@ -420,7 +416,7 @@ func (s *svc) handleLock(w http.ResponseWriter, r *http.Request, ns string) (ret
 		return http.StatusInternalServerError, err
 	}
 	if cs3Status.Code != rpc.Code_CODE_OK {
-		return http.StatusInternalServerError, errtypes.NewErrtypeFromStatus(cs3Status)
+		return http.StatusInternalServerError, ocdavErrors.NewErrFromStatus(cs3Status)
 	}
 
 	return s.lockReference(ctx, w, r, ref)
@@ -444,12 +440,12 @@ func (s *svc) lockReference(ctx context.Context, w http.ResponseWriter, r *http.
 	sublog := appctx.GetLogger(ctx).With().Interface("ref", ref).Logger()
 	duration, err := parseTimeout(r.Header.Get(net.HeaderTimeout))
 	if err != nil {
-		return http.StatusBadRequest, errors.ErrInvalidTimeout
+		return http.StatusBadRequest, ocdavErrors.ErrInvalidTimeout
 	}
 
 	li, status, err := readLockInfo(r.Body)
 	if err != nil {
-		return status, errors.ErrInvalidLockInfo
+		return status, ocdavErrors.ErrInvalidLockInfo
 	}
 
 	u := ctxpkg.ContextMustGetUser(ctx)
@@ -459,17 +455,17 @@ func (s *svc) lockReference(ctx context.Context, w http.ResponseWriter, r *http.
 		// An empty lockInfo means to refresh the lock.
 		ih, ok := parseIfHeader(r.Header.Get(net.HeaderIf))
 		if !ok {
-			return http.StatusBadRequest, errors.ErrInvalidIfHeader
+			return http.StatusBadRequest, ocdavErrors.ErrInvalidIfHeader
 		}
 		if len(ih.lists) == 1 && len(ih.lists[0].conditions) == 1 {
 			token = ih.lists[0].conditions[0].Token
 		}
 		if token == "" {
-			return http.StatusBadRequest, errors.ErrInvalidLockToken
+			return http.StatusBadRequest, ocdavErrors.ErrInvalidLockToken
 		}
 		ld, err = s.LockSystem.Refresh(ctx, now, token, duration)
 		if err != nil {
-			if err == errors.ErrNoSuchLock {
+			if err == ocdavErrors.ErrNoSuchLock {
 				return http.StatusPreconditionFailed, err
 			}
 			return http.StatusInternalServerError, err
@@ -484,7 +480,7 @@ func (s *svc) lockReference(ctx context.Context, w http.ResponseWriter, r *http.
 			if depth != 0 && depth != infiniteDepth {
 				// Section 9.10.3 says that "Values other than 0 or infinity must not be
 				// used with the Depth header on a LOCK method".
-				return http.StatusBadRequest, errors.ErrInvalidDepth
+				return http.StatusBadRequest, ocdavErrors.ErrInvalidDepth
 			}
 		}
 		/* our url path has been shifted, so we don't need to do this?
@@ -505,15 +501,16 @@ func (s *svc) lockReference(ctx context.Context, w http.ResponseWriter, r *http.
 		//      should we do that in the decomposedfs as well? the node does not exist
 		//      this actually is a name based lock ... ugh
 		token, err = s.LockSystem.Create(ctx, now, ld)
+
+		//
 		if err != nil {
-			switch err.(type) {
-			case errtypes.Aborted:
+			switch {
+			case errors.Is(err, ocdavErrors.ErrLocked):
 				return http.StatusLocked, err
-			case errtypes.PermissionDenied:
+			case errors.Is(err, ocdavErrors.ErrForbidden):
 				return http.StatusForbidden, err
 			default:
 				return http.StatusInternalServerError, err
-
 			}
 		}
 
@@ -611,7 +608,7 @@ func (s *svc) handleUnlock(w http.ResponseWriter, r *http.Request, ns string) (s
 		return http.StatusInternalServerError, err
 	}
 	if cs3Status.Code != rpc.Code_CODE_OK {
-		return http.StatusInternalServerError, errtypes.NewErrtypeFromStatus(cs3Status)
+		return http.StatusInternalServerError, ocdavErrors.NewErrFromStatus(cs3Status)
 	}
 
 	return s.unlockReference(ctx, w, r, ref)
@@ -639,16 +636,14 @@ func (s *svc) unlockReference(ctx context.Context, _ http.ResponseWriter, r *htt
 		t = t[1 : len(t)-1]
 	}
 
-	switch err := s.LockSystem.Unlock(ctx, time.Now(), ref, t); err {
-	case nil:
-		return http.StatusNoContent, err
-	case errors.ErrForbidden:
-		return http.StatusForbidden, err
-	case errors.ErrLocked:
+	err := s.LockSystem.Unlock(ctx, time.Now(), ref, t)
+	switch {
+	case err == nil:
+		return http.StatusNoContent, nil
+	case errors.Is(err, ocdavErrors.ErrLocked):
 		return http.StatusLocked, err
-	case errors.ErrNoSuchLock:
-		return http.StatusConflict, err
-	default:
-		return http.StatusInternalServerError, err
+	case errors.Is(err, ocdavErrors.ErrForbidden):
+		return http.StatusForbidden, err
 	}
+	return http.StatusInternalServerError, err
 }

pkg/errtypes/errtypes.go

@@ -295,12 +295,13 @@ func NewErrtypeFromStatus(status *rpc.Status) error {
 	case rpc.Code_CODE_UNIMPLEMENTED:
 		return NotSupported(status.Message)
 	case rpc.Code_CODE_PERMISSION_DENIED:
-		// FIXME add locked status!
-		if strings.HasPrefix(status.Message, "set lock: error: locked by ") {
-			return Locked(strings.TrimPrefix(status.Message, "set lock: error: locked by "))
-		}
 		return PermissionDenied(status.Message)
 	case rpc.Code_CODE_LOCKED:
+		// FIXME make something better for that
+		msg := strings.Split(status.Message, "error: locked by ")
+		if len(msg) > 1 {
+			return Locked(msg[len(msg)-1])
+		}
 		return Locked(status.Message)
 	// case rpc.Code_CODE_DATA_LOSS: ?
 	//	IsPartialContent
@@ -350,3 +351,37 @@ func NewErrtypeFromHTTPStatusCode(code int, message string) error {
 		return InternalError(message)
 	}
 }
+
+// NewHTTPStatusCodeFromErrtype maps an errtype to an http status
+func NewHTTPStatusCodeFromErrtype(err error) int {
+	switch err.(type) {
+	case NotFound:
+		return http.StatusNotFound
+	case AlreadyExists:
+		return http.StatusConflict
+	case NotSupported:
+		return http.StatusNotImplemented
+	case NotModified:
+		return http.StatusNotModified
+	case InvalidCredentials:
+		return http.StatusUnauthorized
+	case PermissionDenied:
+		return http.StatusForbidden
+	case Locked:
+		return http.StatusLocked
+	case Aborted:
+		return http.StatusPreconditionFailed
+	case PreconditionFailed:
+		return http.StatusMethodNotAllowed
+	case InsufficientStorage:
+		return http.StatusInsufficientStorage
+	case BadRequest:
+		return http.StatusBadRequest
+	case PartialContent:
+		return http.StatusPartialContent
+	case ChecksumMismatch:
+		return StatusChecksumMismatch
+	default:
+		return http.StatusInternalServerError
+	}
+}