Skip to content

Commit

Permalink
Move username comparison to provider
Browse files Browse the repository at this point in the history
The decision whether two usernames are considered equal is
provider-specific. For example, some providers have case-insensitive
usernames.
  • Loading branch information
adombeck committed Sep 4, 2024
1 parent 3574ac4 commit cdc34cb
Show file tree
Hide file tree
Showing 7 changed files with 73 additions and 4 deletions.
6 changes: 3 additions & 3 deletions internal/broker/broker.go
Original file line number Diff line number Diff line change
Expand Up @@ -710,9 +710,9 @@ func (b *Broker) fetchUserInfo(ctx context.Context, session *sessionInfo, t *aut
return info.User{}, fmt.Errorf("could not get user info: %v", err)
}

// Some providers are case-insensitive, but we are not. So we need to lowercase the returned username.
if !strings.EqualFold(userInfo.Name, session.username) {
return info.User{}, fmt.Errorf("returned user %q does not match the selected one %q", userInfo.Name, session.username)
err = b.providerInfo.VerifyUser(session.username, userInfo)
if err != nil {
return info.User{}, fmt.Errorf("could not verify user: %v", err)
}

// This means that home was not provided by the claims, so we need to set it to the broker default.
Expand Down
1 change: 0 additions & 1 deletion internal/broker/broker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -736,7 +736,6 @@ func TestFetchUserInfo(t *testing.T) {
"Successfully fetch user info with groups": {},
"Successfully fetch user info without groups": {emptyGroups: true},
"Successfully fetch user info with default home when not provided": {emptyHomeDir: true},
"Successfully fetch user info ignoring different casing in name": {userToken: "uppercased-name"},

"Error when token can not be validated": {userToken: "invalid", wantErr: true},
"Error when ID token claims are invalid": {userToken: "invalid-id", wantErr: true},
Expand Down
20 changes: 20 additions & 0 deletions internal/providers/msentraid/msentraid.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,14 @@ func (p Provider) CurrentAuthenticationModesOffered(
return offeredModes, nil
}

func (p Provider) VerifyUser(requestedUsername string, user info.User) error {

Check failure on line 247 in internal/providers/msentraid/msentraid.go

View workflow job for this annotation

GitHub Actions / Go: Code sanity

exported: exported method Provider.VerifyUser should have comment or be unexported (revive)
// Microsoft Entra usernames are case-insensitive.
if !strings.EqualFold(requestedUsername, user.Name) {
return fmt.Errorf("requested username %q does not match the authenticated user %q", requestedUsername, user.Name)
}
return nil
}

type azureTokenCredential struct {
token *oauth2.Token
}
Expand All @@ -255,3 +263,15 @@ func (c azureTokenCredential) GetToken(_ context.Context, _ policy.TokenRequestO
ExpiresOn: c.token.Expiry,
}, nil
}

func lowercaseASCII(input string) string {

Check failure on line 267 in internal/providers/msentraid/msentraid.go

View workflow job for this annotation

GitHub Actions / Go: Code sanity

func `lowercaseASCII` is unused (unused)
lower := make([]byte, len(input))
for i := 0; i < len(input); i++ {
c := input[i]
if 'A' <= c && c <= 'Z' {
c += 32
}
lower[i] = c
}
return string(lower)
}
35 changes: 35 additions & 0 deletions internal/providers/msentraid/msentraid_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"testing"

"github.com/stretchr/testify/require"
"github.com/ubuntu/authd-oidc-brokers/internal/providers/info"
"github.com/ubuntu/authd-oidc-brokers/internal/providers/msentraid"
"golang.org/x/oauth2"
)
Expand Down Expand Up @@ -51,3 +52,37 @@ func TestCheckTokenScopes(t *testing.T) {
})
}
}

func TestVerifyUser(t *testing.T) {
t.Parallel()

tests := map[string]struct {
requestedUsername string
authenticatedUser string

wantErr bool
}{
"success when usernames are the same": {requestedUsername: "foo@bar", authenticatedUser: "foo@bar"},
"success when usernames differ in case": {requestedUsername: "foo@bar", authenticatedUser: "Foo@bar"},

"error when usernames differ": {requestedUsername: "foo@bar", authenticatedUser: "bar@foo", wantErr: true},
"error when usernames differ only in letters which map to the same lowercase letter": {
requestedUsername: "I@bar", authenticatedUser: "İ@bar", wantErr: true,
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
t.Parallel()

p := msentraid.New()

err := p.VerifyUser(tc.requestedUsername, info.User{Name: tc.authenticatedUser})
if tc.wantErr {
require.Error(t, err, "VerifyUser should return an error")
return
}

require.NoError(t, err, "VerifyUser should not return an error")
})
}
}
7 changes: 7 additions & 0 deletions internal/providers/noprovider/noprovider.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,13 @@ func (p NoProvider) GetUserInfo(ctx context.Context, accessToken *oauth2.Token,
), nil
}

func (p NoProvider) VerifyUser(requestedUsername string, user info.User) error {

Check failure on line 101 in internal/providers/noprovider/noprovider.go

View workflow job for this annotation

GitHub Actions / Go: Code sanity

exported: exported method NoProvider.VerifyUser should have comment or be unexported (revive)
if requestedUsername != user.Name {
return fmt.Errorf("requested username %q does not match the authenticated user %q", requestedUsername, user.Name)
}
return nil
}

type claims struct {
PreferredUserName string `json:"preferred_username"`
Sub string `json:"sub"`
Expand Down
1 change: 1 addition & 0 deletions internal/providers/providers.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,5 @@ type ProviderInfoer interface {
currentAuthStep int,
) ([]string, error)
GetUserInfo(ctx context.Context, accessToken *oauth2.Token, idToken *oidc.IDToken) (info.User, error)
VerifyUser(requestedUsername string, user info.User) error
}
7 changes: 7 additions & 0 deletions internal/testutils/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -363,3 +363,10 @@ func (p MockProviderInfoer) CurrentAuthenticationModesOffered(

return offeredModes, nil
}

func (p *MockProviderInfoer) VerifyUser(requestedUsername string, user info.User) error {

Check failure on line 367 in internal/testutils/provider.go

View workflow job for this annotation

GitHub Actions / Go: Code sanity

exported: exported method MockProviderInfoer.VerifyUser should have comment or be unexported (revive)
if requestedUsername != user.Name {
return fmt.Errorf("requested username %q does not match the authenticated user %q", requestedUsername, user.Name)
}
return nil
}

0 comments on commit cdc34cb

Please sign in to comment.