From 143fd7e4c6ffbe35be9922e1ea24efb972cf0266 Mon Sep 17 00:00:00 2001 From: Ishank Arora Date: Thu, 20 Aug 2020 16:58:23 +0200 Subject: [PATCH 1/5] Handle redirection prefixes when extracting destination from URL --- changelog/unreleased/eosfs-recycle-purge.md | 1 - changelog/unreleased/webdav-move-prefix.md | 8 ++++++++ internal/http/services/owncloud/ocdav/move.go | 8 +++++++- pkg/storage/utils/localfs/upload.go | 14 ++++++++++++-- pkg/user/manager/rest/rest.go | 4 ++++ 5 files changed, 31 insertions(+), 4 deletions(-) create mode 100644 changelog/unreleased/webdav-move-prefix.md diff --git a/changelog/unreleased/eosfs-recycle-purge.md b/changelog/unreleased/eosfs-recycle-purge.md index ce674eafb7..5f0e080c12 100644 --- a/changelog/unreleased/eosfs-recycle-purge.md +++ b/changelog/unreleased/eosfs-recycle-purge.md @@ -1,6 +1,5 @@ Bugfix: do not restore recycle entry on purge - This PR fixes a bug in the EOSFS driver that was restoring a deleted entry when asking for its permanent purge. EOS does not have the functionality to purge individual files, but the whole recycle of the user. diff --git a/changelog/unreleased/webdav-move-prefix.md b/changelog/unreleased/webdav-move-prefix.md new file mode 100644 index 0000000000..bfb41f8a08 --- /dev/null +++ b/changelog/unreleased/webdav-move-prefix.md @@ -0,0 +1,8 @@ +Bugfix: Handle redirection prefixes when extracting destination from URL + +The move function handler in ocdav extracts the destination path from the URL by +removing the base URL prefix from the URL path. This would fail in case there is +a redirection prefix. This PR takes care of that and it also allows zero-size +uploads for localfs. + +https://github.com/cs3org/reva/pull/1111 diff --git a/internal/http/services/owncloud/ocdav/move.go b/internal/http/services/owncloud/ocdav/move.go index fd919e050b..4a2e38563d 100644 --- a/internal/http/services/owncloud/ocdav/move.go +++ b/internal/http/services/owncloud/ocdav/move.go @@ -103,8 +103,14 @@ func (s *svc) handleMove(w http.ResponseWriter, r *http.Request, ns string) { } // TODO check if path is on same storage, return 502 on problems, see https://tools.ietf.org/html/rfc4918#section-9.9.4 + // The base URI might contain redirection prefixes which need to be handled + urlSplit := strings.Split(urlPath, baseURI) + if len(urlSplit) != 2 { + w.WriteHeader(http.StatusBadRequest) + return + } // prefix to namespace - dst := path.Join(ns, urlPath[len(baseURI):]) + dst := path.Join(ns, urlSplit[1]) // check dst exists dstStatRef := &provider.Reference{ diff --git a/pkg/storage/utils/localfs/upload.go b/pkg/storage/utils/localfs/upload.go index d0e0a31bed..019d3e7c47 100644 --- a/pkg/storage/utils/localfs/upload.go +++ b/pkg/storage/utils/localfs/upload.go @@ -171,6 +171,16 @@ func (fs *localfs) NewUpload(ctx context.Context, info tusd.FileInfo) (upload tu fs: fs, } + if !info.SizeIsDeferred && info.Size == 0 { + log.Debug().Interface("info", info).Msg("localfs: finishing upload for empty file") + // no need to create info file and finish directly + err := u.FinishUpload(ctx) + if err != nil { + return nil, err + } + return u, nil + } + // writeInfo creates the file by itself if necessary err = u.writeInfo() if err != nil { @@ -307,10 +317,10 @@ func (upload *fileUpload) FinishUpload(ctx context.Context) error { err := os.Rename(upload.binPath, np) - // only delete the upload if it was successfully written to eos + // only delete the upload if it was successfully written to the fs if err := os.Remove(upload.infoPath); err != nil { log := appctx.GetLogger(ctx) - log.Err(err).Interface("info", upload.info).Msg("eos: could not delete upload info") + log.Err(err).Interface("info", upload.info).Msg("localfs: could not delete upload info") } // TODO: set mtime if specified in metadata diff --git a/pkg/user/manager/rest/rest.go b/pkg/user/manager/rest/rest.go index a482fa648b..8b476bf688 100644 --- a/pkg/user/manager/rest/rest.go +++ b/pkg/user/manager/rest/rest.go @@ -235,6 +235,10 @@ func (m *manager) getUserByParam(ctx context.Context, param, val string) (map[st if err != nil { return nil, err } + if len(responseData) == 0 { + return nil, errors.New("rest: no user found") + } + userData, ok := responseData[0].(map[string]interface{}) if !ok { return nil, errors.New("rest: error in type assertion") From 9a91146b3c8fabcd6bcf0234479542b41c4f9766 Mon Sep 17 00:00:00 2001 From: Ishank Arora Date: Thu, 20 Aug 2020 17:03:17 +0200 Subject: [PATCH 2/5] Remove extra log --- cmd/reva/ocm-share-create.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/cmd/reva/ocm-share-create.go b/cmd/reva/ocm-share-create.go index 186391feea..d12cb2a246 100644 --- a/cmd/reva/ocm-share-create.go +++ b/cmd/reva/ocm-share-create.go @@ -117,8 +117,6 @@ func ocmShareCreateCommand() *command { }, } - fmt.Println("res.Info.Path" + res.Info.Path) - opaqueObj := &types.Opaque{ Map: map[string]*types.OpaqueEntry{ "permissions": &types.OpaqueEntry{ From 70d26aa2878491e6ce37da723bce08d5b4bd0fc6 Mon Sep 17 00:00:00 2001 From: Ishank Arora Date: Thu, 20 Aug 2020 17:21:55 +0200 Subject: [PATCH 3/5] Handle copy and trashbin as well --- internal/http/services/owncloud/ocdav/copy.go | 8 +++++++- internal/http/services/owncloud/ocdav/trashbin.go | 9 ++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/copy.go b/internal/http/services/owncloud/ocdav/copy.go index db12c31c51..7900b3019c 100644 --- a/internal/http/services/owncloud/ocdav/copy.go +++ b/internal/http/services/owncloud/ocdav/copy.go @@ -123,8 +123,14 @@ func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) { } // TODO check if path is on same storage, return 502 on problems, see https://tools.ietf.org/html/rfc4918#section-9.9.4 + // The base URI might contain redirection prefixes which need to be handled + urlSplit := strings.Split(urlPath, baseURI) + if len(urlSplit) != 2 { + w.WriteHeader(http.StatusBadRequest) + return + } // prefix to namespace - dst := path.Join(ns, urlPath[len(baseURI):]) + dst := path.Join(ns, urlSplit[1]) // check dst exists ref = &provider.Reference{ diff --git a/internal/http/services/owncloud/ocdav/trashbin.go b/internal/http/services/owncloud/ocdav/trashbin.go index 7f34a89b09..73234d72f1 100644 --- a/internal/http/services/owncloud/ocdav/trashbin.go +++ b/internal/http/services/owncloud/ocdav/trashbin.go @@ -126,7 +126,14 @@ func (h *TrashbinHandler) Handler(s *svc) http.Handler { w.WriteHeader(http.StatusBadRequest) return } - dst := path.Clean(urlPath[len(baseURI):]) + + urlSplit := strings.Split(urlPath, baseURI) + if len(urlSplit) != 2 { + w.WriteHeader(http.StatusBadRequest) + return + } + + dst := path.Clean(urlSplit[1]) h.restore(w, r, s, u, dst, key) return From 128303f5e9ddb9d3f62fcda1b73bf6576665709a Mon Sep 17 00:00:00 2001 From: Ishank Arora Date: Thu, 20 Aug 2020 17:41:03 +0200 Subject: [PATCH 4/5] Ensure that token creator doesn't accept it --- pkg/ocm/invite/manager/json/json.go | 13 +++++++++---- pkg/ocm/invite/manager/memory/memory.go | 14 ++++++++++---- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/pkg/ocm/invite/manager/json/json.go b/pkg/ocm/invite/manager/json/json.go index 9dd5a525ef..c7bfda7462 100644 --- a/pkg/ocm/invite/manager/json/json.go +++ b/pkg/ocm/invite/manager/json/json.go @@ -250,14 +250,19 @@ func (m *manager) AcceptInvite(ctx context.Context, invite *invitepb.InviteToken return err } - // Add to the list of accepted users - userKey := inviteToken.GetUserId().GetOpaqueId() - for _, acceptedUser := range m.model.AcceptedUsers[userKey] { + currUser := inviteToken.GetUserId() + + // do not allow the user who created the token to accept it + if remoteUser.Id.Idp == currUser.Idp && remoteUser.Id.OpaqueId == currUser.OpaqueId { + return errors.New("json: token creator and recipient are the same") + } + + for _, acceptedUser := range m.model.AcceptedUsers[currUser.GetOpaqueId()] { if acceptedUser.Id.GetOpaqueId() == remoteUser.Id.OpaqueId && acceptedUser.Id.GetIdp() == remoteUser.Id.Idp { return errors.New("json: user already added to accepted users") } } - m.model.AcceptedUsers[userKey] = append(m.model.AcceptedUsers[userKey], remoteUser) + m.model.AcceptedUsers[currUser.GetOpaqueId()] = append(m.model.AcceptedUsers[currUser.GetOpaqueId()], remoteUser) if err := m.model.Save(); err != nil { err = errors.Wrap(err, "json: error saving model") return err diff --git a/pkg/ocm/invite/manager/memory/memory.go b/pkg/ocm/invite/manager/memory/memory.go index eab229e07f..3bdada8ba1 100644 --- a/pkg/ocm/invite/manager/memory/memory.go +++ b/pkg/ocm/invite/manager/memory/memory.go @@ -143,10 +143,16 @@ func (m *manager) AcceptInvite(ctx context.Context, invite *invitepb.InviteToken return err } - currUser := inviteToken.GetUserId().GetOpaqueId() + currUser := inviteToken.GetUserId() + + // do not allow the user who created the token to accept it + if remoteUser.Id.Idp == currUser.Idp && remoteUser.Id.OpaqueId == currUser.OpaqueId { + return errors.New("memory: token creator and recipient are the same") + } + usersList, ok := m.AcceptedUsers.Load(currUser) + acceptedUsers := usersList.([]*userpb.User) if ok { - acceptedUsers := usersList.([]*userpb.User) for _, acceptedUser := range acceptedUsers { if acceptedUser.Id.GetOpaqueId() == remoteUser.Id.OpaqueId && acceptedUser.Id.GetIdp() == remoteUser.Id.Idp { return errors.New("memory: user already added to accepted users") @@ -154,10 +160,10 @@ func (m *manager) AcceptInvite(ctx context.Context, invite *invitepb.InviteToken } acceptedUsers = append(acceptedUsers, remoteUser) - m.AcceptedUsers.Store(currUser, acceptedUsers) + m.AcceptedUsers.Store(currUser.GetOpaqueId(), acceptedUsers) } else { acceptedUsers := []*userpb.User{remoteUser} - m.AcceptedUsers.Store(currUser, acceptedUsers) + m.AcceptedUsers.Store(currUser.GetOpaqueId(), acceptedUsers) } return nil } From 92758d7129c0d088073a12b8bcbaf3f830395bff Mon Sep 17 00:00:00 2001 From: Ishank Arora Date: Fri, 21 Aug 2020 15:27:47 +0200 Subject: [PATCH 5/5] Refactor the extract destination method --- internal/http/services/owncloud/ocdav/copy.go | 38 +++---------------- internal/http/services/owncloud/ocdav/move.go | 36 +++--------------- .../http/services/owncloud/ocdav/ocdav.go | 21 ++++++++++ .../http/services/owncloud/ocdav/trashbin.go | 32 +++------------- 4 files changed, 37 insertions(+), 90 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/copy.go b/internal/http/services/owncloud/ocdav/copy.go index 7900b3019c..517afe93c0 100644 --- a/internal/http/services/owncloud/ocdav/copy.go +++ b/internal/http/services/owncloud/ocdav/copy.go @@ -23,7 +23,6 @@ import ( "fmt" "io" "net/http" - "net/url" "path" "strings" "time" @@ -54,13 +53,15 @@ func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) { depth = "infinity" } - log.Info().Str("source", src).Str("destination", dstHeader). - Str("overwrite", overwrite).Str("depth", depth).Msg("copy") - - if dstHeader == "" { + dst, err := extractDestination(dstHeader, r.Context().Value(ctxKeyBaseURI).(string)) + if err != nil { w.WriteHeader(http.StatusBadRequest) return } + dst = path.Join(ns, dst) + + log.Info().Str("source", src).Str("destination", dst). + Str("overwrite", overwrite).Str("depth", depth).Msg("copy") overwrite = strings.ToUpper(overwrite) if overwrite == "" { @@ -84,23 +85,6 @@ func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) { return } - // strip baseURL from destination - dstURL, err := url.ParseRequestURI(dstHeader) - if err != nil { - w.WriteHeader(http.StatusBadRequest) - return - } - - urlPath := dstURL.Path - baseURI := r.Context().Value(ctxKeyBaseURI).(string) - log.Info().Str("url-path", urlPath).Str("base-uri", baseURI).Msg("copy") - // TODO replace with HasPrefix: - i := strings.Index(urlPath, baseURI) - if i == -1 { - w.WriteHeader(http.StatusBadRequest) - return - } - // check src exists ref := &provider.Reference{ Spec: &provider.Reference_Path{Path: src}, @@ -122,16 +106,6 @@ func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) { return } - // TODO check if path is on same storage, return 502 on problems, see https://tools.ietf.org/html/rfc4918#section-9.9.4 - // The base URI might contain redirection prefixes which need to be handled - urlSplit := strings.Split(urlPath, baseURI) - if len(urlSplit) != 2 { - w.WriteHeader(http.StatusBadRequest) - return - } - // prefix to namespace - dst := path.Join(ns, urlSplit[1]) - // check dst exists ref = &provider.Reference{ Spec: &provider.Reference_Path{Path: dst}, diff --git a/internal/http/services/owncloud/ocdav/move.go b/internal/http/services/owncloud/ocdav/move.go index 4a2e38563d..9aa26979cb 100644 --- a/internal/http/services/owncloud/ocdav/move.go +++ b/internal/http/services/owncloud/ocdav/move.go @@ -20,7 +20,6 @@ package ocdav import ( "net/http" - "net/url" "path" "strings" @@ -39,12 +38,14 @@ func (s *svc) handleMove(w http.ResponseWriter, r *http.Request, ns string) { dstHeader := r.Header.Get("Destination") overwrite := r.Header.Get("Overwrite") - log.Info().Str("src", src).Str("dst", dstHeader).Str("overwrite", overwrite).Msg("move") - - if dstHeader == "" { + dst, err := extractDestination(dstHeader, r.Context().Value(ctxKeyBaseURI).(string)) + if err != nil { w.WriteHeader(http.StatusBadRequest) return } + dst = path.Join(ns, dst) + + log.Info().Str("src", src).Str("dst", dst).Str("overwrite", overwrite).Msg("move") overwrite = strings.ToUpper(overwrite) if overwrite == "" { @@ -63,23 +64,6 @@ func (s *svc) handleMove(w http.ResponseWriter, r *http.Request, ns string) { return } - // strip baseURL from destination - dstURL, err := url.ParseRequestURI(dstHeader) - if err != nil { - w.WriteHeader(http.StatusBadRequest) - return - } - - urlPath := dstURL.Path - baseURI := r.Context().Value(ctxKeyBaseURI).(string) - log.Info().Str("url_path", urlPath).Str("base_uri", baseURI).Msg("move urls") - // TODO replace with HasPrefix: - i := strings.Index(urlPath, baseURI) - if i == -1 { - w.WriteHeader(http.StatusBadRequest) - return - } - // check src exists srcStatReq := &provider.StatRequest{ Ref: &provider.Reference{ @@ -102,16 +86,6 @@ func (s *svc) handleMove(w http.ResponseWriter, r *http.Request, ns string) { return } - // TODO check if path is on same storage, return 502 on problems, see https://tools.ietf.org/html/rfc4918#section-9.9.4 - // The base URI might contain redirection prefixes which need to be handled - urlSplit := strings.Split(urlPath, baseURI) - if len(urlSplit) != 2 { - w.WriteHeader(http.StatusBadRequest) - return - } - // prefix to namespace - dst := path.Join(ns, urlSplit[1]) - // check dst exists dstStatRef := &provider.Reference{ Spec: &provider.Reference_Path{Path: dst}, diff --git a/internal/http/services/owncloud/ocdav/ocdav.go b/internal/http/services/owncloud/ocdav/ocdav.go index 19d8492375..f1c964d5b7 100644 --- a/internal/http/services/owncloud/ocdav/ocdav.go +++ b/internal/http/services/owncloud/ocdav/ocdav.go @@ -23,6 +23,7 @@ import ( "encoding/base64" "fmt" "net/http" + "net/url" "os" "path" "strings" @@ -37,6 +38,7 @@ import ( "github.com/cs3org/reva/pkg/storage/utils/templates" ctxuser "github.com/cs3org/reva/pkg/user" "github.com/mitchellh/mapstructure" + "github.com/pkg/errors" "github.com/rs/zerolog" ) @@ -244,3 +246,22 @@ func addAccessHeaders(w http.ResponseWriter, r *http.Request) { headers.Set("Strict-Transport-Security", "max-age=63072000") } } + +func extractDestination(dstHeader, baseURI string) (string, error) { + if dstHeader == "" { + return "", errors.New("destination header is empty") + } + dstURL, err := url.ParseRequestURI(dstHeader) + if err != nil { + return "", err + } + + // TODO check if path is on same storage, return 502 on problems, see https://tools.ietf.org/html/rfc4918#section-9.9.4 + // Strip the base URI from the destination. The destination might contain redirection prefixes which need to be handled + urlSplit := strings.Split(dstURL.Path, baseURI) + if len(urlSplit) != 2 { + return "", errors.New("destination path does not contain base URI") + } + + return urlSplit[1], nil +} diff --git a/internal/http/services/owncloud/ocdav/trashbin.go b/internal/http/services/owncloud/ocdav/trashbin.go index 73234d72f1..61ae0f2256 100644 --- a/internal/http/services/owncloud/ocdav/trashbin.go +++ b/internal/http/services/owncloud/ocdav/trashbin.go @@ -96,44 +96,22 @@ func (h *TrashbinHandler) Handler(s *svc) http.Handler { return } if key != "" && r.Method == "MOVE" { - dstHeader := r.Header.Get("Destination") - - log.Debug().Str("key", key).Str("dst", dstHeader).Msg("restore") - - if dstHeader == "" { - w.WriteHeader(http.StatusBadRequest) - return - } - // strip baseURL from destination - dstURL, err := url.ParseRequestURI(dstHeader) - if err != nil { - w.WriteHeader(http.StatusBadRequest) - return - } - - urlPath := dstURL.Path - // find path in url relative to trash base trashBase := ctx.Value(ctxKeyBaseURI).(string) baseURI := path.Join(path.Dir(trashBase), "files", userid) ctx = context.WithValue(ctx, ctxKeyBaseURI, baseURI) r = r.WithContext(ctx) - 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 { - w.WriteHeader(http.StatusBadRequest) - return - } - - urlSplit := strings.Split(urlPath, baseURI) - if len(urlSplit) != 2 { + dstHeader := r.Header.Get("Destination") + dst, err := extractDestination(dstHeader, baseURI) + if err != nil { w.WriteHeader(http.StatusBadRequest) return } + dst = path.Clean(dst) - dst := path.Clean(urlSplit[1]) + log.Debug().Str("key", key).Str("dst", dst).Msg("restore") h.restore(w, r, s, u, dst, key) return