Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

simplify findUserByToken in ACL, add missing testcases #2388

Merged
merged 4 commits into from
Jan 30, 2025
Merged

simplify findUserByToken in ACL, add missing testcases #2388

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
86662dd
update users doc on unique constraints
kradalby Jan 28, 2025
7084086
simplify finduser func
kradalby Jan 28, 2025
d9e575f
add initial tests for findUserFromToken
kradalby Jan 28, 2025
209093f
add changelog
kradalby Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 13 additions & 18 deletions hscontrol/policy/acls.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ func (pol *ACLPolicy) CompileSSHPolicy(
}

for _, userStr := range usersFromGroup {
user, err := findUserFromTokenOrErr(users, userStr)
user, err := findUserFromToken(users, userStr)
if err != nil {
log.Trace().Err(err).Msg("user not found")
continue
Expand All @@ -400,7 +400,7 @@ func (pol *ACLPolicy) CompileSSHPolicy(
// corresponds with the User info in the netmap.
// TODO(kradalby): This is a bit of a hack, and it should go
// away with the new policy where users can be reliably determined.
if user, err := findUserFromTokenOrErr(users, srcToken); err == nil {
if user, err := findUserFromToken(users, srcToken); err == nil {
principals = append(principals, &tailcfg.SSHPrincipal{
UserLogin: user.Username(),
})
Expand Down Expand Up @@ -1001,7 +1001,7 @@ func (pol *ACLPolicy) TagsOfNode(
}
var found bool
for _, owner := range owners {
user, err := findUserFromTokenOrErr(users, owner)
user, err := findUserFromToken(users, owner)
if err != nil {
log.Trace().Caller().Err(err).Msg("could not determine user to filter tags by")
}
Expand Down Expand Up @@ -1038,7 +1038,7 @@ 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)
user, err := findUserFromToken(users, userToken)
if err != nil {
log.Trace().Caller().Err(err).Msg("could not determine user to filter nodes by")
return out
Expand All @@ -1058,24 +1058,19 @@ var (
ErrorMultipleUserMatching = errors.New("multiple users matching")
)

func findUserFromTokenOrErr(
users []types.User,
token string,
) (types.User, error) {
// findUserFromToken finds and returns a user based on the given token, prioritizing matches by ProviderIdentifier, followed by email or name.
// If no matching user is found, it returns an error of type ErrorNoUserMatching.
// If multiple users match the token, it returns an error indicating multiple matches.
func findUserFromToken(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
// Prioritize ProviderIdentifier match and exit early
return user, nil
}
if user.Email == token {
potentialUsers = append(potentialUsers, user)
}
if user.Name == token {

if user.Email == token || user.Name == token {
potentialUsers = append(potentialUsers, user)
}
}
Expand Down
312 changes: 312 additions & 0 deletions hscontrol/policy/acls_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4046,3 +4046,315 @@ func TestValidTagInvalidUser(t *testing.T) {
t.Errorf("TestValidTagInvalidUser() unexpected result (-want +got):\n%s", diff)
}
}

func TestFindUserByToken(t *testing.T) {
tests := []struct {
name string
users []types.User
token string
want types.User
wantErr bool
}{
{
name: "exact match by ProviderIdentifier",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: true, String: "token1"}},
{Email: "[email protected]"},
},
token: "token1",
want: types.User{ProviderIdentifier: sql.NullString{Valid: true, String: "token1"}},
wantErr: false,
},
{
name: "no matches found",
users: []types.User{
{Email: "[email protected]"},
{Name: "username"},
},
token: "nonexistent-token",
want: types.User{},
wantErr: true,
},
{
name: "multiple matches by email and name",
users: []types.User{
{Email: "token2", Name: "notoken"},
{Name: "token2", Email: "[email protected]"},
},
token: "token2",
want: types.User{},
wantErr: true,
},
{
name: "match by email",
users: []types.User{
{Email: "[email protected]"},
{ProviderIdentifier: sql.NullString{Valid: true, String: "othertoken"}},
},
token: "[email protected]",
want: types.User{Email: "[email protected]"},
wantErr: false,
},
{
name: "match by name",
users: []types.User{
{Name: "token4"},
{Email: "[email protected]"},
},
token: "token4",
want: types.User{Name: "token4"},
wantErr: false,
},
{
name: "provider identifier takes precedence over email and name matches",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: true, String: "token5"}},
{Email: "[email protected]", Name: "token5"},
},
token: "token5",
want: types.User{ProviderIdentifier: sql.NullString{Valid: true, String: "token5"}},
wantErr: false,
},
{
name: "empty token finds no users",
users: []types.User{
{Email: "[email protected]"},
{Name: "username6"},
},
token: "",
want: types.User{},
wantErr: true,
},
// Test case 1: Duplicate Emails with Unique ProviderIdentifiers
{
name: "duplicate emails with unique provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid1"}, Email: "[email protected]"},
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid2"}, Email: "[email protected]"},
},
token: "[email protected]",
want: types.User{},
wantErr: true,
},

// Test case 2: Duplicate Names with Unique ProviderIdentifiers
{
name: "duplicate names with unique provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid3"}, Name: "John Doe"},
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid4"}, Name: "John Doe"},
},
token: "John Doe",
want: types.User{},
wantErr: true,
},

