switch references #1721

butonic · 2021-05-20T22:36:48Z

A preview of the changes that need to be done when merging cs3org/cs3apis#130

currently uses a go.mod replace to use the generated go-cs3libs at butonic/go-cs3apis@2093bac

Initial notes

After merging this GetPath becomes obsolete because it was only used to merge a relative path with an id based reference that is stored in the share manager. With the new Reference type we can represent the combination with a single struct. Deferred to a subsequent PR.
When creating a stat response or listing the path needs to take into account the original reference: if it was an absolute path we need to return an absolute path. If it was a reference with a storage and node id we need to keep them, and just adjust the path.
- The ResourceInfo needs to hold both, a Reference that includes the name AND an Id property that holds a Reference for the actual file id (composed of storage id and node id) of the node
The storage provider must not know its mount point. That is the job of the registry/gateway. We adressed that in https://github.com/cs3org/reva/pull/1678/files#diff-198f1004a921b3627f7572a239452974429a7de6e4fa47f445c2ad35d2cd9026L1103 by resolving the resource using the storage.FS interface ... when really it should be done by the drivers, tracked in Separation of concerns: storage providers should not know about their mount path #578

update-docs · 2021-05-20T22:36:51Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

lgtm-com · 2021-05-20T23:46:37Z

This pull request introduces 20 alerts when merging 78e1e4b into 3aba8fa - view on LGTM.com

new alerts:

20 for Uncontrolled data used in path expression

labkode · 2021-05-21T06:56:40Z

It needs to be possible to obtain an absolute path from any reference

butonic · 2021-05-21T07:53:39Z

@labkode can you elaborate? The gateway is currently path based. The new reference would allow directly forwarding all requests if they have a storage_id set. The response for a stat or list container countains ResouceInfo elements, which should contain a path relative to the resource that was used to make the request.

That being said: when a request is made with an absolute path, all resulting references must (and will) be path based only again.

Is that what you mean?

refs · 2021-05-21T11:18:17Z

Reminder to update go.mod removing this replace github.com/cs3org/go-cs3apis => github.com/refs/go-cs3apis v0.0.0-20210520134557-97cdc479815b

ishank011 · 2021-05-21T12:56:41Z

pkg/storage/utils/eosfs/eosfs.go

 	u, err := getUser(ctx)
 	if err != nil {
 		return "", errors.Wrap(err, "eos: no user in ctx")
 	}

 	// parts[0] = 868317, parts[1] = photos, ...
-	parts := strings.Split(id.OpaqueId, "/")
+	// FIXME @butonic REFERENCE ... umm ... 868317/photos? @ishank011 HELP?!?
+	parts := strings.Split(id.NodeId, "/")


@butonic pretty sure we can change this to fileID, err := strconv.ParseUint(id.NodeId, 10, 64). I can't remember any scenario where we append the path to the inode. Plus looking at the code, GetPathByID is never used anyway, so we should be good

ok, but maybe not as part of this pr?

lgtm-com · 2021-05-25T08:48:52Z

This pull request introduces 20 alerts when merging ccc2403 into 2327bce - view on LGTM.com

new alerts:

20 for Uncontrolled data used in path expression

lgtm-com · 2021-05-25T10:19:00Z

This pull request introduces 20 alerts when merging 4af4a25 into 2327bce - view on LGTM.com

new alerts:

20 for Uncontrolled data used in path expression

labkode · 2021-05-25T11:48:29Z

@butonic the share manager will persist a share like:
share with id 123 points to resourceID abc:456.

A call to GetPathById(abc:456) is needed to obtain the path of that share and present it to the user.

A user won't access a share by doing ls /myshares/123. That is not user friendly.
I hope is clear. Ping me on Gitter if is not for a quicker chat.

lgtm-com · 2021-05-25T14:50:01Z

This pull request introduces 20 alerts when merging 8eac1b8 into 597e362 - view on LGTM.com

