From 84b3851f300daae5bcea9ee2df208d71b17af79d Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 4 Jun 2024 16:06:50 +0100 Subject: [PATCH 01/11] chore(coderd): refactor TestUserOIDC to allow more easily asserting things about the created user --- coderd/userauth_test.go | 79 ++++++++++++++++++++++++----------------- 1 file changed, 47 insertions(+), 32 deletions(-) diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index f1adbfe869610..e8d2cf5a7c407 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -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" @@ -739,8 +740,7 @@ func TestUserOIDC(t *testing.T) { UserInfoClaims jwt.MapClaims AllowSignups bool EmailDomain []string - Username string - AvatarURL string + AssertUser func(u codersdk.User) StatusCode int IgnoreEmailVerified bool IgnoreUserInfo bool @@ -752,7 +752,9 @@ func TestUserOIDC(t *testing.T) { }, AllowSignups: true, StatusCode: http.StatusOK, - Username: "kyle", + AssertUser: func(u codersdk.User) { + assert.Equal(t, "kyle", u.Username) + }, }, { Name: "EmailNotVerified", @@ -778,9 +780,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(u codersdk.User) { + assert.Equal(t, u.Username, "kyle") + }, IgnoreEmailVerified: true, }, { @@ -839,7 +843,9 @@ func TestUserOIDC(t *testing.T) { "email": "kyle@kwc.io", "email_verified": true, }, - Username: "kyle", + AssertUser: func(u codersdk.User) { + assert.Equal(t, "kyle", u.Username) + }, AllowSignups: true, StatusCode: http.StatusOK, }, @@ -850,7 +856,9 @@ func TestUserOIDC(t *testing.T) { "email_verified": true, "preferred_username": "hotdog", }, - Username: "hotdog", + AssertUser: func(u codersdk.User) { + assert.Equal(t, "hotdog", u.Username) + }, AllowSignups: true, StatusCode: http.StatusOK, }, @@ -863,7 +871,9 @@ func TestUserOIDC(t *testing.T) { "email_verified": true, "preferred_username": "kyle@kwc.io", }, - Username: "kyle", + AssertUser: func(u codersdk.User) { + assert.Equal(t, "kyle", u.Username) + }, AllowSignups: true, StatusCode: http.StatusOK, }, @@ -873,7 +883,9 @@ func TestUserOIDC(t *testing.T) { IDTokenClaims: jwt.MapClaims{ "preferred_username": "kyle@kwc.io", }, - Username: "kyle", + AssertUser: func(u codersdk.User) { + assert.Equal(t, "kyle", u.Username) + }, AllowSignups: true, StatusCode: http.StatusOK, }, @@ -885,9 +897,11 @@ func TestUserOIDC(t *testing.T) { "preferred_username": "kyle", "picture": "/example.png", }, - Username: "kyle", + AssertUser: func(u codersdk.User) { + assert.Equal(t, "/example.png", u.AvatarURL) + assert.Equal(t, "kyle", u.Username) + }, AllowSignups: true, - AvatarURL: "/example.png", StatusCode: http.StatusOK, }, { @@ -900,9 +914,11 @@ func TestUserOIDC(t *testing.T) { "preferred_username": "potato", "picture": "/example.png", }, - Username: "potato", + AssertUser: func(u codersdk.User) { + assert.Equal(t, "/example.png", u.AvatarURL) + assert.Equal(t, "potato", u.Username) + }, AllowSignups: true, - AvatarURL: "/example.png", StatusCode: http.StatusOK, }, { @@ -925,7 +941,9 @@ func TestUserOIDC(t *testing.T) { "email_verified": true, "preferred_username": "user", }, - Username: "user", + AssertUser: func(u codersdk.User) { + assert.Equal(t, "user", u.Username) + }, AllowSignups: true, IgnoreEmailVerified: false, StatusCode: http.StatusOK, @@ -954,7 +972,9 @@ func TestUserOIDC(t *testing.T) { "email": "user.mcname@external.domain", "preferred_username": "Mr. User McName", }, - Username: "user", + AssertUser: func(u codersdk.User) { + assert.Equal(t, "user", u.Username) + }, IgnoreUserInfo: true, AllowSignups: true, StatusCode: http.StatusOK, @@ -965,7 +985,9 @@ func TestUserOIDC(t *testing.T) { "email": "user@domain.tld", "email_verified": true, }, 65536), - Username: "user", + AssertUser: func(u codersdk.User) { + assert.Equal(t, "user", u.Username) + }, AllowSignups: true, StatusCode: http.StatusOK, }, @@ -976,9 +998,11 @@ func TestUserOIDC(t *testing.T) { "email_verified": true, }, UserInfoClaims: inflateClaims(t, jwt.MapClaims{}, 65536), - Username: "user", - AllowSignups: true, - StatusCode: http.StatusOK, + AssertUser: func(u codersdk.User) { + assert.Equal(t, "user", u.Username) + }, + AllowSignups: true, + StatusCode: http.StatusOK, }, } { tc := tc @@ -1013,22 +1037,13 @@ func TestUserOIDC(t *testing.T) { ctx := testutil.Context(t, testutil.WaitLong) - if tc.Username != "" { - 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 != "" { + if tc.AssertUser != nil { user, err := client.User(ctx, "me") require.NoError(t, err) - require.Equal(t, tc.AvatarURL, user.AvatarURL) + tc.AssertUser(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) } }) From 2ac8287adad3a23959ee95bb15efa686e22158a1 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 4 Jun 2024 17:29:11 +0100 Subject: [PATCH 02/11] chore(coderd): RED: update OAuth2/OIDC login tests to require full name from IDP --- coderd/userauth_test.go | 106 ++++++++++++++++++++++++++++++++-------- 1 file changed, 86 insertions(+), 20 deletions(-) diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index e8d2cf5a7c407..f634097fcd609 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -214,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) { @@ -273,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) { @@ -306,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) { @@ -347,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) { @@ -373,6 +379,7 @@ 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) @@ -402,8 +409,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) { @@ -420,6 +429,14 @@ 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) @@ -457,6 +474,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) { @@ -473,6 +491,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) @@ -510,6 +535,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) { @@ -526,6 +552,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) @@ -549,6 +582,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) { @@ -565,6 +599,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) @@ -592,6 +633,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) { @@ -653,6 +695,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) { @@ -740,7 +783,7 @@ func TestUserOIDC(t *testing.T) { UserInfoClaims jwt.MapClaims AllowSignups bool EmailDomain []string - AssertUser func(u codersdk.User) + AssertUser func(t testing.TB, u codersdk.User) StatusCode int IgnoreEmailVerified bool IgnoreUserInfo bool @@ -752,7 +795,7 @@ func TestUserOIDC(t *testing.T) { }, AllowSignups: true, StatusCode: http.StatusOK, - AssertUser: func(u codersdk.User) { + AssertUser: func(t testing.TB, u codersdk.User) { assert.Equal(t, "kyle", u.Username) }, }, @@ -782,7 +825,7 @@ func TestUserOIDC(t *testing.T) { }, AllowSignups: true, StatusCode: http.StatusOK, - AssertUser: func(u codersdk.User) { + AssertUser: func(t testing.TB, u codersdk.User) { assert.Equal(t, u.Username, "kyle") }, IgnoreEmailVerified: true, @@ -806,6 +849,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", }, @@ -843,7 +889,7 @@ func TestUserOIDC(t *testing.T) { "email": "kyle@kwc.io", "email_verified": true, }, - AssertUser: func(u codersdk.User) { + AssertUser: func(t testing.TB, u codersdk.User) { assert.Equal(t, "kyle", u.Username) }, AllowSignups: true, @@ -856,12 +902,25 @@ func TestUserOIDC(t *testing.T) { "email_verified": true, "preferred_username": "hotdog", }, - AssertUser: func(u codersdk.User) { + 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, + }, { // Services like Okta return the email as the username: // https://developer.okta.com/docs/reference/api/oidc/#base-claims-always-present @@ -869,9 +928,10 @@ func TestUserOIDC(t *testing.T) { IDTokenClaims: jwt.MapClaims{ "email": "kyle@kwc.io", "email_verified": true, + "name": "Kylium Carbonate", "preferred_username": "kyle@kwc.io", }, - AssertUser: func(u codersdk.User) { + AssertUser: func(t testing.TB, u codersdk.User) { assert.Equal(t, "kyle", u.Username) }, AllowSignups: true, @@ -883,8 +943,9 @@ func TestUserOIDC(t *testing.T) { IDTokenClaims: jwt.MapClaims{ "preferred_username": "kyle@kwc.io", }, - AssertUser: func(u codersdk.User) { + AssertUser: func(t testing.TB, u codersdk.User) { assert.Equal(t, "kyle", u.Username) + assert.Equal(t, "Kylium Carbonate", u.Name) }, AllowSignups: true, StatusCode: http.StatusOK, @@ -897,7 +958,7 @@ func TestUserOIDC(t *testing.T) { "preferred_username": "kyle", "picture": "/example.png", }, - AssertUser: func(u codersdk.User) { + AssertUser: func(t testing.TB, u codersdk.User) { assert.Equal(t, "/example.png", u.AvatarURL) assert.Equal(t, "kyle", u.Username) }, @@ -913,9 +974,11 @@ func TestUserOIDC(t *testing.T) { UserInfoClaims: jwt.MapClaims{ "preferred_username": "potato", "picture": "/example.png", + "name": "Kylium Carbonate", }, - AssertUser: func(u codersdk.User) { + 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) }, AllowSignups: true, @@ -941,7 +1004,7 @@ func TestUserOIDC(t *testing.T) { "email_verified": true, "preferred_username": "user", }, - AssertUser: func(u codersdk.User) { + AssertUser: func(t testing.TB, u codersdk.User) { assert.Equal(t, "user", u.Username) }, AllowSignups: true, @@ -966,14 +1029,17 @@ 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", }, - AssertUser: func(u codersdk.User) { + AssertUser: func(t testing.TB, u codersdk.User) { assert.Equal(t, "user", u.Username) + assert.Equal(t, "User Name", u.Name) }, IgnoreUserInfo: true, AllowSignups: true, @@ -985,7 +1051,7 @@ func TestUserOIDC(t *testing.T) { "email": "user@domain.tld", "email_verified": true, }, 65536), - AssertUser: func(u codersdk.User) { + AssertUser: func(t testing.TB, u codersdk.User) { assert.Equal(t, "user", u.Username) }, AllowSignups: true, @@ -998,7 +1064,7 @@ func TestUserOIDC(t *testing.T) { "email_verified": true, }, UserInfoClaims: inflateClaims(t, jwt.MapClaims{}, 65536), - AssertUser: func(u codersdk.User) { + AssertUser: func(t testing.TB, u codersdk.User) { assert.Equal(t, "user", u.Username) }, AllowSignups: true, @@ -1041,7 +1107,7 @@ func TestUserOIDC(t *testing.T) { user, err := client.User(ctx, "me") require.NoError(t, err) - tc.AssertUser(user) + 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) From 48b4415d248b1e013222fbc704d68fc18b25cb05 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 5 Jun 2024 14:50:59 +0100 Subject: [PATCH 03/11] feat(coderd): add CODER_OIDC_NAME_FIELD --- cli/server.go | 1 + coderd/userauth.go | 4 ++++ coderd/userauth_test.go | 3 ++- codersdk/deployment.go | 11 +++++++++++ 4 files changed, 18 insertions(+), 1 deletion(-) diff --git a/cli/server.go b/cli/server.go index 409056641a771..855c5e6c547bf 100644 --- a/cli/server.go +++ b/cli/server.go @@ -169,6 +169,7 @@ func createOIDCConfig(ctx context.Context, vals *codersdk.DeploymentValues) (*co EmailDomain: vals.OIDC.EmailDomain, AllowSignups: vals.OIDC.AllowSignups.Value(), UsernameField: vals.OIDC.UsernameField.String(), + NameField: vals.OIDC.NameField.String(), EmailField: vals.OIDC.EmailField.String(), AuthURLParams: vals.OIDC.AuthURLParams.Value, IgnoreUserInfo: vals.OIDC.IgnoreUserInfo.Value(), diff --git a/coderd/userauth.go b/coderd/userauth.go index 3f341db65bcb1..e56e0044d6b39 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -701,6 +701,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 @@ -1222,6 +1225,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. diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index f634097fcd609..853b90cd6363d 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -1039,7 +1039,7 @@ func TestUserOIDC(t *testing.T) { }, AssertUser: func(t testing.TB, u codersdk.User) { assert.Equal(t, "user", u.Username) - assert.Equal(t, "User Name", u.Name) + assert.Equal(t, "User McName", u.Name) }, IgnoreUserInfo: true, AllowSignups: true, @@ -1086,6 +1086,7 @@ func TestUserOIDC(t *testing.T) { cfg.EmailDomain = tc.EmailDomain cfg.IgnoreEmailVerified = tc.IgnoreEmailVerified cfg.IgnoreUserInfo = tc.IgnoreUserInfo + cfg.NameField = "name" }) auditor := audit.NewMock() diff --git a/codersdk/deployment.go b/codersdk/deployment.go index c89a78668637d..21d33ebc81dc0 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -333,6 +333,7 @@ type OIDCConfig struct { Scopes serpent.StringArray `json:"scopes" typescript:",notnull"` IgnoreEmailVerified serpent.Bool `json:"ignore_email_verified" typescript:",notnull"` UsernameField serpent.String `json:"username_field" typescript:",notnull"` + NameField serpent.String `json:"name_field" typescript:",notnull"` EmailField serpent.String `json:"email_field" typescript:",notnull"` AuthURLParams serpent.Struct[map[string]string] `json:"auth_url_params" typescript:",notnull"` IgnoreUserInfo serpent.Bool `json:"ignore_user_info" typescript:",notnull"` @@ -1192,6 +1193,16 @@ when required by your organization's security policy.`, Group: &deploymentGroupOIDC, YAML: "usernameField", }, + { + Name: "OIDC Name Field", + Description: "OIDC claim field to use as the name.", + Flag: "oidc-name-field", + Env: "CODER_OIDC_NAME_FIELD", + Default: "name", + Value: &c.OIDC.NameField, + Group: &deploymentGroupOIDC, + YAML: "nameField", + }, { Name: "OIDC Email Field", Description: "OIDC claim field to use as the email.", From c32a71c2fd38ef299b296ddb486ac83a28f58425 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 5 Jun 2024 14:53:40 +0100 Subject: [PATCH 04/11] feat(coderd): add full name from claims --- coderd/userauth.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index e56e0044d6b39..1b4046cb8c810 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -955,13 +955,21 @@ 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 fullName string + fullNameRaw, ok := mergedClaims[api.OIDCConfig.NameField] + if ok { + fullName, _ = fullNameRaw.(string) + } + 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", fullName)) usingGroups, groups, groupErr := api.oidcGroups(ctx, mergedClaims) if groupErr != nil { groupErr.Write(rw, r) @@ -999,6 +1007,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { AllowSignups: api.OIDCConfig.AllowSignups, Email: email, Username: username, + Name: fullName, AvatarURL: picture, UsingRoles: api.OIDCConfig.RoleSyncEnabled(), Roles: roles, @@ -1548,6 +1557,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 From 66c98cd55c2c47d807357079112a89b906f4f888 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 5 Jun 2024 14:56:03 +0100 Subject: [PATCH 05/11] fix: normalize usernames from idp to match our validation --- coderd/httpapi/name.go | 11 +++++ coderd/httpapi/name_test.go | 17 ++++++-- coderd/userauth.go | 1 + coderd/userauth_test.go | 86 ++++++++++++++++++++++++++++++++++++- 4 files changed, 111 insertions(+), 4 deletions(-) diff --git a/coderd/httpapi/name.go b/coderd/httpapi/name.go index d8b64a71bdc44..cdf4a9ad48971 100644 --- a/coderd/httpapi/name.go +++ b/coderd/httpapi/name.go @@ -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 +} diff --git a/coderd/httpapi/name_test.go b/coderd/httpapi/name_test.go index a6313c54034f5..15da7b3e82a63 100644 --- a/coderd/httpapi/name_test.go +++ b/coderd/httpapi/name_test.go @@ -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") }) } } diff --git a/coderd/userauth.go b/coderd/userauth.go index 1b4046cb8c810..226f4106cdafd 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -961,6 +961,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { fullNameRaw, ok := mergedClaims[api.OIDCConfig.NameField] if ok { fullName, _ = fullNameRaw.(string) + fullName = httpapi.NormalizeRealUsername(fullName) } var picture string diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 853b90cd6363d..7373180986a91 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -386,6 +386,59 @@ func TestUserOAuth2Github(t *testing.T) { 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) + require.NotEqual(t, auditor.AuditLogs()[numLogs-1].UserID, uuid.Nil) + require.Equal(t, database.AuditActionRegister, auditor.AuditLogs()[numLogs-1].Action) + }) t.Run("SignupAllowedTeam", func(t *testing.T) { t.Parallel() auditor := audit.NewMock() @@ -921,6 +974,37 @@ func TestUserOIDC(t *testing.T) { 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 @@ -945,7 +1029,7 @@ func TestUserOIDC(t *testing.T) { }, AssertUser: func(t testing.TB, u codersdk.User) { assert.Equal(t, "kyle", u.Username) - assert.Equal(t, "Kylium Carbonate", u.Name) + assert.Empty(t, u.Name) }, AllowSignups: true, StatusCode: http.StatusOK, From 8e6faa906b1381cc4d1e6e33e930ff83edc5a983 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 5 Jun 2024 17:14:26 +0100 Subject: [PATCH 06/11] make gen --- cli/testdata/coder_server_--help.golden | 3 +++ cli/testdata/server-config.yaml.golden | 3 +++ coderd/apidoc/docs.go | 3 +++ coderd/apidoc/swagger.json | 3 +++ docs/api/general.md | 1 + docs/api/schemas.md | 4 ++++ docs/cli/server.md | 11 +++++++++++ enterprise/cli/testdata/coder_server_--help.golden | 3 +++ site/src/api/typesGenerated.ts | 1 + 9 files changed, 32 insertions(+) diff --git a/cli/testdata/coder_server_--help.golden b/cli/testdata/coder_server_--help.golden index 6d8f866c11c0b..acd2c62ead445 100644 --- a/cli/testdata/coder_server_--help.golden +++ b/cli/testdata/coder_server_--help.golden @@ -407,6 +407,9 @@ OIDC OPTIONS: --oidc-issuer-url string, $CODER_OIDC_ISSUER_URL Issuer URL to use for Login with OIDC. + --oidc-name-field string, $CODER_OIDC_NAME_FIELD (default: name) + OIDC claim field to use as the name. + --oidc-group-regex-filter regexp, $CODER_OIDC_GROUP_REGEX_FILTER (default: .*) If provided any group name not matching the regex is ignored. This allows for filtering out groups that are not needed. This filter is diff --git a/cli/testdata/server-config.yaml.golden b/cli/testdata/server-config.yaml.golden index bf49239bc4e63..9a34d6be56b20 100644 --- a/cli/testdata/server-config.yaml.golden +++ b/cli/testdata/server-config.yaml.golden @@ -306,6 +306,9 @@ oidc: # OIDC claim field to use as the username. # (default: preferred_username, type: string) usernameField: preferred_username + # OIDC claim field to use as the name. + # (default: name, type: string) + nameField: name # OIDC claim field to use as the email. # (default: email, type: string) emailField: email diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index c5e2a6041526f..cca1e1d444407 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -10551,6 +10551,9 @@ const docTemplate = `{ "issuer_url": { "type": "string" }, + "name_field": { + "type": "string" + }, "scopes": { "type": "array", "items": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 66afad1f041f0..4f78e71b67c12 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -9482,6 +9482,9 @@ "issuer_url": { "type": "string" }, + "name_field": { + "type": "string" + }, "scopes": { "type": "array", "items": { diff --git a/docs/api/general.md b/docs/api/general.md index 52313409cb02c..84424331cf488 100644 --- a/docs/api/general.md +++ b/docs/api/general.md @@ -294,6 +294,7 @@ curl -X GET http://coder-server:8080/api/v2/deployment/config \ "ignore_email_verified": true, "ignore_user_info": true, "issuer_url": "string", + "name_field": "string", "scopes": ["string"], "sign_in_text": "string", "signups_disabled_text": "string", diff --git a/docs/api/schemas.md b/docs/api/schemas.md index 7770b091878bd..fffea09e86302 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -2094,6 +2094,7 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "ignore_email_verified": true, "ignore_user_info": true, "issuer_url": "string", + "name_field": "string", "scopes": ["string"], "sign_in_text": "string", "signups_disabled_text": "string", @@ -2467,6 +2468,7 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "ignore_email_verified": true, "ignore_user_info": true, "issuer_url": "string", + "name_field": "string", "scopes": ["string"], "sign_in_text": "string", "signups_disabled_text": "string", @@ -3546,6 +3548,7 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "ignore_email_verified": true, "ignore_user_info": true, "issuer_url": "string", + "name_field": "string", "scopes": ["string"], "sign_in_text": "string", "signups_disabled_text": "string", @@ -3577,6 +3580,7 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o | `ignore_email_verified` | boolean | false | | | | `ignore_user_info` | boolean | false | | | | `issuer_url` | string | false | | | +| `name_field` | string | false | | | | `scopes` | array of string | false | | | | `sign_in_text` | string | false | | | | `signups_disabled_text` | string | false | | | diff --git a/docs/cli/server.md b/docs/cli/server.md index a7c32c2d78420..ea3672a1cb2d7 100644 --- a/docs/cli/server.md +++ b/docs/cli/server.md @@ -514,6 +514,17 @@ Ignore the email_verified claim from the upstream provider. OIDC claim field to use as the username. +### --oidc-name-field + +| | | +| ----------- | ----------------------------------- | +| Type | string | +| Environment | $CODER_OIDC_NAME_FIELD | +| YAML | oidc.nameField | +| Default | name | + +OIDC claim field to use as the name. + ### --oidc-email-field | | | diff --git a/enterprise/cli/testdata/coder_server_--help.golden b/enterprise/cli/testdata/coder_server_--help.golden index 4d4576d6d57cc..2c094e84913f0 100644 --- a/enterprise/cli/testdata/coder_server_--help.golden +++ b/enterprise/cli/testdata/coder_server_--help.golden @@ -408,6 +408,9 @@ OIDC OPTIONS: --oidc-issuer-url string, $CODER_OIDC_ISSUER_URL Issuer URL to use for Login with OIDC. + --oidc-name-field string, $CODER_OIDC_NAME_FIELD (default: name) + OIDC claim field to use as the name. + --oidc-group-regex-filter regexp, $CODER_OIDC_GROUP_REGEX_FILTER (default: .*) If provided any group name not matching the regex is ignored. This allows for filtering out groups that are not needed. This filter is diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 88e5c7e508f67..9cc3113cbb214 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -756,6 +756,7 @@ export interface OIDCConfig { readonly scopes: string[]; readonly ignore_email_verified: boolean; readonly username_field: string; + readonly name_field: string; readonly email_field: string; readonly auth_url_params: Record; readonly ignore_user_info: boolean; From c00822efe01bea0134ddac470e235686d1141227 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 5 Jun 2024 20:14:52 +0100 Subject: [PATCH 07/11] also add name from gh oauth2 --- coderd/userauth.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/coderd/userauth.go b/coderd/userauth.go index 226f4106cdafd..07e03133fc62c 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -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) From 0d6d864b9553844c551bfbb90a15a7c9781de111 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 6 Jun 2024 11:28:33 +0100 Subject: [PATCH 08/11] nolint WET test code --- coderd/userauth_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 7373180986a91..1c647f3cca281 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -494,6 +494,7 @@ func TestUserOAuth2Github(t *testing.T) { 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() @@ -555,6 +556,7 @@ func TestUserOAuth2Github(t *testing.T) { 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() From 29895c3219295b77143d8d4e5b28c332bbf55556 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 6 Jun 2024 11:51:11 +0100 Subject: [PATCH 09/11] chore: update dev-oidc user to have first and last name --- scripts/dev-oidc.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/dev-oidc.sh b/scripts/dev-oidc.sh index 017c7f07c646d..dc253b08c32e2 100755 --- a/scripts/dev-oidc.sh +++ b/scripts/dev-oidc.sh @@ -23,6 +23,8 @@ cat </tmp/example-realm.json { "username": "oidcuser", "email": "oidcuser@coder.com", + "firstName": "OIDC", + "lastName": "user ", "emailVerified": true, "enabled": true, "credentials": [ From c180ca6a554e9a7e4e7895788bd44016a9798361 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 6 Jun 2024 11:51:46 +0100 Subject: [PATCH 10/11] fixup! chore: update dev-oidc user to have first and last name --- scripts/dev-oidc.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/dev-oidc.sh b/scripts/dev-oidc.sh index dc253b08c32e2..6a6d6e08ac705 100755 --- a/scripts/dev-oidc.sh +++ b/scripts/dev-oidc.sh @@ -10,6 +10,7 @@ set -euo pipefail KEYCLOAK_VERSION="${KEYCLOAK_VERSION:-22.0}" +# NOTE: the trailing space in "lastName" is intentional. cat </tmp/example-realm.json { "realm": "coder", From ee1167fc212919a3c87b320b68db72d6640013f7 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 6 Jun 2024 12:46:53 +0100 Subject: [PATCH 11/11] nit: s/fullName/name/g --- coderd/userauth.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 07e03133fc62c..b41f496814306 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -961,11 +961,11 @@ 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 fullName string - fullNameRaw, ok := mergedClaims[api.OIDCConfig.NameField] + var name string + nameRaw, ok := mergedClaims[api.OIDCConfig.NameField] if ok { - fullName, _ = fullNameRaw.(string) - fullName = httpapi.NormalizeRealUsername(fullName) + name, _ = nameRaw.(string) + name = httpapi.NormalizeRealUsername(name) } var picture string @@ -974,7 +974,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { picture, _ = pictureRaw.(string) } - ctx = slog.With(ctx, slog.F("email", email), slog.F("username", username), slog.F("name", fullName)) + 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) @@ -1012,7 +1012,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { AllowSignups: api.OIDCConfig.AllowSignups, Email: email, Username: username, - Name: fullName, + Name: name, AvatarURL: picture, UsingRoles: api.OIDCConfig.RoleSyncEnabled(), Roles: roles,