webdav: only require locks when necessary #93
Conversation
The WebDAV RFC is pretty clear that if the source and destination of a MOVE request are locked, then the request must include lock tokens for both. It doesn't explicitly state that if only one is locked, the request need only include a token for the one, but that seems fairly obvious: if the destination of an operation is not locked, a client should be able to copy or move a locked resource over top of it, particularly if it doesn't exist in the first place. Prior to this commit, the lock validation logic required that both the source and destination of an operation hold a lock if any locks were presented. Consequently, binary operations where only one of the resources were locked would always fail. For example, the following sequence of events would fail with a 412 Precondition Failed: - LOCK /foo - MOVE /foo (Destination: /bar) This commit changes the lock validation to only require locks for resources which are actually locked. It takes some care to ensure that invalid lock tokens will cause a failure, even if the resources did not require locks, since both the litmus test and RFC imply that if there are any conditions, and all conditions fail, the request should fail. Fixes golang/go#43556
| // In this case, we have created temporary locks on any resource we care about. | ||
| // This means that none of the conditions can evaluate to true. Fall through with | ||
| // an empty list to ensure consistent error handling | ||
| ih.lists = []ifList{} |
There was a problem hiding this comment.
I'm not a huge fan of this -- it feels a little messy to depend on the behaviour of atLeastOnIfListPasses, but I think the newly created functions name + the comment make it clear enough what's expected.
|
This PR (HEAD: 15084bc) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/net/+/285754 to see it. Tip: You can toggle comments from me using the |
|
|
||
| func TestMoveLockedSrcUnlockedDst(t *testing.T) { | ||
| // This test reproduces https://github.com/golang/go/issues/43556 | ||
| do := func(method, urlStr string, body string, wantStatusCode int, headers ...string) (http.Header, error) { |
There was a problem hiding this comment.
I think these tests could do with some refactoring. I tend to favour writing simple test harnesses that handle much of the common behaviour between tests. In this case, I think we could refactor the 'do' function as well as the server setup. That would simplify both the existing tests, and any new ones which use a similar pattern. I was going to do it in this PR, but it probably makes more sense to do it in a separate one specific to that refactoring. Let me know if you're interested.
We added a test that depends on this variable. Move it out.
|
This PR (HEAD: 8561cdb) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/net/+/285754 to see it. Tip: You can toggle comments from me using the |
Our net/webdav branch now include the following patches: golang/net#92 golang/net#93 golang/net#94
Our net/webdav branch now include the following patches: golang/net#92 golang/net#93 golang/net#94
|
@klarose thanks for this, MS Excel on Windows cannot save its files to a webdav server that is running |
The WebDAV RFC is pretty clear that if the source and destination of a
MOVE request are locked, then the request must include lock tokens for
both. It doesn't explicitly state that if only one is locked, the
request need only include a token for the one, but that seems fairly
obvious: if the destination of an operation is not locked, a client
should be able to copy or move a locked resource over top of it,
particularly if it doesn't exist in the first place.
Prior to this commit, the lock validation logic required that both the
source and destination of an operation hold a lock if any locks were
presented. Consequently, binary operations where only one of the
resources were locked would always fail. For example, the following
sequence of events would fail with a 412 Precondition Failed:
This commit changes the lock validation to only require locks for
resources which are actually locked. It takes some care to ensure that
invalid lock tokens will cause a failure, even if the resources did not
require locks, since both the litmus test and RFC imply that if there
are any conditions, and all conditions fail, the request should fail.
Fixes golang/go#43556