feat(brokers/cache): Change user info validations to require at least…

… one remote group (#131) We used to allow the brokers to provide user information without any group. This creates problems when managing local groups. To prevent this, we now force the brokers to return at least one group for the user. The user group list must contain at least one remote group (with UGID) to act as the default group for the user. This means that it must be the first group in the list. UDENG-1865
ubuntu · Dec 7, 2023 · 96583a7 · 96583a7
2 parents 65456ed + 9d8fa24
commit 96583a7
Show file tree

Hide file tree

Showing 15 changed files with 112 additions and 81 deletions.
diff --git a/internal/brokers/broker.go b/internal/brokers/broker.go
@@ -336,6 +336,14 @@ func validateUserInfoAndGenerateIDs(brokerName string, rawMsg json.RawMessage) (
 	}
 	uInfo.UID = generateID(brokerName + uInfo.UUID)
 
+	// User must be a part of at least one group.
+	if len(uInfo.Groups) == 0 {
+		return users.UserInfo{}, fmt.Errorf("empty groups")
+	}
+	// The default group for the user is the default and it must have a UGID.
+	if uInfo.Groups[0].UGID == "" {
+		return users.UserInfo{}, fmt.Errorf("default group has empty UGID")
+	}
 	// Validate UGIDs and generate GIDs
 	for _, g := range uInfo.Groups {
 		if g.Name == "" {

diff --git a/internal/brokers/broker_test.go b/internal/brokers/broker_test.go
@@ -214,12 +214,11 @@ func TestIsAuthenticated(t *testing.T) {
 
 		cancelFirstCall bool
 	}{
-		"Successfully authenticate":                             {sessionID: "success"},
-		"Successfully authenticate after cancelling first call": {sessionID: "IA_second_call", secondCall: true},
-		"Denies authentication when broker times out":           {sessionID: "IA_timeout"},
-		"No error when auth.Next and no data":                   {sessionID: "IA_next"},
-
-		"No UGID means local group": {sessionID: "IA_info_missing_ugid"},
+		"Successfully authenticate":                              {sessionID: "success"},
+		"Successfully authenticate after cancelling first call":  {sessionID: "IA_second_call", secondCall: true},
+		"Denies authentication when broker times out":            {sessionID: "IA_timeout"},
+		"No error when auth.Next and no data":                    {sessionID: "IA_next"},
+		"No error when broker returns userinfo with empty gecos": {sessionID: "IA_info_missing_gecos"},
 
 		// broker errors
 		"Error when authenticating":                                           {sessionID: "IA_error"},
@@ -228,7 +227,9 @@ func TestIsAuthenticated(t *testing.T) {
 		"Error when broker returns invalid access":                            {sessionID: "IA_invalid_access"},
 		"Error when broker returns invalid userinfo":                          {sessionID: "IA_invalid_userinfo"},
 		"Error when broker returns userinfo with empty username":              {sessionID: "IA_info_missing_user_name"},
+		"Error when broker returns userinfo with no groups":                   {sessionID: "IA_info_missing_groups"},
 		"Error when broker returns userinfo with empty group name":            {sessionID: "IA_info_missing_group_name"},
+		"Error when broker returns userinfo with first group with empty UGID": {sessionID: "IA_info_missing_ugid"},
 		"Error when broker returns userinfo with empty UUID":                  {sessionID: "IA_info_missing_uuid"},
 		"Error when broker returns userinfo with invalid homedir":             {sessionID: "IA_info_invalid_home"},
 		"Error when broker returns userinfo with invalid shell":               {sessionID: "IA_info_invalid_shell"},

diff --git a/...estdata/TestIsAuthenticated/golden/error_when_broker_returns_userinfo_with_empty_username b/...estdata/TestIsAuthenticated/golden/error_when_broker_returns_userinfo_with_empty_username
@@ -4,7 +4,7 @@ FIRST CALL:
 	err: invalid user information provided by the broker ({
 		"name": "",
 		"uuid": "uuid-IA_info_missing_user_name",
-		"gecos": "gecos for ",
+		"gecos": "gecos for IA_info_missing_user_name",
 		"dir": "/home/IA_info_missing_user_name",
 		"shell": "/bin/sh/IA_info_missing_user_name",
 		"avatar": "avatar for ",

diff --git a/...n_broker_returns_userinfo_with_empty_ugid → ...userinfo_with_first_group_with_empty_ugid b/...n_broker_returns_userinfo_with_empty_ugid → ...userinfo_with_first_group_with_empty_ugid
@@ -9,4 +9,4 @@ FIRST CALL:
 		"shell": "/bin/sh/IA_info_missing_ugid",
 		"avatar": "avatar for IA_info_missing_ugid",
 		"groups": [ {"name": "group-IA_info_missing_ugid", "ugid": ""} ]
-	}): group "group-IA_info_missing_ugid" has empty UGID
+	}): default group has empty UGID
diff --git a/...ers/testdata/TestIsAuthenticated/golden/error_when_broker_returns_userinfo_with_no_groups b/...ers/testdata/TestIsAuthenticated/golden/error_when_broker_returns_userinfo_with_no_groups
@@ -0,0 +1,12 @@
+FIRST CALL:
+	access: 
+	data: 
+	err: invalid user information provided by the broker ({
+		"name": "IA_info_missing_groups",
+		"uuid": "uuid-IA_info_missing_groups",
+		"gecos": "gecos for IA_info_missing_groups",
+		"dir": "/home/IA_info_missing_groups",
+		"shell": "/bin/sh/IA_info_missing_groups",
+		"avatar": "avatar for IA_info_missing_groups",
+		"groups": [  ]
+	}): empty groups
diff --git a/...estdata/TestIsAuthenticated/golden/no_error_when_broker_returns_userinfo_with_empty_gecos b/...estdata/TestIsAuthenticated/golden/no_error_when_broker_returns_userinfo_with_empty_gecos
@@ -0,0 +1,4 @@
+FIRST CALL:
+	access: granted
+	data: {"Name":"IA_info_missing_gecos","UID":67151,"Gecos":"","Dir":"/home/IA_info_missing_gecos","Shell":"/bin/sh/IA_info_missing_gecos","Groups":[{"Name":"group-IA_info_missing_gecos","GID":66857}]}
+	err: <nil>
diff --git a/internal/brokers/testdata/TestIsAuthenticated/golden/no_ugid_means_local_group b/internal/brokers/testdata/TestIsAuthenticated/golden/no_ugid_means_local_group
diff --git a/internal/cache/db_test.go b/internal/cache/db_test.go
@@ -179,6 +179,25 @@ func TestUpdateFromUserInfo(t *testing.T) {
 				{Name: "newgroup1", GID: ptrValue(11111)},
 			},
 		},
+		"user1-with-only-local-group": {
+			Name:  "user1",
+			UID:   1111,
+			Gecos: "User1 gecos\nOn multiple lines",
+			Dir:   "/home/user1",
+			Shell: "/bin/bash",
+			Groups: []users.GroupInfo{
+				{Name: "local-group"},
+			},
+		},
+		"user1-without-gecos": {
+			Name:  "user1",
+			UID:   1111,
+			Dir:   "/home/user1",
+			Shell: "/bin/bash",
+			Groups: []users.GroupInfo{
+				{Name: "group1", GID: ptrValue(11111)},
+			},
+		},
 		"user1-without-groups": {
 			Name:  "user1",
 			UID:   1111,
@@ -249,18 +268,19 @@ func TestUpdateFromUserInfo(t *testing.T) {
 		wantErr     bool
 	}{
 		// New user
-		"Insert new user":                 {userCase: "user1"},
-		"Update last login time for user": {userCase: "user1", dbFile: "one_user_and_group"},
+		"Insert new user":                              {userCase: "user1"},
+		"Update last login time for user":              {userCase: "user1", dbFile: "one_user_and_group"},
+		"Insert new user without optional gecos field": {userCase: "user1-without-gecos"},
 
 		// User and Group renames
-		"Update user by changing attributes":  {userCase: "user1-new-attributes", dbFile: "one_user_and_group"},
-		"Update group by changing attributes": {userCase: "group1-new-attributes", dbFile: "one_user_and_group"},
+		"Update user by changing attributes":                      {userCase: "user1-new-attributes", dbFile: "one_user_and_group"},
+		"Update user by removing optional gecos field if not set": {userCase: "user1-without-gecos", dbFile: "one_user_and_group"},
+		"Update group by changing attributes":                     {userCase: "group1-new-attributes", dbFile: "one_user_and_group"},
 
 		// Group updates
-		"Update user and keep existing groups without specifying them": {userCase: "user1-without-groups", dbFile: "one_user_and_group"},
-		"Update user by adding a new group":                            {userCase: "user1-with-new-group", dbFile: "one_user_and_group"},
-		"Update user by adding a new default group":                    {userCase: "user1-with-new-default-group", dbFile: "one_user_and_group"},
-		"Remove group from user":                                       {userCase: "user1-with-only-new-group", dbFile: "one_user_and_group"},
+		"Update user by adding a new group":         {userCase: "user1-with-new-group", dbFile: "one_user_and_group"},
+		"Update user by adding a new default group": {userCase: "user1-with-new-default-group", dbFile: "one_user_and_group"},
+		"Remove group from user":                    {userCase: "user1-with-only-new-group", dbFile: "one_user_and_group"},
 
 		// Multi users handling
 		"Update only user even if we have multiple of them":     {userCase: "user1", dbFile: "multiple_users_and_groups"},
@@ -271,17 +291,16 @@ func TestUpdateFromUserInfo(t *testing.T) {
 		"Local groups are filtered": {userCase: "user1-with-local-group"},
 
 		// Allowed inconsistent cases
-		"Invalid value entry in groupByID but user restating group recreates entries":       {userCase: "user1", dbFile: "invalid_entry_in_groupByID"},
-		"Invalid value entry in userByID recreates entries":                                 {userCase: "user1", dbFile: "invalid_entry_in_userByID"},
-		"Invalid value entry in groupByName recreates entries":                              {userCase: "user1", dbFile: "invalid_entry_in_groupByName"},
-		"Invalid value entry in groupByName recreates entries even without restating group": {userCase: "user1-without-groups", dbFile: "invalid_entry_in_groupByName"},
-		"Invalid value entry in userByName recreates entries":                               {userCase: "user1", dbFile: "invalid_entry_in_userByName"},
-		"Invalid value entries in other user and groups don't impact current request":       {userCase: "user1", dbFile: "invalid_entries_but_user_and_group1"},
+		"Invalid value entry in groupByID but user restating group recreates entries": {userCase: "user1", dbFile: "invalid_entry_in_groupByID"},
+		"Invalid value entry in userByID recreates entries":                           {userCase: "user1", dbFile: "invalid_entry_in_userByID"},
+		"Invalid value entry in groupByName recreates entries":                        {userCase: "user1", dbFile: "invalid_entry_in_groupByName"},
+		"Invalid value entry in userByName recreates entries":                         {userCase: "user1", dbFile: "invalid_entry_in_userByName"},
+		"Invalid value entries in other user and groups don't impact current request": {userCase: "user1", dbFile: "invalid_entries_but_user_and_group1"},
 
 		// Error cases
-		"Error on new user without any groups":                                                     {userCase: "user1-without-groups", wantErr: true},
+		"Error on user without any groups":                                                         {userCase: "user1-without-groups", wantErr: true},
+		"Error on user with only local group":                                                      {userCase: "user1-with-only-local-group", wantErr: true},
 		"Error on invalid value entry in userToGroups clear database":                              {userCase: "user1", dbFile: "invalid_entry_in_userToGroups", wantErr: true, wantClearDB: true},
-		"Error on invalid value entry in groupByID with user not restating groups clear database":  {userCase: "user1-without-groups", dbFile: "invalid_entry_in_groupByID", wantErr: true, wantClearDB: true},
 		"Error on invalid value entry in groupToUsers clear database":                              {userCase: "user1", dbFile: "invalid_entry_in_groupToUsers", wantErr: true, wantClearDB: true},
 		"Error on invalid value entry in groupToUsers for user dropping from group clear database": {userCase: "user1", dbFile: "invalid_entry_in_groupToUsers_secondary_group", wantErr: true, wantClearDB: true},
 		"Error on invalid value entry in groupByID for user dropping from group clear database":    {userCase: "user1", dbFile: "invalid_entry_in_groupByID_secondary_group", wantErr: true, wantClearDB: true},

diff --git a/internal/cache/getusers.go b/internal/cache/getusers.go
@@ -13,7 +13,7 @@ type UserPasswdShadow struct {
 	Name  string
 	UID   int
 	GID   int
-	Gecos string
+	Gecos string // Gecos is an optional field. It can be empty.
 	Dir   string
 	Shell string
 

diff --git a/...cache/testdata/TestUpdateFromUserInfo/golden/insert_new_user_without_optional_gecos_field b/...cache/testdata/TestUpdateFromUserInfo/golden/insert_new_user_without_optional_gecos_field
@@ -0,0 +1,13 @@
+GroupByID:
+    "11111": '{"Name":"group1","GID":11111}'
+GroupByName:
+    group1: '{"Name":"group1","GID":11111}'
+GroupToUsers:
+    "11111": '{"GID":11111,"UIDs":[1111]}'
+UserByID:
+    "1111": '{"Name":"user1","UID":1111,"GID":11111,"Gecos":"","Dir":"/home/user1","Shell":"/bin/bash","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"ABCDETIME"}'
+UserByName:
+    user1: '{"Name":"user1","UID":1111,"GID":11111,"Gecos":"","Dir":"/home/user1","Shell":"/bin/bash","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"ABCDETIME"}'
+UserToBroker: {}
+UserToGroups:
+    "1111": '{"UID":1111,"GIDs":[11111]}'
diff --git a/.../golden/invalid_value_entry_in_groupbyname_recreates_entries_even_without_restating_group b/.../golden/invalid_value_entry_in_groupbyname_recreates_entries_even_without_restating_group
diff --git a/...estUpdateFromUserInfo/golden/update_user_and_keep_existing_groups_without_specifying_them b/...estUpdateFromUserInfo/golden/update_user_and_keep_existing_groups_without_specifying_them
diff --git a/...ata/TestUpdateFromUserInfo/golden/update_user_by_removing_optional_gecos_field_if_not_set b/...ata/TestUpdateFromUserInfo/golden/update_user_by_removing_optional_gecos_field_if_not_set
@@ -0,0 +1,13 @@
+GroupByID:
+    "11111": '{"Name":"group1","GID":11111}'
+GroupByName:
+    group1: '{"Name":"group1","GID":11111}'
+GroupToUsers:
+    "11111": '{"GID":11111,"UIDs":[1111]}'
+UserByID:
+    "1111": '{"Name":"user1","UID":1111,"GID":11111,"Gecos":"","Dir":"/home/user1","Shell":"/bin/bash","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"ABCDETIME"}'
+UserByName:
+    user1: '{"Name":"user1","UID":1111,"GID":11111,"Gecos":"","Dir":"/home/user1","Shell":"/bin/bash","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"ABCDETIME"}'
+UserToBroker: {}
+UserToGroups:
+    "1111": '{"UID":1111,"GIDs":[11111]}'
diff --git a/internal/cache/update.go b/internal/cache/update.go
@@ -16,15 +16,17 @@ import (
 // UpdateFromUserInfo inserts or updates user and group buckets from the user information.
 func (c *Cache) UpdateFromUserInfo(u users.UserInfo) error {
 	// create bucket contents dynamically
-	gid := -1
-	if len(u.Groups) > 0 && u.Groups[0].GID != nil {
-		gid = *u.Groups[0].GID
+	if len(u.Groups) == 0 {
+		return fmt.Errorf("no group provided for user %s (%v)", u.Name, u.UID)
+	}
+	if u.Groups[0].GID == nil {
+		return fmt.Errorf("no gid provided for default group %q", u.Groups[0].Name)
 	}
 	userDB := userDB{
 		UserPasswdShadow: UserPasswdShadow{
 			Name:           u.Name,
 			UID:            u.UID,
-			GID:            gid,
+			GID:            *u.Groups[0].GID,
 			Gecos:          u.Gecos,
 			Dir:            u.Dir,
 			Shell:          u.Shell,
@@ -66,26 +68,6 @@ func (c *Cache) UpdateFromUserInfo(u users.UserInfo) error {
 			return err
 		}
 
-		// No groups were specified for this request.
-		if userDB.GID == -1 {
-			if len(previousGroupsForCurrentUser.GIDs) == 0 {
-				return fmt.Errorf("no group provided for user %v (%v) and no previous record found", userDB.Name, userDB.UID)
-			}
-
-			for _, gid := range previousGroupsForCurrentUser.GIDs {
-				g, err := getFromBucket[groupDB](buckets[groupByIDBucketName], gid)
-				if err != nil {
-					c.requestClearDatabase()
-					return err
-				}
-				groupContents = append(groupContents, groupDB{
-					Name: g.Name,
-					GID:  g.GID,
-				})
-			}
-			userDB.GID = groupContents[0].GID
-		}
-
 		/* 1. Handle user update */
 		updateUser(buckets, userDB)
 

diff --git a/internal/testutils/broker.go b/internal/testutils/broker.go
@@ -333,6 +333,7 @@ func userInfoFromName(parsedID string, extraGroups []users.GroupInfo) string {
 	group := "group-" + parsedID
 	home := "/home/" + parsedID
 	shell := "/bin/sh/" + parsedID
+	gecos := "gecos for " + parsedID
 	uuid := "uuid-" + parsedID
 	ugid := "ugid-" + parsedID
 
@@ -345,6 +346,10 @@ func userInfoFromName(parsedID string, extraGroups []users.GroupInfo) string {
 		uuid = ""
 	case "IA_info_missing_ugid":
 		ugid = ""
+	case "IA_info_missing_gecos":
+		gecos = ""
+	case "IA_info_missing_groups":
+		group = "-"
 	case "IA_info_invalid_home":
 		home = "this is not a homedir"
 	case "IA_info_invalid_shell":
@@ -367,21 +372,25 @@ func userInfoFromName(parsedID string, extraGroups []users.GroupInfo) string {
 			UGID: ugid,
 		})
 	}
+	if group == "-" {
+		groups = []groupJSONInfo{}
+	}
 
 	user := struct {
 		Name   string
 		UUID   string
 		Home   string
 		Shell  string
 		Groups []groupJSONInfo
-	}{Name: name, UUID: uuid, Home: home, Shell: shell, Groups: groups}
+		Gecos  string
+	}{Name: name, UUID: uuid, Home: home, Shell: shell, Groups: groups, Gecos: gecos}
 
 	// only used for tests, we can ignore the template execution error as the returned data will be failing.
 	var buf bytes.Buffer
 	_ = template.Must(template.New("").Parse(`{
 		"name": "{{.Name}}",
 		"uuid": "{{.UUID}}",
-		"gecos": "gecos for {{.Name}}",
+		"gecos": "{{.Gecos}}",
 		"dir": "{{.Home}}",
 		"shell": "{{.Shell}}",
 		"avatar": "avatar for {{.Name}}",