Skip to content

webdav: add support for locks#190

Draft
krystiancha wants to merge 7 commits intoemersion:masterfrom
krystiancha:lock
Draft

webdav: add support for locks#190
krystiancha wants to merge 7 commits intoemersion:masterfrom
krystiancha:lock

Conversation

@krystiancha
Copy link
Contributor

Supersedes: #187

This is my attempt to partially implement exclusive resource locks to make some clients happy.

Included in this PR: LOCK and UNLOCK methods (partially, see below), lock checking on write operations.

Not included in this PR: locking unmapped URLs, collection locking (indirect locks, lock depth), lock timeouts, active lock discovery, preserving owner information, shared locks, checking if current principal matches lock creator, complex If header conditions.

I've tested this using Windows Explorer and litmus through sogogi (full report). I also added some test cases for the bugs I encountered along the way.

Litmus test summary

PROPPATCH fails because it's not implemented.

-> running `locks':
 0. begin................. pass
 1. options............... pass
 2. precond............... pass
 3. init_locks............ pass
 4. put................... pass
 5. lock_excl............. pass
 6. discover.............. pass
 7. refresh............... pass
 8. notowner_modify....... WARNING: PROPPATCH failed with 0 not 423
    ...................... pass (with 1 warning)
 9. notowner_lock......... pass
10. owner_modify.......... FAIL (PROPPATCH on locked resource on `/litmus/lockme': /litmus/lockme: 403 Forbidden)
11. notowner_modify....... WARNING: PROPPATCH failed with 0 not 423
    ...................... pass (with 1 warning)
12. notowner_lock......... pass
13. copy.................. pass
14. cond_put.............. FAIL (PUT conditional on lock and etag failed: 400 Bad Request)
15. fail_cond_put......... FAIL (conditional PUT with invalid lock-token code got 400)
16. cond_put_with_not..... FAIL (PUT with conditional (Not <DAV:no-lock>) failed: 400 Bad Request)
17. cond_put_corrupt_token WARNING: PUT failed with 400 not 423
    ...................... pass (with 1 warning)
18. complex_cond_put...... FAIL (PUT with complex conditional failed: 400 Bad Request)
19. fail_complex_cond_put. FAIL (PUT with complex bogus conditional should fail with 412: 400 Bad Request)
20. unlock................ pass
21. fail_cond_put_unlocked FAIL (conditional PUT with invalid lock-token should fail: 204 No Content)
22. lock_shared........... FAIL (LOCK on `/litmus/lockme': 400 Bad Request)
23. notowner_modify....... SKIPPED
24. notowner_lock......... SKIPPED
25. owner_modify.......... SKIPPED
26. double_sharedlock..... SKIPPED
27. notowner_modify....... SKIPPED
28. notowner_lock......... SKIPPED
29. unlock................ SKIPPED
30. prep_collection....... pass
31. lock_collection....... FAIL (LOCK on `/litmus/lockcoll/': 400 Bad Request)
32. owner_modify.......... SKIPPED
33. notowner_modify....... SKIPPED
34. refresh............... SKIPPED
35. indirect_refresh...... SKIPPED
36. unlock................ SKIPPED
37. unmapped_lock......... FAIL (LOCK on /litmus/lockcoll/ via /litmus/unmapped_url: 404 Not Found)
38. unlock................ SKIPPED
39. finish................ pass
-> 13 tests were skipped.
<- summary for `locks': of 27 tests run: 17 passed, 10 failed. 63.0%
-> 3 warnings were issued.

I'm not sure about introducing github.com/google/uuid dependency for generating lock tokens. Please let me know if there's a better way to do this.

// server.
type Handler struct {
FileSystem FileSystem

Copy link

Choose a reason for hiding this comment

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

I apologize for coming uninvited; I am somewhat certain this is not the best practice. The ideal implementation should be to fully determine the Locks interface first, and then implement b := backend{FileSystem: h.FileSystem, LockSystem: h.LockSystem} here. Additionally, h.LockSystem should default to MemoryLockSystem (a new independently implemented struct that conforms to the LockSystem interface).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On IRC @emersion suggested that ideally webdav.FileSystem should implement locking. I'm not sure which design would be better. I think we should also mind how other WebDAV RFCs would be implemented. For example ACLs (RFC 3744). Should we bubble everything to webdav.FileSystem?

Copy link

Choose a reason for hiding this comment

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

Should we bubble everything to webdav.FileSystem?

This approach is reasonable, but I believe we should still retain the ability to inject custom lockers to enhance extensibility. For instance, users might want to implement a locker using Redis to synchronize state across multiple gateway services accessing the same storage backend. However, if we expect users to inject an entire file system with built-in locking capabilities from the start, that would be a different matter entirely.

Without implementing MarshalText, these elements are marshaled as
integers, so no "Second-xx" or "infinity".
Lock refresh requests have an empty body, so we shouldn't return error
on empty lockscope or locktype.
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.

3 participants