Skip to content

Commit 46b8226

Browse files
kradalbykradalby
committed
simplify findUserByToken in ACL, add missing testcases (#2388)
* update users doc on unique constraints Signed-off-by: Kristoffer Dalby <[email protected]> * simplify finduser func Signed-off-by: Kristoffer Dalby <[email protected]> * add initial tests for findUserFromToken Signed-off-by: Kristoffer Dalby <[email protected]> * add changelog Signed-off-by: Kristoffer Dalby <[email protected]> --------- Signed-off-by: Kristoffer Dalby <[email protected]>
1 parent f22a48b commit 46b8226

File tree

4 files changed

+340
-22
lines changed

4 files changed

+340
-22
lines changed

‎CHANGELOG.md

+8
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,13 @@
11
# CHANGELOG
22

3+
4+
## 0.24.2 (2025-01-30)
5+
6+
### Changes
7+
8+
- Fix issue where email and username being equal fails to match in Policy
9+
[#2388](https://github.com/juanfont/headscale/pull/2388)
10+
311
## 0.24.1 (2025-01-23)
412

513
### Changes

‎hscontrol/policy/acls.go

+13-18
Original file line numberDiff line numberDiff line change
@@ -381,7 +381,7 @@ func (pol *ACLPolicy) CompileSSHPolicy(
381381
}
382382

383383
for _, userStr := range usersFromGroup {
384-
user, err := findUserFromTokenOrErr(users, userStr)
384+
user, err := findUserFromToken(users, userStr)
385385
if err != nil {
386386
log.Trace().Err(err).Msg("user not found")
387387
continue
@@ -400,7 +400,7 @@ func (pol *ACLPolicy) CompileSSHPolicy(
400400
// corresponds with the User info in the netmap.
401401
// TODO(kradalby): This is a bit of a hack, and it should go
402402
// away with the new policy where users can be reliably determined.
403-
if user, err := findUserFromTokenOrErr(users, srcToken); err == nil {
403+
if user, err := findUserFromToken(users, srcToken); err == nil {
404404
principals = append(principals, &tailcfg.SSHPrincipal{
405405
UserLogin: user.Username(),
406406
})
@@ -1001,7 +1001,7 @@ func (pol *ACLPolicy) TagsOfNode(
10011001
}
10021002
var found bool
10031003
for _, owner := range owners {
1004-
user, err := findUserFromTokenOrErr(users, owner)
1004+
user, err := findUserFromToken(users, owner)
10051005
if err != nil {
10061006
log.Trace().Caller().Err(err).Msg("could not determine user to filter tags by")
10071007
}
@@ -1038,7 +1038,7 @@ func (pol *ACLPolicy) TagsOfNode(
10381038
func filterNodesByUser(nodes types.Nodes, users []types.User, userToken string) types.Nodes {
10391039
var out types.Nodes
10401040

1041-
user, err := findUserFromTokenOrErr(users, userToken)
1041+
user, err := findUserFromToken(users, userToken)
10421042
if err != nil {
10431043
log.Trace().Caller().Err(err).Msg("could not determine user to filter nodes by")
10441044
return out
@@ -1058,24 +1058,19 @@ var (
10581058
ErrorMultipleUserMatching = errors.New("multiple users matching")
10591059
)
10601060

1061-
func findUserFromTokenOrErr(
1062-
users []types.User,
1063-
token string,
1064-
) (types.User, error) {
1061+
// findUserFromToken finds and returns a user based on the given token, prioritizing matches by ProviderIdentifier, followed by email or name.
1062+
// If no matching user is found, it returns an error of type ErrorNoUserMatching.
1063+
// If multiple users match the token, it returns an error indicating multiple matches.
1064+
func findUserFromToken(users []types.User, token string) (types.User, error) {
10651065
var potentialUsers []types.User
1066+
10661067
for _, user := range users {
10671068
if user.ProviderIdentifier.Valid && user.ProviderIdentifier.String == token {
1068-
// If a user is matching with a known unique field,
1069-
// disgard all other users and only keep the current
1070-
// user.
1071-
potentialUsers = []types.User{user}
1072-
1073-
break
1069+
// Prioritize ProviderIdentifier match and exit early
1070+
return user, nil
10741071
}
1075-
if user.Email == token {
1076-
potentialUsers = append(potentialUsers, user)
1077-
}
1078-
if user.Name == token {
1072+
1073+
if user.Email == token || user.Name == token {
10791074
potentialUsers = append(potentialUsers, user)
10801075
}
10811076
}

‎hscontrol/policy/acls_test.go

+312
Original file line numberDiff line numberDiff line change
@@ -4046,3 +4046,315 @@ func TestValidTagInvalidUser(t *testing.T) {
40464046
t.Errorf("TestValidTagInvalidUser() unexpected result (-want +got):\n%s", diff)
40474047
}
40484048
}
4049+
4050+
func TestFindUserByToken(t *testing.T) {
4051+
tests := []struct {
4052+
name string
4053+
users []types.User
4054+
token string
4055+
want types.User
4056+
wantErr bool
4057+
}{
4058+
{
4059+
name: "exact match by ProviderIdentifier",
4060+
users: []types.User{
4061+
{ProviderIdentifier: sql.NullString{Valid: true, String: "token1"}},
4062+
{Email: "[email protected]"},
4063+
},
4064+
token: "token1",
4065+
want: types.User{ProviderIdentifier: sql.NullString{Valid: true, String: "token1"}},
4066+
wantErr: false,
4067+
},
4068+
{
4069+
name: "no matches found",
4070+
users: []types.User{
4071+
{Email: "[email protected]"},
4072+
{Name: "username"},
4073+
},
4074+
token: "nonexistent-token",
4075+
want: types.User{},
4076+
wantErr: true,
4077+
},
4078+
{
4079+
name: "multiple matches by email and name",
4080+
users: []types.User{
4081+
{Email: "token2", Name: "notoken"},
4082+
{Name: "token2", Email: "[email protected]"},
4083+
},
4084+
token: "token2",
4085+
want: types.User{},
4086+
wantErr: true,
4087+
},
4088+
{
4089+
name: "match by email",
4090+
users: []types.User{
4091+
{Email: "[email protected]"},
4092+
{ProviderIdentifier: sql.NullString{Valid: true, String: "othertoken"}},
4093+
},
4094+
token: "[email protected]",
4095+
want: types.User{Email: "[email protected]"},
4096+
wantErr: false,
4097+
},
4098+
{
4099+
name: "match by name",
4100+
users: []types.User{
4101+
{Name: "token4"},
4102+
{Email: "[email protected]"},
4103+
},
4104+
token: "token4",
4105+
want: types.User{Name: "token4"},
4106+
wantErr: false,
4107+
},
4108+
{
4109+
name: "provider identifier takes precedence over email and name matches",
4110+
users: []types.User{
4111+
{ProviderIdentifier: sql.NullString{Valid: true, String: "token5"}},
4112+
{Email: "[email protected]", Name: "token5"},
4113+
},
4114+
token: "token5",
4115+
want: types.User{ProviderIdentifier: sql.NullString{Valid: true, String: "token5"}},
4116+
wantErr: false,
4117+
},
4118+
{
4119+
name: "empty token finds no users",
4120+
users: []types.User{
4121+
{Email: "[email protected]"},
4122+
{Name: "username6"},
4123+
},
4124+
token: "",
4125+
want: types.User{},
4126+
wantErr: true,
4127+
},
4128+
// Test case 1: Duplicate Emails with Unique ProviderIdentifiers
4129+
{
4130+
name: "duplicate emails with unique provider identifiers",
4131+
users: []types.User{
4132+
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid1"}, Email: "[email protected]"},
4133+
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid2"}, Email: "[email protected]"},
4134+
},
4135+
token: "[email protected]",
4136+
want: types.User{},
4137+
wantErr: true,
4138+
},
4139+
4140+
// Test case 2: Duplicate Names with Unique ProviderIdentifiers
4141+
{
4142+
name: "duplicate names with unique provider identifiers",
4143+
users: []types.User{
4144+
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid3"}, Name: "John Doe"},
4145+
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid4"}, Name: "John Doe"},
4146+
},
4147+
token: "John Doe",
4148+
want: types.User{},
4149+
wantErr: true,
4150+
},
4151+
4152+
// Test case 3: Duplicate Emails and Names with Unique ProviderIdentifiers
4153+
{
4154+
name: "duplicate emails and names with unique provider identifiers",
4155+
users: []types.User{
4156+
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid5"}, Email: "[email protected]", Name: "John Doe"},
4157+
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid6"}, Email: "[email protected]", Name: "John Doe"},
4158+
},
4159+
token: "[email protected]",
4160+
want: types.User{},
4161+
wantErr: true,
4162+
},
4163+
4164+
// Test case 4: Unique Names without ProviderIdentifiers
4165+
{
4166+
name: "unique names without provider identifiers",
4167+
users: []types.User{
4168+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
4169+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "Jane Smith", Email: "[email protected]"},
4170+
},
4171+
token: "John Doe",
4172+
want: types.User{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
4173+
wantErr: false,
4174+
},
4175+
4176+
// Test case 5: Duplicate Emails without ProviderIdentifiers but Unique Names
4177+
{
4178+
name: "duplicate emails without provider identifiers but unique names",
4179+
users: []types.User{
4180+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
4181+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "Jane Smith", Email: "[email protected]"},
4182+
},
4183+
token: "John Doe",
4184+
want: types.User{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
4185+
wantErr: false,
4186+
},
4187+
4188+
// Test case 6: Duplicate Names and Emails without ProviderIdentifiers
4189+
{
4190+
name: "duplicate names and emails without provider identifiers",
4191+
users: []types.User{
4192+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
4193+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
4194+
},
4195+
token: "John Doe",
4196+
want: types.User{},
4197+
wantErr: true,
4198+
},
4199+
4200+
// Test case 7: Multiple Users with the Same Email but Different Names and Unique ProviderIdentifiers
4201+
{
4202+
name: "multiple users with same email, different names, unique provider identifiers",
4203+
users: []types.User{
4204+
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid7"}, Email: "[email protected]", Name: "John Doe"},
4205+
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid8"}, Email: "[email protected]", Name: "Jane Smith"},
4206+
},
4207+
token: "[email protected]",
4208+
want: types.User{},
4209+
wantErr: true,
4210+
},
4211+
4212+
// Test case 8: Multiple Users with the Same Name but Different Emails and Unique ProviderIdentifiers
4213+
{
4214+
name: "multiple users with same name, different emails, unique provider identifiers",
4215+
users: []types.User{
4216+
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid9"}, Email: "[email protected]", Name: "John Doe"},
4217+
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid10"}, Email: "[email protected]", Name: "John Doe"},
4218+
},
4219+
token: "John Doe",
4220+
want: types.User{},
4221+
wantErr: true,
4222+
},
4223+
4224+
// Test case 9: Multiple Users with Same Email and Name but Unique ProviderIdentifiers
4225+
{
4226+
name: "multiple users with same email and name, unique provider identifiers",
4227+
users: []types.User{
4228+
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid11"}, Email: "[email protected]", Name: "John Doe"},
4229+
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid12"}, Email: "[email protected]", Name: "John Doe"},
4230+
},
4231+
token: "[email protected]",
4232+
want: types.User{},
4233+
wantErr: true,
4234+
},
4235+
4236+
// Test case 10: Multiple Users without ProviderIdentifiers but with Unique Names and Emails
4237+
{
4238+
name: "multiple users without provider identifiers, unique names and emails",
4239+
users: []types.User{
4240+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
4241+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "Jane Smith", Email: "[email protected]"},
4242+
},
4243+
token: "John Doe",
4244+
want: types.User{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
4245+
wantErr: false,
4246+
},
4247+
4248+
// Test case 11: Multiple Users without ProviderIdentifiers and Duplicate Emails but Unique Names
4249+
{
4250+
name: "multiple users without provider identifiers, duplicate emails but unique names",
4251+
users: []types.User{
4252+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
4253+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "Jane Smith", Email: "[email protected]"},
4254+
},
4255+
token: "John Doe",
4256+
want: types.User{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
4257+
wantErr: false,
4258+
},
4259+
4260+
// Test case 12: Multiple Users without ProviderIdentifiers and Duplicate Names but Unique Emails
4261+
{
4262+
name: "multiple users without provider identifiers, duplicate names but unique emails",
4263+
users: []types.User{
4264+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
4265+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
4266+
},
4267+
token: "John Doe",
4268+
want: types.User{},
4269+
wantErr: true,
4270+
},
4271+
4272+
// Test case 13: Multiple Users without ProviderIdentifiers and Duplicate Both Names and Emails
4273+
{
4274+
name: "multiple users without provider identifiers, duplicate names and emails",
4275+
users: []types.User{
4276+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
4277+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
4278+
},
4279+
token: "John Doe",
4280+
want: types.User{},
4281+
wantErr: true,
4282+
},
4283+
4284+
// Test case 14: Multiple Users with Same Email Without ProviderIdentifiers
4285+
{
4286+
name: "multiple users with same email without provider identifiers",
4287+
users: []types.User{
4288+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
4289+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "Jane Smith", Email: "[email protected]"},
4290+
},
4291+
token: "[email protected]",
4292+
want: types.User{},
4293+
wantErr: true,
4294+
},
4295+
4296+
// Test case 15: Multiple Users with Same Name Without ProviderIdentifiers
4297+
{
4298+
name: "multiple users with same name without provider identifiers",
4299+
users: []types.User{
4300+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
4301+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
4302+
},
4303+
token: "John Doe",
4304+
want: types.User{},
4305+
wantErr: true,
4306+
},
4307+
{
4308+
name: "Name field used as email address match",
4309+
users: []types.User{
4310+
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid3"}, Name: "[email protected]", Email: "[email protected]"},
4311+
},
4312+
token: "[email protected]",
4313+
want: types.User{ProviderIdentifier: sql.NullString{Valid: true, String: "pid3"}, Name: "[email protected]", Email: "[email protected]"},
4314+
wantErr: false,
4315+
},
4316+
{
4317+
name: "multiple users with same name as email and unique provider identifiers",
4318+
users: []types.User{
4319+
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid4"}, Name: "[email protected]", Email: "[email protected]"},
4320+
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid5"}, Name: "[email protected]", Email: "[email protected]"},
4321+
},
4322+
token: "[email protected]",
4323+
want: types.User{},
4324+
wantErr: true,
4325+
},
4326+
{
4327+
name: "no provider identifier and duplicate names as emails",
4328+
users: []types.User{
4329+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "[email protected]", Email: "[email protected]"},
4330+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "[email protected]", Email: "[email protected]"},
4331+
},
4332+
token: "[email protected]",
4333+
want: types.User{},
4334+
wantErr: true,
4335+
},
4336+
{
4337+
name: "name as email with multiple matches when provider identifier is not set",
4338+
users: []types.User{
4339+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "[email protected]", Email: "[email protected]"},
4340+
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "[email protected]", Email: "[email protected]"},
4341+
},
4342+
token: "[email protected]",
4343+
want: types.User{},
4344+
wantErr: true,
4345+
},
4346+
}
4347+
4348+
for _, tt := range tests {
4349+
t.Run(tt.name, func(t *testing.T) {
4350+
gotUser, err := findUserFromToken(tt.users, tt.token)
4351+
if (err != nil) != tt.wantErr {
4352+
t.Errorf("findUserFromToken() error = %v, wantErr %v", err, tt.wantErr)
4353+
return
4354+
}
4355+
if diff := cmp.Diff(tt.want, gotUser, util.Comparers...); diff != "" {
4356+
t.Errorf("findUserFromToken() unexpected result (-want +got):\n%s", diff)
4357+
}
4358+
})
4359+
}
4360+
}

0 commit comments

Comments
 (0)