Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

TUS upload support through datagateway #878

Merged
merged 8 commits into from
Jun 25, 2020

Conversation

kulmann
Copy link
Member

@kulmann kulmann commented Jun 24, 2020

In oCIS we want to be able to upload data through one single entrypoint and let the datagateway take care of dispatching the request to the respective storage data server. Reva already supports this by encoding the upload endpoint in an X-Reva-Transfer token.
This PR is about four things:

  1. TUS clients don't understand the reva transfer token. Instead they expect to just work with a URL. In order to fill this gap, the tus code in ocdav now appends the token to the upload endpoint when the storages data servers are not exposed (i.e. as soon as a transfer token exists). Of course we want reva to still continue to work with the token. To achieve this, the datagateway now checks if a transfer token is present in the token verification step. If it's not present, we use the last segment of the request path as the transfer token.
  2. Bring basic PATCH request support to the datagateway.
  3. TUS code in ocdav didn't set the reva transfer token on the header in cases where it can actually be utilized. This is fixed in put.go.
  4. Remove the hardcoded ExposeDataServer = true.

Also one small change: I've changed the datagateway to expose the X-Reva-Transfer constant and used it in all places where the string was hardcoded before. Makes it easier to find code fragments where the transfer token is used.

Happy to hear feedback on this.

@kulmann
Copy link
Member Author

kulmann commented Jun 24, 2020

cc @PVince81 @butonic

@labkode
Copy link
Member

labkode commented Jun 25, 2020

@kulmann nice work. Happy to merge after approval also from @PVince81 or @butonic.

@labkode labkode requested a review from butonic June 25, 2020 06:37
@PVince81
Copy link
Contributor

PVince81 commented Jun 25, 2020

@kulmann how was this tested? please list scenarios (ex phoenix, cli, etc)

@kulmann
Copy link
Member Author

kulmann commented Jun 25, 2020

@kulmann how was this tested? please list scenarios (ex phoenix, cli, etc)

Together with this PR in ocis-reva which changes default configs to run uploads through the proxy and PR in ocis-proxy which shows the required /data endpoint in the example config, I made successful uploads:

  1. with regular file upload through ocis-web (former phoenix)
  2. with curl: curl -X PUT https://localhost:9200/remote.php/dav/files/einstein/test/newfile1.txt -u einstein:relativity -d 'Hello World!' -k -v

@PVince81

@@ -238,3 +248,69 @@ func (s *svc) doPut(w http.ResponseWriter, r *http.Request) {
log.Err(err).Msg("error writing body after header were set")
}
}

// TODO: put and post code is pretty much the same. Should be solved in a nicer way in the long run.
Copy link
Contributor

Choose a reason for hiding this comment

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

@labkode I was pairing on this with @kulmann and while the patch works we should use the simplehttpproxy from the stdlib here ... @labkode we can look into that after this pr.

@PVince81
Copy link
Contributor

Ok, I've tested TUS upload with Phoenix and it worked fine 👍

Copy link
Contributor

@butonic butonic left a comment

Choose a reason for hiding this comment

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

we need to allow self signed certs ...

2020-06-25T10:23:39+02:00 ERR Could not start TUS upload error="Patch \"https://localhost:9200/data\": x509: certificate signed by unknown authority" pkg=rhttp service=reva traceid=a29cb66d20fc4f6bf38eea42edff4d25

Copy link
Contributor

@butonic butonic left a comment

Choose a reason for hiding this comment

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

allowing self signed certificates in #880

kulmann added 8 commits June 25, 2020 11:20
No-go to have a config variable and then render it useless by hardcoding
it to true.
Since TUS clients don't understand the reva transfer token, we need to
make sure that the upload endpoint comes out as a URL that encodes the
full endpoint path, not only the data gateway path.
@kulmann kulmann force-pushed the allow-transfer-token-in-url branch from 33775ca to 8d0105c Compare June 25, 2020 09:20
@butonic
Copy link
Contributor

butonic commented Jun 25, 2020

rebasing fixed config issues. good to merge!

@labkode labkode merged commit 743296a into cs3org:master Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants