diff --git a/changelog/unreleased/fix-trash.md b/changelog/unreleased/fix-trash.md new file mode 100644 index 0000000000..088dd71c53 --- /dev/null +++ b/changelog/unreleased/fix-trash.md @@ -0,0 +1,5 @@ +Bugfix: restore and delete trash items via ocs + +The OCS api was not passing the correct key and references to the CS3 API. Furthermore, the owncloud storage driver was constructing the wrong target path when restoring. + +https://github.com/cs3org/reva/pull/1103 \ No newline at end of file diff --git a/internal/grpc/services/gateway/storageprovider.go b/internal/grpc/services/gateway/storageprovider.go index d83b75ca05..ced307767b 100644 --- a/internal/grpc/services/gateway/storageprovider.go +++ b/internal/grpc/services/gateway/storageprovider.go @@ -1406,7 +1406,7 @@ func (s *svc) ListRecycleStream(_ *gateway.ListRecycleStreamRequest, _ gateway.G return errors.New("Unimplemented") } -// TODO use the ListRecycleRequest.Ref to only list the trish of a specific storage +// TODO use the ListRecycleRequest.Ref to only list the trash of a specific storage func (s *svc) ListRecycle(ctx context.Context, req *gateway.ListRecycleRequest) (*provider.ListRecycleResponse, error) { c, err := s.find(ctx, req.GetRef()) if err != nil { diff --git a/internal/http/services/owncloud/ocdav/trashbin.go b/internal/http/services/owncloud/ocdav/trashbin.go index c10e1d2a86..7f34a89b09 100644 --- a/internal/http/services/owncloud/ocdav/trashbin.go +++ b/internal/http/services/owncloud/ocdav/trashbin.go @@ -98,7 +98,7 @@ func (h *TrashbinHandler) Handler(s *svc) http.Handler { if key != "" && r.Method == "MOVE" { dstHeader := r.Header.Get("Destination") - log.Info().Str("key", key).Str("dst", dstHeader).Msg("restore") + log.Debug().Str("key", key).Str("dst", dstHeader).Msg("restore") if dstHeader == "" { w.WriteHeader(http.StatusBadRequest) @@ -119,7 +119,7 @@ func (h *TrashbinHandler) Handler(s *svc) http.Handler { ctx = context.WithValue(ctx, ctxKeyBaseURI, baseURI) r = r.WithContext(ctx) - log.Info().Str("url_path", urlPath).Str("base_uri", baseURI).Msg("move urls") + log.Debug().Str("url_path", urlPath).Str("base_uri", baseURI).Msg("move urls") // TODO make request.php optional in destination header i := strings.Index(urlPath, baseURI) if i == -1 { @@ -214,13 +214,13 @@ func (h *TrashbinHandler) formatTrashPropfind(ctx context.Context, s *svc, u *us responses = append(responses, &responseXML{ Href: (&url.URL{Path: ctx.Value(ctxKeyBaseURI).(string) + "/"}).EscapedPath(), // url encode response.Href TODO (jfd) really? /should be ok ... we may actually only need to escape the username Propstat: []propstatXML{ - propstatXML{ + { Status: "HTTP/1.1 200 OK", Prop: []*propertyXML{ s.newProp("d:resourcetype", ""), }, }, - propstatXML{ + { Status: "HTTP/1.1 404 Not Found", Prop: []*propertyXML{ s.newProp("oc:trashbin-original-filename", ""), @@ -231,28 +231,7 @@ func (h *TrashbinHandler) formatTrashPropfind(ctx context.Context, s *svc, u *us }, }, }) - /* - for i := range items { - vi := &provider.ResourceInfo{ - // TODO(jfd) we cannot access version content, this will be a problem when trying to fetch version thumbnails - //Opaque - Type: provider.ResourceType_RESOURCE_TYPE_FILE, - Id: &provider.ResourceId{ - StorageId: "trashbin", // this is a virtual storage - OpaqueId: path.Join("trash-bin", u.Username, items[i].GetKey()), - }, - //Checksum - //Etag: v.ETag, - //MimeType - Mtime: items[i].DeletionTime, - Path: items[i].Key, - //PermissionSet - Size: items[i].Size, - Owner: u.Id, - } - infos = append(infos, vi) - } - */ + for i := range items { res, err := h.itemToPropResponse(ctx, s, pf, items[i]) if err != nil { @@ -271,6 +250,9 @@ func (h *TrashbinHandler) formatTrashPropfind(ctx context.Context, s *svc, u *us return msg, nil } +// itemToPropResponse needs to create a listing that contains a key and destination +// the key is the name of an entry in the trash listing +// for now we need to limit trash to the users home, so we can expect all trash keys to have the home storage as the opaque id func (h *TrashbinHandler) itemToPropResponse(ctx context.Context, s *svc, pf *propfindXML, item *provider.RecycleItem) (*responseXML, error) { baseURI := ctx.Value(ctxKeyBaseURI).(string) @@ -377,6 +359,7 @@ func (h *TrashbinHandler) itemToPropResponse(ctx context.Context, s *svc, pf *pr return &response, nil } +// restore has a destination and a key func (h *TrashbinHandler) restore(w http.ResponseWriter, r *http.Request, s *svc, u *userpb.User, dst string, key string) { ctx := r.Context() log := appctx.GetLogger(ctx) @@ -388,18 +371,28 @@ func (h *TrashbinHandler) restore(w http.ResponseWriter, r *http.Request, s *svc return } - rid := unwrap(key) + getHomeRes, err := client.GetHome(ctx, &provider.GetHomeRequest{}) + if err != nil { + log.Error().Err(err).Msg("error calling GetHomeProvider") + w.WriteHeader(http.StatusInternalServerError) + return + } + if getHomeRes.Status.Code != rpc.Code_CODE_OK { + log.Error().Int32("code", int32(getHomeRes.Status.Code)).Str("trace", getHomeRes.Status.Trace).Msg(getHomeRes.Status.Message) + w.WriteHeader(http.StatusInternalServerError) + } 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 // use the key which is prefixed with the StoragePath to lookup the correct storage ... + // TODO currently limited to the home storage Ref: &provider.Reference{ - Spec: &provider.Reference_Id{ - Id: rid, + Spec: &provider.Reference_Path{ + Path: getHomeRes.Path, }, }, - Key: rid.GetOpaqueId(), + Key: key, RestorePath: dst, } @@ -420,6 +413,7 @@ func (h *TrashbinHandler) restore(w http.ResponseWriter, r *http.Request, s *svc w.WriteHeader(http.StatusNoContent) } +// delete has only a key func (h *TrashbinHandler) delete(w http.ResponseWriter, r *http.Request, s *svc, u *userpb.User, key string) { ctx := r.Context() @@ -432,12 +426,43 @@ func (h *TrashbinHandler) delete(w http.ResponseWriter, r *http.Request, s *svc, return } - rid := unwrap(key) + getHomeRes, err := client.GetHome(ctx, &provider.GetHomeRequest{}) + if err != nil { + log.Error().Err(err).Msg("error calling GetHomeProvider") + w.WriteHeader(http.StatusInternalServerError) + return + } + if getHomeRes.Status.Code != rpc.Code_CODE_OK { + log.Error().Int32("code", int32(getHomeRes.Status.Code)).Str("trace", getHomeRes.Status.Trace).Msg(getHomeRes.Status.Message) + w.WriteHeader(http.StatusInternalServerError) + } + sRes, err := client.Stat(ctx, &provider.StatRequest{ + Ref: &provider.Reference{ + Spec: &provider.Reference_Path{ + Path: getHomeRes.Path, + }, + }, + }) + if err != nil { + log.Error().Err(err).Msg("error calling Stat") + w.WriteHeader(http.StatusInternalServerError) + return + } + if sRes.Status.Code != rpc.Code_CODE_OK { + log.Error().Int32("code", int32(sRes.Status.Code)).Str("trace", sRes.Status.Trace).Msg(sRes.Status.Message) + w.WriteHeader(http.StatusInternalServerError) + } + + // set key as opaque id, the storageprovider will use it as the key for the + // storage drives PurgeRecycleItem key call req := &gateway.PurgeRecycleRequest{ Ref: &provider.Reference{ Spec: &provider.Reference_Id{ - Id: rid, + Id: &provider.ResourceId{ + OpaqueId: key, + StorageId: sRes.Info.Id.StorageId, + }, }, }, } diff --git a/pkg/storage/fs/owncloud/owncloud.go b/pkg/storage/fs/owncloud/owncloud.go index b229a3ff72..618b59f845 100644 --- a/pkg/storage/fs/owncloud/owncloud.go +++ b/pkg/storage/fs/owncloud/owncloud.go @@ -1931,10 +1931,6 @@ func (fs *ocfs) ListRecycle(ctx context.Context) ([]*provider.RecycleItem, error func (fs *ocfs) RestoreRecycleItem(ctx context.Context, key string) error { log := appctx.GetLogger(ctx) - u, ok := user.ContextGetUser(ctx) - if !ok { - return errors.Wrap(errtypes.UserRequired("userrequired"), "error getting user from ctx") - } rp, err := fs.getRecyclePath(ctx) if err != nil { return errors.Wrap(err, "ocfs: error resolving recycle path") @@ -1943,27 +1939,26 @@ func (fs *ocfs) RestoreRecycleItem(ctx context.Context, key string) error { suffix := path.Ext(src) if len(suffix) == 0 || !strings.HasPrefix(suffix, ".d") { - log.Error().Str("path", src).Msg("invalid trash item suffix") + log.Error().Str("key", key).Str("path", src).Msg("invalid trash item suffix") return nil } origin := "/" if v, err := xattr.Get(src, trashOriginPrefix); err != nil { - log.Error().Err(err).Str("path", src).Msg("could not read origin") + log.Error().Err(err).Str("key", key).Str("path", src).Msg("could not read origin") } else { origin = path.Clean(string(v)) } - layout := templates.WithUser(u, fs.c.UserLayout) - tgt := path.Join(fs.wrap(ctx, path.Join("/", layout, origin)), strings.TrimSuffix(path.Base(src), suffix)) + tgt := fs.wrap(ctx, path.Join("/", origin, strings.TrimSuffix(path.Base(src), suffix))) // move back to original location if err := os.Rename(src, tgt); err != nil { - log.Error().Err(err).Str("path", src).Msg("could not restore item") + log.Error().Err(err).Str("key", key).Str("origin", origin).Str("src", src).Str("tgt", tgt).Msg("could not restore item") return errors.Wrap(err, "ocfs: could not restore item") } // unset trash origin location in metadata if err := xattr.Remove(tgt, trashOriginPrefix); err != nil { // just a warning, will be overwritten next time it is deleted - log.Warn().Err(err).Str("path", tgt).Msg("could not unset origin") + log.Warn().Err(err).Str("key", key).Str("tgt", tgt).Msg("could not unset origin") } // TODO(jfd) restore versions