new alerts:

20 for Uncontrolled data used in path expression

lgtm-com · 2021-05-25T15:23:59Z

This pull request introduces 20 alerts when merging d18452f into 597e362 - view on LGTM.com

new alerts:

20 for Uncontrolled data used in path expression

butonic · 2021-05-25T18:42:06Z

@labkode right. I think the first goal should be to just replace the reference. No change in how the current path or id based references are used. /myshares/someid is not user friendly and AFAICT is nothing that this PR will touch or introduce.

The namespace that a user will see when using the CS3 api to list a constainer depends on the type of reference he uses. When he uses an absolute path all returned references will be absolute. When he uses a mixed reference (with id & path) the returned references will be relative to the id. But again, I think that is something that will become clearer when rebasing the spaces PR on this.

lgtm-com · 2021-05-25T19:02:49Z

This pull request introduces 20 alerts when merging 0d9cfe3 into b1d57b9 - view on LGTM.com

new alerts:

20 for Uncontrolled data used in path expression

lgtm-com · 2021-05-26T09:18:42Z

This pull request introduces 20 alerts when merging 8d48d98 into 3fd8259 - view on LGTM.com

new alerts:

20 for Uncontrolled data used in path expression

lgtm-com · 2021-05-26T09:57:55Z

This pull request introduces 20 alerts when merging 9dfa8ec into 3fd8259 - view on LGTM.com

new alerts:

20 for Uncontrolled data used in path expression

refs · 2021-05-26T12:54:37Z

@labkode @ishank011 We've come full circle with a topic: Should ResourceInfo contain 2 different References (see here), and if so, what would be a more descriptive name that carries semmantical meaning?

Some extracts from @butonic and I over RocketChat providing more context:

[...] in a stat request, the id must always be a Reference with storage_id and node_id set, but the response also needs to carry the name of the file

[...] think of RecourceInfo.Ref as Parent (either as id or as path) + Name

[...] a ResouceInfo needs two References: one is the Id of the Resource, the other is a Path to the Resource which can be given either as an absolute path, or as a relative path to a storage space.

If we really need both Reference members, their names should carry more semantic meaning. We thought of renaming ResourceInfo.Ref to ResourceInfo.Path. WDYT? It is not a huge of a deal, but adding more semantic meaning to the API should be always a goal.

butonic · 2021-05-26T13:20:43Z

note that ResourceInfo.Path was renamed to ResourceInfo.Ref in https://github.com/cs3org/cs3apis/pull/126/files#diff-b91600c359c2e479b2dbc31ff8dd5bd52bfaa48dc70c33a0de121215ff0c030eR68

I think we should leave the name Path but change the type from string to Reference

lgtm-com · 2021-05-26T14:48:26Z

This pull request introduces 20 alerts when merging 6805f8f into 3fd8259 - view on LGTM.com

new alerts:

20 for Uncontrolled data used in path expression

lgtm-com · 2021-05-26T15:14:09Z

This pull request introduces 20 alerts when merging 7e777ec into 3fd8259 - view on LGTM.com

new alerts:

20 for Uncontrolled data used in path expression

lgtm-com · 2021-05-26T20:53:39Z

This pull request introduces 20 alerts when merging f756df0 into 1c0c24a - view on LGTM.com

new alerts:

20 for Uncontrolled data used in path expression

butonic · 2021-05-27T06:25:56Z

@labkode @refs @ishank011 I think this can be merged (after review). But it needs to be updated with the newly generated go libs from the official cs3 api changes in cs3org/cs3apis#126. Let me know what you think.

micbar · 2021-05-31T09:29:52Z

@labkode @ishank011 can you review / merge?

labkode · 2021-06-02T08:18:46Z

@butonic @refs we need to differentiate between a resource ID and a Reference. A resource ID is unique mapping 1 to 1 to a resource. On the other hand, multiple reference can point to the same ResourceID. They are used for different purposes.

