Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix restore behavior of the trashbin API #1795

Merged
merged 5 commits into from
Jun 23, 2021
Merged

fix restore behavior of the trashbin API #1795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2066789
check preconditions for ocdav trashbin restore requests
Jun 15, 2021
b4da58c
add headers to trashbin restore response
Jun 15, 2021
faf2298
update expected failures
Jun 16, 2021
e8d4baa
update name when restore to new name
Jun 21, 2021
ca8e669
update expected failures
Jun 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions changelog/unreleased/trashbin-restore.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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

14 changes: 10 additions & 4 deletions internal/http/services/owncloud/ocdav/error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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,
})
}

Expand All @@ -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")
Expand Down
2 changes: 1 addition & 1 deletion internal/http/services/owncloud/ocdav/put.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
92 changes: 90 additions & 2 deletions internal/http/services/owncloud/ocdav/trashbin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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")
Expand All @@ -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
Comment on lines +455 to +457
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this will handle the case where the restore wants to overwrite an existing resource, but the user does not have delete access to the resource.

And it might be more tricky if the user has change access to the resource, but not delete access. client.Delete will probably refuse, but actually the end-effect that we want is to just overwrite. IMO this is also a hassle in oC10 - the back-end code has a similar flow where it deletes the resource first, then adds the restored resource. That kind of edge-case is something for a future PR...

}
}

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
Expand All @@ -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
Expand Down
4 changes: 4 additions & 0 deletions pkg/storage/utils/decomposedfs/tree/tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,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 {
Expand Down
20 changes: 2 additions & 18 deletions tests/acceptance/expected-failures-on-OCIS-storage.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -2015,4 +1999,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)
17 changes: 1 addition & 16 deletions tests/acceptance/expected-failures-on-OWNCLOUD-storage.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
20 changes: 2 additions & 18 deletions tests/acceptance/expected-failures-on-S3NG-storage.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)