webdav: implement parseDepth; restrict LockDetails' depth to only zero

or infinity.

Change-Id: I3e3fba8e05a9c6c3db2240311a0813961db81849
Reviewed-on: https://go-review.googlesource.com/2535
Reviewed-by: Dave Cheney <dave@cheney.net>

Author: nigeltao

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 <owner> 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
 		}

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)

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) {

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, "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n"+