The ResourceInfo needs to contain a ResourceID and Path as before (no change here).
The Reference should only be used as a method to getting information.

From a ResourceInfo:ResourceID and ResourceInfo.Path you can always construct a Refence.

Please change the PR accordingly.

butonic · 2021-06-02T12:24:47Z

@butonic @refs we need to differentiate between a resource ID and a Reference. A resource ID is unique mapping 1 to 1 to a resource. On the other hand, multiple reference can point to the same ResourceID. They are used for different purposes.

✔️

The ResourceInfo needs to contain a ResourceID and Path as before (no change here).
The Reference should only be used as a method to getting information.

✔️

From a ResourceInfo:ResourceID and ResourceInfo.Path you can always construct a Refence.

Only when you know to which node the path is relative to. I wonder why we need a path at all, and not just the basename of the resource. When listing a resource the Reference used to make the determines the path to the resource, anyway. A client listing /path/to/resource already knows that all children are prefixed with that path, e.g. /path/to/resource/child. Why is a name property with child not enough? I know that the gateway currently always looks up the path for a reference. When the reference holds a spaceid we don't need to do that and can just forward the request to the correct storage provider. When listing the available spaces clients always will have references with a space id.

I used a Reference for Path, because there is a corner case for the spaces api: when making an id based request we need to know the parent id so that we can build the full path relative to the storage space root. So I used a Reference to return both, the Id of the parent and the name of the child.

In any case the same reasoning applies to all cases: path, id based and mixed requests. The returned path should be relative to the reference that was used to make the request. But that makes lookung up the path for an id impossible.

Please change the PR accordingly.

I will try to use a path and check with our spaces implementation. IIRC we need to be able to get to parent from an id only based reference.

lgtm-com · 2021-06-07T12:22:41Z

This pull request introduces 20 alerts when merging 7b5bce9 into 6ace58c - view on LGTM.com

new alerts:

20 for Uncontrolled data used in path expression

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

lgtm-com · 2021-06-15T09:48:45Z

This pull request introduces 20 alerts when merging 91a9c7b into 07adb3f - view on LGTM.com

new alerts:

20 for Uncontrolled data used in path expression

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

lgtm-com · 2021-06-15T10:28:50Z

This pull request introduces 20 alerts when merging fb291f1 into 07adb3f - view on LGTM.com

new alerts:

20 for Uncontrolled data used in path expression

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

lgtm-com · 2021-06-15T11:31:05Z

This pull request introduces 20 alerts when merging bc66e39 into 07adb3f - view on LGTM.com

new alerts:

20 for Uncontrolled data used in path expression

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

lgtm-com · 2021-06-15T12:39:53Z

This pull request introduces 20 alerts when merging 9f231fe into 07adb3f - view on LGTM.com

new alerts:

20 for Uncontrolled data used in path expression

labkode · 2021-06-17T08:33:28Z

@butonic can you make LGTM happy as well? I think is important from a security perspective, specially when dealing with files.

ishank011

Hi @butonic. Just a couple of minor comments

ishank011 · 2021-06-17T10:26:06Z

pkg/sdk/action/download.go

-			},
-		},
-	}
+	req := &provider.InitiateFileDownloadRequest{Ref: &provider.Reference{ResourceId: fileInfo.Id}}


Let's leave this as Path: fileInfo.Path?

initiateDownload uses a ResourceInfo. The only stable identifier we get from that is the id. Maybe it should use a new Reference as the parameter?

