feat(coderd): set full name from IDP name claim (#13468)

* Updates OIDC and GitHub OAuth login to fetch set name from relevant claim fields
* Adds CODER_OIDC_NAME_FIELD as configurable source of user name claim
* Adds httpapi function to normalize a username such that it will pass validation
* Adds firstName / lastName fields to dev OIDC setup
This commit is contained in:
Cian Johnston
2024-06-06 13:37:08 +01:00
committed by GitHub
parent e743588843
commit 1131772e79
16 changed files with 301 additions and 42 deletions

3
coderd/apidoc/docs.go generated
View File

@ -10551,6 +10551,9 @@ const docTemplate = `{
"issuer_url": {
"type": "string"
},
"name_field": {
"type": "string"
},
"scopes": {
"type": "array",
"items": {

View File

@ -9482,6 +9482,9 @@
"issuer_url": {
"type": "string"
},
"name_field": {
"type": "string"
},
"scopes": {
"type": "array",
"items": {

View File

@ -91,3 +91,14 @@ func UserRealNameValid(str string) error {
}
return nil
}
// NormalizeUserRealName normalizes a user name such that it will pass
// validation by UserRealNameValid. This is done to avoid blocking
// little Bobby Whitespace from using Coder.
func NormalizeRealUsername(str string) string {
s := strings.TrimSpace(str)
if len(s) > 128 {
s = s[:128]
}
return s
}

View File

@ -1,9 +1,11 @@
package httpapi_test
import (
"strings"
"testing"
"github.com/moby/moby/pkg/namesgenerator"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/coder/coder/v2/coderd/httpapi"
@ -217,6 +219,10 @@ func TestUserRealNameValid(t *testing.T) {
Name string
Valid bool
}{
{"", true},
{" a", false},
{"a ", false},
{" a ", false},
{"1", true},
{"A", true},
{"A1", true},
@ -229,17 +235,22 @@ func TestUserRealNameValid(t *testing.T) {
{"Małgorzata Kalinowska-Iszkowska", true},
{"成龍", true},
{". .", true},
{"Lord Voldemort ", false},
{" Bellatrix Lestrange", false},
{" ", false},
{strings.Repeat("a", 128), true},
{strings.Repeat("a", 129), false},
}
for _, testCase := range testCases {
testCase := testCase
t.Run(testCase.Name, func(t *testing.T) {
t.Parallel()
valid := httpapi.UserRealNameValid(testCase.Name)
require.Equal(t, testCase.Valid, valid == nil)
err := httpapi.UserRealNameValid(testCase.Name)
norm := httpapi.NormalizeRealUsername(testCase.Name)
normErr := httpapi.UserRealNameValid(norm)
assert.NoError(t, normErr)
assert.Equal(t, testCase.Valid, err == nil)
assert.Equal(t, testCase.Valid, norm == testCase.Name, "invalid name should be different after normalization")
})
}
}

View File

@ -607,6 +607,9 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
return
}
ghName := ghUser.GetName()
normName := httpapi.NormalizeRealUsername(ghName)
// If we have a nil GitHub ID, that is a big problem. That would mean we link
// this user and all other users with this bug to the same uuid.
// We should instead throw an error. This should never occur in production.
@ -652,6 +655,7 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
Email: verifiedEmail.GetEmail(),
Username: ghUser.GetLogin(),
AvatarURL: ghUser.GetAvatarURL(),
Name: normName,
DebugContext: OauthDebugContext{},
}).SetInitAuditRequest(func(params *audit.RequestParams) (*audit.Request[database.User], func()) {
return audit.InitRequest[database.User](rw, params)
@ -701,6 +705,9 @@ type OIDCConfig struct {
// EmailField selects the claim field to be used as the created user's
// email.
EmailField string
// NameField selects the claim field to be used as the created user's
// full / given name.
NameField string
// AuthURLParams are additional parameters to be passed to the OIDC provider
// when requesting an access token.
AuthURLParams map[string]string
@ -952,13 +959,22 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
}
}
// The 'name' is an optional property in Coder. If not specified,
// it will be left blank.
var name string
nameRaw, ok := mergedClaims[api.OIDCConfig.NameField]
if ok {
name, _ = nameRaw.(string)
name = httpapi.NormalizeRealUsername(name)
}
var picture string
pictureRaw, ok := mergedClaims["picture"]
if ok {
picture, _ = pictureRaw.(string)
}
ctx = slog.With(ctx, slog.F("email", email), slog.F("username", username))
ctx = slog.With(ctx, slog.F("email", email), slog.F("username", username), slog.F("name", name))
usingGroups, groups, groupErr := api.oidcGroups(ctx, mergedClaims)
if groupErr != nil {
groupErr.Write(rw, r)
@ -996,6 +1012,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
AllowSignups: api.OIDCConfig.AllowSignups,
Email: email,
Username: username,
Name: name,
AvatarURL: picture,
UsingRoles: api.OIDCConfig.RoleSyncEnabled(),
Roles: roles,
@ -1222,6 +1239,7 @@ type oauthLoginParams struct {
AllowSignups bool
Email string
Username string
Name string
AvatarURL string
// Is UsingGroups is true, then the user will be assigned
// to the Groups provided.
@ -1544,6 +1562,10 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
user.AvatarURL = params.AvatarURL
needsUpdate = true
}
if user.Name != params.Name {
user.Name = params.Name
needsUpdate = true
}
// If the upstream email or username has changed we should mirror
// that in Coder. Many enterprises use a user's email/username as

View File

@ -16,6 +16,7 @@ import (
"github.com/google/go-github/v43/github"
"github.com/google/uuid"
"github.com/prometheus/client_golang/prometheus"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"golang.org/x/xerrors"
@ -213,6 +214,7 @@ func TestUserOAuth2Github(t *testing.T) {
return &github.User{
ID: github.Int64(100),
Login: github.String("kyle"),
Name: github.String("Kylium Carbonate"),
}, nil
},
TeamMembership: func(ctx context.Context, client *http.Client, org, team, username string) (*github.Membership, error) {
@ -272,7 +274,9 @@ func TestUserOAuth2Github(t *testing.T) {
},
AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
return &github.User{
ID: github.Int64(100),
ID: github.Int64(100),
Login: github.String("testuser"),
Name: github.String("The Right Honorable Sir Test McUser"),
}, nil
},
ListEmails: func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) {
@ -305,7 +309,9 @@ func TestUserOAuth2Github(t *testing.T) {
},
AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
return &github.User{
ID: github.Int64(100),
ID: github.Int64(100),
Login: github.String("testuser"),
Name: github.String("The Right Honorable Sir Test McUser"),
}, nil
},
ListEmails: func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) {
@ -346,9 +352,10 @@ func TestUserOAuth2Github(t *testing.T) {
},
AuthenticatedUser: func(ctx context.Context, _ *http.Client) (*github.User, error) {
return &github.User{
Login: github.String("kyle"),
ID: i64ptr(1234),
AvatarURL: github.String("/hello-world"),
ID: i64ptr(1234),
Login: github.String("kyle"),
Name: github.String("Kylium Carbonate"),
}, nil
},
ListEmails: func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) {
@ -372,6 +379,60 @@ func TestUserOAuth2Github(t *testing.T) {
require.NoError(t, err)
require.Equal(t, "kyle@coder.com", user.Email)
require.Equal(t, "kyle", user.Username)
require.Equal(t, "Kylium Carbonate", user.Name)
require.Equal(t, "/hello-world", user.AvatarURL)
require.Len(t, auditor.AuditLogs(), numLogs)
require.NotEqual(t, auditor.AuditLogs()[numLogs-1].UserID, uuid.Nil)
require.Equal(t, database.AuditActionRegister, auditor.AuditLogs()[numLogs-1].Action)
})
t.Run("SignupWeirdName", func(t *testing.T) {
t.Parallel()
auditor := audit.NewMock()
client := coderdtest.New(t, &coderdtest.Options{
Auditor: auditor,
GithubOAuth2Config: &coderd.GithubOAuth2Config{
OAuth2Config: &testutil.OAuth2Config{},
AllowOrganizations: []string{"coder"},
AllowSignups: true,
ListOrganizationMemberships: func(_ context.Context, _ *http.Client) ([]*github.Membership, error) {
return []*github.Membership{{
State: &stateActive,
Organization: &github.Organization{
Login: github.String("coder"),
},
}}, nil
},
AuthenticatedUser: func(_ context.Context, _ *http.Client) (*github.User, error) {
return &github.User{
AvatarURL: github.String("/hello-world"),
ID: i64ptr(1234),
Login: github.String("kyle"),
Name: github.String(" " + strings.Repeat("a", 129) + " "),
}, nil
},
ListEmails: func(_ context.Context, _ *http.Client) ([]*github.UserEmail, error) {
return []*github.UserEmail{{
Email: github.String("kyle@coder.com"),
Verified: github.Bool(true),
Primary: github.Bool(true),
}}, nil
},
},
})
numLogs := len(auditor.AuditLogs())
resp := oauth2Callback(t, client)
numLogs++ // add an audit log for login
require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
client.SetSessionToken(authCookieValue(resp.Cookies()))
user, err := client.User(context.Background(), "me")
require.NoError(t, err)
require.Equal(t, "kyle@coder.com", user.Email)
require.Equal(t, "kyle", user.Username)
require.Equal(t, strings.Repeat("a", 128), user.Name)
require.Equal(t, "/hello-world", user.AvatarURL)
require.Len(t, auditor.AuditLogs(), numLogs)
@ -401,8 +462,10 @@ func TestUserOAuth2Github(t *testing.T) {
},
AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
return &github.User{
ID: github.Int64(100),
Login: github.String("kyle"),
AvatarURL: github.String("/hello-world"),
ID: github.Int64(100),
Login: github.String("kyle"),
Name: github.String("Kylium Carbonate"),
}, nil
},
ListEmails: func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) {
@ -419,10 +482,19 @@ func TestUserOAuth2Github(t *testing.T) {
resp := oauth2Callback(t, client)
numLogs++ // add an audit log for login
client.SetSessionToken(authCookieValue(resp.Cookies()))
user, err := client.User(context.Background(), "me")
require.NoError(t, err)
require.Equal(t, "kyle@coder.com", user.Email)
require.Equal(t, "kyle", user.Username)
require.Equal(t, "Kylium Carbonate", user.Name)
require.Equal(t, "/hello-world", user.AvatarURL)
require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
require.Len(t, auditor.AuditLogs(), numLogs)
require.Equal(t, database.AuditActionRegister, auditor.AuditLogs()[numLogs-1].Action)
})
// nolint: dupl
t.Run("SignupAllowedTeamInFirstOrganization", func(t *testing.T) {
t.Parallel()
auditor := audit.NewMock()
@ -456,6 +528,7 @@ func TestUserOAuth2Github(t *testing.T) {
return &github.User{
ID: github.Int64(100),
Login: github.String("mathias"),
Name: github.String("Mathias Mathias"),
}, nil
},
ListEmails: func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) {
@ -472,10 +545,18 @@ func TestUserOAuth2Github(t *testing.T) {
resp := oauth2Callback(t, client)
numLogs++ // add an audit log for login
client.SetSessionToken(authCookieValue(resp.Cookies()))
user, err := client.User(context.Background(), "me")
require.NoError(t, err)
require.Equal(t, "mathias@coder.com", user.Email)
require.Equal(t, "mathias", user.Username)
require.Equal(t, "Mathias Mathias", user.Name)
require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
require.Len(t, auditor.AuditLogs(), numLogs)
require.Equal(t, database.AuditActionRegister, auditor.AuditLogs()[numLogs-1].Action)
})
// nolint: dupl
t.Run("SignupAllowedTeamInSecondOrganization", func(t *testing.T) {
t.Parallel()
auditor := audit.NewMock()
@ -509,6 +590,7 @@ func TestUserOAuth2Github(t *testing.T) {
return &github.User{
ID: github.Int64(100),
Login: github.String("mathias"),
Name: github.String("Mathias Mathias"),
}, nil
},
ListEmails: func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) {
@ -525,6 +607,13 @@ func TestUserOAuth2Github(t *testing.T) {
resp := oauth2Callback(t, client)
numLogs++ // add an audit log for login
client.SetSessionToken(authCookieValue(resp.Cookies()))
user, err := client.User(context.Background(), "me")
require.NoError(t, err)
require.Equal(t, "mathias@coder.com", user.Email)
require.Equal(t, "mathias", user.Username)
require.Equal(t, "Mathias Mathias", user.Name)
require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
require.Len(t, auditor.AuditLogs(), numLogs)
require.Equal(t, database.AuditActionRegister, auditor.AuditLogs()[numLogs-1].Action)
@ -548,6 +637,7 @@ func TestUserOAuth2Github(t *testing.T) {
return &github.User{
ID: github.Int64(100),
Login: github.String("mathias"),
Name: github.String("Mathias Mathias"),
}, nil
},
ListEmails: func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) {
@ -564,6 +654,13 @@ func TestUserOAuth2Github(t *testing.T) {
resp := oauth2Callback(t, client)
numLogs++ // add an audit log for login
client.SetSessionToken(authCookieValue(resp.Cookies()))
user, err := client.User(context.Background(), "me")
require.NoError(t, err)
require.Equal(t, "mathias@coder.com", user.Email)
require.Equal(t, "mathias", user.Username)
require.Equal(t, "Mathias Mathias", user.Name)
require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
require.Len(t, auditor.AuditLogs(), numLogs)
require.Equal(t, database.AuditActionRegister, auditor.AuditLogs()[numLogs-1].Action)
@ -591,6 +688,7 @@ func TestUserOAuth2Github(t *testing.T) {
return &github.User{
ID: github.Int64(100),
Login: github.String("kyle"),
Name: github.String("Kylium Carbonate"),
}, nil
},
ListEmails: func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) {
@ -652,6 +750,7 @@ func TestUserOAuth2Github(t *testing.T) {
return &github.User{
Login: github.String("alice"),
ID: github.Int64(ghID),
Name: github.String("Alice Liddell"),
}, nil
},
ListEmails: func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) {
@ -739,8 +838,7 @@ func TestUserOIDC(t *testing.T) {
UserInfoClaims jwt.MapClaims
AllowSignups bool
EmailDomain []string
Username string
AvatarURL string
AssertUser func(t testing.TB, u codersdk.User)
StatusCode int
IgnoreEmailVerified bool
IgnoreUserInfo bool
@ -752,7 +850,9 @@ func TestUserOIDC(t *testing.T) {
},
AllowSignups: true,
StatusCode: http.StatusOK,
Username: "kyle",
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "kyle", u.Username)
},
},
{
Name: "EmailNotVerified",
@ -778,9 +878,11 @@ func TestUserOIDC(t *testing.T) {
"email": "kyle@kwc.io",
"email_verified": false,
},
AllowSignups: true,
StatusCode: http.StatusOK,
Username: "kyle",
AllowSignups: true,
StatusCode: http.StatusOK,
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, u.Username, "kyle")
},
IgnoreEmailVerified: true,
},
{
@ -802,6 +904,9 @@ func TestUserOIDC(t *testing.T) {
"email_verified": true,
},
AllowSignups: true,
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, u.Username, "kyle")
},
EmailDomain: []string{
"kwc.io",
},
@ -839,7 +944,9 @@ func TestUserOIDC(t *testing.T) {
"email": "kyle@kwc.io",
"email_verified": true,
},
Username: "kyle",
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "kyle", u.Username)
},
AllowSignups: true,
StatusCode: http.StatusOK,
},
@ -850,10 +957,56 @@ func TestUserOIDC(t *testing.T) {
"email_verified": true,
"preferred_username": "hotdog",
},
Username: "hotdog",
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "hotdog", u.Username)
},
AllowSignups: true,
StatusCode: http.StatusOK,
},
{
Name: "FullNameFromClaims",
IDTokenClaims: jwt.MapClaims{
"email": "kyle@kwc.io",
"email_verified": true,
"name": "Hot Dog",
},
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "Hot Dog", u.Name)
},
AllowSignups: true,
StatusCode: http.StatusOK,
},
{
Name: "InvalidFullNameFromClaims",
IDTokenClaims: jwt.MapClaims{
"email": "kyle@kwc.io",
"email_verified": true,
// Full names must be less or equal to than 128 characters in length.
// However, we should not fail to log someone in if their name is too long.
// Just truncate it.
"name": strings.Repeat("a", 129),
},
AllowSignups: true,
StatusCode: http.StatusOK,
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, strings.Repeat("a", 128), u.Name)
},
},
{
Name: "FullNameWhitespace",
IDTokenClaims: jwt.MapClaims{
"email": "kyle@kwc.io",
"email_verified": true,
// Full names must not have leading or trailing whitespace, but this is a
// daft reason to fail a login.
"name": " Bobby Whitespace ",
},
AllowSignups: true,
StatusCode: http.StatusOK,
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "Bobby Whitespace", u.Name)
},
},
{
// Services like Okta return the email as the username:
// https://developer.okta.com/docs/reference/api/oidc/#base-claims-always-present
@ -861,9 +1014,12 @@ func TestUserOIDC(t *testing.T) {
IDTokenClaims: jwt.MapClaims{
"email": "kyle@kwc.io",
"email_verified": true,
"name": "Kylium Carbonate",
"preferred_username": "kyle@kwc.io",
},
Username: "kyle",
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "kyle", u.Username)
},
AllowSignups: true,
StatusCode: http.StatusOK,
},
@ -873,7 +1029,10 @@ func TestUserOIDC(t *testing.T) {
IDTokenClaims: jwt.MapClaims{
"preferred_username": "kyle@kwc.io",
},
Username: "kyle",
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "kyle", u.Username)
assert.Empty(t, u.Name)
},
AllowSignups: true,
StatusCode: http.StatusOK,
},
@ -885,9 +1044,11 @@ func TestUserOIDC(t *testing.T) {
"preferred_username": "kyle",
"picture": "/example.png",
},
Username: "kyle",
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "/example.png", u.AvatarURL)
assert.Equal(t, "kyle", u.Username)
},
AllowSignups: true,
AvatarURL: "/example.png",
StatusCode: http.StatusOK,
},
{
@ -899,10 +1060,14 @@ func TestUserOIDC(t *testing.T) {
UserInfoClaims: jwt.MapClaims{
"preferred_username": "potato",
"picture": "/example.png",
"name": "Kylium Carbonate",
},
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "/example.png", u.AvatarURL)
assert.Equal(t, "Kylium Carbonate", u.Name)
assert.Equal(t, "potato", u.Username)
},
Username: "potato",
AllowSignups: true,
AvatarURL: "/example.png",
StatusCode: http.StatusOK,
},
{
@ -925,7 +1090,9 @@ func TestUserOIDC(t *testing.T) {
"email_verified": true,
"preferred_username": "user",
},
Username: "user",
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "user", u.Username)
},
AllowSignups: true,
IgnoreEmailVerified: false,
StatusCode: http.StatusOK,
@ -948,13 +1115,18 @@ func TestUserOIDC(t *testing.T) {
IDTokenClaims: jwt.MapClaims{
"email": "user@internal.domain",
"email_verified": true,
"name": "User McName",
"preferred_username": "user",
},
UserInfoClaims: jwt.MapClaims{
"email": "user.mcname@external.domain",
"name": "Mr. User McName",
"preferred_username": "Mr. User McName",
},
Username: "user",
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "user", u.Username)
assert.Equal(t, "User McName", u.Name)
},
IgnoreUserInfo: true,
AllowSignups: true,
StatusCode: http.StatusOK,
@ -965,7 +1137,9 @@ func TestUserOIDC(t *testing.T) {
"email": "user@domain.tld",
"email_verified": true,
}, 65536),
Username: "user",
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "user", u.Username)
},
AllowSignups: true,
StatusCode: http.StatusOK,
},
@ -976,9 +1150,11 @@ func TestUserOIDC(t *testing.T) {
"email_verified": true,
},
UserInfoClaims: inflateClaims(t, jwt.MapClaims{}, 65536),
Username: "user",
AllowSignups: true,
StatusCode: http.StatusOK,
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "user", u.Username)
},
AllowSignups: true,
StatusCode: http.StatusOK,
},
} {
tc := tc
@ -996,6 +1172,7 @@ func TestUserOIDC(t *testing.T) {
cfg.EmailDomain = tc.EmailDomain
cfg.IgnoreEmailVerified = tc.IgnoreEmailVerified
cfg.IgnoreUserInfo = tc.IgnoreUserInfo
cfg.NameField = "name"
})
auditor := audit.NewMock()
@ -1013,22 +1190,13 @@ func TestUserOIDC(t *testing.T) {
ctx := testutil.Context(t, testutil.WaitLong)
if tc.Username != "" {
if tc.AssertUser != nil {
user, err := client.User(ctx, "me")
require.NoError(t, err)
require.Equal(t, tc.Username, user.Username)
require.Len(t, auditor.AuditLogs(), numLogs)
require.NotEqual(t, auditor.AuditLogs()[numLogs-1].UserID, uuid.Nil)
require.Equal(t, database.AuditActionRegister, auditor.AuditLogs()[numLogs-1].Action)
}
if tc.AvatarURL != "" {
user, err := client.User(ctx, "me")
require.NoError(t, err)
require.Equal(t, tc.AvatarURL, user.AvatarURL)
tc.AssertUser(t, user)
require.Len(t, auditor.AuditLogs(), numLogs)
require.NotEqual(t, uuid.Nil, auditor.AuditLogs()[numLogs-1].UserID)
require.Equal(t, database.AuditActionRegister, auditor.AuditLogs()[numLogs-1].Action)
}
})