// Test case 3: Duplicate Emails and Names with Unique ProviderIdentifiers
{
name: "duplicate emails and names with unique provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid5"}, Email: "[email protected]", Name: "John Doe"},
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid6"}, Email: "[email protected]", Name: "John Doe"},
},
token: "[email protected]",
want: types.User{},
wantErr: true,
},

// Test case 4: Unique Names without ProviderIdentifiers
{
name: "unique names without provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "Jane Smith", Email: "[email protected]"},
},
token: "John Doe",
want: types.User{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
wantErr: false,
},

// Test case 5: Duplicate Emails without ProviderIdentifiers but Unique Names
{
name: "duplicate emails without provider identifiers but unique names",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "Jane Smith", Email: "[email protected]"},
},
token: "John Doe",
want: types.User{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
wantErr: false,
},

// Test case 6: Duplicate Names and Emails without ProviderIdentifiers
{
name: "duplicate names and emails without provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
},
token: "John Doe",
want: types.User{},
wantErr: true,
},

// Test case 7: Multiple Users with the Same Email but Different Names and Unique ProviderIdentifiers
{
name: "multiple users with same email, different names, unique provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid7"}, Email: "[email protected]", Name: "John Doe"},
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid8"}, Email: "[email protected]", Name: "Jane Smith"},
},
token: "[email protected]",
want: types.User{},
wantErr: true,
},

// Test case 8: Multiple Users with the Same Name but Different Emails and Unique ProviderIdentifiers
{
name: "multiple users with same name, different emails, unique provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid9"}, Email: "[email protected]", Name: "John Doe"},
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid10"}, Email: "[email protected]", Name: "John Doe"},
},
token: "John Doe",
want: types.User{},
wantErr: true,
},

// Test case 9: Multiple Users with Same Email and Name but Unique ProviderIdentifiers
{
name: "multiple users with same email and name, unique provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid11"}, Email: "[email protected]", Name: "John Doe"},
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid12"}, Email: "[email protected]", Name: "John Doe"},
},
token: "[email protected]",
want: types.User{},
wantErr: true,
},

// Test case 10: Multiple Users without ProviderIdentifiers but with Unique Names and Emails
{
name: "multiple users without provider identifiers, unique names and emails",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "Jane Smith", Email: "[email protected]"},
},
token: "John Doe",
want: types.User{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
wantErr: false,
},

// Test case 11: Multiple Users without ProviderIdentifiers and Duplicate Emails but Unique Names
{
name: "multiple users without provider identifiers, duplicate emails but unique names",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "Jane Smith", Email: "[email protected]"},
},
token: "John Doe",
want: types.User{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
wantErr: false,
},

// Test case 12: Multiple Users without ProviderIdentifiers and Duplicate Names but Unique Emails
{
name: "multiple users without provider identifiers, duplicate names but unique emails",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
},
token: "John Doe",
want: types.User{},
wantErr: true,
},

// Test case 13: Multiple Users without ProviderIdentifiers and Duplicate Both Names and Emails
{
name: "multiple users without provider identifiers, duplicate names and emails",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
},
token: "John Doe",
want: types.User{},
wantErr: true,
},

// Test case 14: Multiple Users with Same Email Without ProviderIdentifiers
{
name: "multiple users with same email without provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "Jane Smith", Email: "[email protected]"},
},
token: "[email protected]",
want: types.User{},
wantErr: true,
},

// Test case 15: Multiple Users with Same Name Without ProviderIdentifiers
{
name: "multiple users with same name without provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "[email protected]"},
},
token: "John Doe",
want: types.User{},
wantErr: true,
},
{
name: "Name field used as email address match",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid3"}, Name: "[email protected]", Email: "[email protected]"},
},
token: "[email protected]",
want: types.User{ProviderIdentifier: sql.NullString{Valid: true, String: "pid3"}, Name: "[email protected]", Email: "[email protected]"},
wantErr: false,
},
{
name: "multiple users with same name as email and unique provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid4"}, Name: "[email protected]", Email: "[email protected]"},
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid5"}, Name: "[email protected]", Email: "[email protected]"},
},
token: "[email protected]",
want: types.User{},
wantErr: true,
},
{
name: "no provider identifier and duplicate names as emails",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "[email protected]", Email: "[email protected]"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "[email protected]", Email: "[email protected]"},
},
token: "[email protected]",
want: types.User{},
wantErr: true,
},
{
name: "name as email with multiple matches when provider identifier is not set",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "[email protected]", Email: "[email protected]"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "[email protected]", Email: "[email protected]"},
},
token: "[email protected]",
want: types.User{},
wantErr: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
gotUser, err := findUserFromToken(tt.users, tt.token)
if (err != nil) != tt.wantErr {
t.Errorf("findUserFromToken() error = %v, wantErr %v", err, tt.wantErr)
return
}
if diff := cmp.Diff(tt.want, gotUser, util.Comparers...); diff != "" {
t.Errorf("findUserFromToken() unexpected result (-want +got):\n%s", diff)
}
})
}
}
Loading
Loading