func (action *DownloadAction) Download(fileInfo *storage.ResourceInfo) ([]byte, error) {

also should use a Reference?

Hm taking a look at:

func (action *DownloadAction) DownloadFile(path string) ([]byte, error) { // Get the ResourceInfo object of the specified path fileInfoAct := MustNewFileOperationsAction(action.session) info, err := fileInfoAct.Stat(path) if err != nil { return nil, fmt.Errorf("the path '%v' was not found: %v", path, err) } return action.Download(info) }

shouldn't Download return a not found error? we should not make an extra stat call IMO. between the stat and the actual download someone else might have deleted the file...

I can't really say, haven't looked at this code

pkg/storage/fs/owncloud/owncloud.go

pkg/storage/fs/s3/s3.go

internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go

butonic · 2021-06-17T13:36:20Z

@labkode the file traversal cases are in progress by @C0rby
@ishank011 me and @C0rby will address your issues asap!

lgtm-com · 2021-06-17T14:48:00Z

This pull request introduces 20 alerts when merging eb98f1d into 692f38f - view on LGTM.com

new alerts:

20 for Uncontrolled data used in path expression

pkg/utils/utils.go

pkg/storage/fs/owncloud/owncloud.go

lgtm-com · 2021-06-17T15:27:00Z

This pull request introduces 20 alerts when merging ea4cf92 into 692f38f - view on LGTM.com

new alerts:

20 for Uncontrolled data used in path expression

lgtm-com · 2021-06-17T15:47:03Z

This pull request introduces 20 alerts when merging b61df27 into 692f38f - view on LGTM.com

new alerts:

20 for Uncontrolled data used in path expression

author Jörn Friedrich Dreyer <jfd@butonic.de> 1623945204 +0200 committer Jörn Friedrich Dreyer <jfd@butonic.de> 1623958936 +0000 switch references (cs3org#1721) Co-authored-by: A.Unger <zyxancf@gmail.com> Co-authored-by: David Christofas <dchristofas@owncloud.com> rebase Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> fix nil Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> update changelog Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> add storage provider list spaces interface Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> add stubs Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> initial ocis implementation for list storage spaces Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> minor fixes Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> add thoughts on proper spaces persistence layout Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> Initial spaces implementation. Signed-off-by: Klaas Freitag <kfreitag@owncloud.com> more spaces work Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> filter spaces based on permissions, return name Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> resolve linter issues implement storage space support into the storageprovider Co-authored-by: Jörn Friedrich Dreyer <jfd@butonic.de> fix dav spaces href Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> GET preparations Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> WIP: spaces datatx Add spaces.go distinguish spaces/simple datatx Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> make GET work for spaces Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> MKCol implementation for spaces WIP refactor CreateDir Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> make MKCOL work for spaces implement delete for the spaces api fix: unwrap the requested reference before using it further implement MOVE for spaces simplify check if request body is empty setup constants for webdav verbs fix listcontainers for spaces references implement PROPPATCH for spaces implement COPY for spaces add cases for lock, unlock and report for spaces implement PUT for spaces implement POST for spaces implement HEAD for spaces clean up and deduplicate webdav HEAD code clean up and deduplicate webdav DELETE code clean up and deduplicate webdav GET code clean up and deduplicate webdav PROPFIND code clean up and deduplicate webdav MKCOL code clean up and deduplicate webdav MOVE code clean up and deduplicate webdav PROPPATCH code clean up and deduplicate webdav COPY code clean up and deduplicate webdav TUS POST code clean up and deduplicate webdav PUT code try fixing tests Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> fix path checks before calling WalkPath update the owncloudsql storage driver fix spaceid handling Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> fix linter issues fix CreateDir in the owncloud storage driver fix non space path handling Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> allow creating empty files Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> no longer hardcode storageid in the driver Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> decomposedfs: create storage spaces Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> Id -> ID Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> move change-references changelog to unreleased add spaces changelog Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> add grant test for spaces goimports update to embedded reference Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> relative references should only return the basename Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> do not show shared resources as spaces for owners Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> make sure the usage of user provides paths is secure list spaces from all providers Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

butonic requested a review from labkode as a code owner May 20, 2021 22:36

ishank011 reviewed May 21, 2021

View reviewed changes

refs mentioned this pull request May 26, 2021

[WIP] New References owncloud/ocis#2081

Closed

butonic force-pushed the new-reference branch from 7e777ec to f756df0 Compare May 26, 2021 20:31

butonic changed the title ~~[WIP] switch references~~ switch references May 27, 2021

butonic force-pushed the new-reference branch from f756df0 to 7b5bce9 Compare June 7, 2021 11:55

refs and others added 2 commits June 15, 2021 09:38

start updating references

9aa86a2

continue update

4873399

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

rebase

22ecda8

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

butonic force-pushed the new-reference branch from 91a9c7b to 22ecda8 Compare June 15, 2021 09:59

fix nil

fb291f1

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

fix trash

bc66e39

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

update changelog

9f231fe

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

ishank011 reviewed Jun 17, 2021

View reviewed changes

introduce util methods for references

1701ed5

David Christofas added 2 commits June 17, 2021 17:02

clean critical paths before doing filesystem operations

07a4d39

address review comments

ea4cf92

C0rby force-pushed the new-reference branch from eb98f1d to ea4cf92 Compare June 17, 2021 15:05

ishank011 reviewed Jun 17, 2021

View reviewed changes

pkg/utils/utils.go Outdated Show resolved Hide resolved

pkg/storage/fs/owncloud/owncloud.go Show resolved Hide resolved

remove duplicate method

b61df27

ishank011 approved these changes Jun 17, 2021

View reviewed changes

ishank011 merged commit 9740eed into cs3org:master Jun 17, 2021

C0rby mentioned this pull request Jun 23, 2021

[full-ci] update reva to v1.9 owncloud/ocis#2210

Merged

butonic deleted the new-reference branch July 8, 2021 20:36

butonic mentioned this pull request Sep 17, 2021

enable archive download by resourceid owncloud/ocis#2521

Closed

switch references #1721

switch references #1721

Conversation

butonic commented May 20, 2021 • edited Loading

update-docs bot commented May 20, 2021

lgtm-com bot commented May 20, 2021

labkode commented May 21, 2021

butonic commented May 21, 2021 • edited Loading

refs commented May 21, 2021

ishank011 May 21, 2021

Choose a reason for hiding this comment

butonic Jun 9, 2021

Choose a reason for hiding this comment

lgtm-com bot commented May 25, 2021

lgtm-com bot commented May 25, 2021

labkode commented May 25, 2021

lgtm-com bot commented May 25, 2021

lgtm-com bot commented May 25, 2021

butonic commented May 25, 2021

lgtm-com bot commented May 25, 2021

lgtm-com bot commented May 26, 2021

lgtm-com bot commented May 26, 2021

refs commented May 26, 2021

butonic commented May 26, 2021

lgtm-com bot commented May 26, 2021

lgtm-com bot commented May 26, 2021

lgtm-com bot commented May 26, 2021

butonic commented May 27, 2021

micbar commented May 31, 2021

labkode commented Jun 2, 2021

butonic commented Jun 2, 2021 • edited Loading

lgtm-com bot commented Jun 7, 2021

lgtm-com bot commented Jun 15, 2021

lgtm-com bot commented Jun 15, 2021

lgtm-com bot commented Jun 15, 2021

lgtm-com bot commented Jun 15, 2021

labkode commented Jun 17, 2021

ishank011 left a comment

Choose a reason for hiding this comment

ishank011 Jun 17, 2021

Choose a reason for hiding this comment

butonic Jun 17, 2021

Choose a reason for hiding this comment

butonic Jun 17, 2021

Choose a reason for hiding this comment

ishank011 Jun 17, 2021

Choose a reason for hiding this comment

butonic commented Jun 17, 2021 • edited Loading

lgtm-com bot commented Jun 17, 2021

lgtm-com bot commented Jun 17, 2021

lgtm-com bot commented Jun 17, 2021

butonic commented May 20, 2021 •

edited

Loading

butonic commented May 21, 2021 •

edited

Loading

butonic commented Jun 2, 2021 •

edited

Loading

butonic commented Jun 17, 2021 •

edited

Loading