fix: normalize usernames from idp to match our validation

coder · johnstcn · Jun 6, 2024 · Jun 4, 2024 · Jun 4, 2024 · Jun 5, 2024
commit 66c98cd55c2c47d807357079112a89b906f4f888
diff --git 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
@@ -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
@@ -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
@@ -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,