fix weird behavior with auxiliary groups in user base image

By introducing a new field SkipBaseGroups.

Author: majewsky

CHANGELOG.md

@@ -1,3 +1,13 @@
+# v2.2 (TBD)
+
+Changes:
+
+- User definitions now have a new flag `SkipBaseGroups` which, if set, causes `holo-build` to ignore existing auxiliary
+  groups that this user is in before the first `holo apply` of this user.
+- Fix a bug where user definitions previously behaved as if `SkipBaseGroups = true`, even if the intention always was to
+  behave like `SkipBaseGroups = false` (which is the default).
+- Fix a bug where `user:root` could not be provisioned to because the user ID 0 was misinterpreted as "no UID".
+
 # v2.1 (2017-11-30)
 
 Changes:

cmd/holo-users-groups/definition.go

@@ -43,6 +43,17 @@ const (
 	MergeNumericIDOnly
 )
 
+//SkipMethod is the third argument for EntityDefinition.Merge().
+type SkipMethod uint
+
+const (
+	//SkipDisabled does not skip any fields during merging.
+	SkipDisabled SkipMethod = iota
+	//SkipEnabled skips fields where one of the definitions instructs us to do so
+	//(causing the other side's attribute to win).
+	SkipEnabled
+)
+
 //EntityDefinition contains data from a definition file that describes an entity
 //(a user account or group). Definitions can also be obtained by scanning the
 //user/group databases.
