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

Fixes juanfont#2300 Fixes juanfont#2307 Signed-off-by: Kristoffer Dalby <[email protected]>
kradalby · Dec 18, 2024 · 2ac4d3f · 2ac4d3f
1 parent 4139821
commit 2ac4d3f
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 29 deletions.
diff --git a/hscontrol/policy/acls.go b/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,37 +994,57 @@ func (pol *ACLPolicy) TagsOfNode(
 func filterNodesByUser(nodes types.Nodes, users []types.User, userToken string) types.Nodes {
 	var out types.Nodes
 
+	user, err := findUserFromTokenOrErr(users, userToken)
+	if err != nil {
+		log.Trace().Caller().Err(err).Msg("could not determine user to filter nodes by")
+		return out
+	}
+
+	for _, node := range nodes {
+		if node.User.ID == user.ID {
+			out = append(out, node)
+		}
+	}
+
+	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 == userToken {
+		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 == userToken {
+		if user.Email == token {
 			potentialUsers = append(potentialUsers, user)
 		}
-		if user.Name == userToken {
+		if user.Name == token {
 			potentialUsers = append(potentialUsers, user)
 		}
 	}
 
-	if len(potentialUsers) != 1 {
-		return nil
+	if len(potentialUsers) == 0 {
+		return types.User{}, fmt.Errorf("user with token %q not found: %w", token, ErrorNoUserMatching)
 	}
 
-	user := potentialUsers[0]
-
-	for _, node := range nodes {
-		if node.User.ID == user.ID {
-			out = append(out, node)
-		}
+	if len(potentialUsers) > 1 {
+		return types.User{}, fmt.Errorf("multiple users with token %q found: %w", token, ErrorNoUserMatching)
 	}
 
-	return out
+	return potentialUsers[0], nil
 }
 
 // FilterNodesByACL returns the list of peers authorized to be accessed from a given node.

diff --git a/hscontrol/policy/acls_test.go b/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{
-						Name: "joe",
-					},
+					User: users[0],
 					Hostinfo: &tailcfg.Hostinfo{
 						RequestTags: []string{"tag:valid"},
 					},
@@ -2774,9 +2778,7 @@ func Test_getTags(t *testing.T) {
 					},
 				},
 				node: &types.Node{
-					User: types.User{
-						Name: "joe",
-					},
+					User: users[0],
 					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{
-						Name: "joe",
-					},
+					User: users[0],
 					Hostinfo: &tailcfg.Hostinfo{
 						RequestTags: []string{
 							"tag:invalid",
@@ -2818,9 +2818,7 @@ func Test_getTags(t *testing.T) {
 					},
 				},
 				node: &types.Node{
-					User: types.User{
-						Name: "joe",
-					},
+					User: users[0],
 					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{
-						Name: "joe",
-					},
+					User: users[0],
 					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 {

diff --git a/hscontrol/policy/pm.go b/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
 }