diff --git a/webdav/lock.go b/webdav/lock.go index 215d6dd4..0dc22682 100644 --- a/webdav/lock.go +++ b/webdav/lock.go @@ -103,13 +103,6 @@ type LockDetails struct { // Root is the root resource name being locked. For a zero-depth lock, the // root is the only resource being locked. Root string - // Depth is the lock depth. A negative depth means infinite. - // - // TODO: should depth be restricted to just "0 or infinite" (i.e. change - // this field to "Recursive bool") or just "0 or 1 or infinite"? Is - // validating that the responsibility of the Handler or the LockSystem - // implementations? - Depth int // Duration is the lock timeout. A negative duration means infinite. Duration time.Duration // OwnerXML is the verbatim XML given in a LOCK HTTP request. @@ -118,6 +111,9 @@ type LockDetails struct { // Does the OwnerXML field need to have more structure? See // https://codereview.appspot.com/175140043/#msg2 OwnerXML string + // ZeroDepth is whether the lock has zero depth. If it does not have zero + // depth, it has infinite depth. + ZeroDepth bool } // NewMemLS returns a new in-memory LockSystem. @@ -161,7 +157,7 @@ func (m *memLS) Create(now time.Time, details LockDetails) (string, error) { m.collectExpiredNodes(now) name := path.Clean("/" + details.Root) - if !m.canCreate(name, details.Depth) { + if !m.canCreate(name, details.ZeroDepth) { return "", ErrLocked } n := m.create(name) @@ -205,7 +201,7 @@ func (m *memLS) Unlock(now time.Time, token string) error { return nil } -func (m *memLS) canCreate(name string, depth int) bool { +func (m *memLS) canCreate(name string, zeroDepth bool) bool { return walkToRoot(name, func(name0 string, first bool) bool { n := m.byName[name0] if n == nil { @@ -216,12 +212,12 @@ func (m *memLS) canCreate(name string, depth int) bool { // The target node is already locked. return false } - if depth < 0 { + if !zeroDepth { // The requested lock depth is infinite, and the fact that n exists // (n != nil) means that a descendent of the target node is locked. return false } - } else if n.token != "" && n.details.Depth < 0 { + } else if n.token != "" && !n.details.ZeroDepth { // An ancestor of the target node is locked with infinite depth. return false } diff --git a/webdav/lock_test.go b/webdav/lock_test.go index 090314b1..0ae53082 100644 --- a/webdav/lock_test.go +++ b/webdav/lock_test.go @@ -78,12 +78,12 @@ var lockTestNames = []string{ "/z/_/z", } -func lockTestDepth(name string) int { +func lockTestZeroDepth(name string) bool { switch name[len(name)-1] { case 'i': - return -1 + return false case 'z': - return 0 + return true } panic(fmt.Sprintf("lock name %q did not end with 'i' or 'z'", name)) } @@ -94,16 +94,16 @@ func TestMemLSCanCreate(t *testing.T) { for _, name := range lockTestNames { _, err := m.Create(now, LockDetails{ - Depth: lockTestDepth(name), - Duration: -1, - Root: name, + Root: name, + Duration: -1, + ZeroDepth: lockTestZeroDepth(name), }) if err != nil { t.Fatalf("creating lock for %q: %v", name, err) } } - wantCanCreate := func(name string, depth int) bool { + wantCanCreate := func(name string, zeroDepth bool) bool { for _, n := range lockTestNames { switch { case n == name: @@ -112,7 +112,7 @@ func TestMemLSCanCreate(t *testing.T) { case strings.HasPrefix(n, name): // An existing lock would be a child of the proposed lock, // which conflicts if the proposed lock has infinite depth. - if depth < 0 { + if !zeroDepth { return false } case strings.HasPrefix(name, n): @@ -128,11 +128,11 @@ func TestMemLSCanCreate(t *testing.T) { var check func(int, string) check = func(recursion int, name string) { - for _, depth := range []int{-1, 0} { - got := m.canCreate(name, depth) - want := wantCanCreate(name, depth) + for _, zeroDepth := range []bool{false, true} { + got := m.canCreate(name, zeroDepth) + want := wantCanCreate(name, zeroDepth) if got != want { - t.Errorf("canCreate name=%q depth=%d: got %t, want %t", name, depth, got, want) + t.Errorf("canCreate name=%q zeroDepth=%d: got %t, want %t", name, zeroDepth, got, want) } } if recursion == 6 { @@ -162,9 +162,9 @@ func TestMemLSCreateUnlock(t *testing.T) { tokens[name] = "" } else { token, err := m.Create(now, LockDetails{ - Depth: lockTestDepth(name), - Duration: -1, - Root: name, + Root: name, + Duration: -1, + ZeroDepth: lockTestZeroDepth(name), }) if err != nil { t.Fatalf("iteration #%d: create %q: %v", i, name, err) diff --git a/webdav/webdav.go b/webdav/webdav.go index 1a31bd00..c6ddc974 100644 --- a/webdav/webdav.go +++ b/webdav/webdav.go @@ -38,6 +38,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { status, err = http.StatusInternalServerError, errNoLockSystem } else { // TODO: COPY, MOVE, PROPFIND, PROPPATCH methods. + // MOVE needs to enforce its Depth constraint. See the parseDepth comment. switch r.Method { case "OPTIONS": status, err = h.handleOptions(w, r) @@ -217,20 +218,22 @@ func (h *Handler) handleLock(w http.ResponseWriter, r *http.Request) (retStatus } } else { - depth, err := parseDepth(r.Header.Get("Depth")) - if err != nil { - return http.StatusBadRequest, err - } - if depth > 0 { - // Section 9.10.3 says that "Values other than 0 or infinity must not be - // used with the Depth header on a LOCK method". - return http.StatusBadRequest, errInvalidDepth + // Section 9.10.3 says that "If no Depth header is submitted on a LOCK request, + // then the request MUST act as if a "Depth:infinity" had been submitted." + depth := infiniteDepth + if hdr := r.Header.Get("Depth"); hdr != "" { + depth = parseDepth(hdr) + if depth != 0 && depth != infiniteDepth { + // Section 9.10.3 says that "Values other than 0 or infinity must not be + // used with the Depth header on a LOCK method". + return http.StatusBadRequest, errInvalidDepth + } } ld = LockDetails{ - Depth: depth, - Duration: duration, - OwnerXML: li.Owner.InnerXML, - Root: r.URL.Path, + Root: r.URL.Path, + Duration: duration, + OwnerXML: li.Owner.InnerXML, + ZeroDepth: depth == 0, } token, err = h.LockSystem.Create(now, ld) if err != nil { @@ -288,9 +291,29 @@ func (h *Handler) handleUnlock(w http.ResponseWriter, r *http.Request) (status i } } -func parseDepth(s string) (int, error) { - // TODO: implement. - return -1, nil +const ( + infiniteDepth = -1 + invalidDepth = -2 +) + +// parseDepth maps the strings "0", "1" and "infinity" to 0, 1 and +// infiniteDepth. Parsing any other string returns invalidDepth. +// +// Different WebDAV methods have further constraints on valid depths: +// - PROPFIND has no further restrictions, as per section 9.1. +// - MOVE accepts only "infinity", as per section 9.2.2. +// - LOCK accepts only "0" or "infinity", as per section 9.10.3. +// These constraints are enforced by the handleXxx methods. +func parseDepth(s string) int { + switch s { + case "0": + return 0 + case "1": + return 1 + case "infinity": + return infiniteDepth + } + return invalidDepth } func parseTimeout(s string) (time.Duration, error) { diff --git a/webdav/xml.go b/webdav/xml.go index c48fc0fe..08085d57 100644 --- a/webdav/xml.go +++ b/webdav/xml.go @@ -13,7 +13,6 @@ import ( "fmt" "io" "net/http" - "strconv" "time" ) @@ -65,8 +64,8 @@ func (c *countingReader) Read(p []byte) (int, error) { func writeLockInfo(w io.Writer, token string, ld LockDetails) (int, error) { depth := "infinity" - if d := ld.Depth; d >= 0 { - depth = strconv.Itoa(d) + if ld.ZeroDepth { + depth = "0" } timeout := ld.Duration / time.Second return fmt.Fprintf(w, "\n"+