Fix webdav file versions endpoint bugs (cs3org#1526)

changelog/unreleased/fix_file_versions.md

@@ -0,0 +1,6 @@
+Bugfix: Fix webdav file versions endpoint bugs
+
+Etag and error code related bugs have been fixed for the webdav file versions endpoint and removed from the expected failures file.
+
+
+https://github.com/cs3org/reva/pull/1526

go.mod

@@ -13,7 +13,7 @@ require (
 	github.com/cheggaaa/pb v1.0.29
 	github.com/coreos/go-oidc v2.2.1+incompatible
 	github.com/cs3org/cato v0.0.0-20200828125504-e418fc54dd5e
-	github.com/cs3org/go-cs3apis v0.0.0-20210310133342-f4a10134033c
+	github.com/cs3org/go-cs3apis v0.0.0-20210316113645-e4a74cb8761c
 	github.com/dgrijalva/jwt-go v3.2.0+incompatible
 	github.com/eventials/go-tus v0.0.0-20200718001131-45c7ec8f5d59
 	github.com/ffurano/grpc-proto v0.0.0-20210312134900-65801a1ca184

go.sum

@@ -142,8 +142,8 @@ github.com/creack/pty v1.1.7/go.mod h1:lj5s0c3V2DBrqTV7llrYr5NG6My20zk30Fl46Y7Do
 github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
 github.com/cs3org/cato v0.0.0-20200828125504-e418fc54dd5e h1:tqSPWQeueWTKnJVMJffz4pz0o1WuQxJ28+5x5JgaHD8=
 github.com/cs3org/cato v0.0.0-20200828125504-e418fc54dd5e/go.mod h1:XJEZ3/EQuI3BXTp/6DUzFr850vlxq11I6satRtz0YQ4=
-github.com/cs3org/go-cs3apis v0.0.0-20210310133342-f4a10134033c h1:+HOawEG8T4uBZI/zJNAFcm4ygbiP+Zci0XRaVGZ0rYM=
-github.com/cs3org/go-cs3apis v0.0.0-20210310133342-f4a10134033c/go.mod h1:UXha4TguuB52H14EMoSsCqDj7k8a/t7g4gVP+bgY5LY=
+github.com/cs3org/go-cs3apis v0.0.0-20210316113645-e4a74cb8761c h1:2vcWjiaFkJMhMZHeTbkkXWwhhAOTAIKpul8yjAo95UU=
+github.com/cs3org/go-cs3apis v0.0.0-20210316113645-e4a74cb8761c/go.mod h1:UXha4TguuB52H14EMoSsCqDj7k8a/t7g4gVP+bgY5LY=
 github.com/cucumber/godog v0.8.1/go.mod h1:vSh3r/lM+psC1BPXvdkSEuNjmXfpVqrMGYAElF6hxnA=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=

internal/grpc/services/storageregistry/storageregistry.go

@@ -24,6 +24,7 @@ import (
 
 	registrypb "github.com/cs3org/go-cs3apis/cs3/storage/registry/v1beta1"
 	"github.com/cs3org/reva/pkg/appctx"
+	"github.com/cs3org/reva/pkg/errtypes"
 	"github.com/cs3org/reva/pkg/rgrpc"
 	"github.com/cs3org/reva/pkg/rgrpc/status"
 	"github.com/cs3org/reva/pkg/storage"
@@ -123,9 +124,16 @@ func (s *service) ListStorageProviders(ctx context.Context, req *registrypb.List
 func (s *service) GetStorageProvider(ctx context.Context, req *registrypb.GetStorageProviderRequest) (*registrypb.GetStorageProviderResponse, error) {
 	p, err := s.reg.FindProvider(ctx, req.Ref)
 	if err != nil {
-		return &registrypb.GetStorageProviderResponse{
-			Status: status.NewInternal(ctx, err, "error finding storage provider"),
-		}, nil
+		switch err.(type) {
+		case errtypes.IsNotFound:
+			return &registrypb.GetStorageProviderResponse{
+				Status: status.NewNotFound(ctx, err.Error()),
+			}, nil
+		default:
+			return &registrypb.GetStorageProviderResponse{
+				Status: status.NewInternal(ctx, err, "error finding storage provider"),
+			}, nil
+		}
 	}
 
 	fill(p)

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

@@ -28,6 +28,7 @@ import (
 	"regexp"
 	"strings"
 	"time"
+	"unicode/utf8"
 
 	gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1"
 	userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1"
@@ -221,10 +222,16 @@ func unwrap(rid string) *provider.ResourceId {
 	if err != nil {
 		return nil
 	}
+
 	parts := strings.SplitN(string(decodedID), ":", 2)
 	if len(parts) != 2 {
 		return nil
 	}
+
+	if !utf8.ValidString(parts[0]) || !utf8.ValidString(parts[1]) {
+		return nil
+	}
+
 	return &provider.ResourceId{
 		StorageId: parts[0],
 		OpaqueId:  parts[1],

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

@@ -46,6 +46,11 @@ func (h *VersionsHandler) Handler(s *svc, rid *provider.ResourceId) http.Handler
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		ctx := r.Context()
 
+		if rid == (*provider.ResourceId)(nil) {
+			http.Error(w, "404 Not Found", http.StatusNotFound)
+			return
+		}
+
 		// baseURI is encoded as part of the response payload in href field
 		baseURI := path.Join(ctx.Value(ctxKeyBaseURI).(string), wrapResourceID(rid))
 		ctx = context.WithValue(ctx, ctxKeyBaseURI, baseURI)
@@ -130,17 +135,6 @@ func (h *VersionsHandler) doListVersions(w http.ResponseWriter, r *http.Request,
 	// add version dir . entry, derived from file info
 	infos = append(infos, &provider.ResourceInfo{
 		Type: provider.ResourceType_RESOURCE_TYPE_CONTAINER,
-		Id: &provider.ResourceId{
-			StorageId: "virtual", // this is a virtual storage
-			OpaqueId:  path.Join("meta", wrapResourceID(rid), "v"),
-		},
-		Etag:     info.Etag,
-		MimeType: "httpd/unix-directory",
-		Mtime:    info.Mtime,
-		Path:     "v",
-		// PermissionSet
-		Size:  0,
-		Owner: info.Owner,
 	})
 
 	for i := range versions {
@@ -153,7 +147,7 @@ func (h *VersionsHandler) doListVersions(w http.ResponseWriter, r *http.Request,
 				OpaqueId:  info.Id.OpaqueId + "@" + versions[i].GetKey(),
 			},
 			// Checksum
-			// Etag: v.ETag,
+			Etag: versions[i].Etag,
 			// MimeType
 			Mtime: &types.Timestamp{
 				Seconds: versions[i].Mtime,

pkg/storage/fs/owncloud/owncloud.go

@@ -1953,6 +1953,7 @@ func (fs *ocfs) filterAsRevision(ctx context.Context, bn string, md os.FileInfo)
 			Key:   version,
 			Size:  uint64(md.Size()),
 			Mtime: uint64(mtime),
+			Etag:  calcEtag(ctx, md),
 		}
 	}
 	return nil

pkg/storage/utils/decomposedfs/node/node.go

@@ -306,6 +306,11 @@ func (n *Node) InternalPath() string {
 	return n.lu.InternalPath(n.ID)
 }
 
+// CalculateEtag returns a hash of fileid + tmtime (or mtime)
+func CalculateEtag(nodeID string, tmTime time.Time) (string, error) {
+	return calculateEtag(nodeID, tmTime)
+}
+
 // calculateEtag returns a hash of fileid + tmtime (or mtime)
 func calculateEtag(nodeID string, tmTime time.Time) (string, error) {
 	h := md5.New()

pkg/storage/utils/decomposedfs/revisions.go

@@ -67,15 +67,21 @@ func (fs *Decomposedfs) ListRevisions(ctx context.Context, ref *provider.Referen
 	if items, err := filepath.Glob(np + ".REV.*"); err == nil {
 		for i := range items {
 			if fi, err := os.Stat(items[i]); err == nil {
+				mtime := fi.ModTime()
 				rev := &provider.FileVersion{
 					Key:   filepath.Base(items[i]),
-					Mtime: uint64(fi.ModTime().Unix()),
+					Mtime: uint64(mtime.Unix()),
 				}
 				blobSize, err := node.ReadBlobSizeAttr(items[i])
 				if err != nil {
 					return nil, errors.Wrapf(err, "error reading blobsize xattr")
 				}
 				rev.Size = uint64(blobSize)
+				etag, err := node.CalculateEtag(np, mtime)
+				if err != nil {
+					return nil, errors.Wrapf(err, "error calculating etag")
+				}
+				rev.Etag = etag
 				revisions = append(revisions, rev)
 			}
 		}

pkg/storage/utils/eosfs/eosfs.go

@@ -1339,6 +1339,7 @@ func (fs *eosfs) convertToRevision(ctx context.Context, eosFileInfo *eosclient.F
 		Key:   path.Base(md.Path),
 		Size:  md.Size,
 		Mtime: md.Mtime.Seconds, // TODO do we need nanos here?
+		Etag:  md.Etag,
 	}
 	return revision, nil
 }

pkg/storage/utils/localfs/localfs.go

@@ -1043,6 +1043,7 @@ func (fs *localfs) ListRevisions(ctx context.Context, ref *provider.Reference) (
 			Key:   version,
 			Size:  uint64(mds[i].Size()),
 			Mtime: uint64(mtime),
+			Etag:  calcEtag(ctx, mds[i]),
 		})
 	}
 	return revisions, nil

tests/acceptance/expected-failures-on-OCIS-storage.md

@@ -86,12 +86,6 @@ Basic file management like up and download, move, copy, properties, quota, trash
 -   [apiTrashbin/trashbinRestore.feature:338](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L338)
 -   [apiTrashbin/trashbinRestore.feature:339](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L339)
 
-#### [requesting propfind with invalid fileid gives 502 error](https://github.com/owncloud/ocis/issues/771)
--   [apiVersions/fileVersions.feature:395](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L395)
--   [apiVersions/fileVersions.feature:396](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L396)
--   [apiVersions/fileVersions.feature:397](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L397)
--   [apiVersions/fileVersions.feature:398](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L398)
-
 #### [Implement Versions Feature for ocis storage](https://github.com/owncloud/product/issues/210)
 -   [apiWebdavEtagPropagation2/restoreVersion.feature:10](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavEtagPropagation2/restoreVersion.feature#L10)
 
@@ -121,12 +115,6 @@ Basic file management like up and download, move, copy, properties, quota, trash
 -   [apiWebdavUpload2/uploadFileUsingNewChunking.feature:168](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUpload2/uploadFileUsingNewChunking.feature#L168)
 -   [apiWebdavUpload2/uploadFileUsingNewChunking.feature:169](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUpload2/uploadFileUsingNewChunking.feature#L169)
 
-#### [Version count is 1 more than on oC10](https://github.com/owncloud/ocis/issues/1633)
--   [apiVersions/fileVersions.feature:373](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L373)
--   [apiVersions/fileVersions.feature:409](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L409)
--   [apiVersions/fileVersions.feature:420](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L420)
--   [apiVersions/fileVersions.feature:426](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L426)
-
 #### [PUT request with missing parent must return status code 409](https://github.com/owncloud/ocis/issues/824)
 -   [apiWebdavUpload1/uploadFile.feature:112](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUpload1/uploadFile.feature#L112)
 -   [apiWebdavUpload1/uploadFile.feature:113](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUpload1/uploadFile.feature#L113)
@@ -1371,9 +1359,6 @@ Scenario Outline: Renaming a file to a path with extension .part should not be p
 #### [getting the metadata without permission results in a 403 error](https://github.com/owncloud/ocis/issues/773)
 -   [apiVersions/fileVersionsSharingToShares.feature:256](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersionsSharingToShares.feature#L256)
 
-#### [The version number of a file is incorrect because of the incorrect number of `<d:getetag>` and `<d:getlastmodified>` element](https://github.com/owncloud/ocis/issues/1234)
--   [apiVersions/fileVersionsSharingToShares.feature:267](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersionsSharingToShares.feature#L267)
-
 #### [Sharing seems to work but does not work](https://github.com/owncloud/ocis/issues/1303)
 #### [Expiration date for user shares is not implemented](https://github.com/owncloud/ocis/issues/1250)
 -   [apiShareCreateSpecialToShares1/createShareExpirationDate.feature:26](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareCreateSpecialToShares1/createShareExpirationDate.feature#L26)

tests/acceptance/expected-failures-on-OWNCLOUD-storage.md

@@ -99,12 +99,6 @@ The following scenarios fail on OWNCLOUD storage but not on OCIS storage:
 -   [apiTrashbin/trashbinRestore.feature:338](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L338)
 -   [apiTrashbin/trashbinRestore.feature:339](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L339)
 
-#### [requesting propfind with invalid fileid gives 502 error](https://github.com/owncloud/ocis/issues/771)
--   [apiVersions/fileVersions.feature:395](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L395)
--   [apiVersions/fileVersions.feature:396](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L396)
--   [apiVersions/fileVersions.feature:397](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L397)
--   [apiVersions/fileVersions.feature:398](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L398)
-
 #### [uploading with old-chunking does not work](https://github.com/owncloud/ocis/issues/1343)
 #### [remote.php/dav/uploads endpoint does not exist](https://github.com/owncloud/ocis/issues/1321)
 -   [apiVersions/fileVersions.feature:15](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L15)
@@ -136,10 +130,7 @@ The following scenarios fail on OWNCLOUD storage but not on OCIS storage:
 -   [apiVersions/fileVersions.feature:104](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L104)
 
 #### [Version count is 1 more than on oC10](https://github.com/owncloud/ocis/issues/1633)
--   [apiVersions/fileVersions.feature:373](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L373)
 -   [apiVersions/fileVersions.feature:409](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L409)
--   [apiVersions/fileVersions.feature:420](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L420)
--   [apiVersions/fileVersions.feature:426](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L426)
 
 #### [Version cannot be restored when file has been renamed](https://github.com/owncloud/ocis/issues/1633)
 -   [apiVersions/fileVersions.feature:400](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L400)
@@ -1490,9 +1481,6 @@ Scenario Outline: Moving a file into a shared folder as the sharee and as the sh
 #### [getting the metadata without permission results in a 403 error](https://github.com/owncloud/ocis/issues/773)
 -   [apiVersions/fileVersionsSharingToShares.feature:256](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersionsSharingToShares.feature#L256)
 
-#### [The version number of a file is incorrect because of the incorrect number of `<d:getetag>` and `<d:getlastmodified>` element](https://github.com/owncloud/ocis/issues/1234)
--   [apiVersions/fileVersionsSharingToShares.feature:267](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersionsSharingToShares.feature#L267)
-
 #### [Sharing seems to work but does not work](https://github.com/owncloud/ocis/issues/1303)
 #### [Expiration date for user shares is not implemented](https://github.com/owncloud/ocis/issues/1250)
 -   [apiShareCreateSpecialToShares1/createShareExpirationDate.feature:26](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareCreateSpecialToShares1/createShareExpirationDate.feature#L26)

tests/acceptance/expected-failures-on-S3NG-storage.md

@@ -86,12 +86,6 @@ Basic file management like up and download, move, copy, properties, quota, trash
 -   [apiTrashbin/trashbinRestore.feature:338](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L338)
 -   [apiTrashbin/trashbinRestore.feature:339](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L339)
 
-#### [requesting propfind with invalid fileid gives 502 error](https://github.com/owncloud/ocis/issues/771)
--   [apiVersions/fileVersions.feature:395](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L395)
--   [apiVersions/fileVersions.feature:396](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L396)
--   [apiVersions/fileVersions.feature:397](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L397)
--   [apiVersions/fileVersions.feature:398](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L398)
-
 #### [Implement Versions Feature for ocis storage](https://github.com/owncloud/product/issues/210)
 -   [apiWebdavEtagPropagation2/restoreVersion.feature:10](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavEtagPropagation2/restoreVersion.feature#L10)
 
@@ -121,12 +115,6 @@ Basic file management like up and download, move, copy, properties, quota, trash
 -   [apiWebdavUpload2/uploadFileUsingNewChunking.feature:168](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUpload2/uploadFileUsingNewChunking.feature#L168)
 -   [apiWebdavUpload2/uploadFileUsingNewChunking.feature:169](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUpload2/uploadFileUsingNewChunking.feature#L169)
 
-#### [Version count is 1 more than on oC10](https://github.com/owncloud/ocis/issues/1633)
--   [apiVersions/fileVersions.feature:373](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L373)
--   [apiVersions/fileVersions.feature:409](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L409)
--   [apiVersions/fileVersions.feature:420](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L420)
--   [apiVersions/fileVersions.feature:426](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L426)
-
 #### [PUT request with missing parent must return status code 409](https://github.com/owncloud/ocis/issues/824)
 -   [apiWebdavUpload1/uploadFile.feature:112](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUpload1/uploadFile.feature#L112)
 -   [apiWebdavUpload1/uploadFile.feature:113](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUpload1/uploadFile.feature#L113)
@@ -1371,9 +1359,6 @@ Scenario Outline: Renaming a file to a path with extension .part should not be p
 #### [getting the metadata without permission results in a 403 error](https://github.com/owncloud/ocis/issues/773)
 -   [apiVersions/fileVersionsSharingToShares.feature:256](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersionsSharingToShares.feature#L256)
 
-#### [The version number of a file is incorrect because of the incorrect number of `<d:getetag>` and `<d:getlastmodified>` element](https://github.com/owncloud/ocis/issues/1234)
--   [apiVersions/fileVersionsSharingToShares.feature:267](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersionsSharingToShares.feature#L267)
-
 #### [Sharing seems to work but does not work](https://github.com/owncloud/ocis/issues/1303)
 #### [Expiration date for user shares is not implemented](https://github.com/owncloud/ocis/issues/1250)
 -   [apiShareCreateSpecialToShares1/createShareExpirationDate.feature:26](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareCreateSpecialToShares1/createShareExpirationDate.feature#L26)