fix(enterprise): ensure scim usernames are validated (coder#7925)

coadler · web-flow · commit 30a635aa5feb · 2023-06-08T17:59:49.000-05:00
diff --git a/coderd/users.go b/coderd/users.go
@@ -983,6 +983,12 @@ type CreateUserRequest struct {
 }
 
 func (api *API) CreateUser(ctx context.Context, store database.Store, req CreateUserRequest) (database.User, uuid.UUID, error) {
+	// Ensure the username is valid. It's the caller's responsibility to ensure
+	// the username is valid and unique.
+	if usernameValid := httpapi.NameValid(req.Username); usernameValid != nil {
+		return database.User{}, uuid.Nil, xerrors.Errorf("invalid username %q: %w", req.Username, usernameValid)
+	}
+
 	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
@@ -156,6 +152,20 @@ func (api *API) scimPostUser(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// 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(sUser.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 sUser.UserName == "" {
+			sUser.UserName = email
+		}
+		sUser.UserName = httpapi.UsernameFrom(sUser.UserName)
+	}
+
 	var organizationID uuid.UUID
 	//nolint:gocritic
 	organizations, err := api.Database.GetOrganizations(dbauthz.AsSystemRestricted(ctx))
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) {
