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

Support webdav in WebSession #968

Merged
merged 24 commits into from
Oct 6, 2015
Merged

Support webdav in WebSession #968

merged 24 commits into from
Oct 6, 2015

Conversation

kentonv
Copy link
Member

@kentonv kentonv commented Oct 3, 2015

@mnutt, this is based on your PR, but I added a bunch of changes. Can you review my commits (which are hopefully clear and organized) and also test that this actually works? You'll need to rebuild your spk with the new http-bridge, of course.

If I broke stuff -- which I'd estimate 75% likely that I did -- then can you either: (a) debug, or (b) give me precise instructions on how I can build your app and test it myself?

Thanks!

mnutt and others added 23 commits August 30, 2015 22:13
As part of adding webdav support, this also adds a whitelist of allowed request headers.
fixes placement of whitelisting comment as well
We use a boolean parameter to determine whether the caller wants the entity body.

This makes life easier for people trying to implement the raw API -- they don't necessarily need to pay attention to this parameter, and the front-end will drop the content for them.

It also means that existing spks will properly support HEAD without changes.
Many of these methods take some XML as the input, so we might as well pass that as a string. (In theory we should actually represent the XML structure but no way am I going to add an XML parser to our dependencies).

The UNLOCK method appears to specifically take a "lock token" as input (in fact it appears before this change the token was being lost).

For the couple of methods that take arbitrary content, I'm reusing PostContent. When we do an API revision, we should replace PostContent and PutContent with a single Content struct, rather than repeat ourselves.
We explicitly specify what options can be expressed -- currently, only the DAV header, which can only have certain values. The OPTIONS method can have unexpected security implications so we want to limit what apps can do with it.

By my reading of the spec, the DAV header is only relevant for OPTIONS requests, so remove support for it from other requests.
noContent can now go back to not specifying a status code.
These only apply to certain methods. This way we can keep them out of the whitelist, in case they are assigned a different meaning in other contexts in the future. Also it makes it easier to understand the inputs to the methods to which they apply.
@kentonv
Copy link
Member Author

kentonv commented Oct 3, 2015

Oh, I probably should have brought this up earlier, but also @mnutt can you sign the CLA:

https://sandstorm.io/cla/

Note that this is the same CLA that the Apache Foundation uses.

@kentonv
Copy link
Member Author

kentonv commented Oct 3, 2015

Garply, retest this please.

@mnutt
Copy link
Contributor

mnutt commented Oct 6, 2015

This actually all works perfectly. I tested it with my webdav client and backend and everything worked. Unfortunately my backend is only Class 1, so there are Class 2 things I can't test, but it works great for my use case. Thanks for finishing it!

The way I tested it was to use this spk: https://s3.amazonaws.com/mnutt-misc/davros-0.8.0.spk and litmus (webdav unit test suite) : http://www.webdav.org/neon/litmus/ And then after building litmus run

litmus "http://api.local.sandstorm.io:6080/remote.php/webdav/" bearer <WEBKEY_HASH>

(unfortunately early on OwnCloud hard-coded remote.php in; it's something I just have to live with for interoperability)

@kentonv
Copy link
Member Author

kentonv commented Oct 6, 2015

OK, I'm going to go ahead and merge this so it can go through our testing cycle.

I sort of suspect that we'll turn up some bugs later on, but we can submit fixes then.

kentonv added a commit that referenced this pull request Oct 6, 2015
Support webdav in WebSession
@kentonv kentonv merged commit 7753477 into master Oct 6, 2015
@kentonv kentonv deleted the webdav branch October 6, 2015 05:43
@paulproteus
Copy link
Collaborator

paulproteus commented Oct 6, 2015 via email

@kentonv
Copy link
Member Author

kentonv commented Oct 6, 2015

@mnutt I am seeing a failure with litmus and the spk you linked:

2. options............... FAIL (server does not claim WebDAV compliance)

It looks like the app is in fact returning false for all davClass booleans in response to the options() call, for the path /remote.php/webdav/litmus/. Any idea why that might be?

All the other tests pass, though.

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.

None yet

3 participants