ocdav: upload file to storage provider after assembling chunks

cs3org · Oct 16, 2020 · bb49b9f · bb49b9f
1 parent fb08084
commit bb49b9f
Show file tree

Hide file tree

Showing 5 changed files with 46 additions and 75 deletions.
diff --git a/changelog/unreleased/ocdav-putchunked.md b/changelog/unreleased/ocdav-putchunked.md
@@ -0,0 +1,7 @@
+Bugfix: Upload file to storage provider after assembling chunks
+
+In the PUT handler for chunked uploads in ocdav, we store the individual
+chunks in temporary file but do not write the assembled file to storage.
+This PR fixes that.
+
+https://github.com/cs3org/reva/pull/1253
diff --git a/cmd/reva/upload.go b/cmd/reva/upload.go
@@ -84,7 +84,6 @@ func uploadCommand() *command {
 		if err != nil {
 			return err
 		}
-		defer fd.Close()
 
 		fmt.Printf("Local file size: %d bytes\n", md.Size())
 

diff --git a/internal/http/services/owncloud/ocdav/put.go b/internal/http/services/owncloud/ocdav/put.go
@@ -19,6 +19,7 @@
 package ocdav
 
 import (
+	"io"
 	"net/http"
 	"path"
 	"regexp"
@@ -157,18 +158,34 @@ func (s *svc) handlePut(w http.ResponseWriter, r *http.Request, ns string) {
 		}
 	}
 
+	length, err := strconv.ParseInt(r.Header.Get("Content-Length"), 10, 64)
+	if err != nil {
+		// Fallback to Upload-Length
+		length, err = strconv.ParseInt(r.Header.Get("Upload-Length"), 10, 64)
+		if err != nil {
+			w.WriteHeader(http.StatusBadRequest)
+			return
+		}
+	}
+
+	s.handlePutHelper(w, r, r.Body, fn, length)
+}
+
+func (s *svc) handlePutHelper(w http.ResponseWriter, r *http.Request, content io.Reader, fn string, length int64) {
+	ctx := r.Context()
+	log := appctx.GetLogger(ctx)
+
 	client, err := s.getClient()
 	if err != nil {
 		log.Error().Err(err).Msg("error getting grpc client")
 		w.WriteHeader(http.StatusInternalServerError)
 		return
 	}
 
-	sReq := &provider.StatRequest{
-		Ref: &provider.Reference{
-			Spec: &provider.Reference_Path{Path: fn},
-		},
+	ref := &provider.Reference{
+		Spec: &provider.Reference_Path{Path: fn},
 	}
+	sReq := &provider.StatRequest{Ref: ref}
 	sRes, err := client.Stat(ctx, sReq)
 	if err != nil {
 		log.Error().Err(err).Msg("error sending grpc stat request")
@@ -207,16 +224,6 @@ func (s *svc) handlePut(w http.ResponseWriter, r *http.Request, ns string) {
 		}
 	}
 
-	length, err := strconv.ParseInt(r.Header.Get("Content-Length"), 10, 64)
-	if err != nil {
-		// Fallback to Upload-Length
-		length, err = strconv.ParseInt(r.Header.Get("Upload-Length"), 10, 64)
-		if err != nil {
-			w.WriteHeader(http.StatusBadRequest)
-			return
-		}
-	}
-
 	opaqueMap := map[string]*typespb.OpaqueEntry{
 		"Upload-Length": {
 			Decoder: "plain",
@@ -236,9 +243,7 @@ func (s *svc) handlePut(w http.ResponseWriter, r *http.Request, ns string) {
 	}
 
 	uReq := &provider.InitiateFileUploadRequest{
-		Ref: &provider.Reference{
-			Spec: &provider.Reference_Path{Path: fn},
-		},
+		Ref: ref,
 		Opaque: &typespb.Opaque{
 			Map: opaqueMap,
 		},
@@ -306,7 +311,7 @@ func (s *svc) handlePut(w http.ResponseWriter, r *http.Request, ns string) {
 		//"checksum": fmt.Sprintf("%s %s", storageprovider.GRPC2PKGXS(xsType).String(), xs),
 	}
 
-	upload := tus.NewUpload(r.Body, length, metadata, "")
+	upload := tus.NewUpload(content, length, metadata, "")
 
 	// create the uploader.
 	c.Store.Set(upload.Fingerprint, dataServerURL)

diff --git a/internal/http/services/owncloud/ocdav/putchunked.go b/internal/http/services/owncloud/ocdav/putchunked.go
@@ -29,8 +29,6 @@ import (
 	"strconv"
 	"strings"
 
-	rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
-	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
 	"github.com/cs3org/reva/pkg/appctx"
 )
 
@@ -215,8 +213,7 @@ func (s *svc) saveChunk(ctx context.Context, path string, r io.ReadCloser) (bool
 		return false, "", err
 	}
 
-	tempFileName := assembledFileName
-	return true, tempFileName, nil
+	return true, assembledFileName, nil
 }
 func (s *svc) handlePutChunked(w http.ResponseWriter, r *http.Request) {
 	ctx := r.Context()
@@ -250,57 +247,17 @@ func (s *svc) handlePutChunked(w http.ResponseWriter, r *http.Request) {
 	}
 	defer fd.Close()
 
-	chunkInfo, _ := getChunkBLOBInfo(fn)
-
-	client, err := s.getClient()
-	if err != nil {
-		log.Error().Err(err).Msg("error getting grpc client")
-		w.WriteHeader(http.StatusInternalServerError)
-		return
-	}
-
-	ref := &provider.Reference{
-		Spec: &provider.Reference_Path{Path: chunkInfo.path},
-	}
-	req := &provider.StatRequest{Ref: ref}
-	res, err := client.Stat(ctx, req)
+	md, err := fd.Stat()
 	if err != nil {
-		log.Error().Err(err).Msg("error sending grpc stat request")
+		log.Error().Err(err).Msg("error statting chunk")
 		w.WriteHeader(http.StatusInternalServerError)
 		return
 	}
 
-	if res.Status.Code != rpc.Code_CODE_OK && res.Status.Code != rpc.Code_CODE_NOT_FOUND {
-		switch res.Status.Code {
-		case rpc.Code_CODE_PERMISSION_DENIED:
-			log.Debug().Str("path", fn).Interface("status", res.Status).Msg("permission denied")
-			w.WriteHeader(http.StatusForbidden)
-		default:
-			log.Error().Str("path", fn).Interface("status", res.Status).Msg("grpc stat request failed")
-			w.WriteHeader(http.StatusInternalServerError)
-		}
-		return
-	}
-
-	info := res.Info
-	if info != nil && info.Type != provider.ResourceType_RESOURCE_TYPE_FILE {
-		log.Warn().Msg("resource is not a file")
-		w.WriteHeader(http.StatusConflict)
-		return
-	}
+	chunkInfo, _ := getChunkBLOBInfo(fn)
+	fn = chunkInfo.path
 
-	if info != nil {
-		clientETag := r.Header.Get("If-Match")
-		serverETag := info.Etag
-		if clientETag != "" {
-			if clientETag != serverETag {
-				log.Warn().Str("client-etag", clientETag).Str("server-etag", serverETag).Msg("etags mismatch")
-				w.WriteHeader(http.StatusPreconditionFailed)
-				return
-			}
-		}
-	}
-	w.WriteHeader(http.StatusCreated)
+	s.handlePutHelper(w, r, fd, fn, md.Size())
 
 	// TODO(labkode): implement old chunking
 

diff --git a/pkg/auth/manager/json/json.go b/pkg/auth/manager/json/json.go
@@ -24,6 +24,7 @@ import (
 	"io/ioutil"
 
 	user "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1"
+	typespb "github.com/cs3org/go-cs3apis/cs3/types/v1beta1"
 	"github.com/cs3org/reva/pkg/auth"
 	"github.com/cs3org/reva/pkg/auth/manager/registry"
 	"github.com/cs3org/reva/pkg/errtypes"
@@ -37,13 +38,14 @@ func init() {
 
 // Credentials holds a pair of secret and userid
 type Credentials struct {
-	ID           *user.UserId `mapstructure:"id" json:"id"`
-	Username     string       `mapstructure:"username" json:"username"`
-	Mail         string       `mapstructure:"mail" json:"mail"`
-	MailVerified bool         `mapstructure:"mail_verified" json:"mail_verified"`
-	DisplayName  string       `mapstructure:"display_name" json:"display_name"`
-	Secret       string       `mapstructure:"secret" json:"secret"`
-	Groups       []string     `mapstructure:"groups" json:"groups"`
+	ID           *user.UserId    `mapstructure:"id" json:"id"`
+	Username     string          `mapstructure:"username" json:"username"`
+	Mail         string          `mapstructure:"mail" json:"mail"`
+	MailVerified bool            `mapstructure:"mail_verified" json:"mail_verified"`
+	DisplayName  string          `mapstructure:"display_name" json:"display_name"`
+	Secret       string          `mapstructure:"secret" json:"secret"`
+	Groups       []string        `mapstructure:"groups" json:"groups"`
+	Opaque       *typespb.Opaque `mapstructure:"opaque" json:"opaque"`
 }
 
 type manager struct {
@@ -109,6 +111,7 @@ func (m *manager) Authenticate(ctx context.Context, username string, secret stri
 				MailVerified: c.MailVerified,
 				DisplayName:  c.DisplayName,
 				Groups:       c.Groups,
+				Opaque:       c.Opaque,
 				// TODO add arbitrary keys as opaque data
 			}, nil
 		}