fix restore behavior of the trashbin API (#1795)

changelog/unreleased/trashbin-restore.md

@@ -0,0 +1,8 @@
+Enhancement: Increase trashbin restore API compatibility
+
+* The precondition were not checked before doing a trashbin restore in the ownCloud dav API. Without the checks the API would behave differently compared to the oC10 API.
+* The restore response was missing HTTP headers like `ETag`
+* Update the name when restoring the file from trashbin to a new target name
+
+https://github.com/cs3org/reva/pull/1795
+

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

@@ -31,25 +31,29 @@ type code int
 
 const (
 
-	// SabredavMethodBadRequest maps to HTTP 400
-	SabredavMethodBadRequest code = iota
+	// SabredavBadRequest maps to HTTP 400
+	SabredavBadRequest code = iota
 	// SabredavMethodNotAllowed maps to HTTP 405
 	SabredavMethodNotAllowed
-	// SabredavMethodNotAuthenticated maps to HTTP 401
-	SabredavMethodNotAuthenticated
+	// SabredavNotAuthenticated maps to HTTP 401
+	SabredavNotAuthenticated
+	// SabredavPreconditionFailed maps to HTTP 412
+	SabredavPreconditionFailed
 )
 
 var (
 	codesEnum = []string{
 		"Sabre\\DAV\\Exception\\BadRequest",
 		"Sabre\\DAV\\Exception\\MethodNotAllowed",
 		"Sabre\\DAV\\Exception\\NotAuthenticated",
+		"Sabre\\DAV\\Exception\\PreconditionFailed",
 	}
 )
 
 type exception struct {
 	code    code
 	message string
+	header  string
 }
 
 // Marshal just calls the xml marshaller for a given exception.
@@ -59,6 +63,7 @@ func Marshal(e exception) ([]byte, error) {
 		Xmlnss:    "http://sabredav.org/ns",
 		Exception: codesEnum[e.code],
 		Message:   e.message,
+		Header:    e.header,
 	})
 }
 
@@ -70,6 +75,7 @@ type errorXML struct {
 	Exception string   `xml:"s:exception"`
 	Message   string   `xml:"s:message"`
 	InnerXML  []byte   `xml:",innerxml"`
+	Header    string   `xml:"s:header"`
 }
 
 var errInvalidPropfind = errors.New("webdav: invalid propfind")

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

