fix issue where tags were only assigned to email, not username

Fixes #2300
Fixes #2307

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

hscontrol/policy/acls.go

@@ -934,6 +934,7 @@ func isAutoGroup(str string) bool {
 // Invalid tags are tags added by a user on a node, and that user doesn't have authority to add this tag.
 // Valid tags are tags added by a user that is allowed in the ACL policy to add this tag.
 func (pol *ACLPolicy) TagsOfNode(
+	users []types.User,
 	node *types.Node,
 ) ([]string, []string) {
 	var validTags []string
@@ -956,7 +957,12 @@ func (pol *ACLPolicy) TagsOfNode(
 			}
 			var found bool
 			for _, owner := range owners {
-				if node.User.Username() == owner {
+				user, err := findUserFromTokenOrErr(users, owner)
+				if err != nil {
+					log.Trace().Caller().Err(err).Msg("could not determine user to filter tags by")
+				}
+
+				if node.User.ID == user.ID {
 					found = true
 				}
 			}
@@ -988,30 +994,12 @@ func (pol *ACLPolicy) TagsOfNode(
 func filterNodesByUser(nodes types.Nodes, users []types.User, userToken string) types.Nodes {
 	var out types.Nodes
 
-	var potentialUsers []types.User
+	user, err := findUserFromTokenOrErr(users, userToken)
-	for _, user := range users {
+	if err != nil {
-		if user.ProviderIdentifier.Valid && user.ProviderIdentifier.String == userToken {
+		log.Trace().Caller().Err(err).Msg("could not determine user to filter nodes by")
-			// If a user is matching with a known unique field,
+		return out
-			// disgard all other users and only keep the current
-			// user.
-			potentialUsers = []types.User{user}
-
-			break
-		}
-		if user.Email == userToken {
-			potentialUsers = append(potentialUsers, user)
-		}
-		if user.Name == userToken {
-			potentialUsers = append(potentialUsers, user)
-		}
 	}
 
-	if len(potentialUsers) != 1 {
-		return nil
-	}
-
-	user := potentialUsers[0]
-
 	for _, node := range nodes {
 		if node.User.ID == user.ID {
 			out = append(out, node)
@@ -1021,6 +1009,44 @@ func filterNodesByUser(nodes types.Nodes, users []types.User, userToken string)
 	return out
 }
 
+var (
+	ErrorNoUserMatching       = errors.New("no user matching")
+	ErrorMultipleUserMatching = errors.New("multiple users matching")
+)
+
+func findUserFromTokenOrErr(
+	users []types.User,
+	token string,
+) (types.User, error) {
+	var potentialUsers []types.User
+	for _, user := range users {
+		if user.ProviderIdentifier.Valid && user.ProviderIdentifier.String == token {
+			// If a user is matching with a known unique field,
+			// disgard all other users and only keep the current
+			// user.
+			potentialUsers = []types.User{user}
+
+			break
+		}
+		if user.Email == token {
+			potentialUsers = append(potentialUsers, user)
+		}
+		if user.Name == token {
+			potentialUsers = append(potentialUsers, user)
+		}
+	}
+
+	if len(potentialUsers) == 0 {
+		return types.User{}, fmt.Errorf("user with token %q not found: %w", token, ErrorNoUserMatching)
+	}
+
+	if len(potentialUsers) > 1 {
+		return types.User{}, fmt.Errorf("multiple users with token %q found: %w", token, ErrorNoUserMatching)
+	}
+
+	return potentialUsers[0], nil
+}
+
 // FilterNodesByACL returns the list of peers authorized to be accessed from a given node.
 func FilterNodesByACL(
 	node *types.Node,

hscontrol/policy/acls_test.go

@@ -2735,6 +2735,12 @@ func TestReduceFilterRules(t *testing.T) {
 }
 
 func Test_getTags(t *testing.T) {
+	users := []types.User{
+		{
+			Model: gorm.Model{ID: 1},
+			Name:  "joe",
+		},
+	}
 	type args struct {
 		aclPolicy *ACLPolicy
 		node      *types.Node
@@ -2754,9 +2760,7 @@ func Test_getTags(t *testing.T) {
 					},
 				},
 				node: &types.Node{
-					User: types.User{
+					User: users[0],
-						Name: "joe",
-					},
 					Hostinfo: &tailcfg.Hostinfo{
 						RequestTags: []string{"tag:valid"},
 					},
@@ -2774,9 +2778,7 @@ func Test_getTags(t *testing.T) {
 					},
 				},
 				node: &types.Node{
-					User: types.User{
+					User: users[0],
-						Name: "joe",
-					},
 					Hostinfo: &tailcfg.Hostinfo{
 						RequestTags: []string{"tag:valid", "tag:invalid"},
 					},
@@ -2794,9 +2796,7 @@ func Test_getTags(t *testing.T) {
 					},
 				},
 				node: &types.Node{
-					User: types.User{
+					User: users[0],
-						Name: "joe",
-					},
 					Hostinfo: &tailcfg.Hostinfo{
 						RequestTags: []string{
 							"tag:invalid",
@@ -2818,9 +2818,7 @@ func Test_getTags(t *testing.T) {
 					},
 				},
 				node: &types.Node{
-					User: types.User{
+					User: users[0],
-						Name: "joe",
-					},
 					Hostinfo: &tailcfg.Hostinfo{
 						RequestTags: []string{"tag:invalid", "very-invalid"},
 					},
@@ -2834,9 +2832,7 @@ func Test_getTags(t *testing.T) {
 			args: args{
 				aclPolicy: &ACLPolicy{},
 				node: &types.Node{
-					User: types.User{
+					User: users[0],
-						Name: "joe",
-					},
 					Hostinfo: &tailcfg.Hostinfo{
 						RequestTags: []string{"tag:invalid", "very-invalid"},
 					},
@@ -2849,6 +2845,7 @@ func Test_getTags(t *testing.T) {
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			gotValid, gotInvalid := test.args.aclPolicy.TagsOfNode(
+				users,
 				test.args.node,
 			)
 			for _, valid := range gotValid {

hscontrol/policy/pm.go

@@ -162,7 +162,7 @@ func (pm *PolicyManagerV1) Tags(node *types.Node) []string {
 		return nil
 	}
 
-	tags, invalid := pm.pol.TagsOfNode(node)
+	tags, invalid := pm.pol.TagsOfNode(pm.users, node)
 	log.Debug().Strs("authorised_tags", tags).Strs("unauthorised_tags", invalid).Uint64("node.id", node.ID.Uint64()).Msg("tags provided by policy")
 	return tags
 }