engine: resources: Consistency changes and cleanup for file mode

This makes a few consistency changes and cleanups to the file mode
feature so that it's more in style with the rest of the code base.

engine/resources/file.go

@@ -54,6 +54,12 @@ const (
 	// FileStateUndefined means the file state has not been specified.
 	// TODO: consider moving to *string and express this state as a nil.
 	FileStateUndefined = ""
+
+	// FileModeAllowAssign specifies whether we only use ugo=rwx style
+	// assignment (false) or if we also allow ugo+-rwx style too (true). I
+	// think that it's possibly illogical to allow imperative mode
+	// specifiers in a declarative language, so let's leave it off for now.
+	FileModeAllowAssign = false
 )
 
 // FileRes is a file and directory resource. Dirs are defined by names ending
@@ -173,15 +179,14 @@ func (obj *FileRes) mode() (os.FileMode, error) {
 		return os.FileMode(n), nil
 	}
 
-	// Try parsing symbolic by first getting the files current mode.
+	// Try parsing symbolically by first getting the files current mode.
 	stat, err := os.Stat(obj.getPath())
 	if err != nil {
 		return os.FileMode(0), errwrap.Wrapf(err, "failed to get the current file mode")
 	}
-	m := stat.Mode()
 
 	modes := strings.Split(obj.Mode, ",")
-	m, err = engineUtil.ParseSymbolicModes(modes, m, false)
+	m, err := engineUtil.ParseSymbolicModes(modes, stat.Mode(), FileModeAllowAssign)
 	if err != nil {
 		return os.FileMode(0), errwrap.Wrapf(err, "mode should be an octal number or symbolic mode (%s)", obj.Mode)
 	}

engine/util/mode.go