@@ -282,7 +282,7 @@ func (s *svc) handlePutHelper(w http.ResponseWriter, r *http.Request, content io
 			if httpRes.StatusCode == errtypes.StatusChecksumMismatch {
 				w.WriteHeader(http.StatusBadRequest)
 				b, err := Marshal(exception{
-					code:    SabredavMethodBadRequest,
+					code:    SabredavBadRequest,
 					message: "The computed checksum does not match the one received from the client.",
 				})
 				if err != nil {

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

@@ -80,7 +80,7 @@ func (h *TrashbinHandler) Handler(s *svc) http.Handler {
 			log.Debug().Str("username", username).Interface("user", u).Msg("trying to read another users trash")
 			// listing other users trash is forbidden, no auth will change that
 			b, err := Marshal(exception{
-				code: SabredavMethodNotAuthenticated,
+				code: SabredavNotAuthenticated,
 			})
 			if err != nil {
 				log.Error().Msgf("error marshaling xml response: %s", b)
@@ -370,6 +370,18 @@ func (h *TrashbinHandler) restore(w http.ResponseWriter, r *http.Request, s *svc
 
 	sublog := appctx.GetLogger(ctx).With().Logger()
 
+	overwrite := r.Header.Get("Overwrite")
+
+	overwrite = strings.ToUpper(overwrite)
+	if overwrite == "" {
+		overwrite = "T"
+	}
+
+	if overwrite != "T" && overwrite != "F" {
+		w.WriteHeader(http.StatusBadRequest)
+		return
+	}
+
 	client, err := s.getClient()
 	if err != nil {
 		sublog.Error().Err(err).Msg("error getting grpc client")
@@ -388,6 +400,64 @@ func (h *TrashbinHandler) restore(w http.ResponseWriter, r *http.Request, s *svc
 		return
 	}
 
+	dstRef := &provider.Reference{
+		Path: filepath.Join(getHomeRes.Path, dst),
+	}
+
+	dstStatReq := &provider.StatRequest{
+		Ref: dstRef,
+	}
+
+	dstStatRes, err := client.Stat(ctx, dstStatReq)
+	if err != nil {
+		sublog.Error().Err(err).Msg("error sending grpc stat request")
+		w.WriteHeader(http.StatusInternalServerError)
+		return
+	}
+
+	if dstStatRes.Status.Code != rpc.Code_CODE_OK && dstStatRes.Status.Code != rpc.Code_CODE_NOT_FOUND {
+		HandleErrorStatus(&sublog, w, dstStatRes.Status)
+		return
+	}
+
+	successCode := http.StatusCreated // 201 if new resource was created, see https://tools.ietf.org/html/rfc4918#section-9.9.4
+	if dstStatRes.Status.Code == rpc.Code_CODE_OK {
+		successCode = http.StatusNoContent // 204 if target already existed, see https://tools.ietf.org/html/rfc4918#section-9.9.4
+
+		if overwrite != "T" {
+			sublog.Warn().Str("overwrite", overwrite).Msg("dst already exists")
+			w.WriteHeader(http.StatusPreconditionFailed) // 412, see https://tools.ietf.org/html/rfc4918#section-9.9.4
+			b, err := Marshal(exception{
+				code:    SabredavPreconditionFailed,
+				message: "The destination node already exists, and the overwrite header is set to false",
+				header:  "Overwrite",
+			})
+			if err != nil {
+				sublog.Error().Msgf("error marshaling xml response: %s", b)
+				w.WriteHeader(http.StatusInternalServerError)
+				return
+			}
+			_, err = w.Write(b)
+			if err != nil {
+				sublog.Err(err).Msg("error writing response")
+			}
+			return
+		}
+		// delete existing tree
+		delReq := &provider.DeleteRequest{Ref: dstRef}
+		delRes, err := client.Delete(ctx, delReq)
+		if err != nil {
+			sublog.Error().Err(err).Msg("error sending grpc delete request")
+			w.WriteHeader(http.StatusInternalServerError)
+			return
+		}
+
+		if delRes.Status.Code != rpc.Code_CODE_OK && delRes.Status.Code != rpc.Code_CODE_NOT_FOUND {
+			HandleErrorStatus(&sublog, w, delRes.Status)
+			return
+		}
+	}
+
 	req := &provider.RestoreRecycleItemRequest{
 		// use the target path to find the storage provider
 		// this means we can only undelete on the same storage, not to a different folder
@@ -411,7 +481,25 @@ func (h *TrashbinHandler) restore(w http.ResponseWriter, r *http.Request, s *svc
 		HandleErrorStatus(&sublog, w, res.Status)
 		return
 	}
-	w.WriteHeader(http.StatusCreated)
+
+	dstStatRes, err = client.Stat(ctx, dstStatReq)
+	if err != nil {
+		sublog.Error().Err(err).Msg("error sending grpc stat request")
+		w.WriteHeader(http.StatusInternalServerError)
+		return
+	}
+	if dstStatRes.Status.Code != rpc.Code_CODE_OK {
+		HandleErrorStatus(&sublog, w, dstStatRes.Status)
+		return
+	}
+
+	info := dstStatRes.Info
+	w.Header().Set("Content-Type", info.MimeType)
+	w.Header().Set("ETag", info.Etag)
+	w.Header().Set("OC-FileId", wrapResourceID(info.Id))
+	w.Header().Set("OC-ETag", info.Etag)
+
+	w.WriteHeader(successCode)
 }
 
 // delete has only a key

pkg/storage/utils/decomposedfs/tree/tree.go

@@ -382,6 +382,10 @@ func (t *Tree) RestoreRecycleItemFunc(ctx context.Context, key, restorePath stri
 		}
 
 		n.Exists = true
+		// update name attribute
+		if err := xattr.Set(nodePath, xattrs.NameAttr, []byte(n.Name)); err != nil {
+			return errors.Wrap(err, "Decomposedfs: could not set name attribute")
+		}
 
 		// delete item link in trash
 		if err = os.Remove(trashItem); err != nil {

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

@@ -19,27 +19,11 @@ Basic file management like up and download, move, copy, properties, quota, trash
 -   [apiTrashbin/trashbinFilesFolders.feature:278](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature#L278)
 -   [apiTrashbin/trashbinFilesFolders.feature:279](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature#L279)
 
-#### [cannot restore to a different file-name](https://github.com/owncloud/ocis/issues/1122)
--   [apiTrashbin/trashbinRestore.feature:108](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L108)
--   [apiTrashbin/trashbinRestore.feature:109](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L109)
--   [apiTrashbin/trashbinRestore.feature:110](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L110)
--   [apiTrashbin/trashbinRestore.feature:111](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L111)
-
 #### [trash-bin restore move does not send back Etag and other headers](https://github.com/owncloud/ocis/issues/1121)
--   [apiTrashbin/trashbinRestore.feature:34](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L34)
--   [apiTrashbin/trashbinRestore.feature:35](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L35)
 -   [apiTrashbin/trashbinRestore.feature:130](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L130)
 -   [apiTrashbin/trashbinRestore.feature:131](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L131)
 
-#### [cannot restore to a different file-name](https://github.com/owncloud/ocis/issues/1122)
-#### [trash-bin restore move does not send back Etag and other headers](https://github.com/owncloud/ocis/issues/1121)
--   [apiTrashbin/trashbinRestore.feature:88](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L88)
--   [apiTrashbin/trashbinRestore.feature:89](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L89)
--   [apiTrashbin/trashbinRestore.feature:90](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L90)
--   [apiTrashbin/trashbinRestore.feature:91](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L91)
--   [apiTrashbin/trashbinRestore.feature:92](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L92)
--   [apiTrashbin/trashbinRestore.feature:93](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L93)
-
+#### [href in trashbin PROPFIND response is wrong](https://github.com/owncloud/ocis/issues/1120)
 #### [cannot restore to a different file-name](https://github.com/owncloud/ocis/issues/1122)
 -   [apiTrashbin/trashbinRestore.feature:309](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L309)
 -   [apiTrashbin/trashbinRestore.feature:310](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L310)

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

@@ -33,22 +33,7 @@ The following scenarios fail on OWNCLOUD storage but not on OCIS storage:
 -   [apiTrashbin/trashbinRestore.feature:110](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L110)
 -   [apiTrashbin/trashbinRestore.feature:111](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L111)
 
-
-#### [trash-bin restore move does not send back Etag and other headers](https://github.com/owncloud/ocis/issues/1121)
--   [apiTrashbin/trashbinRestore.feature:34](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L34)
--   [apiTrashbin/trashbinRestore.feature:35](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L35)
--   [apiTrashbin/trashbinRestore.feature:130](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L130)
--   [apiTrashbin/trashbinRestore.feature:131](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L131)
-
-#### [cannot restore to a different file-name](https://github.com/owncloud/ocis/issues/1122)
-#### [trash-bin restore move does not send back Etag and other headers](https://github.com/owncloud/ocis/issues/1121)
--   [apiTrashbin/trashbinRestore.feature:88](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L88)
--   [apiTrashbin/trashbinRestore.feature:89](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L89)
--   [apiTrashbin/trashbinRestore.feature:90](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L90)
--   [apiTrashbin/trashbinRestore.feature:91](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L91)
--   [apiTrashbin/trashbinRestore.feature:92](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L92)
--   [apiTrashbin/trashbinRestore.feature:93](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L93)
-
+#### [href in trashbin PROPFIND response is wrong](https://github.com/owncloud/ocis/issues/1120)
 #### [cannot restore to a different file-name](https://github.com/owncloud/ocis/issues/1122)
 -   [apiTrashbin/trashbinRestore.feature:309](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L309)
 -   [apiTrashbin/trashbinRestore.feature:310](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L310)

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

@@ -19,27 +19,11 @@ Basic file management like up and download, move, copy, properties, quota, trash
 -   [apiTrashbin/trashbinFilesFolders.feature:278](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature#L278)
 -   [apiTrashbin/trashbinFilesFolders.feature:279](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature#L279)
 
-#### [cannot restore to a different file-name](https://github.com/owncloud/ocis/issues/1122)
--   [apiTrashbin/trashbinRestore.feature:108](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L108)
--   [apiTrashbin/trashbinRestore.feature:109](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L109)
--   [apiTrashbin/trashbinRestore.feature:110](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L110)
--   [apiTrashbin/trashbinRestore.feature:111](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L111)
-
 #### [trash-bin restore move does not send back Etag and other headers](https://github.com/owncloud/ocis/issues/1121)
--   [apiTrashbin/trashbinRestore.feature:34](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L34)
--   [apiTrashbin/trashbinRestore.feature:35](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L35)
 -   [apiTrashbin/trashbinRestore.feature:130](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L130)
 -   [apiTrashbin/trashbinRestore.feature:131](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L131)
 
-#### [cannot restore to a different file-name](https://github.com/owncloud/ocis/issues/1122)
-#### [trash-bin restore move does not send back Etag and other headers](https://github.com/owncloud/ocis/issues/1121)
--   [apiTrashbin/trashbinRestore.feature:88](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L88)
--   [apiTrashbin/trashbinRestore.feature:89](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L89)
--   [apiTrashbin/trashbinRestore.feature:90](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L90)
--   [apiTrashbin/trashbinRestore.feature:91](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L91)
--   [apiTrashbin/trashbinRestore.feature:92](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L92)
--   [apiTrashbin/trashbinRestore.feature:93](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L93)
-
+#### [href in trashbin PROPFIND response is wrong](https://github.com/owncloud/ocis/issues/1120)
 #### [cannot restore to a different file-name](https://github.com/owncloud/ocis/issues/1122)
 -   [apiTrashbin/trashbinRestore.feature:309](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L309)
 -   [apiTrashbin/trashbinRestore.feature:310](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L310)
@@ -2020,4 +2004,4 @@ _ocs: api compatibility, return correct status code_
 -   [apiWebdavProperties1/copyFile.feature:437](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties1/copyFile.feature#L437)
 -   [apiWebdavProperties1/copyFile.feature:438](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties1/copyFile.feature#L438)
 -   [apiWebdavProperties1/copyFile.feature:464](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties1/copyFile.feature#L464)
--   [apiWebdavProperties1/copyFile.feature:465](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties1/copyFile.feature#L465)
+-   [apiWebdavProperties1/copyFile.feature:465](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties1/copyFile.feature#L465)