fix(scim): ensure usernames are properly validated

coadler · coadler · commit f30ec21ee27f · 2023-06-08T20:35:01.000Z
diff --git a/coderd/userauth.go b/coderd/userauth.go
@@ -681,20 +681,6 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
 		api.Logger.Debug(ctx, "'groups' claim was returned, but 'oidc-group-field' is not set, check your coder oidc settings.")
 	}
 
-	// The username is a required property in Coder. We make a best-effort
-	// attempt at using what the claims provide, but if that fails we will
-	// generate a random username.
-	usernameValid := httpapi.NameValid(username)
-	if usernameValid != nil {
-		// If no username is provided, we can default to use the email address.
-		// This will be converted in the from function below, so it's safe
-		// to keep the domain.
-		if username == "" {
-			username = email
-		}
-		username = httpapi.UsernameFrom(username)
-	}
-
 	if len(api.OIDCConfig.EmailDomain) > 0 {
 		ok = false
 		for _, domain := range api.OIDCConfig.EmailDomain {
diff --git a/coderd/users.go b/coderd/users.go
@@ -983,6 +983,20 @@ type CreateUserRequest struct {
 }
 
 func (api *API) CreateUser(ctx context.Context, store database.Store, req CreateUserRequest) (database.User, uuid.UUID, error) {
+	// The username is a required property in Coder. We make a best-effort
+	// attempt at using what the claims provide, but if that fails we will
+	// generate a random username.
+	usernameValid := httpapi.NameValid(req.Username)
+	if usernameValid != nil {
+		// If no username is provided, we can default to use the email address.
+		// This will be converted in the from function below, so it's safe
+		// to keep the domain.
+		if req.Username == "" {
+			req.Username = req.Email
+		}
+		req.Username = httpapi.UsernameFrom(req.Username)
+	}
+
 	var user database.User
 	return user, req.OrganizationID, store.InTx(func(tx database.Store) error {
 		orgRoles := make([]string, 0)
diff --git a/enterprise/coderd/scim.go b/enterprise/coderd/scim.go
@@ -71,10 +71,6 @@ func (api *API) scimGetUsers(rw http.ResponseWriter, r *http.Request) {
 // This is done to always force Okta to try and create the user, this way we
 // don't need to implement fetching users twice.
 //
-// scimGetUsers intentionally always returns no users. This is done to always force
-// Okta to try and create each user individually, this way we don't need to
-// implement fetching users twice.
-//
 // @Summary SCIM 2.0: Get user by ID
 // @ID scim-get-user-by-id
 // @Security CoderSessionToken
diff --git a/enterprise/coderd/scim_test.go b/enterprise/coderd/scim_test.go
@@ -128,6 +128,39 @@ func TestScim(t *testing.T) {
 			assert.Equal(t, sUser.Emails[0].Value, userRes.Users[0].Email)
 			assert.Equal(t, sUser.UserName, userRes.Users[0].Username)
 		})
+
+		t.Run("DomainStrips", func(t *testing.T) {
+			t.Parallel()
+
+			ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
+			defer cancel()
+
+			scimAPIKey := []byte("hi")
+			client := coderdenttest.New(t, &coderdenttest.Options{SCIMAPIKey: scimAPIKey})
+			_ = coderdtest.CreateFirstUser(t, client)
+			coderdenttest.AddLicense(t, client, coderdenttest.LicenseOptions{
+				AccountID: "coolin",
+				Features: license.Features{
+					codersdk.FeatureSCIM: 1,
+				},
+			})
+
+			sUser := makeScimUser(t)
+			sUser.UserName = sUser.UserName + "@coder.com"
+			res, err := client.Request(ctx, "POST", "/scim/v2/Users", sUser, setScimAuth(scimAPIKey))
+			require.NoError(t, err)
+			defer res.Body.Close()
+			assert.Equal(t, http.StatusOK, res.StatusCode)
+
+			userRes, err := client.Users(ctx, codersdk.UsersRequest{Search: sUser.Emails[0].Value})
+			require.NoError(t, err)
+			require.Len(t, userRes.Users, 1)
+
+			assert.Equal(t, sUser.Emails[0].Value, userRes.Users[0].Email)
+			// Username should be the same as the given name. They all use the
+			// same string before we modified it above.
+			assert.Equal(t, sUser.Name.GivenName, userRes.Users[0].Username)
+		})
 	})
 
 	t.Run("patchUser", func(t *testing.T) {