@@ -43,16 +43,16 @@ const (
 // false if not.
 func modeIsValidWho(who string) bool {
 	for _, w := range []string{"u", "g", "o"} {
-		who = strings.Replace(who, w, "", -1)
+		who = strings.Replace(who, w, "", -1) // TODO: use ReplaceAll in 1.12
 	}
 	return len(who) == 0
 }
 
 // modeIsValidWhat checks that only the valid mode characters are in the what
-// string ('w', 'r', 'x', 's', 't').
+// string ('r', 'w', 'x', 's', 't').
 func modeIsValidWhat(what string) bool {
-	for _, w := range []string{"w", "r", "x", "s", "t"} {
+	for _, w := range []string{"r", "w", "x", "s", "t"} {
-		what = strings.Replace(what, w, "", -1)
+		what = strings.Replace(what, w, "", -1) // TODO: use ReplaceAll in 1.12
 	}
 	return len(what) == 0
 }
@@ -77,13 +77,13 @@ func modeAssigned(who, what string, from os.FileMode) (os.FileMode, error) {
 
 	for _, c := range what {
 		switch c {
-		case 'w':
+		case 'r':
-			m := modeValueFrom(who, ModeWrite)
+			m := modeValueFrom(who, ModeRead)
 			if from&m == 0 {
 				from = from | m
 			}
-		case 'r':
+		case 'w':
-			m := modeValueFrom(who, ModeRead)
+			m := modeValueFrom(who, ModeWrite)
 			if from&m == 0 {
 				from = from | m
 			}
@@ -116,13 +116,13 @@ func modeAssigned(who, what string, from os.FileMode) (os.FileMode, error) {
 func modeAdded(who, what string, from os.FileMode) (os.FileMode, error) {
 	for _, c := range what {
 		switch c {
-		case 'w':
+		case 'r':
-			m := modeValueFrom(who, ModeWrite)
+			m := modeValueFrom(who, ModeRead)
 			if from&m == 0 {
 				from = from | m
 			}
-		case 'r':
+		case 'w':
-			m := modeValueFrom(who, ModeRead)
+			m := modeValueFrom(who, ModeWrite)
 			if from&m == 0 {
 				from = from | m
 			}
@@ -155,13 +155,13 @@ func modeAdded(who, what string, from os.FileMode) (os.FileMode, error) {
 func modeSubtracted(who, what string, from os.FileMode) (os.FileMode, error) {
 	for _, c := range what {
 		switch c {
-		case 'w':
+		case 'r':
-			m := modeValueFrom(who, ModeWrite)
+			m := modeValueFrom(who, ModeRead)
 			if from&m != 0 {
 				from = from &^ m
 			}
-		case 'r':
+		case 'w':
-			m := modeValueFrom(who, ModeRead)
+			m := modeValueFrom(who, ModeWrite)
 			if from&m != 0 {
 				from = from &^ m
 			}
@@ -214,18 +214,18 @@ func modeValueFrom(who string, modeType uint32) os.FileMode {
 }
 
 // ParseSymbolicModes parses a slice of symbolic mode strings. By default it
-// will accept all symbolic mode strings (=,+,-) but can be set to only accept
+// will only accept the assignment input (=), but if allowAssign is true, then
-// the assignment input with the 'onlyAssign' bool.
+// all symbolic mode strings (=,+,-) can be used as well.
 //
 // Symbolic mode is expected to be a string of who (user, group, other) then
 // the operation (=, +, -) then the change (read, write, execute, setuid,
 // setgid, sticky).
 //
-// Ex. ug=rw
+// Eg: ug=rw
 //
 // If you repeat yourself in the slice (ex. u=rw,u=w) ParseSymbolicModes will
 // fail with an error.
-func ParseSymbolicModes(modes []string, from os.FileMode, onlyAssign bool) (os.FileMode, error) {
+func ParseSymbolicModes(modes []string, from os.FileMode, allowAssign bool) (os.FileMode, error) {
 	symModes := make([]struct {
 		mode, who, what string
 
@@ -235,7 +235,8 @@ func ParseSymbolicModes(modes []string, from os.FileMode, onlyAssign bool) (os.F
 	for i, mode := range modes {
 		symModes[i].mode = mode
 
-		// If string contains '=' and no '+/-' it is safe to guess it is an assign
+		// If string contains '=' and no '+/-' it is safe to guess it is
+		// an assign.
 		if strings.Contains(mode, "=") && !strings.ContainsAny(mode, "+-") {
 			m := strings.Split(mode, "=")
 			if len(m) != 2 {
@@ -245,7 +246,8 @@ func ParseSymbolicModes(modes []string, from os.FileMode, onlyAssign bool) (os.F
 			symModes[i].what = m[1]
 			symModes[i].parse = modeAssigned
 			continue
-		} else if strings.Contains(mode, "+") && !strings.ContainsAny(mode, "=-") && !onlyAssign {
+
+		} else if strings.Contains(mode, "+") && !strings.ContainsAny(mode, "=-") && allowAssign {
 			m := strings.Split(mode, "+")
 			if len(m) != 2 {
 				return os.FileMode(0), fmt.Errorf("only a single %q is allowed but found %d", "+", len(m))
@@ -254,7 +256,8 @@ func ParseSymbolicModes(modes []string, from os.FileMode, onlyAssign bool) (os.F
 			symModes[i].what = m[1]
 			symModes[i].parse = modeAdded
 			continue
-		} else if strings.Contains(mode, "-") && !strings.ContainsAny(mode, "=+") && !onlyAssign {
+
+		} else if strings.Contains(mode, "-") && !strings.ContainsAny(mode, "=+") && allowAssign {
 			m := strings.Split(mode, "-")
 			if len(m) != 2 {
 				return os.FileMode(0), fmt.Errorf("only a single %q is allowed but found %d", "-", len(m))
@@ -268,13 +271,14 @@ func ParseSymbolicModes(modes []string, from os.FileMode, onlyAssign bool) (os.F
 		return os.FileMode(0), fmt.Errorf("%s is not a valid a symbolic mode", symModes[i].mode)
 	}
 
-	// Validate input and verify the slice of symbolic modes does not contain
+	// Validate input and verify the slice of symbolic modes does not
-	// redundancy.
+	// contain redundancy.
-	seen := make(map[rune]struct{}) // To validate all subjects are not dupicated
+	seen := make(map[rune]struct{}) // validate that subjects are not duplicated
 	for i := range symModes {
 		if strings.ContainsRune(symModes[i].who, 'a') || symModes[i].who == "" {
-			// If 'a' or empty who (implicit 'a') is called and there are more
+			// If 'a' or empty who (implicit 'a') is called and
-			// symbolic modes in the slice then it must be a repetition.
+			// there are more symbolic modes in the slice then it
+			// must be a repetition.
 			if len(symModes) > 1 {
 				return os.FileMode(0), fmt.Errorf("subject was repeated: each subject (u,g,o) is only accepted once")
 			}
@@ -294,7 +298,7 @@ func ParseSymbolicModes(modes []string, from os.FileMode, onlyAssign bool) (os.F
 		}
 	}
 
-	// Parse each sybolic mode accumulatively onto the from file mode.
+	// Parse each symbolic mode accumulatively onto the file mode.
 	for _, m := range symModes {
 		var err error
 		from, err = m.parse(m.who, m.what, from)

engine/util/mode_test.go

@@ -15,25 +15,25 @@
 // You should have received a copy of the GNU General Public License
 // along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-package util_test
+// +build !root
+
+package util
 
 import (
 	"fmt"
 	"os"
 	"strings"
 	"testing"
-
-	engineutil "github.com/purpleidea/mgmt/engine/util"
 )
 
 func TestSymbolicMode(t *testing.T) {
 	def := os.FileMode(0644) | os.ModeSetgid
 	symModeTests := []struct {
-		name       string
+		name        string
-		input      []string
+		input       []string
-		expect     os.FileMode
+		expect      os.FileMode
-		onlyAssign bool
+		allowAssign bool
-		err        error
+		err         error
 	}{
 		// Test single mode inputs.
 		{"assign", []string{"a=rwx"}, 0777, false, nil},
@@ -42,36 +42,36 @@ func TestSymbolicMode(t *testing.T) {
 		{"assign", []string{"ug=trwx"}, 0774 | os.ModeSticky, false, nil},
 		{"assign", []string{"o=rx"}, 0645 | os.ModeSetgid, false, nil},
 		{"assign", []string{"ug=srwx"}, 0774 | os.ModeSetgid | os.ModeSetuid, false, nil},
-		{"addition", []string{"o+rwx"}, 0647 | os.ModeSetgid, false, nil},
+		{"addition", []string{"o+rwx"}, 0647 | os.ModeSetgid, true, nil},
-		{"addition", []string{"u+x"}, 0744 | os.ModeSetgid, false, nil},
+		{"addition", []string{"u+x"}, 0744 | os.ModeSetgid, true, nil},
-		{"addition", []string{"u+x"}, 0744 | os.ModeSetgid, false, nil},
+		{"addition", []string{"u+x"}, 0744 | os.ModeSetgid, true, nil},
-		{"addition", []string{"u+s"}, 0644 | os.ModeSetgid | os.ModeSetuid, false, nil},
+		{"addition", []string{"u+s"}, 0644 | os.ModeSetgid | os.ModeSetuid, true, nil},
-		{"addition", []string{"u+t"}, 0644 | os.ModeSetgid | os.ModeSticky, false, nil},
+		{"addition", []string{"u+t"}, 0644 | os.ModeSetgid | os.ModeSticky, true, nil},
-		{"subtraction", []string{"o-rwx"}, 0640 | os.ModeSetgid, false, nil},
+		{"subtraction", []string{"o-rwx"}, 0640 | os.ModeSetgid, true, nil},
-		{"subtraction", []string{"u-w"}, 0444 | os.ModeSetgid, false, nil},
+		{"subtraction", []string{"u-w"}, 0444 | os.ModeSetgid, true, nil},
-		{"subtraction", []string{"g-s"}, 0644, false, nil},
+		{"subtraction", []string{"g-s"}, 0644, true, nil},
-		{"subtraction", []string{"u-t"}, 0644 | os.ModeSetgid, false, nil},
+		{"subtraction", []string{"u-t"}, 0644 | os.ModeSetgid, true, nil},
 
 		// Test multiple mode inputs.
-		{"mixed", []string{"u=rwx", "g+w"}, 0764 | os.ModeSetgid, false, nil},
+		{"mixed", []string{"u=rwx", "g+w"}, 0764 | os.ModeSetgid, true, nil},
-		{"mixed", []string{"u+rwx", "g=w"}, 0724, false, nil},
+		{"mixed", []string{"u+rwx", "g=w"}, 0724, true, nil},
 
-		// Test that a engineutil.ModeError is returned. Value is not checked so the
+		// Test that a ModeError is returned. Value is not checked so
-		// empty string works.
+		// the empty string works.
-		{"invalid separator", []string{"ug_rwx"}, os.FileMode(0), false, fmt.Errorf("ug_rwx is not a valid a symbolic mode")},
+		{"invalid separator", []string{"ug_rwx"}, os.FileMode(0), true, fmt.Errorf("ug_rwx is not a valid a symbolic mode")},
-		{"invalid who", []string{"xg=rwx"}, os.FileMode(0), false, fmt.Errorf("unexpected character assignment in xg=rwx")},
+		{"invalid who", []string{"xg=rwx"}, os.FileMode(0), true, fmt.Errorf("unexpected character assignment in xg=rwx")},
-		{"invalid what", []string{"g=rwy"}, os.FileMode(0), false, fmt.Errorf("unexpected character assignment in g=rwy")},
+		{"invalid what", []string{"g=rwy"}, os.FileMode(0), true, fmt.Errorf("unexpected character assignment in g=rwy")},
-		{"double assignment", []string{"a=rwx", "u=r"}, os.FileMode(0), false, fmt.Errorf("subject was repeated: each subject (u,g,o) is only accepted once")},
+		{"double assignment", []string{"a=rwx", "u=r"}, os.FileMode(0), true, fmt.Errorf("subject was repeated: each subject (u,g,o) is only accepted once")},
 
-		// Test onlyAssign bool
+		// Test allowAssign bool.
-		{"only assign", []string{"u+x", "g=rw"}, os.FileMode(0), true, fmt.Errorf("u+x is not a valid a symbolic mode")},
+		{"only assign", []string{"u+x", "g=rw"}, os.FileMode(0), false, fmt.Errorf("u+x is not a valid a symbolic mode")},
-		{"not only assign", []string{"u+x", "g=rw"}, os.FileMode(0764), false, nil},
+		{"not only assign", []string{"u+x", "g=rw"}, os.FileMode(0764), true, nil},
 	}
 
 	for _, ts := range symModeTests {
 		test := ts
 		t.Run(test.name+" "+strings.Join(test.input, ","), func(t *testing.T) {
-			got, err := engineutil.ParseSymbolicModes(test.input, def, test.onlyAssign)
+			got, err := ParseSymbolicModes(test.input, def, test.allowAssign)
 			if test.err != nil {
 				if err == nil {
 					t.Errorf("input: %s, expected error: %#v, but got nil", def, test.err)