webdav: make properties belong to the File(System), not a PropSystem.

This makes properties consistent even after a COPY, MOVE or DELETE.

A follow-up CL will eliminate the concept of a configurable PropSystem
entirely.

See discussion at
https://groups.google.com/forum/#!topic/golang-dev/2_LiN6sf93A

Litmust test before/after:
<- summary for `props': of 30 tests run: 28 passed, 2 failed. 93.3%
<- summary for `props': of 30 tests run: 29 passed, 1 failed. 96.7%

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

webdav/file.go

@@ -5,6 +5,7 @@
 package webdav
 
 import (
+	"encoding/xml"
 	"io"
 	"net/http"
 	"os"
@@ -44,6 +45,9 @@ type FileSystem interface {
 
 // A File is returned by a FileSystem's OpenFile method and can be served by a
 // Handler.
+//
+// A File may optionally implement the DeadPropsHolder interface, if it can
+// load and save dead properties.
 type File interface {
 	http.File
 	io.Writer
@@ -401,10 +405,11 @@ type memFSNode struct {
 	// children is protected by memFS.mu.
 	children map[string]*memFSNode
 
-	mu      sync.Mutex
-	data    []byte
-	mode    os.FileMode
-	modTime time.Time
+	mu        sync.Mutex
+	data      []byte
+	mode      os.FileMode
+	modTime   time.Time
+	deadProps map[xml.Name]Property
 }
 
 func (n *memFSNode) stat(name string) *memFileInfo {
@@ -418,6 +423,39 @@ func (n *memFSNode) stat(name string) *memFileInfo {
 	}
 }
 
+func (n *memFSNode) DeadProps() map[xml.Name]Property {
+	n.mu.Lock()
+	defer n.mu.Unlock()
+	if len(n.deadProps) == 0 {
+		return nil
+	}
+	ret := make(map[xml.Name]Property, len(n.deadProps))
+	for k, v := range n.deadProps {
+		ret[k] = v
+	}
+	return ret
+}
+
+func (n *memFSNode) Patch(patches []Proppatch) ([]Propstat, error) {
+	n.mu.Lock()
+	defer n.mu.Unlock()
+	pstat := Propstat{Status: http.StatusOK}
+	for _, patch := range patches {
+		for _, p := range patch.Props {
+			pstat.Props = append(pstat.Props, Property{XMLName: p.XMLName})
+			if patch.Remove {
+				delete(n.deadProps, p.XMLName)
+				continue
+			}
+			if n.deadProps == nil {
+				n.deadProps = map[xml.Name]Property{}
+			}
+			n.deadProps[p.XMLName] = p
+		}
+	}
+	return []Propstat{pstat}, nil
+}
+
 type memFileInfo struct {
 	name    string
 	size    int64
@@ -443,6 +481,12 @@ type memFile struct {
 	pos int
 }
 
+// A *memFile implements the optional DeadPropsHolder interface.
+var _ DeadPropsHolder = (*memFile)(nil)
+
+func (f *memFile) DeadProps() map[xml.Name]Property              { return f.n.DeadProps() }
+func (f *memFile) Patch(patches []Proppatch) ([]Propstat, error) { return f.n.Patch(patches) }
+
 func (f *memFile) Close() error {
 	return nil
 }

webdav/litmus_test_server.go

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

webdav/prop.go

@@ -13,9 +13,12 @@ import (
 	"os"
 	"path/filepath"
 	"strconv"
-	"sync"
 )
 
+// TODO(nigeltao): eliminate the concept of a configurable PropSystem, and the
+// MemPS implementation. Properties are now the responsibility of a File
+// implementation, not a PropSystem implementation.
+
 // PropSystem manages the properties of named resources. It allows finding
 // and setting properties as defined in RFC 4918.
 //
@@ -54,8 +57,6 @@ type PropSystem interface {
 	//
 	// Note that the WebDAV RFC requires either all patches to succeed or none.
 	Patch(name string, patches []Proppatch) ([]Propstat, error)
-
-	// TODO(rost) COPY/MOVE/DELETE.
 }
 
 // Proppatch describes a property update instruction as defined in RFC 4918.
@@ -91,46 +92,66 @@ type Propstat struct {
 	ResponseDescription string
 }
 
+// makePropstats returns a slice containing those of x and y whose Props slice
+// is non-empty. If both are empty, it returns a slice containing an otherwise
+// zero Propstat whose HTTP status code is 200 OK.
+func makePropstats(x, y Propstat) []Propstat {
+	pstats := make([]Propstat, 0, 2)
+	if len(x.Props) != 0 {
+		pstats = append(pstats, x)
+	}
+	if len(y.Props) != 0 {
+		pstats = append(pstats, y)
+	}
+	if len(pstats) == 0 {
+		pstats = append(pstats, Propstat{
+			Status: http.StatusOK,
+		})
+	}
+	return pstats
+}
+
+// DeadPropsHolder holds the dead properties of a resource.
+//
+// Dead properties are those properties that are explicitly defined. In
+// comparison, live properties, such as DAV:getcontentlength, are implicitly
+// defined by the underlying resource, and cannot be explicitly overridden or
+// removed. See the Terminology section of
+// http://www.webdav.org/specs/rfc4918.html#rfc.section.3
+//
+// There is a whitelist of the names of live properties. This package handles
+// all live properties, and will only pass non-whitelisted names to the Patch
+// method of DeadPropsHolder implementations.
+type DeadPropsHolder interface {
+	// DeadProps returns a copy of the dead properties held.
+	DeadProps() map[xml.Name]Property
+
+	// Patch patches the dead properties held.
+	//
+	// Patching is atomic; either all or no patches succeed. It returns (nil,
+	// non-nil) if an internal server error occurred, otherwise the Propstats
+	// collectively contain one Property for each proposed patch Property. If
+	// all patches succeed, Patch returns a slice of length one and a Propstat
+	// element with a 200 OK HTTP status code. If none succeed, for reasons
+	// other than an internal server error, no Propstat has status 200 OK.
+	//
+	// For more details on when various HTTP status codes apply, see
+	// http://www.webdav.org/specs/rfc4918.html#PROPPATCH-status
+	Patch([]Proppatch) ([]Propstat, error)
+}
+
 // memPS implements an in-memory PropSystem. It supports all of the mandatory
 // live properties of RFC 4918.
 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
 }
 
-// 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 {
+// NewMemPS returns a new in-memory PropSystem implementation.
+func NewMemPS(fs FileSystem, ls LockSystem) PropSystem {
 	return &memPS{
-		fs:    fs,
-		ls:    ls,
-		m:     m,
-		nodes: make(map[string]*memPSNode),
+		fs: fs,
+		ls: ls,
 	}
 }
 
@@ -185,79 +206,75 @@ var liveProps = map[xml.Name]struct {
 }
 
 func (ps *memPS) Find(name string, propnames []xml.Name) ([]Propstat, error) {
-	ps.mu.RLock()
-	defer ps.mu.RUnlock()
-
-	fi, err := ps.fs.Stat(name)
+	f, err := ps.fs.OpenFile(name, os.O_RDONLY, 0)
+	if err != nil {
+		return nil, err
+	}
+	defer f.Close()
+	fi, err := f.Stat()
 	if err != nil {
 		return nil, err
 	}
+	isDir := fi.IsDir()
 
-	// 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()
+	var deadProps map[xml.Name]Property
+	if dph, ok := f.(DeadPropsHolder); ok {
+		deadProps = dph.DeadProps()
 	}
 
-	pm := make(map[int]Propstat)
+	pstatOK := Propstat{Status: http.StatusOK}
+	pstatNotFound := Propstat{Status: http.StatusNotFound}
 	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
-			}
+		// If this file has dead properties, check if they contain pn.
+		if dp, ok := deadProps[pn]; ok {
+			pstatOK.Props = append(pstatOK.Props, dp)
+			continue
 		}
 		// Otherwise, it must either be a live property or we don't know it.
-		p := Property{XMLName: pn}
-		s := http.StatusNotFound
-		if prop := liveProps[pn]; prop.findFn != nil && (prop.dir || !fi.IsDir()) {
-			xmlvalue, err := prop.findFn(ps, name, fi)
+		if prop := liveProps[pn]; prop.findFn != nil && (prop.dir || !isDir) {
+			innerXML, err := prop.findFn(ps, name, fi)
 			if err != nil {
 				return nil, err
 			}
-			s = http.StatusOK
-			p.InnerXML = []byte(xmlvalue)
+			pstatOK.Props = append(pstatOK.Props, Property{
+				XMLName:  pn,
+				InnerXML: []byte(innerXML),
+			})
+		} else {
+			pstatNotFound.Props = append(pstatNotFound.Props, Property{
+				XMLName: pn,
+			})
 		}
-		pstat := pm[s]
-		pstat.Props = append(pstat.Props, p)
-		pm[s] = pstat
 	}
-
-	pstats := make([]Propstat, 0, len(pm))
-	for s, pstat := range pm {
-		pstat.Status = s
-		pstats = append(pstats, pstat)
-	}
-	return pstats, nil
+	return makePropstats(pstatOK, pstatNotFound), nil
 }
 
 func (ps *memPS) Propnames(name string) ([]xml.Name, error) {
-	fi, err := ps.fs.Stat(name)
+	f, err := ps.fs.OpenFile(name, os.O_RDONLY, 0)
 	if err != nil {
 		return nil, err
 	}
+	defer f.Close()
+	fi, err := f.Stat()
+	if err != nil {
+		return nil, err
+	}
+	isDir := fi.IsDir()
 
-	propnames := make([]xml.Name, 0, len(liveProps))
-	for pn, prop := range liveProps {
-		if prop.findFn != nil && (prop.dir || !fi.IsDir()) {
-			propnames = append(propnames, pn)
-		}
+	var deadProps map[xml.Name]Property
+	if dph, ok := f.(DeadPropsHolder); ok {
+		deadProps = dph.DeadProps()
 	}
 
-	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 := make([]xml.Name, 0, len(liveProps)+len(deadProps))
+	for pn, prop := range liveProps {
+		if prop.findFn != nil && (prop.dir || !isDir) {
 			propnames = append(propnames, pn)
 		}
 	}
-
+	for pn := range deadProps {
+		propnames = append(propnames, pn)
+	}
 	return propnames, nil
 }
 
@@ -280,61 +297,65 @@ func (ps *memPS) Allprop(name string, include []xml.Name) ([]Propstat, error) {
 }
 
 func (ps *memPS) Patch(name string, patches []Proppatch) ([]Propstat, error) {
-	// 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)
+	conflict := false
+loop:
 	for _, patch := range patches {
 		for _, p := range patch.Props {
-			s := http.StatusOK
-			if _, ok := liveProps[p.XMLName]; ok || ps.m == ReadOnly {
-				s = http.StatusForbidden
+			if _, ok := liveProps[p.XMLName]; ok {
+				conflict = true
+				break loop
 			}
-			pstat := pm[s]
-			pstat.Props = append(pstat.Props, Property{XMLName: p.XMLName})
-			pm[s] = pstat
 		}
 	}
-	// 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
-					}
+	if conflict {
+		pstatForbidden := Propstat{
+			Status:   http.StatusForbidden,
+			XMLError: `<error xmlns="DAV:"><cannot-modify-protected-property/></error>`,
+		}
+		pstatFailedDep := Propstat{
+			Status: StatusFailedDependency,
+		}
+		for _, patch := range patches {
+			for _, p := range patch.Props {
+				if _, ok := liveProps[p.XMLName]; ok {
+					pstatForbidden.Props = append(pstatForbidden.Props, Property{XMLName: p.XMLName})
+				} else {
+					pstatFailedDep.Props = append(pstatFailedDep.Props, Property{XMLName: p.XMLName})
 				}
 			}
-			ps.nodes[name] = n
-		} else {
-			pm[StatusFailedDependency] = pm[http.StatusOK]
-			delete(pm, http.StatusOK)
 		}
+		return makePropstats(pstatForbidden, pstatFailedDep), nil
 	}
 
-	pstats := make([]Propstat, 0, len(pm))
-	for s, pstat := range pm {
-		pstat.Status = s
-		pstats = append(pstats, pstat)
+	f, err := ps.fs.OpenFile(name, os.O_RDWR, 0)
+	if err != nil {
+		return nil, err
+	}
+	defer f.Close()
+	if dph, ok := f.(DeadPropsHolder); ok {
+		ret, err := dph.Patch(patches)
+		if err != nil {
+			return nil, err
+		}
+		// http://www.webdav.org/specs/rfc4918.html#ELEMENT_propstat says that
+		// "The contents of the prop XML element must only list the names of
+		// properties to which the result in the status element applies."
+		for _, pstat := range ret {
+			for i, p := range pstat.Props {
+				pstat.Props[i] = Property{XMLName: p.XMLName}
+			}
+		}
+		return ret, nil
+	}
+	// The file doesn't implement the optional DeadPropsHolder interface, so
+	// all patches are forbidden.
+	pstat := Propstat{Status: http.StatusForbidden}
+	for _, patch := range patches {
+		for _, p := range patch.Props {
+			pstat.Props = append(pstat.Props, Property{XMLName: p.XMLName})
+		}
 	}
-	return pstats, nil
+	return []Propstat{pstat}, nil
 }
 
 func (ps *memPS) findResourceType(name string, fi os.FileInfo) (string, error) {

webdav/prop_test.go

@@ -8,6 +8,7 @@ import (
 	"encoding/xml"
 	"fmt"
 	"net/http"
+	"os"
 	"reflect"
 	"sort"
 	"testing"
@@ -49,10 +50,10 @@ func TestMemPS(t *testing.T) {
 	}
 
 	testCases := []struct {
-		desc       string
-		mutability Mutability
-		buildfs    []string
-		propOp     []propOp
+		desc        string
+		noDeadProps bool
+		buildfs     []string
+		propOp      []propOp
 	}{{
 		desc:    "propname",
 		buildfs: []string{"mkdir /dir", "touch /file"},
@@ -238,9 +239,9 @@ func TestMemPS(t *testing.T) {
 			}},
 		}},
 	}, {
-		desc:       "proppatch property on read-only property system",
-		buildfs:    []string{"mkdir /dir"},
-		mutability: ReadOnly,
+		desc:        "proppatch property on no-dead-properties file system",
+		buildfs:     []string{"mkdir /dir"},
+		noDeadProps: true,
 		propOp: []propOp{{
 			op:   "proppatch",
 			name: "/dir",
@@ -264,16 +265,16 @@ func TestMemPS(t *testing.T) {
 				}},
 			}},
 			wantPropstats: []Propstat{{
-				Status: http.StatusForbidden,
+				Status:   http.StatusForbidden,
+				XMLError: `<error xmlns="DAV:"><cannot-modify-protected-property/></error>`,
 				Props: []Property{{
 					XMLName: xml.Name{Space: "DAV:", Local: "getetag"},
 				}},
 			}},
 		}},
 	}, {
-		desc:       "proppatch dead property",
-		buildfs:    []string{"mkdir /dir"},
-		mutability: ReadWrite,
+		desc:    "proppatch dead property",
+		buildfs: []string{"mkdir /dir"},
 		propOp: []propOp{{
 			op:   "proppatch",
 			name: "/dir",
@@ -302,9 +303,8 @@ func TestMemPS(t *testing.T) {
 			}},
 		}},
 	}, {
-		desc:       "proppatch dead property with failed dependency",
-		buildfs:    []string{"mkdir /dir"},
-		mutability: ReadWrite,
+		desc:    "proppatch dead property with failed dependency",
+		buildfs: []string{"mkdir /dir"},
 		propOp: []propOp{{
 			op:   "proppatch",
 			name: "/dir",
@@ -320,7 +320,8 @@ func TestMemPS(t *testing.T) {
 				}},
 			}},
 			wantPropstats: []Propstat{{
-				Status: http.StatusForbidden,
+				Status:   http.StatusForbidden,
+				XMLError: `<error xmlns="DAV:"><cannot-modify-protected-property/></error>`,
 				Props: []Property{{
 					XMLName: xml.Name{Space: "DAV:", Local: "displayname"},
 				}},
@@ -342,9 +343,8 @@ func TestMemPS(t *testing.T) {
 			}},
 		}},
 	}, {
-		desc:       "proppatch remove dead property",
-		buildfs:    []string{"mkdir /dir"},
-		mutability: ReadWrite,
+		desc:    "proppatch remove dead property",
+		buildfs: []string{"mkdir /dir"},
 		propOp: []propOp{{
 			op:   "proppatch",
 			name: "/dir",
@@ -418,9 +418,8 @@ func TestMemPS(t *testing.T) {
 			}},
 		}},
 	}, {
-		desc:       "propname with dead property",
-		buildfs:    []string{"touch /file"},
-		mutability: ReadWrite,
+		desc:    "propname with dead property",
+		buildfs: []string{"touch /file"},
 		propOp: []propOp{{
 			op:   "proppatch",
 			name: "/file",
@@ -450,9 +449,8 @@ func TestMemPS(t *testing.T) {
 			},
 		}},
 	}, {
-		desc:       "proppatch remove unknown dead property",
-		buildfs:    []string{"mkdir /dir"},
-		mutability: ReadWrite,
+		desc:    "proppatch remove unknown dead property",
+		buildfs: []string{"mkdir /dir"},
 		propOp: []propOp{{
 			op:   "proppatch",
 			name: "/dir",
@@ -490,8 +488,11 @@ func TestMemPS(t *testing.T) {
 		if err != nil {
 			t.Fatalf("%s: cannot create test filesystem: %v", tc.desc, err)
 		}
+		if tc.noDeadProps {
+			fs = noDeadPropsFS{fs}
+		}
 		ls := NewMemLS()
-		ps := NewMemPS(fs, ls, tc.mutability)
+		ps := NewMemPS(fs, ls)
 		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 {
@@ -551,36 +552,43 @@ func cmpXMLName(a, b xml.Name) bool {
 
 type byXMLName []xml.Name
 
-func (b byXMLName) Len() int {
-	return len(b)
-}
-func (b byXMLName) Swap(i, j int) {
-	b[i], b[j] = b[j], b[i]
-}
-func (b byXMLName) Less(i, j int) bool {
-	return cmpXMLName(b[i], b[j])
-}
+func (b byXMLName) Len() int           { return len(b) }
+func (b byXMLName) Swap(i, j int)      { b[i], b[j] = b[j], b[i] }
+func (b byXMLName) Less(i, j int) bool { return cmpXMLName(b[i], b[j]) }
 
 type byPropname []Property
 
-func (b byPropname) Len() int {
-	return len(b)
-}
-func (b byPropname) Swap(i, j int) {
-	b[i], b[j] = b[j], b[i]
-}
-func (b byPropname) Less(i, j int) bool {
-	return cmpXMLName(b[i].XMLName, b[j].XMLName)
-}
+func (b byPropname) Len() int           { return len(b) }
+func (b byPropname) Swap(i, j int)      { b[i], b[j] = b[j], b[i] }
+func (b byPropname) Less(i, j int) bool { return cmpXMLName(b[i].XMLName, b[j].XMLName) }
 
 type byStatus []Propstat
 
-func (b byStatus) Len() int {
-	return len(b)
+func (b byStatus) Len() int           { return len(b) }
+func (b byStatus) Swap(i, j int)      { b[i], b[j] = b[j], b[i] }
+func (b byStatus) Less(i, j int) bool { return b[i].Status < b[j].Status }
+
+type noDeadPropsFS struct {
+	FileSystem
 }
-func (b byStatus) Swap(i, j int) {
-	b[i], b[j] = b[j], b[i]
+
+func (fs noDeadPropsFS) OpenFile(name string, flag int, perm os.FileMode) (File, error) {
+	f, err := fs.FileSystem.OpenFile(name, flag, perm)
+	if err != nil {
+		return nil, err
+	}
+	return noDeadPropsFile{f}, nil
 }
-func (b byStatus) Less(i, j int) bool {
-	return b[i].Status < b[j].Status
+
+// noDeadPropsFile wraps a File but strips any optional DeadPropsHolder methods
+// provided by the underlying File implementation.
+type noDeadPropsFile struct {
+	f File
 }
+
+func (f noDeadPropsFile) Close() error                              { return f.f.Close() }
+func (f noDeadPropsFile) Read(p []byte) (int, error)                { return f.f.Read(p) }
+func (f noDeadPropsFile) Readdir(count int) ([]os.FileInfo, error)  { return f.f.Readdir(count) }
+func (f noDeadPropsFile) Seek(off int64, whence int) (int64, error) { return f.f.Seek(off, whence) }
+func (f noDeadPropsFile) Stat() (os.FileInfo, error)                { return f.f.Stat() }
+func (f noDeadPropsFile) Write(p []byte) (int, error)               { return f.f.Write(p) }