incorporate review

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

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

@@ -71,7 +71,9 @@ func (s *svc) handleHead(w http.ResponseWriter, r *http.Request, ns string) {
 	w.Header().Set("ETag", info.Etag)
 	w.Header().Set("OC-FileId", wrapResourceID(info.Id))
 	w.Header().Set("OC-ETag", info.Etag)
-	w.Header().Set("OC-Checksum", fmt.Sprintf("%s:%s", strings.ToUpper(string(storageprovider.GRPC2PKGXS(info.Checksum.Type))), info.Checksum.Sum))
+	if info.Checksum != nil {
+		w.Header().Set("OC-Checksum", fmt.Sprintf("%s:%s", strings.ToUpper(string(storageprovider.GRPC2PKGXS(info.Checksum.Type))), info.Checksum.Sum))
+	}
 	t := utils.TSToTime(info.Mtime).UTC()
 	lastModifiedString := t.Format(time.RFC1123Z)
 	w.Header().Set("Last-Modified", lastModifiedString)

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

@@ -106,6 +106,11 @@ func (s *svc) handlePropfind(w http.ResponseWriter, r *http.Request, ns string)
 
 	metadataKeys := []string{}
 	if pf.Allprop != nil {
+		// TODO this changes the behavior and returns all properties if allprops has been set,
+		// but allprops should only return some default properties
+		// see https://tools.ietf.org/html/rfc4918#section-9.1
+		// the description of arbitrary_metadata_keys in https://cs3org.github.io/cs3apis/#cs3.storage.provider.v1beta1.ListContainerRequest an others may need clarification
+		// tracked in https://github.com/cs3org/cs3apis/issues/104
 		metadataKeys = append(metadataKeys, "*")
 	} else {
 		for i := range pf.Prop {
@@ -410,7 +415,7 @@ func (s *svc) mdToPropResponse(ctx context.Context, pf *propfindXML, md *provide
 				checksums.WriteString(string(e.Value))
 			}
 		}
-		if checksums.Len() > 13 {
+		if checksums.Len() > 0 {
 			checksums.WriteString("</oc:checksum>")
 			response.Propstat[0].Prop = append(response.Propstat[0].Prop, s.newProp("oc:checksums", checksums.String()))
 		}

pkg/storage/fs/ocis/node.go

@@ -23,6 +23,7 @@ import (
 	"crypto/md5"
 	"encoding/hex"
 	"fmt"
+	"hash"
 	"io"
 	"os"
 	"path/filepath"
@@ -642,8 +643,8 @@ func (n *Node) SetTMTime(t time.Time) (err error) {
 }
 
 // SetChecksum writes the checksum with the given checksum type to the extended attributes
-func (n *Node) SetChecksum(csType string, bytes []byte) (err error) {
-	return xattr.Set(n.lu.toInternalPath(n.ID), checksumPrefix+csType, bytes)
+func (n *Node) SetChecksum(csType string, h hash.Hash) (err error) {
+	return xattr.Set(n.lu.toInternalPath(n.ID), checksumPrefix+csType, h.Sum(nil))
 }
 
 // UnsetTempEtag removes the temporary etag attribute

pkg/storage/fs/ocis/upload.go

@@ -25,6 +25,7 @@ import (
 	"encoding/hex"
 	"encoding/json"
 	"fmt"
+	"hash"
 	"hash/adler32"
 	"io"
 	"io/ioutil"
@@ -42,6 +43,7 @@ import (
 	"github.com/cs3org/reva/pkg/user"
 	"github.com/google/uuid"
 	"github.com/pkg/errors"
+	"github.com/rs/zerolog"
 	tusd "github.com/tus/tusd/pkg/handler"
 )
 
@@ -428,25 +430,27 @@ func (upload *fileUpload) FinishUpload(ctx context.Context) (err error) {
 	sublog := appctx.GetLogger(upload.ctx).With().Interface("info", upload.info).Str("binPath", upload.binPath).Str("targetPath", targetPath).Logger()
 
 	// calculate the checksum of the written bytes
-	// TODO only calculate in sync if checksum was sent?
+	// they will all be written to the metadata later, so we cannot omit any of them
+	// TODO only calculate the checksum in sync that was requested to match, the rest could be async ... but the tests currently expect all to be present
+	// TODO the hashes all implement BinaryMarshaler so we could try to persist the state for resumable upload. we would neet do keep track of the copied bytes ...
 	sha1h := sha1.New()
 	md5h := md5.New()
 	adler32h := adler32.New()
+	{
+		f, err := os.Open(upload.binPath)
+		if err != nil {
+			sublog.Err(err).Msg("ocisfs: could not open file for checksumming")
+			// we can continue if no oc checksum header is set
+		}
+		defer f.Close()
 
-	f, err := os.Open(upload.binPath)
-	if err != nil {
-		sublog.Err(err).Msg("ocisfs: could not open file for checksumming")
-		// we can continue if no oc checksum header is set
-	}
-	defer f.Close()
-
-	r1 := io.TeeReader(f, sha1h)
-	r2 := io.TeeReader(r1, md5h)
+		r1 := io.TeeReader(f, sha1h)
+		r2 := io.TeeReader(r1, md5h)
 
-	if _, err := io.Copy(adler32h, r2); err != nil {
-		sublog.Err(err).Msg("ocisfs: could not copy bytes for checksumming")
+		if _, err := io.Copy(adler32h, r2); err != nil {
+			sublog.Err(err).Msg("ocisfs: could not copy bytes for checksumming")
+		}
 	}
-
 	// compare if they match the sent checksum
 	// TODO the tus checksum extension would do this on every chunk, but I currently don't see an easy way to pass in the requested checksum. for now we do it in FinishUpload which is also called for chunked uploads
 	if upload.info.MetaData["checksum"] != "" {
@@ -456,22 +460,16 @@ func (upload *fileUpload) FinishUpload(ctx context.Context) (err error) {
 		}
 		switch parts[0] {
 		case "sha1":
-			if parts[1] != hex.EncodeToString(sha1h.Sum(nil)) {
-				upload.discardChunk()
-				return errtypes.ChecksumMismatch(fmt.Sprintf("invalid checksum: expected %s got %x", upload.info.MetaData["checksum"], sha1h.Sum(nil)))
-			}
+			err = upload.checkHash(parts[1], sha1h)
 		case "md5":
-			if parts[1] != hex.EncodeToString(md5h.Sum(nil)) {
-				upload.discardChunk()
-				return errtypes.ChecksumMismatch(fmt.Sprintf("invalid checksum: expected %s got %x", upload.info.MetaData["checksum"], md5h.Sum(nil)))
-			}
+			err = upload.checkHash(parts[1], md5h)
 		case "adler32":
-			if parts[1] != hex.EncodeToString(adler32h.Sum(nil)) {
-				upload.discardChunk()
-				return errtypes.ChecksumMismatch(fmt.Sprintf("invalid checksum: expected %s got %x", upload.info.MetaData["checksum"], adler32h.Sum(nil)))
-			}
+			err = upload.checkHash(parts[1], adler32h)
 		default:
-			return errtypes.BadRequest("unsupported checksum algorithm: " + parts[0])
+			err = errtypes.BadRequest("unsupported checksum algorithm: " + parts[0])
+		}
+		if err != nil {
+			return err
 		}
 	}
 
@@ -500,28 +498,10 @@ func (upload *fileUpload) FinishUpload(ctx context.Context) (err error) {
 		return
 	}
 
-	// now write the checksums
-	if err := n.SetChecksum("sha1", sha1h.Sum(nil)); err != nil {
-		sublog.Err(err).
-			Str("csType", "sha1").
-			Bytes("hash", sha1h.Sum(nil)).
-			Msg("ocisfs: could not write checksum")
-		// this is not critical, the bytes are there so we will continue
-	}
-	if err := n.SetChecksum("md5", md5h.Sum(nil)); err != nil {
-		sublog.Err(err).
-			Str("csType", "md5").
-			Bytes("hash", md5h.Sum(nil)).
-			Msg("ocisfs: could not write checksum")
-		// this is not critical, the bytes are there so we will continue
-	}
-	if err := n.SetChecksum("adler32", adler32h.Sum(nil)); err != nil {
-		sublog.Err(err).
-			Str("csType", "adler32").
-			Bytes("hash", adler32h.Sum(nil)).
-			Msg("ocisfs: could not write checksum")
-		// this is not critical, the bytes are there so we will continue
-	}
+	// now try write all checksums
+	tryWritingChecksum(&sublog, n, "sha1", sha1h)
+	tryWritingChecksum(&sublog, n, "md5", md5h)
+	tryWritingChecksum(&sublog, n, "adler32", adler32h)
 
 	// who will become the owner?  the owner of the parent actually ... not the currently logged in user
 	err = n.writeMetadata(&userpb.UserId{
@@ -574,6 +554,23 @@ func (upload *fileUpload) FinishUpload(ctx context.Context) (err error) {
 	return upload.fs.tp.Propagate(upload.ctx, n)
 }
 
+func (upload *fileUpload) checkHash(expected string, h hash.Hash) error {
+	if expected != hex.EncodeToString(h.Sum(nil)) {
+		upload.discardChunk()
+		return errtypes.ChecksumMismatch(fmt.Sprintf("invalid checksum: expected %s got %x", upload.info.MetaData["checksum"], h.Sum(nil)))
+	}
+	return nil
+}
+func tryWritingChecksum(log *zerolog.Logger, n *Node, algo string, h hash.Hash) {
+	if err := n.SetChecksum(algo, h); err != nil {
+		log.Err(err).
+			Str("csType", algo).
+			Bytes("hash", h.Sum(nil)).
+			Msg("ocisfs: could not write checksum")
+		// this is not critical, the bytes are there so we will continue
+	}
+}
+
 func (upload *fileUpload) discardChunk() {
 	if err := os.Remove(upload.binPath); err != nil {
 		if !os.IsNotExist(err) {