From fea09e94b38129a123e6f135cb73ad95cf625751 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Thu, 16 Jun 2022 11:18:11 +0000 Subject: [PATCH 01/24] ocdav: skip space lookup on spaces propfind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../owncloud/ocdav/propfind/propfind.go | 79 +++++++++++-------- .../services/owncloud/ocdav/publicfile.go | 2 +- .../http/services/owncloud/ocdav/report.go | 2 +- .../http/services/owncloud/ocdav/versions.go | 2 +- 4 files changed, 47 insertions(+), 38 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/propfind/propfind.go b/internal/http/services/owncloud/ocdav/propfind/propfind.go index 52a95dbb73..5c3ebe635b 100644 --- a/internal/http/services/owncloud/ocdav/propfind/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind/propfind.go @@ -36,6 +36,7 @@ import ( rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" link "github.com/cs3org/go-cs3apis/cs3/sharing/link/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" "github.com/cs3org/reva/v2/internal/grpc/services/storageprovider" "github.com/cs3org/reva/v2/internal/http/services/owncloud/ocdav/errors" "github.com/cs3org/reva/v2/internal/http/services/owncloud/ocdav/net" @@ -232,7 +233,7 @@ func (p *Handler) HandlePathPropfind(w http.ResponseWriter, r *http.Request, ns // getResourceInfos handles responses in case of an error so we can just return here. return } - p.propfindResponse(ctx, w, r, ns, root.SpaceType, pf, sendTusHeaders, resourceInfos, sublog) + p.propfindResponse(ctx, w, r, ns /*root.SpaceType,*/, pf, sendTusHeaders, resourceInfos, sublog) } // HandleSpacesPropfind handles a spaces based propfind request @@ -241,12 +242,13 @@ func (p *Handler) HandleSpacesPropfind(w http.ResponseWriter, r *http.Request, s defer span.End() sublog := appctx.GetLogger(ctx).With().Str("path", r.URL.Path).Str("spaceid", spaceID).Logger() - client, err := p.getClient() + /*client, err := p.getClient() if err != nil { sublog.Error().Err(err).Msg("error getting grpc client") w.WriteHeader(http.StatusInternalServerError) return } + */ pf, status, err := ReadPropfind(r.Body) if err != nil { @@ -255,20 +257,35 @@ func (p *Handler) HandleSpacesPropfind(w http.ResponseWriter, r *http.Request, s return } - // retrieve a specific storage space - space, rpcStatus, err := spacelookup.LookUpStorageSpaceByID(ctx, client, spaceID) - if err != nil { - sublog.Error().Err(err).Msg("error looking up the space by id") - w.WriteHeader(http.StatusInternalServerError) - return - } - - if rpcStatus.Code != rpc.Code_CODE_OK { - errors.HandleErrorStatus(&sublog, w, rpcStatus) - return - } + /* + // retrieve a specific storage space + space, rpcStatus, err := spacelookup.LookUpStorageSpaceByID(ctx, client, spaceID) + if err != nil { + sublog.Error().Err(err).Msg("error looking up the space by id") + w.WriteHeader(http.StatusInternalServerError) + return + } - resourceInfos, sendTusHeaders, ok := p.getResourceInfos(ctx, w, r, pf, []*provider.StorageSpace{space}, r.URL.Path, true, sublog) + if rpcStatus.Code != rpc.Code_CODE_OK { + errors.HandleErrorStatus(&sublog, w, rpcStatus) + return + } + */ + + resourceID, err := storagespace.ParseID(spaceID) + // fake a space root + root := &provider.StorageSpace{ + Opaque: &typesv1beta1.Opaque{ + Map: map[string]*typesv1beta1.OpaqueEntry{ + "path": { + Decoder: "plain", + Value: []byte("/"), + }, + }, + }, + Root: &resourceID, + } + resourceInfos, sendTusHeaders, ok := p.getResourceInfos(ctx, w, r, pf, []*provider.StorageSpace{root /*space*/}, r.URL.Path, true, sublog) if !ok { // getResourceInfos handles responses in case of an error so we can just return here. return @@ -279,11 +296,11 @@ func (p *Handler) HandleSpacesPropfind(w http.ResponseWriter, r *http.Request, s resourceInfos[i].Path = path.Join("/", spaceID, resourceInfos[i].Path) } - p.propfindResponse(ctx, w, r, "", space.SpaceType, pf, sendTusHeaders, resourceInfos, sublog) + p.propfindResponse(ctx, w, r, "" /*space.SpaceType,*/, pf, sendTusHeaders, resourceInfos, sublog) } -func (p *Handler) propfindResponse(ctx context.Context, w http.ResponseWriter, r *http.Request, namespace, spaceType string, pf XML, sendTusHeaders bool, resourceInfos []*provider.ResourceInfo, log zerolog.Logger) { +func (p *Handler) propfindResponse(ctx context.Context, w http.ResponseWriter, r *http.Request, namespace /*, spaceType*/ string, pf XML, sendTusHeaders bool, resourceInfos []*provider.ResourceInfo, log zerolog.Logger) { ctx, span := appctx.GetTracerProvider(r.Context()).Tracer(tracerName).Start(ctx, "propfind_response") defer span.End() @@ -315,7 +332,7 @@ func (p *Handler) propfindResponse(ctx context.Context, w http.ResponseWriter, r } } - propRes, err := MultistatusResponse(ctx, &pf, resourceInfos, p.PublicURL, namespace, spaceType, linkshares) + propRes, err := MultistatusResponse(ctx, &pf, resourceInfos, p.PublicURL, namespace /*spaceType,*/, linkshares) if err != nil { log.Error().Err(err).Msg("error formatting propfind") w.WriteHeader(http.StatusInternalServerError) @@ -337,7 +354,7 @@ func (p *Handler) propfindResponse(ctx context.Context, w http.ResponseWriter, r } // TODO this is just a stat -> rename -func (p *Handler) statSpace(ctx context.Context, client gateway.GatewayAPIClient, space *provider.StorageSpace, ref *provider.Reference, metadataKeys []string) (*provider.ResourceInfo, *rpc.Status, error) { +func (p *Handler) statSpace(ctx context.Context, client gateway.GatewayAPIClient, ref *provider.Reference, metadataKeys []string) (*provider.ResourceInfo, *rpc.Status, error) { req := &provider.StatRequest{ Ref: ref, ArbitraryMetadataKeys: metadataKeys, @@ -395,16 +412,17 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r spaceMap = make(map[*provider.ResourceInfo]spaceData, len(spaces)) ) for _, space := range spaces { - spacePath, ok := getMountPoint(*space) - if !ok { + spacePath := "" + if spacePath = utils.ReadPlainFromOpaque(space.Opaque, "path"); spacePath == "" { continue // not mounted } + // TODO separate stats to the path or to the children, after statting all children update the mtime/etag // TODO get mtime, and size from space as well, so we no longer have to stat here? would require sending the requested metadata keys as well // root should be a ResourceInfo so it can contain the full stat, not only the id ... do we even need spaces then? // metadata keys could all be prefixed with "root." to indicate we want more than the root id ... spaceRef := spacelookup.MakeRelativeReference(space, requestPath, spacesPropfind) - info, status, err := p.statSpace(ctx, client, space, spaceRef, metadataKeys) + info, status, err := p.statSpace(ctx, client, spaceRef, metadataKeys) if err != nil || status.GetCode() != rpc.Code_CODE_OK { continue } @@ -604,15 +622,6 @@ func addChild(childInfos map[string]*provider.ResourceInfo, } } -func getMountPoint(space provider.StorageSpace) (string, bool) { - if space.Opaque == nil || - space.Opaque.Map["path"] == nil || - space.Opaque.Map["path"].Decoder != "plain" { - return "", false - } - return string(space.Opaque.Map["path"].Value), true -} - func requiresExplicitFetching(n *xml.Name) bool { switch n.Space { case net.NsDav: @@ -670,10 +679,10 @@ func ReadPropfind(r io.Reader) (pf XML, status int, err error) { } // MultistatusResponse converts a list of resource infos into a multistatus response string -func MultistatusResponse(ctx context.Context, pf *XML, mds []*provider.ResourceInfo, publicURL, ns, spaceType string, linkshares map[string]struct{}) ([]byte, error) { +func MultistatusResponse(ctx context.Context, pf *XML, mds []*provider.ResourceInfo, publicURL, ns /*, spaceType*/ string, linkshares map[string]struct{}) ([]byte, error) { responses := make([]*ResponseXML, 0, len(mds)) for i := range mds { - res, err := mdToPropResponse(ctx, pf, mds[i], publicURL, ns, spaceType, linkshares) + res, err := mdToPropResponse(ctx, pf, mds[i], publicURL, ns /*spaceType,*/, linkshares) if err != nil { return nil, err } @@ -692,7 +701,7 @@ func MultistatusResponse(ctx context.Context, pf *XML, mds []*provider.ResourceI // mdToPropResponse converts the CS3 metadata into a webdav PropResponse // ns is the CS3 namespace that needs to be removed from the CS3 path before // prefixing it with the baseURI -func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, publicURL, ns, spaceType string, linkshares map[string]struct{}) (*ResponseXML, error) { +func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, publicURL, ns /*, spaceType*/ string, linkshares map[string]struct{}) (*ResponseXML, error) { sublog := appctx.GetLogger(ctx).With().Interface("md", md).Str("ns", ns).Logger() md.Path = strings.TrimPrefix(md.Path, ns) @@ -740,7 +749,7 @@ func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, p role := conversions.RoleFromResourcePermissions(md.PermissionSet) - isShared := spaceType != _spaceTypeProject && !net.IsCurrentUserOwner(ctx, md.Owner) + isShared := /*spaceType != _spaceTypeProject &&*/ !net.IsCurrentUserOwner(ctx, md.Owner) var wdp string isPublic := ls != nil if md.PermissionSet != nil { diff --git a/internal/http/services/owncloud/ocdav/publicfile.go b/internal/http/services/owncloud/ocdav/publicfile.go index fc19def345..eff1c11e57 100644 --- a/internal/http/services/owncloud/ocdav/publicfile.go +++ b/internal/http/services/owncloud/ocdav/publicfile.go @@ -111,7 +111,7 @@ func (s *svc) handlePropfindOnToken(w http.ResponseWriter, r *http.Request, ns s infos := s.getPublicFileInfos(onContainer, depth == net.DepthZero, tokenStatInfo) - propRes, err := propfind.MultistatusResponse(ctx, &pf, infos, s.c.PublicURL, ns, "", nil) + propRes, err := propfind.MultistatusResponse(ctx, &pf, infos, s.c.PublicURL, ns /*"",*/, nil) if err != nil { sublog.Error().Err(err).Msg("error formatting propfind") w.WriteHeader(http.StatusInternalServerError) diff --git a/internal/http/services/owncloud/ocdav/report.go b/internal/http/services/owncloud/ocdav/report.go index 1c93756072..e5657ad87f 100644 --- a/internal/http/services/owncloud/ocdav/report.go +++ b/internal/http/services/owncloud/ocdav/report.go @@ -110,7 +110,7 @@ func (s *svc) doFilterFiles(w http.ResponseWriter, r *http.Request, ff *reportFi infos = append(infos, statRes.Info) } - responsesXML, err := propfind.MultistatusResponse(ctx, &propfind.XML{Prop: ff.Prop}, infos, s.c.PublicURL, namespace, "", nil) + responsesXML, err := propfind.MultistatusResponse(ctx, &propfind.XML{Prop: ff.Prop}, infos, s.c.PublicURL, namespace /*"",*/, nil) if err != nil { log.Error().Err(err).Msg("error formatting propfind") w.WriteHeader(http.StatusInternalServerError) diff --git a/internal/http/services/owncloud/ocdav/versions.go b/internal/http/services/owncloud/ocdav/versions.go index 37b2d935d4..a65529e467 100644 --- a/internal/http/services/owncloud/ocdav/versions.go +++ b/internal/http/services/owncloud/ocdav/versions.go @@ -189,7 +189,7 @@ func (h *VersionsHandler) doListVersions(w http.ResponseWriter, r *http.Request, infos = append(infos, vi) } - propRes, err := propfind.MultistatusResponse(ctx, &pf, infos, s.c.PublicURL, "", "", nil) + propRes, err := propfind.MultistatusResponse(ctx, &pf, infos, s.c.PublicURL, "" /*"",*/, nil) if err != nil { sublog.Error().Err(err).Msg("error formatting propfind") w.WriteHeader(http.StatusInternalServerError) From 3221654111fb10603ad081eda2d35a4b58eabf67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Thu, 16 Jun 2022 12:01:17 +0000 Subject: [PATCH 02/24] add changelog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- changelog/unreleased/skip-space-lookup-on-space-propfind.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/unreleased/skip-space-lookup-on-space-propfind.md diff --git a/changelog/unreleased/skip-space-lookup-on-space-propfind.md b/changelog/unreleased/skip-space-lookup-on-space-propfind.md new file mode 100644 index 0000000000..bef1452b42 --- /dev/null +++ b/changelog/unreleased/skip-space-lookup-on-space-propfind.md @@ -0,0 +1,5 @@ +Enhancement: skip space lookup on space propfind + +We now construct the space id from the /dav/spaces URL intead of making a request to the registry. + +https://github.com/cs3org/reva/pull/2977 From 7c0554bae4269c154e6f0f5e9efb449b65ec038d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Thu, 16 Jun 2022 13:29:34 +0000 Subject: [PATCH 03/24] update unit test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../owncloud/ocdav/propfind/propfind_test.go | 64 +++++++++++-------- 1 file changed, 36 insertions(+), 28 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/propfind/propfind_test.go b/internal/http/services/owncloud/ocdav/propfind/propfind_test.go index daafb791e6..f09b817e04 100644 --- a/internal/http/services/owncloud/ocdav/propfind/propfind_test.go +++ b/internal/http/services/owncloud/ocdav/propfind/propfind_test.go @@ -609,27 +609,32 @@ var _ = Describe("Propfind", func() { }) Describe("HandleSpacesPropfind", func() { - JustBeforeEach(func() { - client.On("ListStorageSpaces", mock.Anything, mock.Anything).Return( - func(_ context.Context, req *sprovider.ListStorageSpacesRequest, _ ...grpc.CallOption) *sprovider.ListStorageSpacesResponse { - var spaces []*sprovider.StorageSpace + /* + JustBeforeEach(func() { + client.On("Stat", mock.Anything, mock.Anything).Return(func(_ context.Context, req *sprovider.StatRequest, _ ...grpc.CallOption) *sprovider.StatResponse { switch { - case req.Filters[0].Term.(*sprovider.ListStorageSpacesRequest_Filter_Id).Id.OpaqueId == "foospace": - spaces = []*sprovider.StorageSpace{foospace} + case req.Ref.ResourceId.OpaqueId == "foospace": + return &sprovider.StatResponse{ + Status: status.NewOK(ctx), + Info: &sprovider.ResourceInfo{ + Type: sprovider.ResourceType_RESOURCE_TYPE_CONTAINER, + Id: &sprovider.ResourceId{OpaqueId: "foospaceroot", StorageId: "foospaceroot"}, + Size: 131, + Path: ".", + }, + } default: - spaces = []*sprovider.StorageSpace{} - } - return &sprovider.ListStorageSpacesResponse{ - Status: status.NewOK(ctx), - StorageSpaces: spaces, + return &sprovider.StatResponse{ + Status: status.NewNotFound(ctx, "not found"), + } } }, nil) - }) + }) + */ It("handles invalid space ids", func() { - client.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(&sprovider.ListStorageSpacesResponse{ - Status: status.NewOK(ctx), - StorageSpaces: []*sprovider.StorageSpace{}, + client.On("Stat", mock.Anything, mock.Anything).Return(&sprovider.StatResponse{ + Status: status.NewNotFound(ctx, "not found"), }, nil) rr := httptest.NewRecorder() @@ -641,12 +646,15 @@ var _ = Describe("Propfind", func() { }) It("stats the space root", func() { + client.On("Stat", mock.Anything, mock.Anything).Return(&sprovider.StatResponse{ + Status: status.NewNotFound(ctx, "not found"), + }, nil) rr := httptest.NewRecorder() req, err := http.NewRequest("GET", "/", strings.NewReader("")) Expect(err).ToNot(HaveOccurred()) req = req.WithContext(ctx) - handler.HandleSpacesPropfind(rr, req, "foospace") + handler.HandleSpacesPropfind(rr, req, "foospaceroot") Expect(rr.Code).To(Equal(http.StatusMultiStatus)) res, _, err := readResponse(rr.Result().Body) @@ -654,19 +662,19 @@ var _ = Describe("Propfind", func() { Expect(len(res.Responses)).To(Equal(4)) root := res.Responses[0] - Expect(root.Href).To(Equal("http:/127.0.0.1:3000/foospace/")) + Expect(root.Href).To(Equal("http:/127.0.0.1:3000/foospaceroot/")) Expect(string(root.Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("131")) bar := res.Responses[1] - Expect(bar.Href).To(Equal("http:/127.0.0.1:3000/foospace/bar")) + Expect(bar.Href).To(Equal("http:/127.0.0.1:3000/foospaceroot/bar")) Expect(string(bar.Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("100")) baz := res.Responses[2] - Expect(baz.Href).To(Equal("http:/127.0.0.1:3000/foospace/baz")) + Expect(baz.Href).To(Equal("http:/127.0.0.1:3000/foospaceroot/baz")) Expect(string(baz.Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("1")) dir := res.Responses[3] - Expect(dir.Href).To(Equal("http:/127.0.0.1:3000/foospace/dir/")) + Expect(dir.Href).To(Equal("http:/127.0.0.1:3000/foospaceroot/dir/")) Expect(string(dir.Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("30")) }) @@ -676,7 +684,7 @@ var _ = Describe("Propfind", func() { Expect(err).ToNot(HaveOccurred()) req = req.WithContext(ctx) - handler.HandleSpacesPropfind(rr, req, "foospace") + handler.HandleSpacesPropfind(rr, req, "foospaceroot") Expect(rr.Code).To(Equal(http.StatusMultiStatus)) res, _, err := readResponse(rr.Result().Body) @@ -702,7 +710,7 @@ var _ = Describe("Propfind", func() { Expect(err).ToNot(HaveOccurred()) req = req.WithContext(ctx) - handler.HandleSpacesPropfind(rr, req, "foospace") + handler.HandleSpacesPropfind(rr, req, "foospaceroot") Expect(rr.Code).To(Equal(http.StatusMultiStatus)) res, _, err := readResponse(rr.Result().Body) @@ -719,7 +727,7 @@ var _ = Describe("Propfind", func() { Expect(err).ToNot(HaveOccurred()) req = req.WithContext(ctx) - handler.HandleSpacesPropfind(rr, req, "foospace") + handler.HandleSpacesPropfind(rr, req, "foospaceroot") Expect(rr.Code).To(Equal(http.StatusMultiStatus)) res, _, err := readResponse(rr.Result().Body) @@ -731,11 +739,11 @@ var _ = Describe("Propfind", func() { paths = append(paths, r.Href) } Expect(paths).To(ConsistOf( - "http:/127.0.0.1:3000/foospace/", - "http:/127.0.0.1:3000/foospace/bar", - "http:/127.0.0.1:3000/foospace/baz", - "http:/127.0.0.1:3000/foospace/dir/", - "http:/127.0.0.1:3000/foospace/dir/entry", + "http:/127.0.0.1:3000/foospaceroot/", + "http:/127.0.0.1:3000/foospaceroot/bar", + "http:/127.0.0.1:3000/foospaceroot/baz", + "http:/127.0.0.1:3000/foospaceroot/dir/", + "http:/127.0.0.1:3000/foospaceroot/dir/entry", )) }) }) From 7b1b7e76f2e67da93d5d3e3f78e3dd58cd84e29d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Thu, 16 Jun 2022 14:45:25 +0000 Subject: [PATCH 04/24] lint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../owncloud/ocdav/propfind/propfind.go | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/propfind/propfind.go b/internal/http/services/owncloud/ocdav/propfind/propfind.go index 5c3ebe635b..0f7657c05f 100644 --- a/internal/http/services/owncloud/ocdav/propfind/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind/propfind.go @@ -54,8 +54,8 @@ import ( ) const ( - tracerName = "ocdav" - _spaceTypeProject = "project" + tracerName = "ocdav" + // _spaceTypeProject = "project" ) type countingReader struct { @@ -212,7 +212,7 @@ func (p *Handler) HandlePathPropfind(w http.ResponseWriter, r *http.Request, ns return } - var root *provider.StorageSpace + /*var root *provider.StorageSpace switch { case len(spaces) == 1: @@ -226,7 +226,7 @@ func (p *Handler) HandlePathPropfind(w http.ResponseWriter, r *http.Request, ns if root == nil { root = spaces[0] } - } + }*/ resourceInfos, sendTusHeaders, ok := p.getResourceInfos(ctx, w, r, pf, spaces, fn, false, sublog) if !ok { @@ -273,6 +273,14 @@ func (p *Handler) HandleSpacesPropfind(w http.ResponseWriter, r *http.Request, s */ resourceID, err := storagespace.ParseID(spaceID) + if err != nil { + sublog.Debug().Str("spaceID", spaceID).Msg(err.Error()) + w.WriteHeader(http.StatusBadRequest) + m := fmt.Sprintf("Invalid space id: %v", spaceID) + b, err := errors.Marshal(http.StatusBadRequest, m, "") + errors.HandleWebdavError(&sublog, w, b, err) + return + } // fake a space root root := &provider.StorageSpace{ Opaque: &typesv1beta1.Opaque{ @@ -1364,6 +1372,7 @@ func (pn *Props) UnmarshalXML(d *xml.Decoder, start xml.StartElement) error { } // isVirtualRootResourceID returns true if the id points to the share jail root. The providerid is optional for legacy ids +/* func isVirtualRootResourceID(id *provider.ResourceId) bool { switch { case id == nil: @@ -1374,3 +1383,4 @@ func isVirtualRootResourceID(id *provider.ResourceId) bool { providerID, spaceID := storagespace.SplitStorageID(id.StorageId) return spaceID == utils.ShareStorageProviderID && (providerID == "" || providerID == utils.ShareStorageProviderID) } +*/ From f0a4096e7ba50b6fac1245a0ab206a54d5d863ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Tue, 21 Jun 2022 20:54:13 +0000 Subject: [PATCH 05/24] reduce stat requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../sharesstorageprovider.go | 48 ++- .../http/services/owncloud/ocdav/locks.go | 18 +- .../owncloud/ocdav/propfind/propfind.go | 316 ++++++++++++------ .../owncloud/ocdav/spacelookup/spacelookup.go | 11 +- 4 files changed, 256 insertions(+), 137 deletions(-) diff --git a/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go b/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go index 2ff40af8ef..ef108120e3 100644 --- a/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go +++ b/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go @@ -24,7 +24,6 @@ import ( "path/filepath" "strings" - "github.com/cs3org/reva/v2/pkg/share" "github.com/cs3org/reva/v2/pkg/storagespace" "google.golang.org/grpc" codes "google.golang.org/grpc/codes" @@ -369,10 +368,10 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora } var receivedShares []*collaboration.ReceivedShare - var shareMd map[string]share.Metadata + var shareInfo map[string]*provider.ResourceInfo var err error if fetchShares { - receivedShares, shareMd, err = s.fetchShares(ctx) + receivedShares, shareInfo, err = s.fetchShares(ctx) if err != nil { return nil, errors.Wrap(err, "sharesstorageprovider: error calling ListReceivedSharesRequest") } @@ -389,13 +388,13 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora OpaqueId: utils.ShareStorageProviderID, } if spaceID == nil || isShareJailRoot(spaceID) { - earliestShare, atLeastOneAccepted := findEarliestShare(receivedShares, shareMd) + earliestShare, atLeastOneAccepted := findEarliestShare(receivedShares, shareInfo) var opaque *typesv1beta1.Opaque var mtime *typesv1beta1.Timestamp if earliestShare != nil { - if md, ok := shareMd[earliestShare.Id.OpaqueId]; ok { - mtime = md.Mtime - opaque = utils.AppendPlainToOpaque(opaque, "etag", md.ETag) + if info, ok := shareInfo[earliestShare.Id.OpaqueId]; ok { + mtime = info.Mtime + opaque = utils.AppendPlainToOpaque(opaque, "etag", info.Etag) } } // only display the shares jail if we have accepted shares @@ -424,20 +423,16 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora // none of our business continue } - var opaque *typesv1beta1.Opaque - if md, ok := shareMd[receivedShare.Share.Id.OpaqueId]; ok { - opaque = utils.AppendPlainToOpaque(opaque, "etag", md.ETag) - } // we know a grant for this resource space := &provider.StorageSpace{ - Opaque: opaque, Id: &provider.StorageSpaceId{ OpaqueId: storagespace.FormatResourceID(*root), }, SpaceType: "grant", Owner: &userv1beta1.User{Id: receivedShare.Share.Owner}, // the sharesstorageprovider keeps track of mount points - Root: root, + Root: root, + RootInfo: shareInfo[receivedShare.Share.Id.OpaqueId], } res.StorageSpaces = append(res.StorageSpaces, space) @@ -465,9 +460,7 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora } } var opaque *typesv1beta1.Opaque - if md, ok := shareMd[receivedShare.Share.Id.OpaqueId]; ok { - opaque = utils.AppendPlainToOpaque(opaque, "etag", md.ETag) - } else { + if _, ok := shareInfo[receivedShare.Share.Id.OpaqueId]; !ok { // we could not stat the share, skip it continue } @@ -490,7 +483,8 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora SpaceType: "mountpoint", Owner: &userv1beta1.User{Id: receivedShare.Share.Owner}, // FIXME actually, the mount point belongs to the recipient // the sharesstorageprovider keeps track of mount points - Root: root, + Root: root, + RootInfo: shareInfo[receivedShare.Share.Id.OpaqueId], } // TODO in the future the spaces registry will handle the alias for share spaces. @@ -711,7 +705,7 @@ func (s *service) Stat(ctx context.Context, req *provider.StatRequest) (*provide PermissionSet: &provider.ResourcePermissions{ // TODO }, - Etag: shareMd[earliestShare.Id.OpaqueId].ETag, + Etag: shareMd[earliestShare.Id.OpaqueId].Etag, Owner: owner.Id, }, }, nil @@ -1031,7 +1025,7 @@ func (s *service) rejectReceivedShare(ctx context.Context, receivedShare *collab return errtypes.NewErrtypeFromStatus(res.Status) } -func (s *service) fetchShares(ctx context.Context) ([]*collaboration.ReceivedShare, map[string]share.Metadata, error) { +func (s *service) fetchShares(ctx context.Context) ([]*collaboration.ReceivedShare, map[string]*provider.ResourceInfo, error) { lsRes, err := s.sharesProviderClient.ListReceivedShares(ctx, &collaboration.ListReceivedSharesRequest{ // FIXME filter by received shares for resource id - listing all shares is tooo expensive! }) @@ -1042,7 +1036,7 @@ func (s *service) fetchShares(ctx context.Context) ([]*collaboration.ReceivedSha return nil, nil, fmt.Errorf("sharesstorageprovider: error calling ListReceivedSharesRequest") } - shareMetaData := make(map[string]share.Metadata, len(lsRes.Shares)) + shareMetaData := make(map[string]*provider.ResourceInfo, len(lsRes.Shares)) for _, rs := range lsRes.Shares { // only stat accepted shares if rs.State != collaboration.ShareState_SHARE_STATE_ACCEPTED { @@ -1063,13 +1057,13 @@ func (s *service) fetchShares(ctx context.Context) ([]*collaboration.ReceivedSha Msg("ListRecievedShares: failed to stat the resource") continue } - shareMetaData[rs.Share.Id.OpaqueId] = share.Metadata{ETag: sRes.Info.Etag, Mtime: sRes.Info.Mtime} + shareMetaData[rs.Share.Id.OpaqueId] = sRes.Info } return lsRes.Shares, shareMetaData, nil } -func findEarliestShare(receivedShares []*collaboration.ReceivedShare, shareMd map[string]share.Metadata) (earliestShare *collaboration.Share, atLeastOneAccepted bool) { +func findEarliestShare(receivedShares []*collaboration.ReceivedShare, shareInfo map[string]*provider.ResourceInfo) (earliestShare *collaboration.Share, atLeastOneAccepted bool) { for _, rs := range receivedShares { var hasCurrentMd bool var hasEarliestMd bool @@ -1081,10 +1075,10 @@ func findEarliestShare(receivedShares []*collaboration.ReceivedShare, shareMd ma // We cannot assume that every share has metadata if current.Id != nil { - _, hasCurrentMd = shareMd[current.Id.OpaqueId] + _, hasCurrentMd = shareInfo[current.Id.OpaqueId] } if earliestShare != nil && earliestShare.Id != nil { - _, hasEarliestMd = shareMd[earliestShare.Id.OpaqueId] + _, hasEarliestMd = shareInfo[earliestShare.Id.OpaqueId] } switch { @@ -1093,10 +1087,10 @@ func findEarliestShare(receivedShares []*collaboration.ReceivedShare, shareMd ma // ignore if one of the shares has no metadata case !hasEarliestMd || !hasCurrentMd: continue - case shareMd[current.Id.OpaqueId].Mtime.Seconds > shareMd[earliestShare.Id.OpaqueId].Mtime.Seconds: + case shareInfo[current.Id.OpaqueId].Mtime.Seconds > shareInfo[earliestShare.Id.OpaqueId].Mtime.Seconds: earliestShare = current - case shareMd[current.Id.OpaqueId].Mtime.Seconds == shareMd[earliestShare.Id.OpaqueId].Mtime.Seconds && - shareMd[current.Id.OpaqueId].Mtime.Nanos > shareMd[earliestShare.Id.OpaqueId].Mtime.Nanos: + case shareInfo[current.Id.OpaqueId].Mtime.Seconds == shareInfo[earliestShare.Id.OpaqueId].Mtime.Seconds && + shareInfo[current.Id.OpaqueId].Mtime.Nanos > shareInfo[earliestShare.Id.OpaqueId].Mtime.Nanos: earliestShare = current } } diff --git a/internal/http/services/owncloud/ocdav/locks.go b/internal/http/services/owncloud/ocdav/locks.go index 99500580f0..3b048ccdd1 100644 --- a/internal/http/services/owncloud/ocdav/locks.go +++ b/internal/http/services/owncloud/ocdav/locks.go @@ -400,23 +400,13 @@ func (s *svc) handleSpacesLock(w http.ResponseWriter, r *http.Request, spaceID s span.SetAttributes(attribute.String("component", "ocdav")) - client, err := s.getClient() + // build storage space reference + ref, err := spacelookup.MakeStorageSpaceReference(spaceID, r.URL.Path) if err != nil { - return http.StatusInternalServerError, err + return http.StatusBadRequest, fmt.Errorf("invalid space id") } - // retrieve a specific storage space - space, cs3Status, err := spacelookup.LookUpStorageSpaceByID(ctx, client, spaceID) - if err != nil { - return http.StatusInternalServerError, err - } - if cs3Status.Code != rpc.Code_CODE_OK { - return http.StatusInternalServerError, errtypes.NewErrtypeFromStatus(cs3Status) - } - - ref := spacelookup.MakeRelativeReference(space, r.URL.Path, true) - - return s.lockReference(ctx, w, r, ref) + return s.lockReference(ctx, w, r, &ref) } func (s *svc) lockReference(ctx context.Context, w http.ResponseWriter, r *http.Request, ref *provider.Reference) (retStatus int, retErr error) { diff --git a/internal/http/services/owncloud/ocdav/propfind/propfind.go b/internal/http/services/owncloud/ocdav/propfind/propfind.go index 0f7657c05f..038a7e61f1 100644 --- a/internal/http/services/owncloud/ocdav/propfind/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind/propfind.go @@ -51,11 +51,12 @@ import ( "github.com/cs3org/reva/v2/pkg/utils" "github.com/rs/zerolog" "go.opentelemetry.io/otel/codes" + "google.golang.org/protobuf/types/known/fieldmaskpb" ) const ( - tracerName = "ocdav" - // _spaceTypeProject = "project" + tracerName = "ocdav" + _spaceTypeProject = "project" ) type countingReader struct { @@ -200,6 +201,7 @@ func (p *Handler) HandlePathPropfind(w http.ResponseWriter, r *http.Request, ns return } + // TODO look up all spaces and request the root_info in the field mask spaces, rpcStatus, err := spacelookup.LookUpStorageSpacesForPathWithChildren(ctx, client, fn) if err != nil { sublog.Error().Err(err).Msg("error sending a grpc request") @@ -212,28 +214,12 @@ func (p *Handler) HandlePathPropfind(w http.ResponseWriter, r *http.Request, ns return } - /*var root *provider.StorageSpace - - switch { - case len(spaces) == 1: - root = spaces[0] - case len(spaces) > 1: - for _, space := range spaces { - if isVirtualRootResourceID(space.Root) { - root = space - } - } - if root == nil { - root = spaces[0] - } - }*/ - - resourceInfos, sendTusHeaders, ok := p.getResourceInfos(ctx, w, r, pf, spaces, fn, false, sublog) + resourceInfos, sendTusHeaders, ok := p.getResourceInfos(ctx, w, r, pf, spaces, fn, sublog) if !ok { // getResourceInfos handles responses in case of an error so we can just return here. return } - p.propfindResponse(ctx, w, r, ns /*root.SpaceType,*/, pf, sendTusHeaders, resourceInfos, sublog) + p.propfindResponse(ctx, w, r, ns, pf, sendTusHeaders, resourceInfos, sublog) } // HandleSpacesPropfind handles a spaces based propfind request @@ -242,13 +228,17 @@ func (p *Handler) HandleSpacesPropfind(w http.ResponseWriter, r *http.Request, s defer span.End() sublog := appctx.GetLogger(ctx).With().Str("path", r.URL.Path).Str("spaceid", spaceID).Logger() - /*client, err := p.getClient() + dh := r.Header.Get(net.HeaderDepth) + + depth, err := net.ParseDepth(dh) if err != nil { - sublog.Error().Err(err).Msg("error getting grpc client") - w.WriteHeader(http.StatusInternalServerError) + sublog.Debug().Str("depth", dh).Msg(err.Error()) + w.WriteHeader(http.StatusBadRequest) + m := fmt.Sprintf("Invalid Depth header value: %v", dh) + b, err := errors.Marshal(http.StatusBadRequest, m, "") + errors.HandleWebdavError(&sublog, w, b, err) return } - */ pf, status, err := ReadPropfind(r.Body) if err != nil { @@ -257,58 +247,94 @@ func (p *Handler) HandleSpacesPropfind(w http.ResponseWriter, r *http.Request, s return } - /* - // retrieve a specific storage space - space, rpcStatus, err := spacelookup.LookUpStorageSpaceByID(ctx, client, spaceID) - if err != nil { - sublog.Error().Err(err).Msg("error looking up the space by id") - w.WriteHeader(http.StatusInternalServerError) - return - } - - if rpcStatus.Code != rpc.Code_CODE_OK { - errors.HandleErrorStatus(&sublog, w, rpcStatus) - return - } - */ - - resourceID, err := storagespace.ParseID(spaceID) + ref, err := spacelookup.MakeStorageSpaceReference(spaceID, r.URL.Path) if err != nil { - sublog.Debug().Str("spaceID", spaceID).Msg(err.Error()) + w.WriteHeader(http.StatusBadRequest) + sublog.Debug().Msg("invalid space id") w.WriteHeader(http.StatusBadRequest) m := fmt.Sprintf("Invalid space id: %v", spaceID) b, err := errors.Marshal(http.StatusBadRequest, m, "") errors.HandleWebdavError(&sublog, w, b, err) return } - // fake a space root - root := &provider.StorageSpace{ - Opaque: &typesv1beta1.Opaque{ - Map: map[string]*typesv1beta1.OpaqueEntry{ - "path": { - Decoder: "plain", - Value: []byte("/"), + + client, err := p.getClient() + if err != nil { + sublog.Error().Err(err).Msg("error getting grpc client") + w.WriteHeader(http.StatusInternalServerError) + return + } + + // stat the reference and request the space in the field mask + res, err := client.Stat(ctx, &provider.StatRequest{ + Ref: &ref, + ArbitraryMetadataKeys: metadataKeys(pf), + FieldMask: &fieldmaskpb.FieldMask{Paths: []string{"*"}}, + }) + if err != nil { + sublog.Error().Err(err).Msg("error getting grpc client") + w.WriteHeader(http.StatusInternalServerError) + return + } + if res.Status.Code != rpc.Code_CODE_OK { + errors.HandleErrorStatus(&sublog, w, res.Status) + return + } + var space *provider.StorageSpace + if res.Info.Space == nil { + sublog.Debug().Msg("stat did not include a space, executing an additional lookup request") + // TODO look up space? hm can the isShared check even work when stating a space? hm yeah ... well ... *mindblown* + // fake a space root + space = &provider.StorageSpace{ + Id: &provider.StorageSpaceId{OpaqueId: spaceID}, + Opaque: &typesv1beta1.Opaque{ + Map: map[string]*typesv1beta1.OpaqueEntry{ + "path": { + Decoder: "plain", + Value: []byte("/"), + }, }, }, - }, - Root: &resourceID, + Root: ref.ResourceId, + RootInfo: res.Info, + } } - resourceInfos, sendTusHeaders, ok := p.getResourceInfos(ctx, w, r, pf, []*provider.StorageSpace{root /*space*/}, r.URL.Path, true, sublog) - if !ok { - // getResourceInfos handles responses in case of an error so we can just return here. - return + + resourceInfos := []*provider.ResourceInfo{ + res.Info, + } + if res.Info.Type == provider.ResourceType_RESOURCE_TYPE_CONTAINER && depth != net.DepthZero { + childInfos, ok := p.getSpaceResourceInfos(ctx, w, r, pf, &ref, r.URL.Path, depth, sublog) + if !ok { + // getResourceInfos handles responses in case of an error so we can just return here. + return + } + resourceInfos = append(resourceInfos, childInfos...) } // prefix space id to paths for i := range resourceInfos { resourceInfos[i].Path = path.Join("/", spaceID, resourceInfos[i].Path) + // add space to info so propfindResponse can access space type + if resourceInfos[i].Space == nil { + resourceInfos[i].Space = space + } } - p.propfindResponse(ctx, w, r, "" /*space.SpaceType,*/, pf, sendTusHeaders, resourceInfos, sublog) + sendTusHeaders := true + // let clients know this collection supports tus.io POST requests to start uploads + if res.Info.Type == provider.ResourceType_RESOURCE_TYPE_CONTAINER { + if res.Info.Opaque != nil { + _, ok := res.Info.Opaque.Map["disable_tus"] + sendTusHeaders = !ok + } + } + + p.propfindResponse(ctx, w, r, "", pf, sendTusHeaders, resourceInfos, sublog) } -func (p *Handler) propfindResponse(ctx context.Context, w http.ResponseWriter, r *http.Request, namespace /*, spaceType*/ string, pf XML, sendTusHeaders bool, resourceInfos []*provider.ResourceInfo, log zerolog.Logger) { +func (p *Handler) propfindResponse(ctx context.Context, w http.ResponseWriter, r *http.Request, namespace string, pf XML, sendTusHeaders bool, resourceInfos []*provider.ResourceInfo, log zerolog.Logger) { ctx, span := appctx.GetTracerProvider(r.Context()).Tracer(tracerName).Start(ctx, "propfind_response") defer span.End() @@ -340,7 +366,7 @@ func (p *Handler) propfindResponse(ctx context.Context, w http.ResponseWriter, r } } - propRes, err := MultistatusResponse(ctx, &pf, resourceInfos, p.PublicURL, namespace /*spaceType,*/, linkshares) + propRes, err := MultistatusResponse(ctx, &pf, resourceInfos, p.PublicURL, namespace, linkshares) if err != nil { log.Error().Err(err).Msg("error formatting propfind") w.WriteHeader(http.StatusInternalServerError) @@ -374,7 +400,7 @@ func (p *Handler) statSpace(ctx context.Context, client gateway.GatewayAPIClient return res.GetInfo(), res.GetStatus(), nil } -func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r *http.Request, pf XML, spaces []*provider.StorageSpace, requestPath string, spacesPropfind bool, log zerolog.Logger) ([]*provider.ResourceInfo, bool, bool) { +func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r *http.Request, pf XML, spaces []*provider.StorageSpace, requestPath string, log zerolog.Logger) ([]*provider.ResourceInfo, bool, bool) { dh := r.Header.Get(net.HeaderDepth) depth, err := net.ParseDepth(dh) if err != nil { @@ -393,23 +419,7 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r return nil, false, false } - var metadataKeys []string - - if pf.Allprop != nil { - // TODO this changes the behavior and returns all properties if allprops has been set, - // but allprops should only return some default properties - // see https://tools.ietf.org/html/rfc4918#section-9.1 - // the description of arbitrary_metadata_keys in https://cs3org.github.io/cs3apis/#cs3.storage.provider.v1beta1.ListContainerRequest an others may need clarification - // tracked in https://github.com/cs3org/cs3apis/issues/104 - metadataKeys = append(metadataKeys, "*") - } else { - metadataKeys = make([]string, 0, len(pf.Prop)) - for i := range pf.Prop { - if requiresExplicitFetching(&pf.Prop[i]) { - metadataKeys = append(metadataKeys, metadataKeyOf(&pf.Prop[i])) - } - } - } + metadataKeys := metadataKeys(pf) // we need to stat all spaces to aggregate the root etag, mtime and size // TODO cache per space (hah, no longer per user + per space!) @@ -424,24 +434,37 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r if spacePath = utils.ReadPlainFromOpaque(space.Opaque, "path"); spacePath == "" { continue // not mounted } + if space.RootInfo == nil { + spaceRef, err := spacelookup.MakeStorageSpaceReference(space.Id.OpaqueId, ".") + if err != nil { + continue + } + info, status, err := p.statSpace(ctx, client, &spaceRef, metadataKeys) + if err != nil || status.GetCode() != rpc.Code_CODE_OK { + continue + } + space.RootInfo = info + } // TODO separate stats to the path or to the children, after statting all children update the mtime/etag // TODO get mtime, and size from space as well, so we no longer have to stat here? would require sending the requested metadata keys as well // root should be a ResourceInfo so it can contain the full stat, not only the id ... do we even need spaces then? // metadata keys could all be prefixed with "root." to indicate we want more than the root id ... - spaceRef := spacelookup.MakeRelativeReference(space, requestPath, spacesPropfind) - info, status, err := p.statSpace(ctx, client, spaceRef, metadataKeys) - if err != nil || status.GetCode() != rpc.Code_CODE_OK { - continue + // TODO can we reuse the space.rootinfo? + spaceRef := spacelookup.MakeRelativeReference(space, requestPath, false) + var info *provider.ResourceInfo + if spaceRef.Path == "." && utils.ResourceIDEqual(spaceRef.ResourceId, space.Root) { + info = space.RootInfo + } else { + var status *rpc.Status + info, status, err = p.statSpace(ctx, client, spaceRef, metadataKeys) + if err != nil || status.GetCode() != rpc.Code_CODE_OK { + continue + } } // adjust path - if spacesPropfind { - // we need to prefix the path with / to make subsequent prefix matches work - info.Path = filepath.Join("/", spaceRef.Path) - } else { - info.Path = filepath.Join(spacePath, spaceRef.Path) - } + info.Path = filepath.Join(spacePath, spaceRef.Path) spaceMap[info] = spaceData{Ref: spaceRef, SpaceType: space.SpaceType} @@ -496,12 +519,12 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r childInfos := map[string]*provider.ResourceInfo{} for spaceInfo, spaceData := range spaceMap { switch { - case !spacesPropfind && spaceInfo.Type != provider.ResourceType_RESOURCE_TYPE_CONTAINER && depth != net.DepthInfinity: + case spaceInfo.Type != provider.ResourceType_RESOURCE_TYPE_CONTAINER && depth != net.DepthInfinity: addChild(childInfos, spaceInfo, requestPath, rootInfo) case spaceInfo.Type == provider.ResourceType_RESOURCE_TYPE_CONTAINER && depth == net.DepthOne: switch { - case strings.HasPrefix(requestPath, spaceInfo.Path) && (spacesPropfind || spaceData.SpaceType != "virtual"): + case strings.HasPrefix(requestPath, spaceInfo.Path) && spaceData.SpaceType != "virtual": req := &provider.ListContainerRequest{ Ref: spaceData.Ref, ArbitraryMetadataKeys: metadataKeys, @@ -562,9 +585,6 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r for i := len(res.Infos) - 1; i >= 0; i-- { // add path to resource res.Infos[i].Path = filepath.Join(info.Path, res.Infos[i].Path) - if spacesPropfind { - res.Infos[i].Path = utils.MakeRelativePath(res.Infos[i].Path) - } if res.Infos[i].Type == provider.ResourceType_RESOURCE_TYPE_CONTAINER { stack = append(stack, res.Infos[i]) } @@ -596,6 +616,112 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r return resourceInfos, sendTusHeaders, true } +func (p *Handler) getSpaceResourceInfos(ctx context.Context, w http.ResponseWriter, r *http.Request, pf XML, ref *provider.Reference, requestPath string, depth net.Depth, log zerolog.Logger) ([]*provider.ResourceInfo, bool) { + + client, err := p.getClient() + if err != nil { + log.Error().Err(err).Msg("error getting grpc client") + w.WriteHeader(http.StatusInternalServerError) + return nil, false + } + + metadataKeys := metadataKeys(pf) + + // we need to prefix the path with / to make subsequent prefix matches work + //info.Path = filepath.Join("/", spaceRef.Path) + + resourceInfos := []*provider.ResourceInfo{} + + req := &provider.ListContainerRequest{ + Ref: ref, + ArbitraryMetadataKeys: metadataKeys, + } + res, err := client.ListContainer(ctx, req) + if err != nil { + log.Error().Err(err).Msg("error sending list container grpc request") + w.WriteHeader(http.StatusInternalServerError) + return nil, false + } + + if res.Status.Code != rpc.Code_CODE_OK { + log.Debug().Interface("status", res.Status).Msg("List Container not ok, skipping") + return nil, false + } + for _, info := range res.Infos { + info.Path = path.Join(requestPath, info.Path) + } + resourceInfos = append(resourceInfos, res.Infos...) + + if depth == net.DepthInfinity { + // use a stack to explore sub-containers breadth-first + stack := resourceInfos + for len(stack) != 0 { + info := stack[0] + stack = stack[1:] + + if info.Type != provider.ResourceType_RESOURCE_TYPE_CONTAINER /*|| space.SpaceType == "virtual"*/ { + continue + } + req := &provider.ListContainerRequest{ + Ref: &provider.Reference{ + ResourceId: info.Id, + Path: ".", + }, + ArbitraryMetadataKeys: metadataKeys, + } + res, err := client.ListContainer(ctx, req) // FIXME public link depth infinity -> "gateway: could not find provider: gateway: error calling ListStorageProviders: rpc error: code = PermissionDenied desc = auth: core access token is invalid" + if err != nil { + log.Error().Err(err).Interface("info", info).Msg("error sending list container grpc request") + w.WriteHeader(http.StatusInternalServerError) + return nil, false + } + if res.Status.Code != rpc.Code_CODE_OK { + log.Debug().Interface("status", res.Status).Msg("List Container not ok, skipping") + continue + } + + // check sub-containers in reverse order and add them to the stack + // the reversed order here will produce a more logical sorting of results + for i := len(res.Infos) - 1; i >= 0; i-- { + // add path to resource + res.Infos[i].Path = filepath.Join(info.Path, res.Infos[i].Path) + res.Infos[i].Path = utils.MakeRelativePath(res.Infos[i].Path) + if res.Infos[i].Type == provider.ResourceType_RESOURCE_TYPE_CONTAINER { + stack = append(stack, res.Infos[i]) + } + } + + resourceInfos = append(resourceInfos, res.Infos...) + // TODO: stream response to avoid storing too many results in memory + // we can do that after having stated the root. + } + } + + return resourceInfos, true +} + +func metadataKeys(pf XML) []string { + + var metadataKeys []string + + if pf.Allprop != nil { + // TODO this changes the behavior and returns all properties if allprops has been set, + // but allprops should only return some default properties + // see https://tools.ietf.org/html/rfc4918#section-9.1 + // the description of arbitrary_metadata_keys in https://cs3org.github.io/cs3apis/#cs3.storage.provider.v1beta1.ListContainerRequest an others may need clarification + // tracked in https://github.com/cs3org/cs3apis/issues/104 + metadataKeys = append(metadataKeys, "*") + } else { + metadataKeys = make([]string, 0, len(pf.Prop)) + for i := range pf.Prop { + if requiresExplicitFetching(&pf.Prop[i]) { + metadataKeys = append(metadataKeys, metadataKeyOf(&pf.Prop[i])) + } + } + } + return metadataKeys +} + func addChild(childInfos map[string]*provider.ResourceInfo, spaceInfo *provider.ResourceInfo, requestPath string, @@ -687,10 +813,10 @@ func ReadPropfind(r io.Reader) (pf XML, status int, err error) { } // MultistatusResponse converts a list of resource infos into a multistatus response string -func MultistatusResponse(ctx context.Context, pf *XML, mds []*provider.ResourceInfo, publicURL, ns /*, spaceType*/ string, linkshares map[string]struct{}) ([]byte, error) { +func MultistatusResponse(ctx context.Context, pf *XML, mds []*provider.ResourceInfo, publicURL, ns string, linkshares map[string]struct{}) ([]byte, error) { responses := make([]*ResponseXML, 0, len(mds)) for i := range mds { - res, err := mdToPropResponse(ctx, pf, mds[i], publicURL, ns /*spaceType,*/, linkshares) + res, err := mdToPropResponse(ctx, pf, mds[i], publicURL, ns, linkshares) if err != nil { return nil, err } @@ -709,7 +835,7 @@ func MultistatusResponse(ctx context.Context, pf *XML, mds []*provider.ResourceI // mdToPropResponse converts the CS3 metadata into a webdav PropResponse // ns is the CS3 namespace that needs to be removed from the CS3 path before // prefixing it with the baseURI -func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, publicURL, ns /*, spaceType*/ string, linkshares map[string]struct{}) (*ResponseXML, error) { +func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, publicURL, ns string, linkshares map[string]struct{}) (*ResponseXML, error) { sublog := appctx.GetLogger(ctx).With().Interface("md", md).Str("ns", ns).Logger() md.Path = strings.TrimPrefix(md.Path, ns) @@ -757,7 +883,9 @@ func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, p role := conversions.RoleFromResourcePermissions(md.PermissionSet) - isShared := /*spaceType != _spaceTypeProject &&*/ !net.IsCurrentUserOwner(ctx, md.Owner) + // TODO we need a better way to determine if the webdav permissions should list S + // afaict when the storage space root info of the resource is a grant ... and not an actual space root. + isShared := md.Space != nil && md.Space.SpaceType != _spaceTypeProject && !net.IsCurrentUserOwner(ctx, md.Owner) var wdp string isPublic := ls != nil if md.PermissionSet != nil { diff --git a/internal/http/services/owncloud/ocdav/spacelookup/spacelookup.go b/internal/http/services/owncloud/ocdav/spacelookup/spacelookup.go index c7cbc7eb78..43d03e6c4d 100644 --- a/internal/http/services/owncloud/ocdav/spacelookup/spacelookup.go +++ b/internal/http/services/owncloud/ocdav/spacelookup/spacelookup.go @@ -31,6 +31,7 @@ import ( "github.com/cs3org/reva/v2/pkg/rgrpc/status" "github.com/cs3org/reva/v2/pkg/storagespace" "github.com/cs3org/reva/v2/pkg/utils" + "google.golang.org/protobuf/types/known/fieldmaskpb" ) // LookupReferenceForPath returns: @@ -100,10 +101,16 @@ func LookUpStorageSpacesForPathWithChildren(ctx context.Context, client gateway. lSSReq := &storageProvider.ListStorageSpacesRequest{ Opaque: &typesv1beta1.Opaque{ Map: map[string]*typesv1beta1.OpaqueEntry{ - "path": {Decoder: "plain", Value: []byte(path)}, - "withChildMounts": {Decoder: "plain", Value: []byte("true")}, + // TODO encode requested metadata as json + //"metadata": {Decoder: "json", Value: []byte("*")}, }}, + // get all fields, including root_info + FieldMask: &fieldmaskpb.FieldMask{Paths: []string{"*"}}, } + // list all providers at or below the given path + lSSReq.Opaque = utils.AppendPlainToOpaque(lSSReq.Opaque, "path", path) + // we want to get all metadata? really? when looking up the space roots we actually only want etag, mtime and type so we can construct a child ... + lSSReq.Opaque = utils.AppendPlainToOpaque(lSSReq.Opaque, "metadata", "*") lSSRes, err := client.ListStorageSpaces(ctx, lSSReq) if err != nil { From 6af44b17aa040d86e4608f4e0ed90d30e45573c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Tue, 28 Jun 2022 15:56:50 +0000 Subject: [PATCH 06/24] update propfind unit tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../owncloud/ocdav/propfind/propfind.go | 38 ++- .../owncloud/ocdav/propfind/propfind_test.go | 223 +++++++++++------- 2 files changed, 167 insertions(+), 94 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/propfind/propfind.go b/internal/http/services/owncloud/ocdav/propfind/propfind.go index 038a7e61f1..78a9c6bbc7 100644 --- a/internal/http/services/owncloud/ocdav/propfind/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind/propfind.go @@ -50,6 +50,7 @@ import ( "github.com/cs3org/reva/v2/pkg/storagespace" "github.com/cs3org/reva/v2/pkg/utils" "github.com/rs/zerolog" + "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" "google.golang.org/protobuf/types/known/fieldmaskpb" ) @@ -353,16 +354,23 @@ func (p *Handler) propfindResponse(ctx context.Context, w http.ResponseWriter, r var linkshares map[string]struct{} // public link access does not show share-types + // oc:share-type is not part of an allprops response if namespace != "/public" { - listResp, err := client.ListPublicShares(ctx, &link.ListPublicSharesRequest{Filters: filters}) - if err == nil { - linkshares = make(map[string]struct{}, len(listResp.Share)) - for i := range listResp.Share { - linkshares[listResp.Share[i].ResourceId.OpaqueId] = struct{}{} + // only fetch this if property was queried + for _, p := range pf.Prop { + if p.Space == net.NsOwncloud && p.Local == "share-type" { + listResp, err := client.ListPublicShares(ctx, &link.ListPublicSharesRequest{Filters: filters}) + if err == nil { + linkshares = make(map[string]struct{}, len(listResp.Share)) + for i := range listResp.Share { + linkshares[listResp.Share[i].ResourceId.OpaqueId] = struct{}{} + } + } else { + log.Error().Err(err).Msg("propfindResponse: couldn't list public shares") + span.SetStatus(codes.Error, err.Error()) + } } - } else { - log.Error().Err(err).Msg("propfindResponse: couldn't list public shares") - span.SetStatus(codes.Error, err.Error()) + break } } @@ -401,6 +409,10 @@ func (p *Handler) statSpace(ctx context.Context, client gateway.GatewayAPIClient } func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r *http.Request, pf XML, spaces []*provider.StorageSpace, requestPath string, log zerolog.Logger) ([]*provider.ResourceInfo, bool, bool) { + ctx, span := appctx.GetTracerProvider(r.Context()).Tracer(tracerName).Start(r.Context(), "get_resource_infos") + span.SetAttributes(attribute.KeyValue{Key: "requestPath", Value: attribute.StringValue(requestPath)}) + defer span.End() + dh := r.Header.Get(net.HeaderDepth) depth, err := net.ParseDepth(dh) if err != nil { @@ -411,6 +423,7 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r errors.HandleWebdavError(&log, w, b, err) return nil, false, false } + span.SetAttributes(attribute.KeyValue{Key: "depth", Value: attribute.StringValue(depth.String())}) client, err := p.getClient() if err != nil { @@ -617,6 +630,10 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r } func (p *Handler) getSpaceResourceInfos(ctx context.Context, w http.ResponseWriter, r *http.Request, pf XML, ref *provider.Reference, requestPath string, depth net.Depth, log zerolog.Logger) ([]*provider.ResourceInfo, bool) { + ctx, span := appctx.GetTracerProvider(r.Context()).Tracer(tracerName).Start(r.Context(), "get_space_resource_infos") + span.SetAttributes(attribute.KeyValue{Key: "requestPath", Value: attribute.StringValue(requestPath)}) + span.SetAttributes(attribute.KeyValue{Key: "depth", Value: attribute.StringValue(depth.String())}) + defer span.End() client, err := p.getClient() if err != nil { @@ -836,6 +853,11 @@ func MultistatusResponse(ctx context.Context, pf *XML, mds []*provider.ResourceI // ns is the CS3 namespace that needs to be removed from the CS3 path before // prefixing it with the baseURI func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, publicURL, ns string, linkshares map[string]struct{}) (*ResponseXML, error) { + ctx, span := appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "md_to_prop_response") + span.SetAttributes(attribute.KeyValue{Key: "publicURL", Value: attribute.StringValue(publicURL)}) + span.SetAttributes(attribute.KeyValue{Key: "ns", Value: attribute.StringValue(ns)}) + defer span.End() + sublog := appctx.GetLogger(ctx).With().Interface("md", md).Str("ns", ns).Logger() md.Path = strings.TrimPrefix(md.Path, ns) diff --git a/internal/http/services/owncloud/ocdav/propfind/propfind_test.go b/internal/http/services/owncloud/ocdav/propfind/propfind_test.go index f09b817e04..209387c8de 100644 --- a/internal/http/services/owncloud/ocdav/propfind/propfind_test.go +++ b/internal/http/services/owncloud/ocdav/propfind/propfind_test.go @@ -82,6 +82,20 @@ var _ = Describe("Propfind", func() { }, nil) } + foospace *sprovider.StorageSpace + fooquxspace *sprovider.StorageSpace + fooFileShareSpace *sprovider.StorageSpace + fooFileShare2Space *sprovider.StorageSpace + fooDirShareSpace *sprovider.StorageSpace + ) + + JustBeforeEach(func() { + ctx = context.WithValue(context.Background(), net.CtxKeyBaseURI, "http://127.0.0.1:3000") + client = &mocks.GatewayAPIClient{} + handler = propfind.NewHandler("127.0.0.1:3000", func() (gateway.GatewayAPIClient, error) { + return client, nil + }) + foospace = &sprovider.StorageSpace{ Opaque: &typesv1beta1.Opaque{ Map: map[string]*typesv1beta1.OpaqueEntry{ @@ -91,8 +105,8 @@ var _ = Describe("Propfind", func() { }, }, }, - Id: &sprovider.StorageSpaceId{OpaqueId: "foospace"}, - Root: &sprovider.ResourceId{OpaqueId: "foospaceroot"}, + Id: &sprovider.StorageSpaceId{OpaqueId: "foospace!root"}, + Root: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "root"}, Name: "foospace", } fooquxspace = &sprovider.StorageSpace{ @@ -104,8 +118,8 @@ var _ = Describe("Propfind", func() { }, }, }, - Id: &sprovider.StorageSpaceId{OpaqueId: "fooquxspace"}, - Root: &sprovider.ResourceId{OpaqueId: "fooquxspaceroot"}, + Id: &sprovider.StorageSpaceId{OpaqueId: "fooquxspace!root"}, + Root: &sprovider.ResourceId{StorageId: "fooquxspace", OpaqueId: "root"}, Name: "fooquxspace", } fooFileShareSpace = &sprovider.StorageSpace{ @@ -117,8 +131,8 @@ var _ = Describe("Propfind", func() { }, }, }, - Id: &sprovider.StorageSpaceId{OpaqueId: "fooFileShareSpace"}, - Root: &sprovider.ResourceId{OpaqueId: "sharedfile"}, + Id: &sprovider.StorageSpaceId{OpaqueId: "fooFileShareSpace!sharedfile"}, + Root: &sprovider.ResourceId{StorageId: "fooFileShareSpace", OpaqueId: "sharedfile"}, Name: "fooFileShareSpace", } fooFileShare2Space = &sprovider.StorageSpace{ @@ -130,8 +144,8 @@ var _ = Describe("Propfind", func() { }, }, }, - Id: &sprovider.StorageSpaceId{OpaqueId: "fooFileShareSpace2"}, - Root: &sprovider.ResourceId{OpaqueId: "sharedfile2"}, + Id: &sprovider.StorageSpaceId{OpaqueId: "fooFileShareSpace2!sharedfile2"}, + Root: &sprovider.ResourceId{StorageId: "fooFileShareSpace2", OpaqueId: "sharedfile2"}, Name: "fooFileShareSpace2", } fooDirShareSpace = &sprovider.StorageSpace{ @@ -143,80 +157,131 @@ var _ = Describe("Propfind", func() { }, }, }, - Id: &sprovider.StorageSpaceId{OpaqueId: "fooDirShareSpace"}, - Root: &sprovider.ResourceId{OpaqueId: "shareddir"}, + Id: &sprovider.StorageSpaceId{OpaqueId: "fooDirShareSpace!shareddir"}, + Root: &sprovider.ResourceId{StorageId: "fooDirShareSpace", OpaqueId: "shareddir"}, Name: "fooDirShareSpace", } - ) - - JustBeforeEach(func() { - ctx = context.WithValue(context.Background(), net.CtxKeyBaseURI, "http://127.0.0.1:3000") - client = &mocks.GatewayAPIClient{} - handler = propfind.NewHandler("127.0.0.1:3000", func() (gateway.GatewayAPIClient, error) { - return client, nil - }) - mockStat(&sprovider.Reference{ResourceId: &sprovider.ResourceId{OpaqueId: "foospaceroot"}, Path: "."}, + // For the space mounted a /foo we assign a storageid "foospace" and a root opaqueid "root" + // it contains four resources + // - ./bar, file, 100 bytes, opaqueid "bar" + // - ./baz, file, 1 byte, opaqueid "baz" + // - ./dir, folder, 30 bytes, opaqueid "dir" + // - ./dir/entry, file, 30 bytes, opaqueid "direntry" + mockStat(&sprovider.Reference{ResourceId: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "root"}, Path: "."}, &sprovider.ResourceInfo{ - Id: &sprovider.ResourceId{OpaqueId: "foospaceroot", StorageId: "foospaceroot"}, + Id: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "root"}, Type: sprovider.ResourceType_RESOURCE_TYPE_CONTAINER, Path: ".", Size: uint64(131), }) - mockListContainer(&sprovider.Reference{ResourceId: &sprovider.ResourceId{OpaqueId: "foospaceroot"}, Path: "."}, + mockListContainer(&sprovider.Reference{ResourceId: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "root"}, Path: "."}, []*sprovider.ResourceInfo{ { Type: sprovider.ResourceType_RESOURCE_TYPE_FILE, + Id: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "bar"}, Path: "bar", Size: 100, }, { Type: sprovider.ResourceType_RESOURCE_TYPE_FILE, + Id: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "baz"}, Path: "baz", Size: 1, }, { Type: sprovider.ResourceType_RESOURCE_TYPE_CONTAINER, + Id: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "dir"}, Path: "dir", Size: 30, }, }) - mockStat(&sprovider.Reference{ResourceId: &sprovider.ResourceId{OpaqueId: "foospaceroot"}, Path: "./bar"}, + mockStat(&sprovider.Reference{ResourceId: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "root"}, Path: "./bar"}, &sprovider.ResourceInfo{ - Id: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "foospacebar"}, + Id: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "bar"}, Type: sprovider.ResourceType_RESOURCE_TYPE_FILE, Path: "./bar", Size: uint64(100), }) - mockListContainer(&sprovider.Reference{ResourceId: &sprovider.ResourceId{OpaqueId: "foospaceroot"}, Path: "./dir"}, + mockStat(&sprovider.Reference{ResourceId: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "bar"}, Path: "."}, + &sprovider.ResourceInfo{ + Id: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "bar"}, + Type: sprovider.ResourceType_RESOURCE_TYPE_FILE, + Path: "./bar", + Size: uint64(100), + }) + mockStat(&sprovider.Reference{ResourceId: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "root"}, Path: "./baz"}, + &sprovider.ResourceInfo{ + Id: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "baz"}, + Type: sprovider.ResourceType_RESOURCE_TYPE_FILE, + Path: "./baz", + Size: uint64(1), + }) + mockStat(&sprovider.Reference{ResourceId: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "baz"}, Path: "."}, + &sprovider.ResourceInfo{ + Id: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "baz"}, + Type: sprovider.ResourceType_RESOURCE_TYPE_FILE, + Path: "./baz", + Size: uint64(1), + }) + mockStat(&sprovider.Reference{ResourceId: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "root"}, Path: "./dir"}, + &sprovider.ResourceInfo{ + Id: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "dir"}, + Type: sprovider.ResourceType_RESOURCE_TYPE_CONTAINER, + Path: "./dir", + Size: uint64(30), + }) + mockStat(&sprovider.Reference{ResourceId: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "dir"}, Path: "."}, + &sprovider.ResourceInfo{ + Id: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "dir"}, + Type: sprovider.ResourceType_RESOURCE_TYPE_CONTAINER, + Path: "./dir", + Size: uint64(30), + }) + mockListContainer(&sprovider.Reference{ResourceId: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "root"}, Path: "./dir"}, []*sprovider.ResourceInfo{ { - Id: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "dirent"}, + Id: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "direntry"}, + Type: sprovider.ResourceType_RESOURCE_TYPE_FILE, + Path: "entry", + Size: 30, + }, + }) + mockListContainer(&sprovider.Reference{ResourceId: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "dir"}, Path: "."}, + []*sprovider.ResourceInfo{ + { + Id: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "direntry"}, Type: sprovider.ResourceType_RESOURCE_TYPE_FILE, Path: "entry", Size: 30, }, }) - mockStat(&sprovider.Reference{ResourceId: &sprovider.ResourceId{OpaqueId: "fooquxspaceroot"}, Path: "."}, + // For the space mounted a /foo/qux we assign a storageid "foospace" and a root opaqueid "root" + // it contains one resource + // - ./quux, file, 1000 bytes, opaqueid "quux" + mockStat(&sprovider.Reference{ResourceId: &sprovider.ResourceId{StorageId: "fooquxspace", OpaqueId: "root"}, Path: "."}, &sprovider.ResourceInfo{ + Id: &sprovider.ResourceId{StorageId: "fooquxspace", OpaqueId: "root"}, Type: sprovider.ResourceType_RESOURCE_TYPE_CONTAINER, Path: ".", Size: uint64(1000), }) - mockListContainer(&sprovider.Reference{ResourceId: &sprovider.ResourceId{OpaqueId: "fooquxspaceroot"}, Path: "."}, + mockListContainer(&sprovider.Reference{ResourceId: &sprovider.ResourceId{StorageId: "fooquxspace", OpaqueId: "root"}, Path: "."}, []*sprovider.ResourceInfo{ { - Id: &sprovider.ResourceId{OpaqueId: "fooquxspaceroot", StorageId: "fooquxspaceroot"}, + Id: &sprovider.ResourceId{StorageId: "fooquxspace", OpaqueId: "quux"}, Type: sprovider.ResourceType_RESOURCE_TYPE_FILE, - Path: "quux", + Path: "./quux", Size: 1000, }, }) - mockStat(&sprovider.Reference{ResourceId: &sprovider.ResourceId{OpaqueId: "sharedfile"}, Path: "."}, + // For the space mounted a /foo/Shares/sharedFile we assign a storageid "fooFileShareSpace" and a root opaqueid "sharedfile" + // it is a file resource, 2000 bytes, opaqueid "sharedfile" + mockStat(&sprovider.Reference{ResourceId: &sprovider.ResourceId{StorageId: "fooFileShareSpace", OpaqueId: "sharedfile"}, Path: "."}, &sprovider.ResourceInfo{ - Id: &sprovider.ResourceId{OpaqueId: "sharedfile", StorageId: "sharedfile"}, + Id: &sprovider.ResourceId{StorageId: "fooFileShareSpace", OpaqueId: "sharedfile"}, Type: sprovider.ResourceType_RESOURCE_TYPE_FILE, Path: ".", Size: uint64(2000), @@ -224,28 +289,34 @@ var _ = Describe("Propfind", func() { Etag: "1", }) - mockStat(&sprovider.Reference{ResourceId: &sprovider.ResourceId{OpaqueId: "sharedfile2"}, Path: "."}, + // For the space mounted a /foo/Shares/sharedFile2 we assign a storageid "fooFileShareSpace2" and a root opaqueid "sharedfile2" + // it is a file resource, 2500 bytes, opaqueid "sharedfile2" + mockStat(&sprovider.Reference{ResourceId: &sprovider.ResourceId{StorageId: "fooFileShareSpace2", OpaqueId: "sharedfile2"}, Path: "."}, &sprovider.ResourceInfo{ - Id: &sprovider.ResourceId{OpaqueId: "sharedfile2", StorageId: "sharedfile2"}, + Id: &sprovider.ResourceId{StorageId: "fooFileShareSpace2", OpaqueId: "sharedfile2"}, Type: sprovider.ResourceType_RESOURCE_TYPE_FILE, Path: ".", Size: uint64(2500), Mtime: &typesv1beta1.Timestamp{Seconds: 2}, Etag: "2", }) - mockStat(&sprovider.Reference{ResourceId: &sprovider.ResourceId{OpaqueId: "shareddir"}, Path: "."}, + + // For the space mounted a /foo/Shares/sharedFile2 we assign a storageid "fooDirShareSpace" and a root opaqueid "shareddir" + // it is a folder containing one resource + // ./something, file, 1500 bytes, opaqueid "shareddirsomething" + mockStat(&sprovider.Reference{ResourceId: &sprovider.ResourceId{StorageId: "fooDirShareSpace", OpaqueId: "shareddir"}, Path: "."}, &sprovider.ResourceInfo{ - Id: &sprovider.ResourceId{OpaqueId: "shareddir", StorageId: "shareddir"}, + Id: &sprovider.ResourceId{StorageId: "fooDirShareSpace", OpaqueId: "shareddir"}, Type: sprovider.ResourceType_RESOURCE_TYPE_CONTAINER, Path: ".", Size: uint64(1500), Mtime: &typesv1beta1.Timestamp{Seconds: 3}, Etag: "3", }) - mockListContainer(&sprovider.Reference{ResourceId: &sprovider.ResourceId{OpaqueId: "shareddir"}, Path: "."}, + mockListContainer(&sprovider.Reference{ResourceId: &sprovider.ResourceId{StorageId: "fooDirShareSpace", OpaqueId: "shareddir"}, Path: "."}, []*sprovider.ResourceInfo{ { - Id: &sprovider.ResourceId{OpaqueId: "shareddir", StorageId: "shareddir"}, + Id: &sprovider.ResourceId{StorageId: "fooDirShareSpace", OpaqueId: "shareddirsomething"}, Type: sprovider.ResourceType_RESOURCE_TYPE_FILE, Path: "something", Size: 1500, @@ -261,11 +332,11 @@ var _ = Describe("Propfind", func() { } else { term := req.Filters[0].Term.(*link.ListPublicSharesRequest_Filter_ResourceId) switch { - case term != nil && term.ResourceId != nil && term.ResourceId.OpaqueId == "foospacebar": + case term != nil && term.ResourceId != nil && term.ResourceId.OpaqueId == "bar": shares = []*link.PublicShare{ { Id: &link.PublicShareId{OpaqueId: "share1"}, - ResourceId: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "foospacebar"}, + ResourceId: &sprovider.ResourceId{StorageId: "foospace", OpaqueId: "bar"}, }, } default: @@ -299,6 +370,13 @@ var _ = Describe("Propfind", func() { Status: status.NewOK(ctx), StorageSpaces: []*sprovider.StorageSpace{}, }, nil) + mockStat(&sprovider.Reference{ResourceId: &sprovider.ResourceId{OpaqueId: "foospace", StorageId: "foospace"}, Path: "."}, + &sprovider.ResourceInfo{ + Id: &sprovider.ResourceId{OpaqueId: "foospace", StorageId: "foospace"}, + Type: sprovider.ResourceType_RESOURCE_TYPE_CONTAINER, + Path: ".", + Size: uint64(131), + }) }) It("verifies the depth header", func() { @@ -550,7 +628,7 @@ var _ = Describe("Propfind", func() { Expect(rr.Code).To(Equal(http.StatusOK)) }) - It("mounts embedded spaces", func() { + FIt("mounts embedded spaces", func() { rr := httptest.NewRecorder() req, err := http.NewRequest("GET", "/foo", strings.NewReader("")) Expect(err).ToNot(HaveOccurred()) @@ -561,25 +639,9 @@ var _ = Describe("Propfind", func() { res, _, err := readResponse(rr.Result().Body) Expect(err).ToNot(HaveOccurred()) - Expect(len(res.Responses)).To(Equal(5)) - - root := res.Responses[0] - Expect(root.Href).To(Equal("http:/127.0.0.1:3000/foo/")) - Expect(string(root.Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("1131")) - - bar := res.Responses[1] - Expect(bar.Href).To(Equal("http:/127.0.0.1:3000/foo/bar")) - Expect(string(bar.Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("100")) - - baz := res.Responses[2] - Expect(baz.Href).To(Equal("http:/127.0.0.1:3000/foo/baz")) - Expect(string(baz.Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("1")) - - dir := res.Responses[3] - Expect(dir.Href).To(Equal("http:/127.0.0.1:3000/foo/dir/")) - Expect(string(dir.Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("30")) + Expect(len(res.Responses)).To(Equal(1)) - qux := res.Responses[4] + qux := res.Responses[0] Expect(qux.Href).To(Equal("http:/127.0.0.1:3000/foo/qux/")) Expect(string(qux.Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("1000")) }) @@ -595,7 +657,7 @@ var _ = Describe("Propfind", func() { res, _, err := readResponse(rr.Result().Body) Expect(err).ToNot(HaveOccurred()) - Expect(len(res.Responses)).To(Equal(2)) + Expect(len(res.Responses)).To(Equal(4)) qux := res.Responses[0] Expect(qux.Href).To(Equal("http:/127.0.0.1:3000/foo/qux/")) @@ -654,7 +716,7 @@ var _ = Describe("Propfind", func() { Expect(err).ToNot(HaveOccurred()) req = req.WithContext(ctx) - handler.HandleSpacesPropfind(rr, req, "foospaceroot") + handler.HandleSpacesPropfind(rr, req, "foospace!root") Expect(rr.Code).To(Equal(http.StatusMultiStatus)) res, _, err := readResponse(rr.Result().Body) @@ -662,19 +724,19 @@ var _ = Describe("Propfind", func() { Expect(len(res.Responses)).To(Equal(4)) root := res.Responses[0] - Expect(root.Href).To(Equal("http:/127.0.0.1:3000/foospaceroot/")) + Expect(root.Href).To(Equal("http:/127.0.0.1:3000/foospace%21root/")) Expect(string(root.Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("131")) bar := res.Responses[1] - Expect(bar.Href).To(Equal("http:/127.0.0.1:3000/foospaceroot/bar")) + Expect(bar.Href).To(Equal("http:/127.0.0.1:3000/foospace%21root/bar")) Expect(string(bar.Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("100")) baz := res.Responses[2] - Expect(baz.Href).To(Equal("http:/127.0.0.1:3000/foospaceroot/baz")) + Expect(baz.Href).To(Equal("http:/127.0.0.1:3000/foospace%21root/baz")) Expect(string(baz.Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("1")) dir := res.Responses[3] - Expect(dir.Href).To(Equal("http:/127.0.0.1:3000/foospaceroot/dir/")) + Expect(dir.Href).To(Equal("http:/127.0.0.1:3000/foospace%21root/dir/")) Expect(string(dir.Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("30")) }) @@ -684,7 +746,7 @@ var _ = Describe("Propfind", func() { Expect(err).ToNot(HaveOccurred()) req = req.WithContext(ctx) - handler.HandleSpacesPropfind(rr, req, "foospaceroot") + handler.HandleSpacesPropfind(rr, req, "foospace!root") Expect(rr.Code).To(Equal(http.StatusMultiStatus)) res, _, err := readResponse(rr.Result().Body) @@ -694,30 +756,19 @@ var _ = Describe("Propfind", func() { }) It("stats a directory", func() { - mockStat(&sprovider.Reference{Path: "./baz"}, &sprovider.ResourceInfo{ - Type: sprovider.ResourceType_RESOURCE_TYPE_CONTAINER, - Size: 50, - }) - mockListContainer(&sprovider.Reference{Path: "./baz"}, []*sprovider.ResourceInfo{ - { - Type: sprovider.ResourceType_RESOURCE_TYPE_CONTAINER, - Size: 50, - }, - }) - rr := httptest.NewRecorder() - req, err := http.NewRequest("GET", "/baz", strings.NewReader("")) + req, err := http.NewRequest("GET", "/dir", strings.NewReader("")) Expect(err).ToNot(HaveOccurred()) req = req.WithContext(ctx) - handler.HandleSpacesPropfind(rr, req, "foospaceroot") + handler.HandleSpacesPropfind(rr, req, "foospace!root") Expect(rr.Code).To(Equal(http.StatusMultiStatus)) res, _, err := readResponse(rr.Result().Body) Expect(err).ToNot(HaveOccurred()) Expect(len(res.Responses)).To(Equal(2)) - Expect(string(res.Responses[0].Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("50")) - Expect(string(res.Responses[1].Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("50")) + Expect(string(res.Responses[0].Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("30")) + Expect(string(res.Responses[1].Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("30")) }) It("includes all the things™ when depth is infinity", func() { @@ -727,7 +778,7 @@ var _ = Describe("Propfind", func() { Expect(err).ToNot(HaveOccurred()) req = req.WithContext(ctx) - handler.HandleSpacesPropfind(rr, req, "foospaceroot") + handler.HandleSpacesPropfind(rr, req, "foospace!root") Expect(rr.Code).To(Equal(http.StatusMultiStatus)) res, _, err := readResponse(rr.Result().Body) @@ -739,11 +790,11 @@ var _ = Describe("Propfind", func() { paths = append(paths, r.Href) } Expect(paths).To(ConsistOf( - "http:/127.0.0.1:3000/foospaceroot/", - "http:/127.0.0.1:3000/foospaceroot/bar", - "http:/127.0.0.1:3000/foospaceroot/baz", - "http:/127.0.0.1:3000/foospaceroot/dir/", - "http:/127.0.0.1:3000/foospaceroot/dir/entry", + "http:/127.0.0.1:3000/foospace%21root/", + "http:/127.0.0.1:3000/foospace%21root/bar", + "http:/127.0.0.1:3000/foospace%21root/baz", + "http:/127.0.0.1:3000/foospace%21root/dir/", + "http:/127.0.0.1:3000/foospace%21root/dir/entry", )) }) }) From 12e016458d6908b9b93ce145e6237408b5d5625c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 29 Jun 2022 07:50:47 +0000 Subject: [PATCH 07/24] fix moq propfind comparison in propfind tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../owncloud/ocdav/propfind/propfind_test.go | 29 +++++++++++++++---- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/propfind/propfind_test.go b/internal/http/services/owncloud/ocdav/propfind/propfind_test.go index 209387c8de..0816ed7ef6 100644 --- a/internal/http/services/owncloud/ocdav/propfind/propfind_test.go +++ b/internal/http/services/owncloud/ocdav/propfind/propfind_test.go @@ -34,6 +34,7 @@ import ( "github.com/cs3org/reva/v2/internal/http/services/owncloud/ocdav/net" "github.com/cs3org/reva/v2/internal/http/services/owncloud/ocdav/propfind" "github.com/cs3org/reva/v2/pkg/rgrpc/status" + "github.com/cs3org/reva/v2/pkg/utils" "github.com/cs3org/reva/v2/tests/cs3mocks/mocks" "github.com/stretchr/testify/mock" "google.golang.org/grpc" @@ -64,7 +65,7 @@ var _ = Describe("Propfind", func() { mockStat = func(ref *sprovider.Reference, info *sprovider.ResourceInfo) { client.On("Stat", mock.Anything, mock.MatchedBy(func(req *sprovider.StatRequest) bool { - return (ref.ResourceId.GetOpaqueId() == "" || req.Ref.ResourceId.GetOpaqueId() == ref.ResourceId.GetOpaqueId()) && + return utils.ResourceIDEqual(req.Ref.ResourceId, ref.ResourceId) && (ref.Path == "" || req.Ref.Path == ref.Path) })).Return(&sprovider.StatResponse{ Status: status.NewOK(ctx), @@ -73,7 +74,7 @@ var _ = Describe("Propfind", func() { } mockListContainer = func(ref *sprovider.Reference, infos []*sprovider.ResourceInfo) { client.On("ListContainer", mock.Anything, mock.MatchedBy(func(req *sprovider.ListContainerRequest) bool { - match := (ref.ResourceId.GetOpaqueId() == "" || req.Ref.ResourceId.GetOpaqueId() == ref.ResourceId.GetOpaqueId()) && + match := utils.ResourceIDEqual(req.Ref.ResourceId, ref.ResourceId) && (ref.Path == "" || req.Ref.Path == ref.Path) return match })).Return(&sprovider.ListContainerResponse{ @@ -628,7 +629,7 @@ var _ = Describe("Propfind", func() { Expect(rr.Code).To(Equal(http.StatusOK)) }) - FIt("mounts embedded spaces", func() { + It("mounts embedded spaces", func() { rr := httptest.NewRecorder() req, err := http.NewRequest("GET", "/foo", strings.NewReader("")) Expect(err).ToNot(HaveOccurred()) @@ -639,9 +640,25 @@ var _ = Describe("Propfind", func() { res, _, err := readResponse(rr.Result().Body) Expect(err).ToNot(HaveOccurred()) - Expect(len(res.Responses)).To(Equal(1)) + Expect(len(res.Responses)).To(Equal(5)) - qux := res.Responses[0] + root := res.Responses[0] + Expect(root.Href).To(Equal("http:/127.0.0.1:3000/foo/")) + Expect(string(root.Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("1131")) + + bar := res.Responses[1] + Expect(bar.Href).To(Equal("http:/127.0.0.1:3000/foo/bar")) + Expect(string(bar.Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("100")) + + baz := res.Responses[2] + Expect(baz.Href).To(Equal("http:/127.0.0.1:3000/foo/baz")) + Expect(string(baz.Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("1")) + + dir := res.Responses[3] + Expect(dir.Href).To(Equal("http:/127.0.0.1:3000/foo/dir/")) + Expect(string(dir.Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("30")) + + qux := res.Responses[4] Expect(qux.Href).To(Equal("http:/127.0.0.1:3000/foo/qux/")) Expect(string(qux.Propstat[0].Prop[0].InnerXML)).To(ContainSubstring("1000")) }) @@ -657,7 +674,7 @@ var _ = Describe("Propfind", func() { res, _, err := readResponse(rr.Result().Body) Expect(err).ToNot(HaveOccurred()) - Expect(len(res.Responses)).To(Equal(4)) + Expect(len(res.Responses)).To(Equal(2)) qux := res.Responses[0] Expect(qux.Href).To(Equal("http:/127.0.0.1:3000/foo/qux/")) From 33d9b5df2d11c2f02e6bf2b4b78654b412b7a3f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 29 Jun 2022 08:59:57 +0000 Subject: [PATCH 08/24] lint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../http/services/owncloud/ocdav/propfind/propfind.go | 9 ++++----- .../services/owncloud/ocdav/spacelookup/spacelookup.go | 2 +- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/propfind/propfind.go b/internal/http/services/owncloud/ocdav/propfind/propfind.go index 78a9c6bbc7..f4c5b18483 100644 --- a/internal/http/services/owncloud/ocdav/propfind/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind/propfind.go @@ -369,8 +369,8 @@ func (p *Handler) propfindResponse(ctx context.Context, w http.ResponseWriter, r log.Error().Err(err).Msg("propfindResponse: couldn't list public shares") span.SetStatus(codes.Error, err.Error()) } + break } - break } } @@ -409,14 +409,13 @@ func (p *Handler) statSpace(ctx context.Context, client gateway.GatewayAPIClient } func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r *http.Request, pf XML, spaces []*provider.StorageSpace, requestPath string, log zerolog.Logger) ([]*provider.ResourceInfo, bool, bool) { - ctx, span := appctx.GetTracerProvider(r.Context()).Tracer(tracerName).Start(r.Context(), "get_resource_infos") + ctx, span := appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "get_resource_infos") span.SetAttributes(attribute.KeyValue{Key: "requestPath", Value: attribute.StringValue(requestPath)}) defer span.End() dh := r.Header.Get(net.HeaderDepth) depth, err := net.ParseDepth(dh) if err != nil { - log.Debug().Str("depth", dh).Msg(err.Error()) w.WriteHeader(http.StatusBadRequest) m := fmt.Sprintf("Invalid Depth header value: %v", dh) b, err := errors.Marshal(http.StatusBadRequest, m, "") @@ -630,7 +629,7 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r } func (p *Handler) getSpaceResourceInfos(ctx context.Context, w http.ResponseWriter, r *http.Request, pf XML, ref *provider.Reference, requestPath string, depth net.Depth, log zerolog.Logger) ([]*provider.ResourceInfo, bool) { - ctx, span := appctx.GetTracerProvider(r.Context()).Tracer(tracerName).Start(r.Context(), "get_space_resource_infos") + ctx, span := appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "get_space_resource_infos") span.SetAttributes(attribute.KeyValue{Key: "requestPath", Value: attribute.StringValue(requestPath)}) span.SetAttributes(attribute.KeyValue{Key: "depth", Value: attribute.StringValue(depth.String())}) defer span.End() @@ -645,7 +644,7 @@ func (p *Handler) getSpaceResourceInfos(ctx context.Context, w http.ResponseWrit metadataKeys := metadataKeys(pf) // we need to prefix the path with / to make subsequent prefix matches work - //info.Path = filepath.Join("/", spaceRef.Path) + // info.Path = filepath.Join("/", spaceRef.Path) resourceInfos := []*provider.ResourceInfo{} diff --git a/internal/http/services/owncloud/ocdav/spacelookup/spacelookup.go b/internal/http/services/owncloud/ocdav/spacelookup/spacelookup.go index 43d03e6c4d..93a8bee8ae 100644 --- a/internal/http/services/owncloud/ocdav/spacelookup/spacelookup.go +++ b/internal/http/services/owncloud/ocdav/spacelookup/spacelookup.go @@ -102,7 +102,7 @@ func LookUpStorageSpacesForPathWithChildren(ctx context.Context, client gateway. Opaque: &typesv1beta1.Opaque{ Map: map[string]*typesv1beta1.OpaqueEntry{ // TODO encode requested metadata as json - //"metadata": {Decoder: "json", Value: []byte("*")}, + // "metadata": {Decoder: "json", Value: []byte("*")}, }}, // get all fields, including root_info FieldMask: &fieldmaskpb.FieldMask{Paths: []string{"*"}}, From e8801ba55ca7163d8ad5d91e9c85758d39d32476 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 29 Jun 2022 09:45:03 +0000 Subject: [PATCH 09/24] error handling and response codes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../owncloud/ocdav/propfind/propfind.go | 27 +++++++++++++-- .../http/services/owncloud/ocdav/proppatch.go | 34 +++++++++++-------- 2 files changed, 45 insertions(+), 16 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/propfind/propfind.go b/internal/http/services/owncloud/ocdav/propfind/propfind.go index f4c5b18483..c18ae5302a 100644 --- a/internal/http/services/owncloud/ocdav/propfind/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind/propfind.go @@ -46,6 +46,7 @@ import ( "github.com/cs3org/reva/v2/pkg/appctx" ctxpkg "github.com/cs3org/reva/v2/pkg/ctx" "github.com/cs3org/reva/v2/pkg/publicshare" + rstatus "github.com/cs3org/reva/v2/pkg/rgrpc/status" "github.com/cs3org/reva/v2/pkg/rhttp/router" "github.com/cs3org/reva/v2/pkg/storagespace" "github.com/cs3org/reva/v2/pkg/utils" @@ -250,7 +251,6 @@ func (p *Handler) HandleSpacesPropfind(w http.ResponseWriter, r *http.Request, s ref, err := spacelookup.MakeStorageSpaceReference(spaceID, r.URL.Path) if err != nil { - w.WriteHeader(http.StatusBadRequest) sublog.Debug().Msg("invalid space id") w.WriteHeader(http.StatusBadRequest) m := fmt.Sprintf("Invalid space id: %v", spaceID) @@ -278,7 +278,30 @@ func (p *Handler) HandleSpacesPropfind(w http.ResponseWriter, r *http.Request, s return } if res.Status.Code != rpc.Code_CODE_OK { - errors.HandleErrorStatus(&sublog, w, res.Status) + status := rstatus.HTTPStatusFromCode(res.Status.Code) + if res.Status.Code == rpc.Code_CODE_ABORTED { + // aborted is used for etag an lock mismatches, which translates to 412 + // in case a real Conflict response is needed, the calling code needs to send the header + status = http.StatusPreconditionFailed + } + m := res.Status.Message + if res.Status.Code == rpc.Code_CODE_PERMISSION_DENIED { + // check if user has access to resource + sRes, err := client.Stat(ctx, &provider.StatRequest{Ref: &provider.Reference{ResourceId: ref.GetResourceId()}}) + if err != nil { + sublog.Error().Err(err).Msg("error performing stat grpc request") + w.WriteHeader(http.StatusInternalServerError) + return + } + if sRes.Status.Code != rpc.Code_CODE_OK { + // return not found error so we dont leak existence of a space + status = http.StatusNotFound + m = fmt.Sprintf("%v not found", ref.Path) + } + } + w.WriteHeader(status) + b, err := errors.Marshal(status, m, "") + errors.HandleWebdavError(&sublog, w, b, err) return } var space *provider.StorageSpace diff --git a/internal/http/services/owncloud/ocdav/proppatch.go b/internal/http/services/owncloud/ocdav/proppatch.go index b4c9e948f3..afe940809f 100644 --- a/internal/http/services/owncloud/ocdav/proppatch.go +++ b/internal/http/services/owncloud/ocdav/proppatch.go @@ -180,6 +180,13 @@ func (s *svc) handleProppatch(ctx context.Context, w http.ResponseWriter, r *htt } if res.Status.Code != rpc.Code_CODE_OK { + status := rstatus.HTTPStatusFromCode(res.Status.Code) + if res.Status.Code == rpc.Code_CODE_ABORTED { + // aborted is used for etag an lock mismatches, which translates to 412 + // in case a real Conflict response is needed, the calling code needs to send the header + status = http.StatusPreconditionFailed + } + m := res.Status.Message if res.Status.Code == rpc.Code_CODE_PERMISSION_DENIED { // check if user has access to resource sRes, err := c.Stat(ctx, &provider.StatRequest{Ref: ref}) @@ -188,8 +195,6 @@ func (s *svc) handleProppatch(ctx context.Context, w http.ResponseWriter, r *htt w.WriteHeader(http.StatusInternalServerError) return nil, nil, false } - status := http.StatusForbidden - m := fmt.Sprintf("Permission denied to remove properties on resource %v", ref.Path) if sRes.Status.Code != rpc.Code_CODE_OK { // return not found error so we dont leak existence of a file // TODO hide permission failed for users without access in every kind of request @@ -197,12 +202,10 @@ func (s *svc) handleProppatch(ctx context.Context, w http.ResponseWriter, r *htt status = http.StatusNotFound m = fmt.Sprintf("%v not found", ref.Path) } - w.WriteHeader(status) - b, err := errors.Marshal(status, m, "") - errors.HandleWebdavError(&log, w, b, err) - return nil, nil, false } - errors.HandleErrorStatus(&log, w, res.Status) + w.WriteHeader(status) + b, err := errors.Marshal(status, m, "") + errors.HandleWebdavError(&log, w, b, err) return nil, nil, false } if key == "http://owncloud.org/ns/favorite" { @@ -229,6 +232,13 @@ func (s *svc) handleProppatch(ctx context.Context, w http.ResponseWriter, r *htt } if res.Status.Code != rpc.Code_CODE_OK { + status := rstatus.HTTPStatusFromCode(res.Status.Code) + if res.Status.Code == rpc.Code_CODE_ABORTED { + // aborted is used for etag an lock mismatches, which translates to 412 + // in case a real Conflict response is needed, the calling code needs to send the header + status = http.StatusPreconditionFailed + } + m := res.Status.Message if res.Status.Code == rpc.Code_CODE_PERMISSION_DENIED { // check if user has access to resource sRes, err := c.Stat(ctx, &provider.StatRequest{Ref: ref}) @@ -237,8 +247,6 @@ func (s *svc) handleProppatch(ctx context.Context, w http.ResponseWriter, r *htt w.WriteHeader(http.StatusInternalServerError) return nil, nil, false } - status := http.StatusForbidden - m := fmt.Sprintf("Permission denied to set properties on resource %v", ref.Path) if sRes.Status.Code != rpc.Code_CODE_OK { // return not found error so we dont leak existence of a file // TODO hide permission failed for users without access in every kind of request @@ -246,12 +254,10 @@ func (s *svc) handleProppatch(ctx context.Context, w http.ResponseWriter, r *htt status = http.StatusNotFound m = fmt.Sprintf("%v not found", ref.Path) } - w.WriteHeader(status) - b, err := errors.Marshal(status, m, "") - errors.HandleWebdavError(&log, w, b, err) - return nil, nil, false } - errors.HandleErrorStatus(&log, w, res.Status) + w.WriteHeader(status) + b, err := errors.Marshal(status, m, "") + errors.HandleWebdavError(&log, w, b, err) return nil, nil, false } From 576a476cb10e04a245cc98d2694226db46dbe074 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 29 Jun 2022 10:03:47 +0000 Subject: [PATCH 10/24] remove commented code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../services/owncloud/ocdav/propfind/propfind.go | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/propfind/propfind.go b/internal/http/services/owncloud/ocdav/propfind/propfind.go index c18ae5302a..c70f547bb7 100644 --- a/internal/http/services/owncloud/ocdav/propfind/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind/propfind.go @@ -1542,17 +1542,3 @@ func (pn *Props) UnmarshalXML(d *xml.Decoder, start xml.StartElement) error { } } } - -// isVirtualRootResourceID returns true if the id points to the share jail root. The providerid is optional for legacy ids -/* -func isVirtualRootResourceID(id *provider.ResourceId) bool { - switch { - case id == nil: - return false - case id.OpaqueId != utils.ShareStorageProviderID: - return false - } - providerID, spaceID := storagespace.SplitStorageID(id.StorageId) - return spaceID == utils.ShareStorageProviderID && (providerID == "" || providerID == utils.ShareStorageProviderID) -} -*/ From 28c7531316a516ad73687d2e73598496c0af372a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 29 Jun 2022 10:18:58 +0000 Subject: [PATCH 11/24] typos, mimic oc10 not found error message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- internal/http/services/owncloud/ocdav/delete.go | 2 +- internal/http/services/owncloud/ocdav/propfind/propfind.go | 2 +- internal/http/services/owncloud/ocdav/proppatch.go | 4 ++-- pkg/storage/utils/decomposedfs/node/node.go | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/delete.go b/internal/http/services/owncloud/ocdav/delete.go index 168d398f64..7252888aa6 100644 --- a/internal/http/services/owncloud/ocdav/delete.go +++ b/internal/http/services/owncloud/ocdav/delete.go @@ -124,7 +124,7 @@ func (s *svc) handleDelete(ctx context.Context, w http.ResponseWriter, r *http.R // TODO should this be done in the driver? status = http.StatusNotFound // TODO path might be empty or relative... - m = fmt.Sprintf("%s not fount", ref.Path) + m = fmt.Sprintf("%s not found", ref.Path) } w.WriteHeader(status) b, err := errors.Marshal(status, m, "") diff --git a/internal/http/services/owncloud/ocdav/propfind/propfind.go b/internal/http/services/owncloud/ocdav/propfind/propfind.go index c70f547bb7..c07c1b85c2 100644 --- a/internal/http/services/owncloud/ocdav/propfind/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind/propfind.go @@ -296,7 +296,7 @@ func (p *Handler) HandleSpacesPropfind(w http.ResponseWriter, r *http.Request, s if sRes.Status.Code != rpc.Code_CODE_OK { // return not found error so we dont leak existence of a space status = http.StatusNotFound - m = fmt.Sprintf("%v not found", ref.Path) + m = "Resource not found" // mimic the oc10 error message } } w.WriteHeader(status) diff --git a/internal/http/services/owncloud/ocdav/proppatch.go b/internal/http/services/owncloud/ocdav/proppatch.go index afe940809f..00849a4499 100644 --- a/internal/http/services/owncloud/ocdav/proppatch.go +++ b/internal/http/services/owncloud/ocdav/proppatch.go @@ -200,7 +200,7 @@ func (s *svc) handleProppatch(ctx context.Context, w http.ResponseWriter, r *htt // TODO hide permission failed for users without access in every kind of request // TODO should this be done in the driver? status = http.StatusNotFound - m = fmt.Sprintf("%v not found", ref.Path) + m = "Resource not found" // mimic the oc10 error message } } w.WriteHeader(status) @@ -252,7 +252,7 @@ func (s *svc) handleProppatch(ctx context.Context, w http.ResponseWriter, r *htt // TODO hide permission failed for users without access in every kind of request // TODO should this be done in the driver? status = http.StatusNotFound - m = fmt.Sprintf("%v not found", ref.Path) + m = "Resource not found" // mimic the oc10 error message } } w.WriteHeader(status) diff --git a/pkg/storage/utils/decomposedfs/node/node.go b/pkg/storage/utils/decomposedfs/node/node.go index eae80b1ce1..198f290146 100644 --- a/pkg/storage/utils/decomposedfs/node/node.go +++ b/pkg/storage/utils/decomposedfs/node/node.go @@ -784,7 +784,7 @@ func readChecksumIntoResourceChecksum(ctx context.Context, nodePath, algo string case xattrs.IsAttrUnset(err): appctx.GetLogger(ctx).Debug().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("checksum not set") case xattrs.IsNotExist(err): - appctx.GetLogger(ctx).Error().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("file not fount") + appctx.GetLogger(ctx).Error().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("file not found") default: appctx.GetLogger(ctx).Error().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("could not read checksum") } @@ -806,7 +806,7 @@ func readChecksumIntoOpaque(ctx context.Context, nodePath, algo string, ri *prov case xattrs.IsAttrUnset(err): appctx.GetLogger(ctx).Debug().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("checksum not set") case xattrs.IsNotExist(err): - appctx.GetLogger(ctx).Error().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("file not fount") + appctx.GetLogger(ctx).Error().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("file not found") default: appctx.GetLogger(ctx).Error().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("could not read checksum") } From 9cecf95120327d05647f505ebc360a60ae739984 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 29 Jun 2022 14:40:39 +0000 Subject: [PATCH 12/24] add fieldmask to ListContainer and Stat MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../grpc/services/gateway/storageprovider.go | 8 +- .../services/gateway/storageprovidercache.go | 7 +- .../storageprovider/storageprovider.go | 8 +- .../owncloud/ocdav/propfind/propfind.go | 92 +++++++++++++------ .../storage/eoshomewrapper/eoshomewrapper.go | 8 +- pkg/cbox/storage/eoswrapper/eoswrapper.go | 10 +- pkg/rhttp/datatx/utils/download/download.go | 2 +- pkg/storage/fs/nextcloud/nextcloud.go | 4 +- pkg/storage/fs/nextcloud/nextcloud_test.go | 2 +- pkg/storage/fs/owncloudsql/owncloudsql.go | 4 +- pkg/storage/fs/s3/s3.go | 4 +- pkg/storage/storage.go | 4 +- .../utils/decomposedfs/decomposedfs.go | 11 ++- .../decomposedfs_concurrency_test.go | 2 +- pkg/storage/utils/decomposedfs/node/node.go | 70 +++++++++----- .../utils/decomposedfs/node/node_test.go | 8 +- .../utils/decomposedfs/tree/tree_test.go | 4 +- pkg/storage/utils/eosfs/eosfs.go | 14 +-- pkg/storage/utils/eosfs/spaces.go | 2 +- pkg/storage/utils/localfs/localfs.go | 4 +- 20 files changed, 169 insertions(+), 99 deletions(-) diff --git a/internal/grpc/services/gateway/storageprovider.go b/internal/grpc/services/gateway/storageprovider.go index 1b6b3b8dcb..23dfa13e9c 100644 --- a/internal/grpc/services/gateway/storageprovider.go +++ b/internal/grpc/services/gateway/storageprovider.go @@ -837,7 +837,12 @@ func (s *svc) Stat(ctx context.Context, req *provider.StatRequest) (*provider.St }, nil } - return c.Stat(ctx, &provider.StatRequest{Opaque: req.Opaque, Ref: ref, ArbitraryMetadataKeys: req.ArbitraryMetadataKeys}) + return c.Stat(ctx, &provider.StatRequest{ + Opaque: req.Opaque, + Ref: ref, + ArbitraryMetadataKeys: req.ArbitraryMetadataKeys, + FieldMask: req.FieldMask, + }) } func (s *svc) ListContainerStream(_ *provider.ListContainerStreamRequest, _ gateway.GatewayAPI_ListContainerStreamServer) error { @@ -858,6 +863,7 @@ func (s *svc) ListContainer(ctx context.Context, req *provider.ListContainerRequ Opaque: req.Opaque, Ref: ref, ArbitraryMetadataKeys: req.ArbitraryMetadataKeys, + FieldMask: req.FieldMask, }) } diff --git a/internal/grpc/services/gateway/storageprovidercache.go b/internal/grpc/services/gateway/storageprovidercache.go index 916135446b..54d97d0061 100644 --- a/internal/grpc/services/gateway/storageprovidercache.go +++ b/internal/grpc/services/gateway/storageprovidercache.go @@ -229,7 +229,7 @@ type cachedAPIClient struct { // generates a user specific key pointing to ref - used for statcache // a key looks like: uid:1234-1233!sid:5678-5677!oid:9923-9934!path:/path/to/source // as you see it adds "uid:"/"sid:"/"oid:" prefixes to the uuids so they can be differentiated -func statKey(user *userpb.User, ref *provider.Reference, metaDataKeys []string) string { +func statKey(user *userpb.User, ref *provider.Reference, metaDataKeys, fieldMaskPaths []string) string { if ref == nil || ref.ResourceId == nil || ref.ResourceId.StorageId == "" { return "" } @@ -238,6 +238,9 @@ func statKey(user *userpb.User, ref *provider.Reference, metaDataKeys []string) for _, k := range metaDataKeys { key += "!mdk:" + k } + for _, k := range fieldMaskPaths { + key += "!fmp:" + k + } return key } @@ -246,7 +249,7 @@ func statKey(user *userpb.User, ref *provider.Reference, metaDataKeys []string) func (c *cachedAPIClient) Stat(ctx context.Context, in *provider.StatRequest, opts ...grpc.CallOption) (*provider.StatResponse, error) { cache := c.caches[stat] - key := statKey(ctxpkg.ContextMustGetUser(ctx), in.Ref, in.ArbitraryMetadataKeys) + key := statKey(ctxpkg.ContextMustGetUser(ctx), in.GetRef(), in.GetArbitraryMetadataKeys(), in.GetFieldMask().GetPaths()) if key != "" { s := &provider.StatResponse{} if err := pullFromCache(cache, key, s); err == nil { diff --git a/internal/grpc/services/storageprovider/storageprovider.go b/internal/grpc/services/storageprovider/storageprovider.go index 63c955043d..99248a7655 100644 --- a/internal/grpc/services/storageprovider/storageprovider.go +++ b/internal/grpc/services/storageprovider/storageprovider.go @@ -672,7 +672,7 @@ func (s *service) Delete(ctx context.Context, req *provider.DeleteRequest) (*pro } } - md, err := s.storage.GetMD(ctx, req.Ref, []string{}) + md, err := s.storage.GetMD(ctx, req.Ref, nil, []string{"id"}) if err != nil { return &provider.DeleteResponse{ Status: status.NewStatusFromErrType(ctx, "can't stat resource to delete", err), @@ -718,7 +718,7 @@ func (s *service) Stat(ctx context.Context, req *provider.StatRequest) (*provide Value: attribute.StringValue(req.Ref.String()), }) - md, err := s.storage.GetMD(ctx, req.Ref, req.ArbitraryMetadataKeys) + md, err := s.storage.GetMD(ctx, req.GetRef(), req.GetArbitraryMetadataKeys(), req.GetFieldMask().GetPaths()) if err != nil { return &provider.StatResponse{ Status: status.NewStatusFromErrType(ctx, "stat", err), @@ -747,7 +747,7 @@ func (s *service) ListContainerStream(req *provider.ListContainerStreamRequest, ctx := ss.Context() log := appctx.GetLogger(ctx) - mds, err := s.storage.ListFolder(ctx, req.Ref, req.ArbitraryMetadataKeys) + mds, err := s.storage.ListFolder(ctx, req.GetRef(), req.GetArbitraryMetadataKeys(), req.GetFieldMask().GetPaths()) if err != nil { var st *rpc.Status switch err.(type) { @@ -792,7 +792,7 @@ func (s *service) ListContainer(ctx context.Context, req *provider.ListContainer providerID := unwrapProviderID(req.Ref.GetResourceId()) defer rewrapProviderID(req.Ref.GetResourceId(), providerID) - mds, err := s.storage.ListFolder(ctx, req.Ref, req.ArbitraryMetadataKeys) + mds, err := s.storage.ListFolder(ctx, req.GetRef(), req.GetArbitraryMetadataKeys(), req.GetFieldMask().GetPaths()) res := &provider.ListContainerResponse{ Status: status.NewStatusFromErrType(ctx, "list container", err), Infos: mds, diff --git a/internal/http/services/owncloud/ocdav/propfind/propfind.go b/internal/http/services/owncloud/ocdav/propfind/propfind.go index c07c1b85c2..ede4943487 100644 --- a/internal/http/services/owncloud/ocdav/propfind/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind/propfind.go @@ -266,11 +266,13 @@ func (p *Handler) HandleSpacesPropfind(w http.ResponseWriter, r *http.Request, s return } + metadataKeys, _ := metadataKeys(pf) + // stat the reference and request the space in the field mask res, err := client.Stat(ctx, &provider.StatRequest{ Ref: &ref, - ArbitraryMetadataKeys: metadataKeys(pf), - FieldMask: &fieldmaskpb.FieldMask{Paths: []string{"*"}}, + ArbitraryMetadataKeys: metadataKeys, + FieldMask: &fieldmaskpb.FieldMask{Paths: []string{"*"}}, // TODO use more sophisticated filter }) if err != nil { sublog.Error().Err(err).Msg("error getting grpc client") @@ -381,7 +383,7 @@ func (p *Handler) propfindResponse(ctx context.Context, w http.ResponseWriter, r if namespace != "/public" { // only fetch this if property was queried for _, p := range pf.Prop { - if p.Space == net.NsOwncloud && p.Local == "share-type" { + if p.Space == net.NsOwncloud && (p.Local == "share-types" || p.Local == "permissions") { listResp, err := client.ListPublicShares(ctx, &link.ListPublicSharesRequest{Filters: filters}) if err == nil { linkshares = make(map[string]struct{}, len(listResp.Share)) @@ -419,10 +421,11 @@ func (p *Handler) propfindResponse(ctx context.Context, w http.ResponseWriter, r } // TODO this is just a stat -> rename -func (p *Handler) statSpace(ctx context.Context, client gateway.GatewayAPIClient, ref *provider.Reference, metadataKeys []string) (*provider.ResourceInfo, *rpc.Status, error) { +func (p *Handler) statSpace(ctx context.Context, client gateway.GatewayAPIClient, ref *provider.Reference, metadataKeys, fieldMaskPaths []string) (*provider.ResourceInfo, *rpc.Status, error) { req := &provider.StatRequest{ Ref: ref, ArbitraryMetadataKeys: metadataKeys, + FieldMask: &fieldmaskpb.FieldMask{Paths: fieldMaskPaths}, } res, err := client.Stat(ctx, req) if err != nil { @@ -454,7 +457,7 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r return nil, false, false } - metadataKeys := metadataKeys(pf) + metadataKeys, fieldMaskPaths := metadataKeys(pf) // we need to stat all spaces to aggregate the root etag, mtime and size // TODO cache per space (hah, no longer per user + per space!) @@ -474,7 +477,7 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r if err != nil { continue } - info, status, err := p.statSpace(ctx, client, &spaceRef, metadataKeys) + info, status, err := p.statSpace(ctx, client, &spaceRef, metadataKeys, fieldMaskPaths) if err != nil || status.GetCode() != rpc.Code_CODE_OK { continue } @@ -492,7 +495,7 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r info = space.RootInfo } else { var status *rpc.Status - info, status, err = p.statSpace(ctx, client, spaceRef, metadataKeys) + info, status, err = p.statSpace(ctx, client, spaceRef, metadataKeys, fieldMaskPaths) if err != nil || status.GetCode() != rpc.Code_CODE_OK { continue } @@ -664,7 +667,7 @@ func (p *Handler) getSpaceResourceInfos(ctx context.Context, w http.ResponseWrit return nil, false } - metadataKeys := metadataKeys(pf) + metadataKeys, _ := metadataKeys(pf) // we need to prefix the path with / to make subsequent prefix matches work // info.Path = filepath.Join("/", spaceRef.Path) @@ -674,6 +677,7 @@ func (p *Handler) getSpaceResourceInfos(ctx context.Context, w http.ResponseWrit req := &provider.ListContainerRequest{ Ref: ref, ArbitraryMetadataKeys: metadataKeys, + FieldMask: &fieldmaskpb.FieldMask{Paths: []string{"*"}}, // TODO use more sophisticated filter } res, err := client.ListContainer(ctx, req) if err != nil { @@ -739,9 +743,11 @@ func (p *Handler) getSpaceResourceInfos(ctx context.Context, w http.ResponseWrit return resourceInfos, true } -func metadataKeys(pf XML) []string { +// metadataKeys splits the propfind properties into arbitrary metadata and ResourceInfo field mask paths +func metadataKeys(pf XML) ([]string, []string) { var metadataKeys []string + var fieldMaskKeys []string if pf.Allprop != nil { // TODO this changes the behavior and returns all properties if allprops has been set, @@ -750,15 +756,24 @@ func metadataKeys(pf XML) []string { // the description of arbitrary_metadata_keys in https://cs3org.github.io/cs3apis/#cs3.storage.provider.v1beta1.ListContainerRequest an others may need clarification // tracked in https://github.com/cs3org/cs3apis/issues/104 metadataKeys = append(metadataKeys, "*") + fieldMaskKeys = append(fieldMaskKeys, "*") } else { metadataKeys = make([]string, 0, len(pf.Prop)) + fieldMaskKeys = make([]string, 0, len(pf.Prop)) for i := range pf.Prop { if requiresExplicitFetching(&pf.Prop[i]) { - metadataKeys = append(metadataKeys, metadataKeyOf(&pf.Prop[i])) + key := metadataKeyOf(&pf.Prop[i]) + switch key { + case "share-types": + fieldMaskKeys = append(fieldMaskKeys, key) + default: + metadataKeys = append(metadataKeys, key) + } + } } } - return metadataKeys + return metadataKeys, fieldMaskKeys } func addChild(childInfos map[string]*provider.ResourceInfo, @@ -903,6 +918,7 @@ func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, p quota := net.PropQuotaUnknown size := strconv.FormatUint(md.Size, 10) var lock *provider.Lock + shareTypes := "" // TODO refactor helper functions: GetOpaqueJSONEncoded(opaque, key string, *struct) err, GetOpaquePlainEncoded(opaque, key) value, err // or use ok like pattern and return bool? if md.Opaque != nil && md.Opaque.Map != nil { @@ -913,8 +929,8 @@ func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, p sublog.Error().Err(err).Msg("could not unmarshal link json") } } - if md.Opaque.Map["quota"] != nil && md.Opaque.Map["quota"].Decoder == "plain" { - quota = string(md.Opaque.Map["quota"].Value) + if quota := utils.ReadPlainFromOpaque(md.Opaque, "quota"); quota == "" { + quota = net.PropQuotaUnknown } if md.Opaque.Map["lock"] != nil && md.Opaque.Map["lock"].Decoder == "json" { lock = &provider.Lock{} @@ -923,23 +939,24 @@ func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, p sublog.Error().Err(err).Msg("could not unmarshal locks json") } } + shareTypes = utils.ReadPlainFromOpaque(md.Opaque, "share-types") } role := conversions.RoleFromResourcePermissions(md.PermissionSet) - // TODO we need a better way to determine if the webdav permissions should list S - // afaict when the storage space root info of the resource is a grant ... and not an actual space root. - isShared := md.Space != nil && md.Space.SpaceType != _spaceTypeProject && !net.IsCurrentUserOwner(ctx, md.Owner) + if md.Space != nil && md.Space.SpaceType != "grant" && utils.ResourceIDEqual(md.Space.Root, md.Id) { + // a space root is never shared + shareTypes = "" + } var wdp string isPublic := ls != nil if md.PermissionSet != nil { wdp = role.WebDAVPermissions( md.Type == provider.ResourceType_RESOURCE_TYPE_CONTAINER, - isShared, + shareTypes != "", false, isPublic, ) - sublog.Debug().Interface("role", role).Str("dav-permissions", wdp).Msg("converted PermissionSet") } // replace fileid of /public/{token} mountpoint with grant fileid @@ -1217,12 +1234,22 @@ func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, p } case "share-types": // desktop var types strings.Builder - k := md.GetArbitraryMetadata() - amd := k.GetMetadata() - if amdv, ok := amd[metadataKeyOf(&pf.Prop[i])]; ok { - types.WriteString("") - types.WriteString(amdv) - types.WriteString("") + + sts := strings.Split(shareTypes, ",") + for _, shareType := range sts { + // TODO implement conversion + switch shareType { + case "1": // provider.GranteeType_GRANTEE_TYPE_USER + types.WriteString("") + types.WriteString(strconv.Itoa(int(conversions.ShareTypeUser))) + types.WriteString("") + case "2": // provider.GranteeType_GRANTEE_TYPE_GROUP + types.WriteString("") + types.WriteString(strconv.Itoa(int(conversions.ShareTypeGroup))) + types.WriteString("") + default: + sublog.Warn().Interface("shareType", shareType).Msg("unknown share type") + } } if md.Id != nil { @@ -1503,12 +1530,19 @@ func (c *countingReader) Read(p []byte) (int, error) { } func metadataKeyOf(n *xml.Name) string { - switch { - case n.Space == net.NsDav && n.Local == "quota-available-bytes": - return "quota" - default: - return fmt.Sprintf("%s/%s", n.Space, n.Local) + switch n.Space { + case net.NsDav: + switch n.Local { + case "quota-available-bytes": + return "quota" + } + case net.NsOwncloud: + switch n.Local { + case "share-types": + return "share-types" + } } + return fmt.Sprintf("%s/%s", n.Space, n.Local) } // UnmarshalXML appends the property names enclosed within start to pn. diff --git a/pkg/cbox/storage/eoshomewrapper/eoshomewrapper.go b/pkg/cbox/storage/eoshomewrapper/eoshomewrapper.go index 1813a8e732..3e46f65104 100644 --- a/pkg/cbox/storage/eoshomewrapper/eoshomewrapper.go +++ b/pkg/cbox/storage/eoshomewrapper/eoshomewrapper.go @@ -88,8 +88,8 @@ func New(m map[string]interface{}) (storage.FS, error) { // We need to override the two methods, GetMD and ListFolder to fill the // StorageId in the ResourceInfo objects. -func (w *wrapper) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []string) (*provider.ResourceInfo, error) { - res, err := w.FS.GetMD(ctx, ref, mdKeys) +func (w *wrapper) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []string, fieldMask []string) (*provider.ResourceInfo, error) { + res, err := w.FS.GetMD(ctx, ref, mdKeys, fieldMask) if err != nil { return nil, err } @@ -102,8 +102,8 @@ func (w *wrapper) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []s return res, nil } -func (w *wrapper) ListFolder(ctx context.Context, ref *provider.Reference, mdKeys []string) ([]*provider.ResourceInfo, error) { - res, err := w.FS.ListFolder(ctx, ref, mdKeys) +func (w *wrapper) ListFolder(ctx context.Context, ref *provider.Reference, mdKeys, fieldMask []string) ([]*provider.ResourceInfo, error) { + res, err := w.FS.ListFolder(ctx, ref, mdKeys, fieldMask) if err != nil { return nil, err } diff --git a/pkg/cbox/storage/eoswrapper/eoswrapper.go b/pkg/cbox/storage/eoswrapper/eoswrapper.go index bf247a1c28..3c7cad877f 100644 --- a/pkg/cbox/storage/eoswrapper/eoswrapper.go +++ b/pkg/cbox/storage/eoswrapper/eoswrapper.go @@ -107,8 +107,8 @@ func New(m map[string]interface{}) (storage.FS, error) { // We need to override the methods, GetMD, GetPathByID and ListFolder to fill the // StorageId in the ResourceInfo objects. -func (w *wrapper) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []string) (*provider.ResourceInfo, error) { - res, err := w.FS.GetMD(ctx, ref, mdKeys) +func (w *wrapper) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []string, fieldMask []string) (*provider.ResourceInfo, error) { + res, err := w.FS.GetMD(ctx, ref, mdKeys, fieldMask) if err != nil { return nil, err } @@ -132,8 +132,8 @@ func (w *wrapper) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []s return res, nil } -func (w *wrapper) ListFolder(ctx context.Context, ref *provider.Reference, mdKeys []string) ([]*provider.ResourceInfo, error) { - res, err := w.FS.ListFolder(ctx, ref, mdKeys) +func (w *wrapper) ListFolder(ctx context.Context, ref *provider.Reference, mdKeys, fieldMask []string) ([]*provider.ResourceInfo, error) { + res, err := w.FS.ListFolder(ctx, ref, mdKeys, fieldMask) if err != nil { return nil, err } @@ -272,7 +272,7 @@ func (w *wrapper) userIsProjectAdmin(ctx context.Context, ref *provider.Referenc return nil } - res, err := w.FS.GetMD(ctx, ref, nil) + res, err := w.FS.GetMD(ctx, ref, nil, nil) if err != nil { return err } diff --git a/pkg/rhttp/datatx/utils/download/download.go b/pkg/rhttp/datatx/utils/download/download.go index 356545bab9..77f6960cc5 100644 --- a/pkg/rhttp/datatx/utils/download/download.go +++ b/pkg/rhttp/datatx/utils/download/download.go @@ -69,7 +69,7 @@ func GetOrHeadFile(w http.ResponseWriter, r *http.Request, fs storage.FS, spaceI // do a stat to set a Content-Length header - if md, err = fs.GetMD(ctx, ref, nil); err != nil { + if md, err = fs.GetMD(ctx, ref, nil, []string{"size", "mimetype"}); err != nil { handleError(w, &sublog, err, "stat") return } diff --git a/pkg/storage/fs/nextcloud/nextcloud.go b/pkg/storage/fs/nextcloud/nextcloud.go index 94cf4be949..20803d8eec 100644 --- a/pkg/storage/fs/nextcloud/nextcloud.go +++ b/pkg/storage/fs/nextcloud/nextcloud.go @@ -301,7 +301,7 @@ func (nc *StorageDriver) Move(ctx context.Context, oldRef, newRef *provider.Refe } // GetMD as defined in the storage.FS interface -func (nc *StorageDriver) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []string) (*provider.ResourceInfo, error) { +func (nc *StorageDriver) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []string, fieldMask []string) (*provider.ResourceInfo, error) { type paramsObj struct { Ref *provider.Reference `json:"ref"` MdKeys []string `json:"mdKeys"` @@ -331,7 +331,7 @@ func (nc *StorageDriver) GetMD(ctx context.Context, ref *provider.Reference, mdK } // ListFolder as defined in the storage.FS interface -func (nc *StorageDriver) ListFolder(ctx context.Context, ref *provider.Reference, mdKeys []string) ([]*provider.ResourceInfo, error) { +func (nc *StorageDriver) ListFolder(ctx context.Context, ref *provider.Reference, mdKeys, fieldMask []string) ([]*provider.ResourceInfo, error) { type paramsObj struct { Ref *provider.Reference `json:"ref"` MdKeys []string `json:"mdKeys"` diff --git a/pkg/storage/fs/nextcloud/nextcloud_test.go b/pkg/storage/fs/nextcloud/nextcloud_test.go index 4852eb3122..cd41e96cfa 100644 --- a/pkg/storage/fs/nextcloud/nextcloud_test.go +++ b/pkg/storage/fs/nextcloud/nextcloud_test.go @@ -224,7 +224,7 @@ var _ = Describe("Nextcloud", func() { Path: "/some/path", } mdKeys := []string{"val1", "val2", "val3"} - result, err := nc.GetMD(ctx, ref, mdKeys) + result, err := nc.GetMD(ctx, ref, mdKeys, nil) Expect(err).ToNot(HaveOccurred()) Expect(*result).To(Equal(provider.ResourceInfo{ Opaque: &types.Opaque{ diff --git a/pkg/storage/fs/owncloudsql/owncloudsql.go b/pkg/storage/fs/owncloudsql/owncloudsql.go index 86d5e5deef..41e0010d27 100644 --- a/pkg/storage/fs/owncloudsql/owncloudsql.go +++ b/pkg/storage/fs/owncloudsql/owncloudsql.go @@ -1307,7 +1307,7 @@ func (fs *owncloudsqlfs) Move(ctx context.Context, oldRef, newRef *provider.Refe return nil } -func (fs *owncloudsqlfs) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []string) (*provider.ResourceInfo, error) { +func (fs *owncloudsqlfs) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []string, fieldMask []string) (*provider.ResourceInfo, error) { ip, err := fs.resolve(ctx, ref) if err != nil { // TODO return correct errtype @@ -1351,7 +1351,7 @@ func (fs *owncloudsqlfs) GetMD(ctx context.Context, ref *provider.Reference, mdK return fs.convertToResourceInfo(ctx, entry, ip, mdKeys) } -func (fs *owncloudsqlfs) ListFolder(ctx context.Context, ref *provider.Reference, mdKeys []string) ([]*provider.ResourceInfo, error) { +func (fs *owncloudsqlfs) ListFolder(ctx context.Context, ref *provider.Reference, mdKeys, fieldMask []string) ([]*provider.ResourceInfo, error) { log := appctx.GetLogger(ctx) ip, err := fs.resolve(ctx, ref) diff --git a/pkg/storage/fs/s3/s3.go b/pkg/storage/fs/s3/s3.go index a174003bea..9459f3751c 100644 --- a/pkg/storage/fs/s3/s3.go +++ b/pkg/storage/fs/s3/s3.go @@ -516,7 +516,7 @@ func (fs *s3FS) Move(ctx context.Context, oldRef, newRef *provider.Reference) er return nil } -func (fs *s3FS) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []string) (*provider.ResourceInfo, error) { +func (fs *s3FS) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []string, fieldMask []string) (*provider.ResourceInfo, error) { log := appctx.GetLogger(ctx) fn, err := fs.resolve(ctx, ref) @@ -579,7 +579,7 @@ func (fs *s3FS) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []str return fs.normalizeHead(ctx, output, fn), nil } -func (fs *s3FS) ListFolder(ctx context.Context, ref *provider.Reference, mdKeys []string) ([]*provider.ResourceInfo, error) { +func (fs *s3FS) ListFolder(ctx context.Context, ref *provider.Reference, mdKeys, fieldMask []string) ([]*provider.ResourceInfo, error) { fn, err := fs.resolve(ctx, ref) if err != nil { return nil, errors.Wrap(err, "error resolving ref") diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 98068b715c..d02b912b5f 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -39,8 +39,8 @@ type FS interface { TouchFile(ctx context.Context, ref *provider.Reference) error Delete(ctx context.Context, ref *provider.Reference) error Move(ctx context.Context, oldRef, newRef *provider.Reference) error - GetMD(ctx context.Context, ref *provider.Reference, mdKeys []string) (*provider.ResourceInfo, error) - ListFolder(ctx context.Context, ref *provider.Reference, mdKeys []string) ([]*provider.ResourceInfo, error) + GetMD(ctx context.Context, ref *provider.Reference, mdKeys, fieldMask []string) (*provider.ResourceInfo, error) + ListFolder(ctx context.Context, ref *provider.Reference, mdKeys, fieldMask []string) ([]*provider.ResourceInfo, error) InitiateUpload(ctx context.Context, ref *provider.Reference, uploadLength int64, metadata map[string]string) (map[string]string, error) Upload(ctx context.Context, ref *provider.Reference, r io.ReadCloser, uploadFunc UploadFinishedFunc) error Download(ctx context.Context, ref *provider.Reference) (io.ReadCloser, error) diff --git a/pkg/storage/utils/decomposedfs/decomposedfs.go b/pkg/storage/utils/decomposedfs/decomposedfs.go index f4ec758038..2c3165cd46 100644 --- a/pkg/storage/utils/decomposedfs/decomposedfs.go +++ b/pkg/storage/utils/decomposedfs/decomposedfs.go @@ -173,7 +173,8 @@ func (fs *Decomposedfs) GetQuota(ctx context.Context, ref *provider.Reference) ( return 0, 0, 0, errtypes.PermissionDenied(n.ID) } - ri, err := n.AsResourceInfo(ctx, &rp, []string{"treesize", "quota"}, true) + // FIXME move treesize & quota to fieldmask + ri, err := n.AsResourceInfo(ctx, &rp, []string{"treesize", "quota"}, []string{}, true) if err != nil { return 0, 0, 0, err } @@ -498,7 +499,7 @@ func (fs *Decomposedfs) Move(ctx context.Context, oldRef, newRef *provider.Refer } // GetMD returns the metadata for the specified resource -func (fs *Decomposedfs) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []string) (ri *provider.ResourceInfo, err error) { +func (fs *Decomposedfs) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []string, fieldMask []string) (ri *provider.ResourceInfo, err error) { var node *node.Node if node, err = fs.lu.NodeFromResource(ctx, ref); err != nil { return @@ -517,11 +518,11 @@ func (fs *Decomposedfs) GetMD(ctx context.Context, ref *provider.Reference, mdKe return nil, errtypes.PermissionDenied(node.ID) } - return node.AsResourceInfo(ctx, &rp, mdKeys, utils.IsRelativeReference(ref)) + return node.AsResourceInfo(ctx, &rp, mdKeys, fieldMask, utils.IsRelativeReference(ref)) } // ListFolder returns a list of resources in the specified folder -func (fs *Decomposedfs) ListFolder(ctx context.Context, ref *provider.Reference, mdKeys []string) (finfos []*provider.ResourceInfo, err error) { +func (fs *Decomposedfs) ListFolder(ctx context.Context, ref *provider.Reference, mdKeys []string, fieldMask []string) (finfos []*provider.ResourceInfo, err error) { var n *node.Node if n, err = fs.lu.NodeFromResource(ctx, ref); err != nil { return @@ -554,7 +555,7 @@ func (fs *Decomposedfs) ListFolder(ctx context.Context, ref *provider.Reference, // add this childs permissions pset := n.PermissionSet(ctx) node.AddPermissions(&np, &pset) - if ri, err := children[i].AsResourceInfo(ctx, &np, mdKeys, utils.IsRelativeReference(ref)); err == nil { + if ri, err := children[i].AsResourceInfo(ctx, &np, mdKeys, fieldMask, utils.IsRelativeReference(ref)); err == nil { finfos = append(finfos, ri) } } diff --git a/pkg/storage/utils/decomposedfs/decomposedfs_concurrency_test.go b/pkg/storage/utils/decomposedfs/decomposedfs_concurrency_test.go index 7bc6fe1cb8..1fa374a867 100644 --- a/pkg/storage/utils/decomposedfs/decomposedfs_concurrency_test.go +++ b/pkg/storage/utils/decomposedfs/decomposedfs_concurrency_test.go @@ -111,7 +111,7 @@ var _ = Describe("Decomposed", func() { } if err := env.Fs.CreateDir(env.Ctx, ref); err != nil { Expect(err).To(MatchError(ContainSubstring("already exists"))) - rinfo, err := env.Fs.GetMD(env.Ctx, ref, nil) + rinfo, err := env.Fs.GetMD(env.Ctx, ref, nil, nil) Expect(err).ToNot(HaveOccurred()) Expect(rinfo).ToNot(BeNil()) } diff --git a/pkg/storage/utils/decomposedfs/node/node.go b/pkg/storage/utils/decomposedfs/node/node.go index 198f290146..167afba728 100644 --- a/pkg/storage/utils/decomposedfs/node/node.go +++ b/pkg/storage/utils/decomposedfs/node/node.go @@ -581,7 +581,7 @@ func (n *Node) IsDir() bool { } // AsResourceInfo return the node as CS3 ResourceInfo -func (n *Node) AsResourceInfo(ctx context.Context, rp *provider.ResourcePermissions, mdKeys []string, returnBasename bool) (ri *provider.ResourceInfo, err error) { +func (n *Node) AsResourceInfo(ctx context.Context, rp *provider.ResourcePermissions, mdKeys, fieldMask []string, returnBasename bool) (ri *provider.ResourceInfo, err error) { sublog := appctx.GetLogger(ctx).With().Interface("node", n.ID).Logger() var fn string @@ -679,15 +679,25 @@ func (n *Node) AsResourceInfo(ctx context.Context, rp *provider.ResourcePermissi mdKeysMap[k] = struct{}{} } - var returnAllKeys bool + var returnAllMetadata bool if _, ok := mdKeysMap["*"]; len(mdKeys) == 0 || ok { - returnAllKeys = true + returnAllMetadata = true } metadata := map[string]string{} + fieldMaskKeysMap := make(map[string]struct{}) + for _, k := range fieldMask { + fieldMaskKeysMap[k] = struct{}{} + } + + var returnAllFields bool + if _, ok := fieldMaskKeysMap["*"]; len(fieldMask) == 0 || ok { + returnAllFields = true + } + // read favorite flag for the current user - if _, ok := mdKeysMap[FavoriteKey]; returnAllKeys || ok { + if _, ok := mdKeysMap[FavoriteKey]; returnAllMetadata || ok { favorite := "" if u, ok := ctxpkg.ContextGetUser(ctx); ok { // the favorite flag is specific to the user, so we need to incorporate the userid @@ -708,7 +718,8 @@ func (n *Node) AsResourceInfo(ctx context.Context, rp *provider.ResourcePermissi metadata[FavoriteKey] = favorite } // read locks - if _, ok := mdKeysMap[LockdiscoveryKey]; returnAllKeys || ok { + // FIXME move to fieldmask + if _, ok := mdKeysMap[LockdiscoveryKey]; returnAllMetadata || ok { if n.hasLocks(ctx) { err = readLocksIntoOpaque(ctx, n, ri) if err != nil { @@ -718,21 +729,36 @@ func (n *Node) AsResourceInfo(ctx context.Context, rp *provider.ResourcePermissi } // share indicator - if _, ok := mdKeysMap[ShareTypesKey]; returnAllKeys || ok { - if n.hasUserShares(ctx) { - metadata[ShareTypesKey] = UserShareType + if _, ok := fieldMaskKeysMap["share-types"]; returnAllFields || ok { + granteeTypes := n.getGranteeTypes(ctx) + if len(granteeTypes) > 0 { + // TODO add optional property to CS3 ResourceInfo to transport grants? + var s strings.Builder + first := true + for _, t := range granteeTypes { + if !first { + s.WriteString(",") + } else { + first = false + } + s.WriteString(strconv.Itoa(int(t))) + } + ri.Opaque = utils.AppendPlainToOpaque(ri.Opaque, "share-types", s.String()) } } // checksums - if _, ok := mdKeysMap[ChecksumsKey]; (nodeType == provider.ResourceType_RESOURCE_TYPE_FILE) && (returnAllKeys || ok) { + // FIXME move to fieldmask + if _, ok := mdKeysMap[ChecksumsKey]; (nodeType == provider.ResourceType_RESOURCE_TYPE_FILE) && (returnAllMetadata || ok) { // TODO which checksum was requested? sha1 adler32 or md5? for now hardcode sha1? + // TODO make ResourceInfo carry multiple checksums readChecksumIntoResourceChecksum(ctx, nodePath, storageprovider.XSSHA1, ri) readChecksumIntoOpaque(ctx, nodePath, storageprovider.XSMD5, ri) readChecksumIntoOpaque(ctx, nodePath, storageprovider.XSAdler32, ri) } // quota - if _, ok := mdKeysMap[QuotaKey]; (nodeType == provider.ResourceType_RESOURCE_TYPE_CONTAINER) && returnAllKeys || ok { + // FIXME move to fieldmask + if _, ok := mdKeysMap[QuotaKey]; (nodeType == provider.ResourceType_RESOURCE_TYPE_CONTAINER) && returnAllMetadata || ok { if n.SpaceRoot != nil && n.SpaceRoot.InternalPath() != "" { readQuotaIntoOpaque(ctx, n.SpaceRoot.InternalPath(), ri) } @@ -750,7 +776,7 @@ func (n *Node) AsResourceInfo(ctx context.Context, rp *provider.ResourcePermissi } // only read when key was requested k := attrs[i][len(xattrs.MetadataPrefix):] - if _, ok := mdKeysMap[k]; returnAllKeys || ok { + if _, ok := mdKeysMap[k]; returnAllMetadata || ok { if val, err := xattrs.Get(nodePath, attrs[i]); err == nil { metadata[k] = val } else { @@ -1077,19 +1103,19 @@ func ReadBlobIDAttr(path string) (string, error) { return attr, nil } -func (n *Node) hasUserShares(ctx context.Context) bool { - g, err := n.ListGrantees(ctx) - if err != nil { - appctx.GetLogger(ctx).Error().Err(err).Msg("hasUserShares: listGrantees") - return false - } - - for i := range g { - if strings.HasPrefix(g[i], xattrs.GrantUserAcePrefix) { - return true +func (n *Node) getGranteeTypes(ctx context.Context) []provider.GranteeType { + types := []provider.GranteeType{} + if g, err := n.ListGrantees(ctx); err == nil { + for i := range g { + switch { + case strings.HasPrefix(g[i], xattrs.GrantUserAcePrefix): + types = append(types, provider.GranteeType_GRANTEE_TYPE_USER) + case strings.HasPrefix(g[i], xattrs.GrantGroupAcePrefix): + types = append(types, provider.GranteeType_GRANTEE_TYPE_GROUP) + } } } - return false + return types } func parseMTime(v string) (t time.Time, err error) { diff --git a/pkg/storage/utils/decomposedfs/node/node_test.go b/pkg/storage/utils/decomposedfs/node/node_test.go index cfae71aadb..9b6a460421 100644 --- a/pkg/storage/utils/decomposedfs/node/node_test.go +++ b/pkg/storage/utils/decomposedfs/node/node_test.go @@ -184,14 +184,14 @@ var _ = Describe("Node", func() { Describe("the Etag field", func() { It("is set", func() { perms := node.OwnerPermissions() - ri, err := n.AsResourceInfo(env.Ctx, &perms, []string{}, false) + ri, err := n.AsResourceInfo(env.Ctx, &perms, []string{}, []string{}, false) Expect(err).ToNot(HaveOccurred()) Expect(len(ri.Etag)).To(Equal(34)) }) It("changes when the tmtime is set", func() { perms := node.OwnerPermissions() - ri, err := n.AsResourceInfo(env.Ctx, &perms, []string{}, false) + ri, err := n.AsResourceInfo(env.Ctx, &perms, []string{}, []string{}, false) Expect(err).ToNot(HaveOccurred()) Expect(len(ri.Etag)).To(Equal(34)) before := ri.Etag @@ -199,7 +199,7 @@ var _ = Describe("Node", func() { tmtime := time.Now() Expect(n.SetTMTime(&tmtime)).To(Succeed()) - ri, err = n.AsResourceInfo(env.Ctx, &perms, []string{}, false) + ri, err = n.AsResourceInfo(env.Ctx, &perms, []string{}, []string{}, false) Expect(err).ToNot(HaveOccurred()) Expect(len(ri.Etag)).To(Equal(34)) Expect(ri.Etag).ToNot(Equal(before)) @@ -215,7 +215,7 @@ var _ = Describe("Node", func() { Expect(err).ToNot(HaveOccurred()) perms := node.OwnerPermissions() - ri, err := n.AsResourceInfo(env.Ctx, &perms, []string{}, false) + ri, err := n.AsResourceInfo(env.Ctx, &perms, []string{}, []string{}, false) Expect(err).ToNot(HaveOccurred()) Expect(ri.Opaque).ToNot(BeNil()) Expect(ri.Opaque.Map["lock"]).ToNot(BeNil()) diff --git a/pkg/storage/utils/decomposedfs/tree/tree_test.go b/pkg/storage/utils/decomposedfs/tree/tree_test.go index 4b85656d23..7112e3fe74 100644 --- a/pkg/storage/utils/decomposedfs/tree/tree_test.go +++ b/pkg/storage/utils/decomposedfs/tree/tree_test.go @@ -302,13 +302,13 @@ var _ = Describe("Tree", func() { Expect(err).ToNot(HaveOccurred()) perms := node.OwnerPermissions() - riBefore, err := dir.AsResourceInfo(env.Ctx, &perms, []string{}, false) + riBefore, err := dir.AsResourceInfo(env.Ctx, &perms, []string{}, []string{}, false) Expect(err).ToNot(HaveOccurred()) err = env.Tree.Propagate(env.Ctx, file) Expect(err).ToNot(HaveOccurred()) - riAfter, err := dir.AsResourceInfo(env.Ctx, &perms, []string{}, false) + riAfter, err := dir.AsResourceInfo(env.Ctx, &perms, []string{}, []string{}, false) Expect(err).ToNot(HaveOccurred()) Expect(riAfter.Etag).ToNot(Equal(riBefore.Etag)) }) diff --git a/pkg/storage/utils/eosfs/eosfs.go b/pkg/storage/utils/eosfs/eosfs.go index 00bd59d232..c0f7c45e72 100644 --- a/pkg/storage/utils/eosfs/eosfs.go +++ b/pkg/storage/utils/eosfs/eosfs.go @@ -792,7 +792,7 @@ func (fs *eosfs) ListGrants(ctx context.Context, ref *provider.Reference) ([]*pr return grantList, nil } -func (fs *eosfs) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []string) (*provider.ResourceInfo, error) { +func (fs *eosfs) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []string, fieldMask []string) (*provider.ResourceInfo, error) { log := appctx.GetLogger(ctx) log.Info().Msg("eosfs: get md for ref:" + ref.String()) @@ -887,7 +887,7 @@ func (fs *eosfs) getMDShareFolder(ctx context.Context, p string, mdKeys []string return fs.convertToFileReference(ctx, eosFileInfo) } -func (fs *eosfs) ListFolder(ctx context.Context, ref *provider.Reference, mdKeys []string) ([]*provider.ResourceInfo, error) { +func (fs *eosfs) ListFolder(ctx context.Context, ref *provider.Reference, mdKeys, fieldMask []string) ([]*provider.ResourceInfo, error) { p, err := fs.resolve(ctx, ref) if err != nil { return nil, errors.Wrap(err, "eosfs: error resolving reference") @@ -1444,7 +1444,7 @@ func (fs *eosfs) ListRevisions(ctx context.Context, ref *provider.Reference) ([] // We need to access the revisions for a non-home reference. // We'll get the owner of the particular resource and impersonate them // if we have access to it. - md, err := fs.GetMD(ctx, ref, nil) + md, err := fs.GetMD(ctx, ref, nil, nil) if err != nil { return nil, err } @@ -1487,7 +1487,7 @@ func (fs *eosfs) DownloadRevision(ctx context.Context, ref *provider.Reference, // We need to access the revisions for a non-home reference. // We'll get the owner of the particular resource and impersonate them // if we have access to it. - md, err := fs.GetMD(ctx, ref, nil) + md, err := fs.GetMD(ctx, ref, nil, nil) if err != nil { return nil, err } @@ -1520,7 +1520,7 @@ func (fs *eosfs) RestoreRevision(ctx context.Context, ref *provider.Reference, r // We need to access the revisions for a non-home reference. // We'll get the owner of the particular resource and impersonate them // if we have access to it. - md, err := fs.GetMD(ctx, ref, nil) + md, err := fs.GetMD(ctx, ref, nil, nil) if err != nil { return err } @@ -1568,7 +1568,7 @@ func (fs *eosfs) ListRecycle(ctx context.Context, ref *provider.Reference, key, // We need to access the recycle bin for a non-home reference. // We'll get the owner of the particular resource and impersonate them // if we have access to it. - md, err := fs.GetMD(ctx, &provider.Reference{Path: ref.Path}, nil) + md, err := fs.GetMD(ctx, &provider.Reference{Path: ref.Path}, nil, nil) if err != nil { return nil, err } @@ -1619,7 +1619,7 @@ func (fs *eosfs) RestoreRecycleItem(ctx context.Context, ref *provider.Reference // We need to access the recycle bin for a non-home reference. // We'll get the owner of the particular resource and impersonate them // if we have access to it. - md, err := fs.GetMD(ctx, &provider.Reference{Path: ref.Path}, nil) + md, err := fs.GetMD(ctx, &provider.Reference{Path: ref.Path}, nil, nil) if err != nil { return err } diff --git a/pkg/storage/utils/eosfs/spaces.go b/pkg/storage/utils/eosfs/spaces.go index 477265eb15..78d1cc20a0 100644 --- a/pkg/storage/utils/eosfs/spaces.go +++ b/pkg/storage/utils/eosfs/spaces.go @@ -228,7 +228,7 @@ func (fs *eosfs) listProjectStorageSpaces(ctx context.Context, user *userpb.User for rows.Next() { var name, relPath string if err = rows.Scan(&name, &relPath); err == nil { - info, err := fs.GetMD(ctx, &provider.Reference{Path: relPath}, []string{}) + info, err := fs.GetMD(ctx, &provider.Reference{Path: relPath}, []string{}, nil) if err == nil { if (spaceID == "" || spaceID == info.Id.OpaqueId) && (spacePath == "" || spacePath == relPath) { // If the request was for a relative ref, return just the base path diff --git a/pkg/storage/utils/localfs/localfs.go b/pkg/storage/utils/localfs/localfs.go index 452432b276..a013d7d0ba 100644 --- a/pkg/storage/utils/localfs/localfs.go +++ b/pkg/storage/utils/localfs/localfs.go @@ -920,7 +920,7 @@ func (fs *localfs) moveReferences(ctx context.Context, oldName, newName string) return nil } -func (fs *localfs) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []string) (*provider.ResourceInfo, error) { +func (fs *localfs) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []string, fieldMask []string) (*provider.ResourceInfo, error) { fn, err := fs.resolve(ctx, ref) if err != nil { return nil, errors.Wrap(err, "localfs: error resolving ref") @@ -961,7 +961,7 @@ func (fs *localfs) getMDShareFolder(ctx context.Context, p string, mdKeys []stri return fs.convertToFileReference(ctx, md, fn, mdKeys) } -func (fs *localfs) ListFolder(ctx context.Context, ref *provider.Reference, mdKeys []string) ([]*provider.ResourceInfo, error) { +func (fs *localfs) ListFolder(ctx context.Context, ref *provider.Reference, mdKeys, fieldMask []string) ([]*provider.ResourceInfo, error) { fn, err := fs.resolve(ctx, ref) if err != nil { return nil, errors.Wrap(err, "localfs: error resolving ref") From 1b591d7194c07fcaf3134898cf227bc2c565eac6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 29 Jun 2022 15:13:43 +0000 Subject: [PATCH 13/24] mimic oc10 not found error message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../http/services/owncloud/ocdav/propfind/propfind.go | 4 +++- internal/http/services/owncloud/ocdav/proppatch.go | 8 ++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/propfind/propfind.go b/internal/http/services/owncloud/ocdav/propfind/propfind.go index ede4943487..2e282b40e5 100644 --- a/internal/http/services/owncloud/ocdav/propfind/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind/propfind.go @@ -298,9 +298,11 @@ func (p *Handler) HandleSpacesPropfind(w http.ResponseWriter, r *http.Request, s if sRes.Status.Code != rpc.Code_CODE_OK { // return not found error so we dont leak existence of a space status = http.StatusNotFound - m = "Resource not found" // mimic the oc10 error message } } + if status == http.StatusNotFound { + m = "Resource not found" // mimic the oc10 error message + } w.WriteHeader(status) b, err := errors.Marshal(status, m, "") errors.HandleWebdavError(&sublog, w, b, err) diff --git a/internal/http/services/owncloud/ocdav/proppatch.go b/internal/http/services/owncloud/ocdav/proppatch.go index 00849a4499..00d43e7b36 100644 --- a/internal/http/services/owncloud/ocdav/proppatch.go +++ b/internal/http/services/owncloud/ocdav/proppatch.go @@ -200,9 +200,11 @@ func (s *svc) handleProppatch(ctx context.Context, w http.ResponseWriter, r *htt // TODO hide permission failed for users without access in every kind of request // TODO should this be done in the driver? status = http.StatusNotFound - m = "Resource not found" // mimic the oc10 error message } } + if status == http.StatusNotFound { + m = "Resource not found" // mimic the oc10 error message + } w.WriteHeader(status) b, err := errors.Marshal(status, m, "") errors.HandleWebdavError(&log, w, b, err) @@ -252,9 +254,11 @@ func (s *svc) handleProppatch(ctx context.Context, w http.ResponseWriter, r *htt // TODO hide permission failed for users without access in every kind of request // TODO should this be done in the driver? status = http.StatusNotFound - m = "Resource not found" // mimic the oc10 error message } } + if status == http.StatusNotFound { + m = "Resource not found" // mimic the oc10 error message + } w.WriteHeader(status) b, err := errors.Marshal(status, m, "") errors.HandleWebdavError(&log, w, b, err) From 2bd1f7c03a1f9e3fe518d583d238ce6916e360b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 29 Jun 2022 15:18:38 +0000 Subject: [PATCH 14/24] mimic oc10 not found error message in more places MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- internal/http/services/owncloud/ocdav/delete.go | 8 +++----- internal/http/services/owncloud/ocdav/mkcol.go | 4 ++-- .../http/services/owncloud/ocdav/propfind/propfind.go | 2 +- internal/http/services/owncloud/ocdav/proppatch.go | 4 ++-- internal/http/services/owncloud/ocdav/put.go | 7 ++++--- 5 files changed, 12 insertions(+), 13 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/delete.go b/internal/http/services/owncloud/ocdav/delete.go index 7252888aa6..f28432da6d 100644 --- a/internal/http/services/owncloud/ocdav/delete.go +++ b/internal/http/services/owncloud/ocdav/delete.go @@ -96,8 +96,7 @@ func (s *svc) handleDelete(ctx context.Context, w http.ResponseWriter, r *http.R w.WriteHeader(http.StatusNoContent) case rpc.Code_CODE_NOT_FOUND: w.WriteHeader(http.StatusNotFound) - // TODO path might be empty or relative... - m := fmt.Sprintf("Resource %v not found", ref.Path) + m := "Resource not found" // mimic the oc10 error message b, err := errors.Marshal(http.StatusNotFound, m, "") errors.HandleWebdavError(&log, w, b, err) case rpc.Code_CODE_PERMISSION_DENIED: @@ -119,12 +118,11 @@ func (s *svc) handleDelete(ctx context.Context, w http.ResponseWriter, r *http.R return } if sRes.Status.Code != rpc.Code_CODE_OK { - // return not found error so we dont leak existence of a file + // return not found error so we do not leak existence of a file // TODO hide permission failed for users without access in every kind of request // TODO should this be done in the driver? status = http.StatusNotFound - // TODO path might be empty or relative... - m = fmt.Sprintf("%s not found", ref.Path) + m = "Resource not found" // mimic the oc10 error message } w.WriteHeader(status) b, err := errors.Marshal(status, m, "") diff --git a/internal/http/services/owncloud/ocdav/mkcol.go b/internal/http/services/owncloud/ocdav/mkcol.go index 6a16925e3e..e6a318b4e9 100644 --- a/internal/http/services/owncloud/ocdav/mkcol.go +++ b/internal/http/services/owncloud/ocdav/mkcol.go @@ -136,10 +136,10 @@ func (s *svc) handleMkcol(ctx context.Context, w http.ResponseWriter, r *http.Re return http.StatusInternalServerError, err } if sRes.Status.Code != rpc.Code_CODE_OK { - // return not found error so we dont leak existence of a file + // return not found error so we do not leak existence of a file // TODO hide permission failed for users without access in every kind of request // TODO should this be done in the driver? - return http.StatusNotFound, fmt.Errorf(sRes.Status.Message) + return http.StatusNotFound, fmt.Errorf("Resource not found") } return http.StatusForbidden, fmt.Errorf(sRes.Status.Message) case res.Status.Code == rpc.Code_CODE_ABORTED: diff --git a/internal/http/services/owncloud/ocdav/propfind/propfind.go b/internal/http/services/owncloud/ocdav/propfind/propfind.go index 2e282b40e5..b05d709f3f 100644 --- a/internal/http/services/owncloud/ocdav/propfind/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind/propfind.go @@ -296,7 +296,7 @@ func (p *Handler) HandleSpacesPropfind(w http.ResponseWriter, r *http.Request, s return } if sRes.Status.Code != rpc.Code_CODE_OK { - // return not found error so we dont leak existence of a space + // return not found error so we do not leak existence of a space status = http.StatusNotFound } } diff --git a/internal/http/services/owncloud/ocdav/proppatch.go b/internal/http/services/owncloud/ocdav/proppatch.go index 00d43e7b36..0714bf0a48 100644 --- a/internal/http/services/owncloud/ocdav/proppatch.go +++ b/internal/http/services/owncloud/ocdav/proppatch.go @@ -196,7 +196,7 @@ func (s *svc) handleProppatch(ctx context.Context, w http.ResponseWriter, r *htt return nil, nil, false } if sRes.Status.Code != rpc.Code_CODE_OK { - // return not found error so we dont leak existence of a file + // return not found error so we do not leak existence of a file // TODO hide permission failed for users without access in every kind of request // TODO should this be done in the driver? status = http.StatusNotFound @@ -250,7 +250,7 @@ func (s *svc) handleProppatch(ctx context.Context, w http.ResponseWriter, r *htt return nil, nil, false } if sRes.Status.Code != rpc.Code_CODE_OK { - // return not found error so we dont leak existence of a file + // return not found error so we don't leak existence of a file // TODO hide permission failed for users without access in every kind of request // TODO should this be done in the driver? status = http.StatusNotFound diff --git a/internal/http/services/owncloud/ocdav/put.go b/internal/http/services/owncloud/ocdav/put.go index 568f8cd93b..fe7fab892b 100644 --- a/internal/http/services/owncloud/ocdav/put.go +++ b/internal/http/services/owncloud/ocdav/put.go @@ -20,7 +20,6 @@ package ocdav import ( "context" - "fmt" "net/http" "path" "strconv" @@ -233,11 +232,13 @@ func (s *svc) handlePut(ctx context.Context, w http.ResponseWriter, r *http.Requ return } if sRes.Status.Code != rpc.Code_CODE_OK { - // return not found error so we dont leak existence of a file + // return not found error so we do not leak existence of a file // TODO hide permission failed for users without access in every kind of request // TODO should this be done in the driver? status = http.StatusNotFound - m = fmt.Sprintf("%v not found", ref.Path) + } + if status == http.StatusNotFound { + m = "Resource not found" // mimic the oc10 error message } w.WriteHeader(status) b, err := errors.Marshal(status, m, "") From 9314060dbf0cc88fa96bfca73bcf65cf1875a395 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 29 Jun 2022 19:31:36 +0000 Subject: [PATCH 15/24] fix oc:permissions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../owncloud/ocdav/propfind/propfind.go | 16 ++++++---------- pkg/storage/utils/decomposedfs/node/node.go | 17 +++++++++++++---- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/propfind/propfind.go b/internal/http/services/owncloud/ocdav/propfind/propfind.go index b05d709f3f..51c61291eb 100644 --- a/internal/http/services/owncloud/ocdav/propfind/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind/propfind.go @@ -952,10 +952,11 @@ func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, p } var wdp string isPublic := ls != nil + isShared := shareTypes != "" && !net.IsCurrentUserOwner(ctx, md.Owner) if md.PermissionSet != nil { wdp = role.WebDAVPermissions( md.Type == provider.ResourceType_RESOURCE_TYPE_CONTAINER, - shareTypes != "", + isShared, false, isPublic, ) @@ -1234,23 +1235,18 @@ func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, p } else { propstatNotFound.Prop = append(propstatNotFound.Prop, prop.NotFound("oc:checksums")) } - case "share-types": // desktop + case "share-types": // used to render share indicators to share owners var types strings.Builder sts := strings.Split(shareTypes, ",") for _, shareType := range sts { - // TODO implement conversion switch shareType { case "1": // provider.GranteeType_GRANTEE_TYPE_USER - types.WriteString("") - types.WriteString(strconv.Itoa(int(conversions.ShareTypeUser))) - types.WriteString("") + types.WriteString("" + strconv.Itoa(int(conversions.ShareTypeUser)) + "") case "2": // provider.GranteeType_GRANTEE_TYPE_GROUP - types.WriteString("") - types.WriteString(strconv.Itoa(int(conversions.ShareTypeGroup))) - types.WriteString("") + types.WriteString("" + strconv.Itoa(int(conversions.ShareTypeGroup)) + "") default: - sublog.Warn().Interface("shareType", shareType).Msg("unknown share type") + sublog.Debug().Interface("shareType", shareType).Msg("unknown share type, ignoring") } } diff --git a/pkg/storage/utils/decomposedfs/node/node.go b/pkg/storage/utils/decomposedfs/node/node.go index 167afba728..328e95d983 100644 --- a/pkg/storage/utils/decomposedfs/node/node.go +++ b/pkg/storage/utils/decomposedfs/node/node.go @@ -1106,14 +1106,23 @@ func ReadBlobIDAttr(path string) (string, error) { func (n *Node) getGranteeTypes(ctx context.Context) []provider.GranteeType { types := []provider.GranteeType{} if g, err := n.ListGrantees(ctx); err == nil { + hasUserShares, hasGroupShares := false, false for i := range g { switch { - case strings.HasPrefix(g[i], xattrs.GrantUserAcePrefix): - types = append(types, provider.GranteeType_GRANTEE_TYPE_USER) - case strings.HasPrefix(g[i], xattrs.GrantGroupAcePrefix): - types = append(types, provider.GranteeType_GRANTEE_TYPE_GROUP) + case !hasUserShares && strings.HasPrefix(g[i], xattrs.GrantUserAcePrefix): + hasUserShares = true + case !hasGroupShares && strings.HasPrefix(g[i], xattrs.GrantGroupAcePrefix): + hasGroupShares = true + case hasUserShares && hasGroupShares: + break } } + if hasUserShares { + types = append(types, provider.GranteeType_GRANTEE_TYPE_USER) + } + if hasGroupShares { + types = append(types, provider.GranteeType_GRANTEE_TYPE_GROUP) + } } return types } From e85c9d851704514ec52475fa5958ec831ff78d99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 29 Jun 2022 20:14:46 +0000 Subject: [PATCH 16/24] fix test compile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- pkg/storage/fs/nextcloud/nextcloud_test.go | 2 +- pkg/storage/utils/decomposedfs/upload_test.go | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/storage/fs/nextcloud/nextcloud_test.go b/pkg/storage/fs/nextcloud/nextcloud_test.go index cd41e96cfa..d74dc693ff 100644 --- a/pkg/storage/fs/nextcloud/nextcloud_test.go +++ b/pkg/storage/fs/nextcloud/nextcloud_test.go @@ -319,7 +319,7 @@ var _ = Describe("Nextcloud", func() { Path: "/some", } mdKeys := []string{"val1", "val2", "val3"} - results, err := nc.ListFolder(ctx, ref, mdKeys) + results, err := nc.ListFolder(ctx, ref, mdKeys, []string{}) Expect(err).NotTo(HaveOccurred()) Expect(len(results)).To(Equal(1)) Expect(*results[0]).To(Equal(provider.ResourceInfo{ diff --git a/pkg/storage/utils/decomposedfs/upload_test.go b/pkg/storage/utils/decomposedfs/upload_test.go index 0d13917e57..5f4276ed5b 100644 --- a/pkg/storage/utils/decomposedfs/upload_test.go +++ b/pkg/storage/utils/decomposedfs/upload_test.go @@ -193,7 +193,7 @@ var _ = Describe("File uploads", func() { Expect(uploadIds["simple"]).ToNot(BeEmpty()) Expect(uploadIds["tus"]).ToNot(BeEmpty()) - resources, err := fs.ListFolder(ctx, rootRef, []string{}) + resources, err := fs.ListFolder(ctx, rootRef, []string{}, []string{}) Expect(err).ToNot(HaveOccurred()) Expect(len(resources)).To(Equal(0)) @@ -209,7 +209,7 @@ var _ = Describe("File uploads", func() { Expect(uploadIds["simple"]).ToNot(BeEmpty()) Expect(uploadIds["tus"]).ToNot(BeEmpty()) - resources, err := fs.ListFolder(ctx, rootRef, []string{}) + resources, err := fs.ListFolder(ctx, rootRef, []string{}, []string{}) Expect(err).ToNot(HaveOccurred()) Expect(len(resources)).To(Equal(0)) @@ -246,7 +246,7 @@ var _ = Describe("File uploads", func() { Expect(err).ToNot(HaveOccurred()) bs.AssertCalled(GinkgoT(), "Upload", mock.Anything, mock.Anything, mock.Anything) - resources, err := fs.ListFolder(ctx, rootRef, []string{}) + resources, err := fs.ListFolder(ctx, rootRef, []string{}, []string{}) Expect(err).ToNot(HaveOccurred()) Expect(len(resources)).To(Equal(1)) @@ -284,7 +284,7 @@ var _ = Describe("File uploads", func() { Expect(err).ToNot(HaveOccurred()) bs.AssertCalled(GinkgoT(), "Upload", mock.Anything, mock.Anything, mock.Anything) - resources, err := fs.ListFolder(ctx, rootRef, []string{}) + resources, err := fs.ListFolder(ctx, rootRef, []string{}, []string{}) Expect(err).ToNot(HaveOccurred()) Expect(len(resources)).To(Equal(1)) @@ -303,7 +303,7 @@ var _ = Describe("File uploads", func() { Expect(err).To(HaveOccurred()) - resources, err := fs.ListFolder(ctx, rootRef, []string{}) + resources, err := fs.ListFolder(ctx, rootRef, []string{}, []string{}) Expect(err).ToNot(HaveOccurred()) Expect(len(resources)).To(Equal(0)) From 9beebbcd0c87e65e8d7912fcc0a5f9b491345bbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 29 Jun 2022 20:43:08 +0000 Subject: [PATCH 17/24] fix nc tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- internal/grpc/services/storageprovider/storageprovider.go | 2 +- pkg/storage/fs/nextcloud/nextcloud.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/grpc/services/storageprovider/storageprovider.go b/internal/grpc/services/storageprovider/storageprovider.go index 99248a7655..ce34ddcbec 100644 --- a/internal/grpc/services/storageprovider/storageprovider.go +++ b/internal/grpc/services/storageprovider/storageprovider.go @@ -672,7 +672,7 @@ func (s *service) Delete(ctx context.Context, req *provider.DeleteRequest) (*pro } } - md, err := s.storage.GetMD(ctx, req.Ref, nil, []string{"id"}) + md, err := s.storage.GetMD(ctx, req.Ref, []string{}, []string{"id"}) if err != nil { return &provider.DeleteResponse{ Status: status.NewStatusFromErrType(ctx, "can't stat resource to delete", err), diff --git a/pkg/storage/fs/nextcloud/nextcloud.go b/pkg/storage/fs/nextcloud/nextcloud.go index 20803d8eec..bd75318d5f 100644 --- a/pkg/storage/fs/nextcloud/nextcloud.go +++ b/pkg/storage/fs/nextcloud/nextcloud.go @@ -301,6 +301,7 @@ func (nc *StorageDriver) Move(ctx context.Context, oldRef, newRef *provider.Refe } // GetMD as defined in the storage.FS interface +// TODO forward fieldMask func (nc *StorageDriver) GetMD(ctx context.Context, ref *provider.Reference, mdKeys []string, fieldMask []string) (*provider.ResourceInfo, error) { type paramsObj struct { Ref *provider.Reference `json:"ref"` From 726b1ecd499c1902fb7be9f421a673b2e535e054 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 29 Jun 2022 20:46:04 +0000 Subject: [PATCH 18/24] lint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../http/services/owncloud/ocdav/propfind/propfind.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/propfind/propfind.go b/internal/http/services/owncloud/ocdav/propfind/propfind.go index 51c61291eb..0c6158b77f 100644 --- a/internal/http/services/owncloud/ocdav/propfind/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind/propfind.go @@ -57,8 +57,7 @@ import ( ) const ( - tracerName = "ocdav" - _spaceTypeProject = "project" + tracerName = "ocdav" ) type countingReader struct { @@ -931,7 +930,7 @@ func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, p sublog.Error().Err(err).Msg("could not unmarshal link json") } } - if quota := utils.ReadPlainFromOpaque(md.Opaque, "quota"); quota == "" { + if quota = utils.ReadPlainFromOpaque(md.Opaque, "quota"); quota == "" { quota = net.PropQuotaUnknown } if md.Opaque.Map["lock"] != nil && md.Opaque.Map["lock"].Decoder == "json" { @@ -1530,13 +1529,11 @@ func (c *countingReader) Read(p []byte) (int, error) { func metadataKeyOf(n *xml.Name) string { switch n.Space { case net.NsDav: - switch n.Local { - case "quota-available-bytes": + if n.Local == "quota-available-bytes" { return "quota" } case net.NsOwncloud: - switch n.Local { - case "share-types": + if n.Local == "share-types" { return "share-types" } } From c64eb57742cb862dee521ab14aaa7bdcdb1797d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 29 Jun 2022 21:15:32 +0000 Subject: [PATCH 19/24] small cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- internal/grpc/services/gateway/storageprovidercache.go | 4 ++-- internal/http/services/owncloud/ocdav/publicfile.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/grpc/services/gateway/storageprovidercache.go b/internal/grpc/services/gateway/storageprovidercache.go index 54d97d0061..c72c6b0a1d 100644 --- a/internal/grpc/services/gateway/storageprovidercache.go +++ b/internal/grpc/services/gateway/storageprovidercache.go @@ -238,8 +238,8 @@ func statKey(user *userpb.User, ref *provider.Reference, metaDataKeys, fieldMask for _, k := range metaDataKeys { key += "!mdk:" + k } - for _, k := range fieldMaskPaths { - key += "!fmp:" + k + for _, p := range fieldMaskPaths { + key += "!fmp:" + p } return key diff --git a/internal/http/services/owncloud/ocdav/publicfile.go b/internal/http/services/owncloud/ocdav/publicfile.go index eff1c11e57..da8a341ed4 100644 --- a/internal/http/services/owncloud/ocdav/publicfile.go +++ b/internal/http/services/owncloud/ocdav/publicfile.go @@ -111,7 +111,7 @@ func (s *svc) handlePropfindOnToken(w http.ResponseWriter, r *http.Request, ns s infos := s.getPublicFileInfos(onContainer, depth == net.DepthZero, tokenStatInfo) - propRes, err := propfind.MultistatusResponse(ctx, &pf, infos, s.c.PublicURL, ns /*"",*/, nil) + propRes, err := propfind.MultistatusResponse(ctx, &pf, infos, s.c.PublicURL, ns, nil) if err != nil { sublog.Error().Err(err).Msg("error formatting propfind") w.WriteHeader(http.StatusInternalServerError) From 2cfd2807b5856adb0330f219dec9991b7ff57ef0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 29 Jun 2022 21:16:52 +0000 Subject: [PATCH 20/24] decomposedfs: add space if fieldmask is empty, * or has 'space' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../utils/decomposedfs/decomposedfs.go | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/pkg/storage/utils/decomposedfs/decomposedfs.go b/pkg/storage/utils/decomposedfs/decomposedfs.go index 2c3165cd46..372f97f6e9 100644 --- a/pkg/storage/utils/decomposedfs/decomposedfs.go +++ b/pkg/storage/utils/decomposedfs/decomposedfs.go @@ -518,7 +518,25 @@ func (fs *Decomposedfs) GetMD(ctx context.Context, ref *provider.Reference, mdKe return nil, errtypes.PermissionDenied(node.ID) } - return node.AsResourceInfo(ctx, &rp, mdKeys, fieldMask, utils.IsRelativeReference(ref)) + md, err := node.AsResourceInfo(ctx, &rp, mdKeys, fieldMask, utils.IsRelativeReference(ref)) + if err != nil { + return nil, err + } + + addSpace := len(fieldMask) == 0 + for _, p := range fieldMask { + if p == "space" || p == "*" { + addSpace = true + break + } + } + if addSpace { + if md.Space, err = fs.storageSpaceFromNode(ctx, node, node.InternalPath(), false, false); err != nil { + return nil, err + } + } + + return md, nil } // ListFolder returns a list of resources in the specified folder From 50135f7616a8714135c71c0b33989ccb4db9ceff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Thu, 30 Jun 2022 11:42:51 +0000 Subject: [PATCH 21/24] fix propfind root resource path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- internal/http/services/owncloud/ocdav/propfind/propfind.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/http/services/owncloud/ocdav/propfind/propfind.go b/internal/http/services/owncloud/ocdav/propfind/propfind.go index 0c6158b77f..768d577dd2 100644 --- a/internal/http/services/owncloud/ocdav/propfind/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind/propfind.go @@ -271,7 +271,7 @@ func (p *Handler) HandleSpacesPropfind(w http.ResponseWriter, r *http.Request, s res, err := client.Stat(ctx, &provider.StatRequest{ Ref: &ref, ArbitraryMetadataKeys: metadataKeys, - FieldMask: &fieldmaskpb.FieldMask{Paths: []string{"*"}}, // TODO use more sophisticated filter + FieldMask: &fieldmaskpb.FieldMask{Paths: []string{"*"}}, // TODO use more sophisticated filter? we don't need all space properties, afaict only the spacetype }) if err != nil { sublog.Error().Err(err).Msg("error getting grpc client") @@ -327,6 +327,8 @@ func (p *Handler) HandleSpacesPropfind(w http.ResponseWriter, r *http.Request, s } } + res.Info.Path = r.URL.Path + resourceInfos := []*provider.ResourceInfo{ res.Info, } From 758715a92b9f03c88d553d79c84758cf3c5d70fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Thu, 30 Jun 2022 14:17:15 +0000 Subject: [PATCH 22/24] update tests & changelog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../skip-space-lookup-on-space-propfind.md | 3 +++ .../acceptance/expected-failures-on-OCIS-storage.md | 13 ------------- .../acceptance/expected-failures-on-S3NG-storage.md | 13 ------------- 3 files changed, 3 insertions(+), 26 deletions(-) diff --git a/changelog/unreleased/skip-space-lookup-on-space-propfind.md b/changelog/unreleased/skip-space-lookup-on-space-propfind.md index bef1452b42..63600068a2 100644 --- a/changelog/unreleased/skip-space-lookup-on-space-propfind.md +++ b/changelog/unreleased/skip-space-lookup-on-space-propfind.md @@ -3,3 +3,6 @@ Enhancement: skip space lookup on space propfind We now construct the space id from the /dav/spaces URL intead of making a request to the registry. https://github.com/cs3org/reva/pull/2977 +https://github.com/owncloud/ocis/issues/1277 +https://github.com/owncloud/ocis/issues/2144 +https://github.com/owncloud/ocis/issues/3073 \ No newline at end of file diff --git a/tests/acceptance/expected-failures-on-OCIS-storage.md b/tests/acceptance/expected-failures-on-OCIS-storage.md index aa77ab3f5d..1f361564c9 100644 --- a/tests/acceptance/expected-failures-on-OCIS-storage.md +++ b/tests/acceptance/expected-failures-on-OCIS-storage.md @@ -1289,16 +1289,6 @@ _ocs: api compatibility, return correct status code_ - [apiShareOperationsToShares2/shareAccessByID.feature:162](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareOperationsToShares2/shareAccessByID.feature#L162) - [apiShareOperationsToShares2/shareAccessByID.feature:163](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareOperationsToShares2/shareAccessByID.feature#L163) -#### [[OC-storage] share-types field empty for shared file folder in webdav response](https://github.com/owncloud/ocis/issues/2144) -- [apiWebdavProperties2/getFileProperties.feature:215](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties2/getFileProperties.feature#L215) -- [apiWebdavProperties2/getFileProperties.feature:216](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties2/getFileProperties.feature#L216) -- [apiWebdavProperties2/getFileProperties.feature:221](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties2/getFileProperties.feature#L221) - -#### [Different share permissions provides varying roles in oc10 and ocis](https://github.com/owncloud/ocis/issues/1277) -- [apiWebdavProperties2/getFileProperties.feature:275](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties2/getFileProperties.feature#L275) -- [apiWebdavProperties2/getFileProperties.feature:276](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties2/getFileProperties.feature#L276) -- [apiWebdavProperties2/getFileProperties.feature:281](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties2/getFileProperties.feature#L281) - #### [Cannot move folder/file from one received share to another](https://github.com/owncloud/ocis/issues/2442) - [apiShareUpdateToShares/updateShare.feature:242](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareUpdateToShares/updateShare.feature#L242) - [apiShareUpdateToShares/updateShare.feature:196](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareUpdateToShares/updateShare.feature#L196) @@ -1407,9 +1397,6 @@ moving outside of the Shares folder gives 501 Not Implemented. - [apiWebdavOperations/search.feature:289](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavOperations/search.feature#L289) - [apiWebdavOperations/search.feature:314](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavOperations/search.feature#L314) -#### [Incorrect response while listing resources of a folder with depth infinity](https://github.com/owncloud/ocis/issues/3073) -- [apiWebdavOperations/listFiles.feature:182](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavOperations/listFiles.feature#L182) - #### [Cannot disable the dav propfind depth infinity for resources](https://github.com/owncloud/ocis/issues/3720) - [apiWebdavOperations/listFiles.feature:398](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavOperations/listFiles.feature#L398) - [apiWebdavOperations/listFiles.feature:399](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavOperations/listFiles.feature#L399) diff --git a/tests/acceptance/expected-failures-on-S3NG-storage.md b/tests/acceptance/expected-failures-on-S3NG-storage.md index 408f81139e..6b53ac9c6b 100644 --- a/tests/acceptance/expected-failures-on-S3NG-storage.md +++ b/tests/acceptance/expected-failures-on-S3NG-storage.md @@ -1307,16 +1307,6 @@ _ocs: api compatibility, return correct status code_ - [apiShareOperationsToShares2/shareAccessByID.feature:162](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareOperationsToShares2/shareAccessByID.feature#L162) - [apiShareOperationsToShares2/shareAccessByID.feature:163](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareOperationsToShares2/shareAccessByID.feature#L163) -#### [[OC-storage] share-types field empty for shared file folder in webdav response](https://github.com/owncloud/ocis/issues/2144) -- [apiWebdavProperties2/getFileProperties.feature:215](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties2/getFileProperties.feature#L215) -- [apiWebdavProperties2/getFileProperties.feature:216](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties2/getFileProperties.feature#L216) -- [apiWebdavProperties2/getFileProperties.feature:221](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties2/getFileProperties.feature#L221) - -#### [Different share permissions provides varying roles in oc10 and ocis](https://github.com/owncloud/ocis/issues/1277) -- [apiWebdavProperties2/getFileProperties.feature:275](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties2/getFileProperties.feature#L275) -- [apiWebdavProperties2/getFileProperties.feature:276](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties2/getFileProperties.feature#L276) -- [apiWebdavProperties2/getFileProperties.feature:281](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties2/getFileProperties.feature#L281) - #### [Cannot move folder/file from one received share to another](https://github.com/owncloud/ocis/issues/2442) - [apiShareUpdateToShares/updateShare.feature:242](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareUpdateToShares/updateShare.feature#L242) - [apiShareUpdateToShares/updateShare.feature:196](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiShareUpdateToShares/updateShare.feature#L196) @@ -1410,9 +1400,6 @@ _ocs: api compatibility, return correct status code_ - [apiWebdavOperations/search.feature:289](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavOperations/search.feature#L289) - [apiWebdavOperations/search.feature:314](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavOperations/search.feature#L314) -#### [Incorrect response while listing resources of a folder with depth infinity](https://github.com/owncloud/ocis/issues/3073) -- [apiWebdavOperations/listFiles.feature:182](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavOperations/listFiles.feature#L182) - #### [Cannot disable the dav propfind depth infinity for resources](https://github.com/owncloud/ocis/issues/3720) - [apiWebdavOperations/listFiles.feature:398](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavOperations/listFiles.feature#L398) - [apiWebdavOperations/listFiles.feature:399](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavOperations/listFiles.feature#L399) From b90148ed2457fe4133787ae3d463497331e4aa6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Fri, 1 Jul 2022 11:29:25 +0000 Subject: [PATCH 23/24] storageprovidercache: use stringbuffer to build statKey MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../services/gateway/storageprovidercache.go | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/internal/grpc/services/gateway/storageprovidercache.go b/internal/grpc/services/gateway/storageprovidercache.go index c72c6b0a1d..978006fd32 100644 --- a/internal/grpc/services/gateway/storageprovidercache.go +++ b/internal/grpc/services/gateway/storageprovidercache.go @@ -234,15 +234,25 @@ func statKey(user *userpb.User, ref *provider.Reference, metaDataKeys, fieldMask return "" } - key := "uid:" + user.Id.OpaqueId + "!sid:" + ref.ResourceId.StorageId + "!oid:" + ref.ResourceId.OpaqueId + "!path:" + ref.Path + key := strings.Builder{} + key.WriteString("uid:") + key.WriteString(user.Id.OpaqueId) + key.WriteString("!sid:") + key.WriteString(ref.ResourceId.StorageId) + key.WriteString("!oid:") + key.WriteString(ref.ResourceId.OpaqueId) + key.WriteString("!path:") + key.WriteString(ref.Path) for _, k := range metaDataKeys { - key += "!mdk:" + k + key.WriteString("!mdk:") + key.WriteString(k) } for _, p := range fieldMaskPaths { - key += "!fmp:" + p + key.WriteString("!fmp:") + key.WriteString(p) } - return key + return key.String() } // Stat looks in cache first before forwarding to storage provider From 0d040c3760ad95612516f1e2429674b3ab9aadba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Tue, 5 Jul 2022 09:21:43 +0000 Subject: [PATCH 24/24] comment cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- internal/http/services/owncloud/ocdav/locks.go | 1 - .../http/services/owncloud/ocdav/propfind/propfind.go | 4 ---- internal/http/services/owncloud/ocdav/report.go | 2 +- .../services/owncloud/ocdav/spacelookup/spacelookup.go | 5 ----- internal/http/services/owncloud/ocdav/versions.go | 2 +- pkg/storage/utils/decomposedfs/decomposedfs.go | 1 + pkg/storage/utils/decomposedfs/node/node.go | 4 ++++ pkg/storage/utils/decomposedfs/node/permissions.go | 9 +-------- 8 files changed, 8 insertions(+), 20 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/locks.go b/internal/http/services/owncloud/ocdav/locks.go index 3b048ccdd1..f7dc30cfcc 100644 --- a/internal/http/services/owncloud/ocdav/locks.go +++ b/internal/http/services/owncloud/ocdav/locks.go @@ -400,7 +400,6 @@ func (s *svc) handleSpacesLock(w http.ResponseWriter, r *http.Request, spaceID s span.SetAttributes(attribute.String("component", "ocdav")) - // build storage space reference ref, err := spacelookup.MakeStorageSpaceReference(spaceID, r.URL.Path) if err != nil { return http.StatusBadRequest, fmt.Errorf("invalid space id") diff --git a/internal/http/services/owncloud/ocdav/propfind/propfind.go b/internal/http/services/owncloud/ocdav/propfind/propfind.go index 768d577dd2..df90d295d4 100644 --- a/internal/http/services/owncloud/ocdav/propfind/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind/propfind.go @@ -310,7 +310,6 @@ func (p *Handler) HandleSpacesPropfind(w http.ResponseWriter, r *http.Request, s var space *provider.StorageSpace if res.Info.Space == nil { sublog.Debug().Msg("stat did not include a space, executing an additional lookup request") - // TODO look up space? hm can the isShared check even work when stating a space? hm yeah ... well ... *mindblown* // fake a space root space = &provider.StorageSpace{ Id: &provider.StorageSpaceId{OpaqueId: spaceID}, @@ -672,9 +671,6 @@ func (p *Handler) getSpaceResourceInfos(ctx context.Context, w http.ResponseWrit metadataKeys, _ := metadataKeys(pf) - // we need to prefix the path with / to make subsequent prefix matches work - // info.Path = filepath.Join("/", spaceRef.Path) - resourceInfos := []*provider.ResourceInfo{} req := &provider.ListContainerRequest{ diff --git a/internal/http/services/owncloud/ocdav/report.go b/internal/http/services/owncloud/ocdav/report.go index e5657ad87f..b5ad80a537 100644 --- a/internal/http/services/owncloud/ocdav/report.go +++ b/internal/http/services/owncloud/ocdav/report.go @@ -110,7 +110,7 @@ func (s *svc) doFilterFiles(w http.ResponseWriter, r *http.Request, ff *reportFi infos = append(infos, statRes.Info) } - responsesXML, err := propfind.MultistatusResponse(ctx, &propfind.XML{Prop: ff.Prop}, infos, s.c.PublicURL, namespace /*"",*/, nil) + responsesXML, err := propfind.MultistatusResponse(ctx, &propfind.XML{Prop: ff.Prop}, infos, s.c.PublicURL, namespace, nil) if err != nil { log.Error().Err(err).Msg("error formatting propfind") w.WriteHeader(http.StatusInternalServerError) diff --git a/internal/http/services/owncloud/ocdav/spacelookup/spacelookup.go b/internal/http/services/owncloud/ocdav/spacelookup/spacelookup.go index 93a8bee8ae..a7b4afe132 100644 --- a/internal/http/services/owncloud/ocdav/spacelookup/spacelookup.go +++ b/internal/http/services/owncloud/ocdav/spacelookup/spacelookup.go @@ -99,11 +99,6 @@ func LookUpStorageSpacesForPathWithChildren(ctx context.Context, client gateway. // TODO use ListContainerStream to listen for changes // retrieve a specific storage space lSSReq := &storageProvider.ListStorageSpacesRequest{ - Opaque: &typesv1beta1.Opaque{ - Map: map[string]*typesv1beta1.OpaqueEntry{ - // TODO encode requested metadata as json - // "metadata": {Decoder: "json", Value: []byte("*")}, - }}, // get all fields, including root_info FieldMask: &fieldmaskpb.FieldMask{Paths: []string{"*"}}, } diff --git a/internal/http/services/owncloud/ocdav/versions.go b/internal/http/services/owncloud/ocdav/versions.go index a65529e467..24e4696ff9 100644 --- a/internal/http/services/owncloud/ocdav/versions.go +++ b/internal/http/services/owncloud/ocdav/versions.go @@ -189,7 +189,7 @@ func (h *VersionsHandler) doListVersions(w http.ResponseWriter, r *http.Request, infos = append(infos, vi) } - propRes, err := propfind.MultistatusResponse(ctx, &pf, infos, s.c.PublicURL, "" /*"",*/, nil) + propRes, err := propfind.MultistatusResponse(ctx, &pf, infos, s.c.PublicURL, "", nil) if err != nil { sublog.Error().Err(err).Msg("error formatting propfind") w.WriteHeader(http.StatusInternalServerError) diff --git a/pkg/storage/utils/decomposedfs/decomposedfs.go b/pkg/storage/utils/decomposedfs/decomposedfs.go index 372f97f6e9..39a8694432 100644 --- a/pkg/storage/utils/decomposedfs/decomposedfs.go +++ b/pkg/storage/utils/decomposedfs/decomposedfs.go @@ -292,6 +292,7 @@ func (fs *Decomposedfs) CreateDir(ctx context.Context, ref *provider.Reference) if n, err = fs.lu.NodeFromResource(ctx, parentRef); err != nil { return } + // TODO check if user has access to root / space if !n.Exists { return errtypes.PreconditionFailed(parentRef.Path) } diff --git a/pkg/storage/utils/decomposedfs/node/node.go b/pkg/storage/utils/decomposedfs/node/node.go index 328e95d983..1f3ebb4c35 100644 --- a/pkg/storage/utils/decomposedfs/node/node.go +++ b/pkg/storage/utils/decomposedfs/node/node.go @@ -201,11 +201,15 @@ func ReadNode(ctx context.Context, lu PathLookup, spaceID, nodeID string, canLis } r.Exists = true + // TODO ReadNode should not check permissions if !canListDisabledSpace && r.IsDisabled() { // no permission = not found return nil, errtypes.NotFound(spaceID) } + // if current user cannot stat the root return not found? + // no for shares the root might be a different resource + // check if this is a space root if spaceID == nodeID { return r, nil diff --git a/pkg/storage/utils/decomposedfs/node/permissions.go b/pkg/storage/utils/decomposedfs/node/permissions.go index 618f23f274..8dc69a8189 100644 --- a/pkg/storage/utils/decomposedfs/node/permissions.go +++ b/pkg/storage/utils/decomposedfs/node/permissions.go @@ -181,13 +181,6 @@ func (p *Permissions) HasPermission(ctx context.Context, n *Node, check func(*pr return check(perms), nil } - // determine root - /* - if err = n.FindStorageSpaceRoot(); err != nil { - return false, err - } - */ - // for an efficient group lookup convert the list of groups to a map // groups are just strings ... groupnames ... or group ids ??? AAARGH !!! groupsMap := make(map[string]bool, len(u.Groups)) @@ -207,7 +200,7 @@ func (p *Permissions) HasPermission(ctx context.Context, n *Node, check func(*pr } } - // also check permissions on root, eg. for for project spaces + // also check permissions on root, eg. for project spaces return nodeHasPermission(ctx, cn, groupsMap, u.Id.OpaqueId, check), nil }