@@ -76,7 +87,7 @@ type EntityDefinition interface {
 	//
 	//The merge `method` tells which attributes may be merged. Possible values
 	//are MergeWhereCompatible, MergeEmptyOnly and MergeNumericIDOnly.
-	Merge(other EntityDefinition, method MergeMethod) (EntityDefinition, []error)
+	Merge(other EntityDefinition, mMethod MergeMethod, sMethod SkipMethod) (EntityDefinition, []error)
 	//Apply provisions this entity. The argument indicates the currently
 	//provisioned state. The argument's concrete type must match the callee.
 	Apply(provisioned EntityDefinition) error
@@ -116,14 +127,15 @@ type GroupDefinition struct {
 
 //UserDefinition represents a UNIX user account (as registered in /etc/passwd).
 type UserDefinition struct {
-	Name    string   `toml:"name"`              //the user name (the first field in /etc/passwd)
-	Comment string   `toml:"comment,omitempty"` //the full name (sometimes also called "comment"; the fifth field in /etc/passwd)
-	UID     *int     `toml:"uid,omitzero"`      //the user ID (the third field in /etc/passwd), or nil if no specific UID is enforced
-	System  bool     `toml:"system,omitempty"`  //whether the group is a system group (this influences the GID selection if gid = 0)
-	Home    string   `toml:"home,omitempty"`    //path to the user's home directory (or empty to use the default)
-	Group   string   `toml:"group,omitempty"`   //the name of the user's initial login group (or empty to use the default)
-	Groups  []string `toml:"groups,omitempty"`  //the names of supplementary groups which the user is also a member of
-	Shell   string   `toml:"shell,omitempty"`   //path to the user's login shell (or empty to use the default)
+	Name           string   `toml:"name"`                     //the user name (the first field in /etc/passwd)
+	Comment        string   `toml:"comment,omitempty"`        //the full name (sometimes also called "comment"; the fifth field in /etc/passwd)
+	UID            *int     `toml:"uid,omitzero"`             //the user ID (the third field in /etc/passwd), or nil if no specific UID is enforced
+	System         bool     `toml:"system,omitempty"`         //whether the group is a system group (this influences the GID selection if gid = 0)
+	Home           string   `toml:"home,omitempty"`           //path to the user's home directory (or empty to use the default)
+	Group          string   `toml:"group,omitempty"`          //the name of the user's initial login group (or empty to use the default)
+	Groups         []string `toml:"groups,omitempty"`         //the names of supplementary groups which the user is also a member of
+	Shell          string   `toml:"shell,omitempty"`          //path to the user's login shell (or empty to use the default)
+	SkipBaseGroups bool     `toml:"skipBaseGroups,omitempty"` //whether to consider supplementary groups in the base image during merging
 }
 
 //TypeName implements the EntityDefinition interface.
@@ -172,7 +184,11 @@ func (u *UserDefinition) Attributes() string {
 		attrs = append(attrs, "login group: "+u.Group)
 	}
 	if len(u.Groups) > 0 {
-		attrs = append(attrs, "groups: "+strings.Join(u.Groups, ","))
+		if u.SkipBaseGroups {
+			attrs = append(attrs, "exact groups: "+strings.Join(u.Groups, ","))
+		} else {
+			attrs = append(attrs, "groups: "+strings.Join(u.Groups, ","))
+		}
 	}
 	if u.Shell != "" {
 		attrs = append(attrs, "login shell: "+u.Shell)
@@ -194,9 +210,12 @@ func (g *GroupDefinition) WithSerializableState(callback func(EntityDefinition))
 
 //WithSerializableState implements the EntityDefinition interface.
 func (u *UserDefinition) WithSerializableState(callback func(EntityDefinition)) {
-	//we don't want to serialize the `system` attribute in diffs etc.
+	//we don't want to serialize the `system` and `skipBaseGroups` attributes in diffs etc.
 	system := u.System
+	skipBaseGroups := u.SkipBaseGroups
 	u.System = false
+	u.SkipBaseGroups = false
 	callback(u)
 	u.System = system
+	u.SkipBaseGroups = skipBaseGroups
 }

cmd/holo-users-groups/entity.go

@@ -1,6 +1,6 @@
 /*******************************************************************************
 *
-* Copyright 2015-2016 Stefan Majewsky <majewsky@gmx.net>
+* Copyright 2015-2017 Stefan Majewsky <majewsky@gmx.net>
 *
 * This file is part of Holo.
 *
@@ -110,9 +110,11 @@ func (e *Entity) Apply(withForce bool) error {
 	//case, conflicts worthy of "--force" are detected by comparing that to the
 	//definition)
 	conflictCheckImage := e.Definition
+	conflictCheckMethod := MergeWhereCompatible
 	provisionedState, err := ProvisionedImageDir.LoadImageFor(e.Definition)
 	if err == nil {
-		conflictCheckImage, _ = conflictCheckImage.Merge(provisionedState, MergeEmptyOnly)
+		conflictCheckImage, _ = conflictCheckImage.Merge(provisionedState, MergeEmptyOnly, SkipDisabled)
+		conflictCheckMethod = MergeEmptyOnly
 	} else {
 		if os.IsNotExist(err) {
 			provisionedState = nil
@@ -132,22 +134,22 @@ func (e *Entity) Apply(withForce bool) error {
 		//   actual state and provisioned state)
 		//3. the entity has *not* been provisioned yet and the definition conflicts
 		//   with the current state (i.e. the base image)
-		_, conflicts := actualState.Merge(conflictCheckImage, MergeEmptyOnly)
+		_, conflicts := actualState.Merge(conflictCheckImage, conflictCheckMethod, SkipEnabled)
 		if len(conflicts) > 0 {
 			PrintCommandMessage("requires --force to overwrite\n")
 			return nil
 		}
 	}
 
 	//desired state is obtained by merging the definition with the base image
-	desiredState, _ := e.Definition.Merge(baseImage, MergeWhereCompatible)
+	desiredState, _ := e.Definition.Merge(baseImage, MergeWhereCompatible, SkipEnabled)
 	//but make sure that we don't see a difference just because the definition
 	//does not define a particular attribute
-	desiredState, _ = desiredState.Merge(actualState, MergeEmptyOnly)
+	desiredState, _ = desiredState.Merge(actualState, MergeEmptyOnly, SkipDisabled)
 	//but if no one knows what to do, re-use the provisioned state (this is
 	//important when restoring an entity that was deleted by someone else)
 	if provisionedState != nil {
-		desiredState, _ = desiredState.Merge(provisionedState, MergeEmptyOnly)
+		desiredState, _ = desiredState.Merge(provisionedState, MergeEmptyOnly, SkipDisabled)
 	}
 
 	//check if changes are necessary
@@ -217,7 +219,7 @@ func (e *Entity) PrepareDiff() error {
 	if baseState == nil {
 		baseState = actualState
 	}
-	desiredState, _ := e.Definition.Merge(baseState, MergeWhereCompatible)
+	desiredState, _ := e.Definition.Merge(baseState, MergeWhereCompatible, SkipDisabled)
 	desiredPath := filepath.Join(tempDir, "desired.toml")
 	err = SerializeDefinitionIntoFile(desiredState, desiredPath)
 	if err != nil {

cmd/holo-users-groups/merge.go

@@ -42,7 +42,7 @@ func (e MergeError) Error() string {
 }
 
 //Merge implements the EntityDefinition interface.
-func (g *GroupDefinition) Merge(other EntityDefinition, method MergeMethod) (EntityDefinition, []error) {
+func (g *GroupDefinition) Merge(other EntityDefinition, mMethod MergeMethod, sMethod SkipMethod) (EntityDefinition, []error) {
 	//start by cloning `other`
 	if other.EntityID() != g.EntityID() {
 		panic("tried to merge entities with different IDs")
@@ -59,7 +59,7 @@ func (g *GroupDefinition) Merge(other EntityDefinition, method MergeMethod) (Ent
 	}
 
 	//with MergeNumericIDOnly, only the GID may be merged
-	if method == MergeNumericIDOnly {
+	if mMethod == MergeNumericIDOnly {
 		//we need all the other attributes from `u`, so it's easier to just take another copy
 		theResult := *g
 		theResult.GID = result.GID
@@ -73,7 +73,7 @@ func (g *GroupDefinition) Merge(other EntityDefinition, method MergeMethod) (Ent
 }
 
 //Merge implements the EntityDefinition interface.
-func (u *UserDefinition) Merge(other EntityDefinition, method MergeMethod) (EntityDefinition, []error) {
+func (u *UserDefinition) Merge(other EntityDefinition, mMethod MergeMethod, sMethod SkipMethod) (EntityDefinition, []error) {
 	//start by cloning `other`
 	if other.EntityID() != u.EntityID() {
 		panic("tried to merge entities with different IDs")
@@ -91,7 +91,7 @@ func (u *UserDefinition) Merge(other EntityDefinition, method MergeMethod) (Enti
 	}
 
 	//with MergeNumericIDOnly, only the UID may be merged
-	if method == MergeNumericIDOnly {
+	if mMethod == MergeNumericIDOnly {
 		//we need all the other attributes from `u`, so it's easier to just take another copy
 		theResult := *u
 		theResult.UID = result.UID
@@ -127,7 +127,7 @@ func (u *UserDefinition) Merge(other EntityDefinition, method MergeMethod) (Enti
 
 	//auxiliary groups can always be added, but only under the
 	//MergeWhereCompatible method
-	if method == MergeEmptyOnly {
+	if mMethod == MergeEmptyOnly {
 		if len(result.Groups) > 0 && len(u.Groups) > 0 {
 			sort.Strings(result.Groups)
 			sort.Strings(u.Groups)
@@ -141,8 +141,15 @@ func (u *UserDefinition) Merge(other EntityDefinition, method MergeMethod) (Enti
 			result.Groups = u.Groups
 		}
 	} else {
-		for _, group := range u.Groups {
-			result.Groups, _ = appendIfMissing(result.Groups, group)
+		//auxiliary groups will not be merged if we are respecting a SkipBaseGroups flag
+		if sMethod == SkipEnabled && u.SkipBaseGroups {
+			result.Groups = u.Groups
+		} else if sMethod == SkipEnabled && result.SkipBaseGroups {
+			//do not change result.Groups
+		} else {
+			for _, group := range u.Groups {
+				result.Groups, _ = appendIfMissing(result.Groups, group)
+			}
 		}
 	}
 	//make sure that the groups list is always sorted (esp. for reproducible test output)

cmd/holo-users-groups/passwd.go

@@ -70,7 +70,7 @@ func StoreAppliedState(def EntityDefinition, previous EntityDefinition) {
 
 		//merge attributes from previous actual state that were not specified
 		//in the newly applied state
-		def, _ = def.Merge(previous, MergeEmptyOnly)
+		def, _ = def.Merge(previous, MergeEmptyOnly, SkipDisabled)
 
 		appliedStates[def.EntityID()] = def
 	}

cmd/holo-users-groups/scan.go

@@ -167,7 +167,7 @@ func readDefinitionFile(definitionPath string, entities *map[string]*Entity) err
 		entity, exists := (*entities)[id]
 		if exists {
 			//stacked definition for this entity -> merge into existing entity
-			mergedDef, mergeErrors := def.Merge(entity.Definition, MergeWhereCompatible)
+			mergedDef, mergeErrors := def.Merge(entity.Definition, MergeWhereCompatible, SkipDisabled)
 			if len(mergeErrors) == 0 {
 				entity.Definition = mergedDef
 			} else {

doc/holo-users-groups.8.pod

@@ -56,6 +56,32 @@ Whenever an entity is successfully applied, its state will be recorded into
 F</var/lib/holo/users-groups/provisioned/$entity_id.toml>. This provisioned
 image will be used for diffing.
 
+=head3 Merging of attributes
+
+The desired state of the entity is computed by merging each attribute of the
+entity definition into the base image at
+F</var/lib/holo/users-groups/base/$entity_id.toml>. For most attributes, merging
+succeeds if the attribute is set by only one side or the other (or by neither
+side), or if the attribute is set to the same value on both sides.
+
+As an exception, the C<groups> field (the list of auxiliary groups) of a user
+definition is merged by compiling a list of all groups mentioned on either side.
+For example, if C</etc/passwd> contains a user C<http> by default, which is part
+of the auxiliary group C<grp1>, and you then add an entity definition like so:
+
+    [[user]]
+    name = "http"
+    groups = ["grp2"]
+
+Then after C<holo apply user:http>, the user will be part of both C<grp1>
+and C<grp2>. If you want to overwrite the set of auxiliary groups from the
+base image, set the C<skipBaseGroups> flag like so:
+
+    [[user]]
+    name = "http"
+    groups = ["grp2"]
+    skipBaseGroups = true
+
 =head2 Diff operation
 
 Display a diff if the current state of the entity conflicts with the entity

test/users-groups/02-users/expected-apply-force-output

@@ -18,9 +18,9 @@ MOCK: usermod --gid users wronggroup
 
 Working on user:wronggroups
   found in target/usr/share/holo/users-groups/01-users.toml
-      with groups: network
+      with exact groups: network
 
-MOCK: usermod --groups network,video wronggroups
+MOCK: usermod --groups network wronggroups
 
 Working on user:wronghome
   found in target/usr/share/holo/users-groups/01-users.toml

test/users-groups/02-users/expected-apply-output

@@ -29,20 +29,9 @@ Working on user:wronggroup
 
 Working on user:wronggroups
   found in target/usr/share/holo/users-groups/01-users.toml
-      with groups: network
+      with exact groups: network
 
-!! Entity has been modified by user (use --force to overwrite)
-
-    diff --holo target/tmp/holo/users-groups/user:wronggroups/desired.toml target/tmp/holo/users-groups/user:wronggroups/actual.toml
-    --- target/tmp/holo/users-groups/user:wronggroups/desired.toml
-    +++ target/tmp/holo/users-groups/user:wronggroups/actual.toml
-    @@ -3,5 +3,5 @@ name = "wronggroups"
-     uid = 1005
-     home = "/home/wronggroups"
-     group = "users"
-    -groups = ["network", "video"]
-    +groups = ["video"]
-     shell = "/bin/zsh"
+MOCK: usermod --groups network wronggroups
 
 Working on user:wronghome
   found in target/usr/share/holo/users-groups/01-users.toml

test/users-groups/02-users/expected-scan-output

@@ -16,7 +16,7 @@ user:wronggroup
 
 user:wronggroups
     found in target/usr/share/holo/users-groups/01-users.toml
-        with groups: network
+        with exact groups: network
 
 user:wronghome
     found in target/usr/share/holo/users-groups/01-users.toml