webdav: Add PROPPATCH support to in-memory property system.

This CL adds support to store arbitrary WebDAV properties in the
in-memory property system reference implementation. It covers the
the majority of property related litmus test cases. However, this CL
does not add support for COPY/MOVE/DELETE requests to the PropSystem
interface or implementation; memory occupied by dead properties of
affected resources will not be released. I propose to first agree on
how to store and lock dead properties in this CL and add support for
COPY/MOVE/DELETE in a follow-up change.

Before: Coverage of litmus 'props' test suite
16 tests were skipped, 14 tests run. 10 passed, 4 failed. 71.4%

After: Coverage of litmus 'props' test suite
0  tests were skipped, 30 tests run. 28 passed, 2 failed. 93.3%

Change-Id: Ie9af665fc588332ed30c7de256f47f8405078db3
Reviewed-on: https://go-review.googlesource.com/9374Reviewed-by: Nigel Tao <nigeltao@golang.org>

webdav/litmus_test_server.go

@@ -37,7 +37,7 @@ func main() {
 	http.Handle("/", &webdav.Handler{
 		FileSystem: fs,
 		LockSystem: ls,
-		PropSystem: webdav.NewMemPS(fs, ls),
+		PropSystem: webdav.NewMemPS(fs, ls, webdav.ReadWrite),
 		Logger: func(r *http.Request, err error) {
 			litmus := r.Header.Get("X-Litmus")
 			if len(litmus) > 19 {

webdav/prop.go

@@ -13,6 +13,7 @@ import (
 	"os"
 	"path/filepath"
 	"strconv"
+	"sync"
 )
 
 // PropSystem manages the properties of named resources. It allows finding
@@ -95,19 +96,50 @@ type Propstat struct {
 type memPS struct {
 	fs FileSystem
 	ls LockSystem
+	m  Mutability
+
+	mu    sync.RWMutex
+	nodes map[string]*memPSNode
+}
+
+// memPSNode stores the dead properties of a resource.
+type memPSNode struct {
+	mu        sync.RWMutex
+	deadProps map[xml.Name]Property
 }
 
-// NewMemPS returns a new in-memory PropSystem implementation.
-func NewMemPS(fs FileSystem, ls LockSystem) PropSystem {
-	return &memPS{fs: fs, ls: ls}
+// BUG(rost): In this development version, the in-memory property system does
+// not handle COPY/MOVE/DELETE requests. As a result, dead properties are not
+// released if the according DAV resource is deleted or moved. It is not
+// recommended to use a read-writeable property system in production.
+
+// Mutability indicates the mutability of a property system.
+type Mutability bool
+
+const (
+	ReadOnly  = Mutability(false)
+	ReadWrite = Mutability(true)
+)
+
+// NewMemPS returns a new in-memory PropSystem implementation. A read-only
+// property system rejects all patches. A read-writeable property system
+// stores arbitrary properties but refuses to change any DAV: property
+// specified in RFC 4918. It imposes no limit on the size of property values.
+func NewMemPS(fs FileSystem, ls LockSystem, m Mutability) PropSystem {
+	return &memPS{
+		fs:    fs,
+		ls:    ls,
+		m:     m,
+		nodes: make(map[string]*memPSNode),
+	}
 }
 
-// davProps contains all supported DAV: properties and their optional
-// propfind functions. A nil findFn indicates a hidden, protected property.
-// The dir field indicates if the property applies to directories in addition
-// to regular files.
-var davProps = map[xml.Name]struct {
+// liveProps contains all supported, protected DAV: properties.
+var liveProps = map[xml.Name]struct {
+	// findFn implements the propfind function of this property. If nil,
+	// it indicates a hidden property.
 	findFn func(*memPS, string, os.FileInfo) (string, error)
+	// dir is true if the property applies to directories.
 	dir bool
 }{
 	xml.Name{Space: "DAV:", Local: "resourcetype"}: {
@@ -138,31 +170,51 @@ var davProps = map[xml.Name]struct {
 		findFn: (*memPS).findContentType,
 		dir:    true,
 	},
+	xml.Name{Space: "DAV:", Local: "getetag"}: {
+		findFn: (*memPS).findETag,
 		// memPS implements ETag as the concatenated hex values of a file's
 		// modification time and size. This is not a reliable synchronization
 		// mechanism for directories, so we do not advertise getetag for
 		// DAV collections.
-	xml.Name{Space: "DAV:", Local: "getetag"}: {
-		findFn: (*memPS).findETag,
 		dir: false,
 	},
 
 	// TODO(nigeltao) Lock properties will be defined later.
-	// xml.Name{Space: "DAV:", Local: "lockdiscovery"}
-	// xml.Name{Space: "DAV:", Local: "supportedlock"}
+	xml.Name{Space: "DAV:", Local: "lockdiscovery"}: {},
+	xml.Name{Space: "DAV:", Local: "supportedlock"}: {},
 }
 
 func (ps *memPS) Find(name string, propnames []xml.Name) ([]Propstat, error) {
+	ps.mu.RLock()
+	defer ps.mu.RUnlock()
+
 	fi, err := ps.fs.Stat(name)
 	if err != nil {
 		return nil, err
 	}
 
+	// Lookup the dead properties of this resource. It's OK if there are none.
+	n, ok := ps.nodes[name]
+	if ok {
+		n.mu.RLock()
+		defer n.mu.RUnlock()
+	}
+
 	pm := make(map[int]Propstat)
 	for _, pn := range propnames {
+		// If this node has dead properties, check if they contain pn.
+		if n != nil {
+			if dp, ok := n.deadProps[pn]; ok {
+				pstat := pm[http.StatusOK]
+				pstat.Props = append(pstat.Props, dp)
+				pm[http.StatusOK] = pstat
+				continue
+			}
+		}
+		// Otherwise, it must either be a live property or we don't know it.
 		p := Property{XMLName: pn}
 		s := http.StatusNotFound
-		if prop := davProps[pn]; prop.findFn != nil && (prop.dir || !fi.IsDir()) {
+		if prop := liveProps[pn]; prop.findFn != nil && (prop.dir || !fi.IsDir()) {
 			xmlvalue, err := prop.findFn(ps, name, fi)
 			if err != nil {
 				return nil, err
@@ -188,12 +240,24 @@ func (ps *memPS) Propnames(name string) ([]xml.Name, error) {
 	if err != nil {
 		return nil, err
 	}
-	propnames := make([]xml.Name, 0, len(davProps))
-	for pn, prop := range davProps {
+
+	propnames := make([]xml.Name, 0, len(liveProps))
+	for pn, prop := range liveProps {
 		if prop.findFn != nil && (prop.dir || !fi.IsDir()) {
 			propnames = append(propnames, pn)
 		}
 	}
+
+	ps.mu.RLock()
+	defer ps.mu.RUnlock()
+	if n, ok := ps.nodes[name]; ok {
+		n.mu.RLock()
+		defer n.mu.RUnlock()
+		for pn := range n.deadProps {
+			propnames = append(propnames, pn)
+		}
+	}
+
 	return propnames, nil
 }
 
@@ -216,14 +280,61 @@ func (ps *memPS) Allprop(name string, include []xml.Name) ([]Propstat, error) {
 }
 
 func (ps *memPS) Patch(name string, patches []Proppatch) ([]Propstat, error) {
-	// TODO(rost): Support to patch "dead" DAV properties in the next CL.
-	pstat := Propstat{Status: http.StatusForbidden}
+	// A DELETE/COPY/MOVE might fly in, so we need to keep all nodes locked until
+	// the end of this PROPPATCH.
+	ps.mu.Lock()
+	defer ps.mu.Unlock()
+	n, ok := ps.nodes[name]
+	if !ok {
+		n = &memPSNode{deadProps: make(map[xml.Name]Property)}
+	}
+	n.mu.Lock()
+	defer n.mu.Unlock()
+
+	_, err := ps.fs.Stat(name)
+	if err != nil {
+		return nil, err
+	}
+
+	// Perform a dry-run to identify any patch conflicts. A read-only property
+	// system always fails at this stage.
+	pm := make(map[int]Propstat)
 	for _, patch := range patches {
 		for _, p := range patch.Props {
+			s := http.StatusOK
+			if _, ok := liveProps[p.XMLName]; ok || ps.m == ReadOnly {
+				s = http.StatusForbidden
+			}
+			pstat := pm[s]
 			pstat.Props = append(pstat.Props, Property{XMLName: p.XMLName})
+			pm[s] = pstat
 		}
 	}
-	return []Propstat{pstat}, nil
+	// Based on the dry-run, either apply the patches or handle conflicts.
+	if _, ok = pm[http.StatusOK]; ok {
+		if len(pm) == 1 {
+			for _, patch := range patches {
+				for _, p := range patch.Props {
+					if patch.Remove {
+						delete(n.deadProps, p.XMLName)
+					} else {
+						n.deadProps[p.XMLName] = p
+					}
+				}
+			}
+			ps.nodes[name] = n
+		} else {
+			pm[StatusFailedDependency] = pm[http.StatusOK]
+			delete(pm, http.StatusOK)
+		}
+	}
+
+	pstats := make([]Propstat, 0, len(pm))
+	for s, pstat := range pm {
+		pstat.Status = s
+		pstats = append(pstats, pstat)
+	}
+	return pstats, nil
 }
 
 func (ps *memPS) findResourceType(name string, fi os.FileInfo) (string, error) {

webdav/prop_test.go

@@ -43,18 +43,20 @@ func TestMemPS(t *testing.T) {
 		op            string
 		name          string
 		propnames     []xml.Name
+		patches       []Proppatch
 		wantNames     []xml.Name
 		wantPropstats []Propstat
 	}
 
 	testCases := []struct {
 		desc       string
+		mutability Mutability
 		buildfs    []string
 		propOp     []propOp
 	}{{
-		"propname",
-		[]string{"mkdir /dir", "touch /file"},
-		[]propOp{{
+		desc:    "propname",
+		buildfs: []string{"mkdir /dir", "touch /file"},
+		propOp: []propOp{{
 			op:   "propname",
 			name: "/dir",
 			wantNames: []xml.Name{
@@ -77,9 +79,9 @@ func TestMemPS(t *testing.T) {
 			},
 		}},
 	}, {
-		"allprop dir and file",
-		[]string{"mkdir /dir", "write /file foobarbaz"},
-		[]propOp{{
+		desc:    "allprop dir and file",
+		buildfs: []string{"mkdir /dir", "write /file foobarbaz"},
+		propOp: []propOp{{
 			op:   "allprop",
 			name: "/dir",
 			wantPropstats: []Propstat{{
@@ -161,9 +163,9 @@ func TestMemPS(t *testing.T) {
 			},
 		}},
 	}, {
-		"propfind DAV:resourcetype",
-		[]string{"mkdir /dir", "touch /file"},
-		[]propOp{{
+		desc:    "propfind DAV:resourcetype",
+		buildfs: []string{"mkdir /dir", "touch /file"},
+		propOp: []propOp{{
 			op:        "propfind",
 			name:      "/dir",
 			propnames: []xml.Name{{"DAV:", "resourcetype"}},
@@ -187,9 +189,9 @@ func TestMemPS(t *testing.T) {
 			}},
 		}},
 	}, {
-		"propfind unsupported DAV properties",
-		[]string{"mkdir /dir"},
-		[]propOp{{
+		desc:    "propfind unsupported DAV properties",
+		buildfs: []string{"mkdir /dir"},
+		propOp: []propOp{{
 			op:        "propfind",
 			name:      "/dir",
 			propnames: []xml.Name{{"DAV:", "getcontentlanguage"}},
@@ -211,9 +213,9 @@ func TestMemPS(t *testing.T) {
 			}},
 		}},
 	}, {
-		"propfind getetag for files but not for directories",
-		[]string{"mkdir /dir", "touch /file"},
-		[]propOp{{
+		desc:    "propfind getetag for files but not for directories",
+		buildfs: []string{"mkdir /dir", "touch /file"},
+		propOp: []propOp{{
 			op:        "propfind",
 			name:      "/dir",
 			propnames: []xml.Name{{"DAV:", "getetag"}},
@@ -236,9 +238,241 @@ func TestMemPS(t *testing.T) {
 			}},
 		}},
 	}, {
-		"bad: propfind unknown property",
-		[]string{"mkdir /dir"},
-		[]propOp{{
+		desc:       "proppatch property on read-only property system",
+		buildfs:    []string{"mkdir /dir"},
+		mutability: ReadOnly,
+		propOp: []propOp{{
+			op:   "proppatch",
+			name: "/dir",
+			patches: []Proppatch{{
+				Props: []Property{{
+					XMLName: xml.Name{Space: "foo", Local: "bar"},
+				}},
+			}},
+			wantPropstats: []Propstat{{
+				Status: http.StatusForbidden,
+				Props: []Property{{
+					XMLName: xml.Name{Space: "foo", Local: "bar"},
+				}},
+			}},
+		}, {
+			op:   "proppatch",
+			name: "/dir",
+			patches: []Proppatch{{
+				Props: []Property{{
+					XMLName: xml.Name{Space: "DAV:", Local: "getetag"},
+				}},
+			}},
+			wantPropstats: []Propstat{{
+				Status: http.StatusForbidden,
+				Props: []Property{{
+					XMLName: xml.Name{Space: "DAV:", Local: "getetag"},
+				}},
+			}},
+		}},
+	}, {
+		desc:       "proppatch dead property",
+		buildfs:    []string{"mkdir /dir"},
+		mutability: ReadWrite,
+		propOp: []propOp{{
+			op:   "proppatch",
+			name: "/dir",
+			patches: []Proppatch{{
+				Props: []Property{{
+					XMLName:  xml.Name{Space: "foo", Local: "bar"},
+					InnerXML: []byte("baz"),
+				}},
+			}},
+			wantPropstats: []Propstat{{
+				Status: http.StatusOK,
+				Props: []Property{{
+					XMLName: xml.Name{Space: "foo", Local: "bar"},
+				}},
+			}},
+		}, {
+			op:        "propfind",
+			name:      "/dir",
+			propnames: []xml.Name{{Space: "foo", Local: "bar"}},
+			wantPropstats: []Propstat{{
+				Status: http.StatusOK,
+				Props: []Property{{
+					XMLName:  xml.Name{Space: "foo", Local: "bar"},
+					InnerXML: []byte("baz"),
+				}},
+			}},
+		}},
+	}, {
+		desc:       "proppatch dead property with failed dependency",
+		buildfs:    []string{"mkdir /dir"},
+		mutability: ReadWrite,
+		propOp: []propOp{{
+			op:   "proppatch",
+			name: "/dir",
+			patches: []Proppatch{{
+				Props: []Property{{
+					XMLName:  xml.Name{Space: "foo", Local: "bar"},
+					InnerXML: []byte("baz"),
+				}},
+			}, {
+				Props: []Property{{
+					XMLName:  xml.Name{Space: "DAV:", Local: "displayname"},
+					InnerXML: []byte("xxx"),
+				}},
+			}},
+			wantPropstats: []Propstat{{
+				Status: http.StatusForbidden,
+				Props: []Property{{
+					XMLName: xml.Name{Space: "DAV:", Local: "displayname"},
+				}},
+			}, {
+				Status: StatusFailedDependency,
+				Props: []Property{{
+					XMLName: xml.Name{Space: "foo", Local: "bar"},
+				}},
+			}},
+		}, {
+			op:        "propfind",
+			name:      "/dir",
+			propnames: []xml.Name{{Space: "foo", Local: "bar"}},
+			wantPropstats: []Propstat{{
+				Status: http.StatusNotFound,
+				Props: []Property{{
+					XMLName: xml.Name{Space: "foo", Local: "bar"},
+				}},
+			}},
+		}},
+	}, {
+		desc:       "proppatch remove dead property",
+		buildfs:    []string{"mkdir /dir"},
+		mutability: ReadWrite,
+		propOp: []propOp{{
+			op:   "proppatch",
+			name: "/dir",
+			patches: []Proppatch{{
+				Props: []Property{{
+					XMLName:  xml.Name{Space: "foo", Local: "bar"},
+					InnerXML: []byte("baz"),
+				}, {
+					XMLName:  xml.Name{Space: "spam", Local: "ham"},
+					InnerXML: []byte("eggs"),
+				}},
+			}},
+			wantPropstats: []Propstat{{
+				Status: http.StatusOK,
+				Props: []Property{{
+					XMLName: xml.Name{Space: "foo", Local: "bar"},
+				}, {
+					XMLName: xml.Name{Space: "spam", Local: "ham"},
+				}},
+			}},
+		}, {
+			op:   "propfind",
+			name: "/dir",
+			propnames: []xml.Name{
+				{Space: "foo", Local: "bar"},
+				{Space: "spam", Local: "ham"},
+			},
+			wantPropstats: []Propstat{{
+				Status: http.StatusOK,
+				Props: []Property{{
+					XMLName:  xml.Name{Space: "foo", Local: "bar"},
+					InnerXML: []byte("baz"),
+				}, {
+					XMLName:  xml.Name{Space: "spam", Local: "ham"},
+					InnerXML: []byte("eggs"),
+				}},
+			}},
+		}, {
+			op:   "proppatch",
+			name: "/dir",
+			patches: []Proppatch{{
+				Remove: true,
+				Props: []Property{{
+					XMLName: xml.Name{Space: "foo", Local: "bar"},
+				}},
+			}},
+			wantPropstats: []Propstat{{
+				Status: http.StatusOK,
+				Props: []Property{{
+					XMLName: xml.Name{Space: "foo", Local: "bar"},
+				}},
+			}},
+		}, {
+			op:   "propfind",
+			name: "/dir",
+			propnames: []xml.Name{
+				{Space: "foo", Local: "bar"},
+				{Space: "spam", Local: "ham"},
+			},
+			wantPropstats: []Propstat{{
+				Status: http.StatusNotFound,
+				Props: []Property{{
+					XMLName: xml.Name{Space: "foo", Local: "bar"},
+				}},
+			}, {
+				Status: http.StatusOK,
+				Props: []Property{{
+					XMLName:  xml.Name{Space: "spam", Local: "ham"},
+					InnerXML: []byte("eggs"),
+				}},
+			}},
+		}},
+	}, {
+		desc:       "propname with dead property",
+		buildfs:    []string{"touch /file"},
+		mutability: ReadWrite,
+		propOp: []propOp{{
+			op:   "proppatch",
+			name: "/file",
+			patches: []Proppatch{{
+				Props: []Property{{
+					XMLName:  xml.Name{Space: "foo", Local: "bar"},
+					InnerXML: []byte("baz"),
+				}},
+			}},
+			wantPropstats: []Propstat{{
+				Status: http.StatusOK,
+				Props: []Property{{
+					XMLName: xml.Name{Space: "foo", Local: "bar"},
+				}},
+			}},
+		}, {
+			op:   "propname",
+			name: "/file",
+			wantNames: []xml.Name{
+				xml.Name{Space: "DAV:", Local: "resourcetype"},
+				xml.Name{Space: "DAV:", Local: "displayname"},
+				xml.Name{Space: "DAV:", Local: "getcontentlength"},
+				xml.Name{Space: "DAV:", Local: "getlastmodified"},
+				xml.Name{Space: "DAV:", Local: "getcontenttype"},
+				xml.Name{Space: "DAV:", Local: "getetag"},
+				xml.Name{Space: "foo", Local: "bar"},
+			},
+		}},
+	}, {
+		desc:       "proppatch remove unknown dead property",
+		buildfs:    []string{"mkdir /dir"},
+		mutability: ReadWrite,
+		propOp: []propOp{{
+			op:   "proppatch",
+			name: "/dir",
+			patches: []Proppatch{{
+				Remove: true,
+				Props: []Property{{
+					XMLName: xml.Name{Space: "foo", Local: "bar"},
+				}},
+			}},
+			wantPropstats: []Propstat{{
+				Status: http.StatusOK,
+				Props: []Property{{
+					XMLName: xml.Name{Space: "foo", Local: "bar"},
+				}},
+			}},
+		}},
+	}, {
+		desc:    "bad: propfind unknown property",
+		buildfs: []string{"mkdir /dir"},
+		propOp: []propOp{{
 			op:        "propfind",
 			name:      "/dir",
 			propnames: []xml.Name{{"foo:", "bar"}},
@@ -257,7 +491,7 @@ func TestMemPS(t *testing.T) {
 			t.Fatalf("%s: cannot create test filesystem: %v", tc.desc, err)
 		}
 		ls := NewMemLS()
-		ps := NewMemPS(fs, ls)
+		ps := NewMemPS(fs, ls, tc.mutability)
 		for _, op := range tc.propOp {
 			desc := fmt.Sprintf("%s: %s %s", tc.desc, op.op, op.name)
 			if err = calcProps(op.name, fs, op.wantPropstats); err != nil {
@@ -283,6 +517,8 @@ func TestMemPS(t *testing.T) {
 				propstats, err = ps.Allprop(op.name, op.propnames)
 			case "propfind":
 				propstats, err = ps.Find(op.name, op.propnames)
+			case "proppatch":
+				propstats, err = ps.Patch(op.name, op.patches)
 			default:
 				t.Fatalf("%s: %s not implemented", desc, op.op)
 			}
@@ -290,7 +526,7 @@ func TestMemPS(t *testing.T) {
 				t.Errorf("%s: got error %v, want nil", desc, err)
 				continue
 			}
-			// Compare return values from allprop or propfind.
+			// Compare return values from allprop, propfind or proppatch.
 			for _, pst := range propstats {
 				sort.Sort(byPropname(pst.Props))
 			}