From be13c84cce306911abc37759f7c8baee8793f36e Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Fri, 21 Feb 2025 12:41:34 +0000 Subject: [PATCH 1/4] backend changes --- coderd/userauth.go | 33 ++++++++++++++++++++++++++++++++- coderd/users.go | 39 ++++++++++++++++++--------------------- 2 files changed, 50 insertions(+), 22 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 74a1a718ef58f..709d22389fba3 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -27,6 +27,7 @@ import ( "github.com/coder/coder/v2/coderd/cryptokeys" "github.com/coder/coder/v2/coderd/idpsync" "github.com/coder/coder/v2/coderd/jwtutils" + "github.com/coder/coder/v2/coderd/telemetry" "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/coderd/apikey" @@ -1054,6 +1055,10 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { defer params.CommitAuditLogs() if err != nil { if httpErr := idpsync.IsHTTPError(err); httpErr != nil { + // In the device flow, the error page is rendered client-side. + if api.GithubOAuth2Config.DeviceFlowEnabled && httpErr.RenderStaticPage { + httpErr.RenderStaticPage = false + } httpErr.Write(rw, r) return } @@ -1634,7 +1639,17 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C isConvertLoginType = true } - if user.ID == uuid.Nil && !params.AllowSignups { + // nolint:gocritic // Getting user count is a system function. + userCount, err := tx.GetUserCount(dbauthz.AsSystemRestricted(ctx)) + if err != nil { + return xerrors.Errorf("unable to fetch user count: %w", err) + } + + // Allow the first user to sign up with OIDC, regardless of + // whether signups are enabled or not. + allowSignup := userCount == 0 || params.AllowSignups + + if user.ID == uuid.Nil && !allowSignup { signupsDisabledText := "Please contact your Coder administrator to request access." if api.OIDCConfig != nil && api.OIDCConfig.SignupsDisabledText != "" { signupsDisabledText = render.HTMLFromMarkdown(api.OIDCConfig.SignupsDisabledText) @@ -1695,6 +1710,12 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C return xerrors.Errorf("unable to fetch default organization: %w", err) } + rbacRoles := []string{} + // If this is the first user, add the owner role. + if userCount == 0 { + rbacRoles = append(rbacRoles, rbac.RoleOwner().String()) + } + //nolint:gocritic user, err = api.CreateUser(dbauthz.AsSystemRestricted(ctx), tx, CreateUserRequest{ CreateUserRequestWithOrgs: codersdk.CreateUserRequestWithOrgs{ @@ -1709,10 +1730,20 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C }, LoginType: params.LoginType, accountCreatorName: "oauth", + RBACRoles: rbacRoles, }) if err != nil { return xerrors.Errorf("create user: %w", err) } + + if userCount == 0 { + telemetryUser := telemetry.ConvertUser(user) + // The email is not anonymized for the first user. + telemetryUser.Email = &user.Email + api.Telemetry.Report(&telemetry.Snapshot{ + Users: []telemetry.User{telemetryUser}, + }) + } } // Activate dormant user on sign-in diff --git a/coderd/users.go b/coderd/users.go index 5f8866903bc6f..bf5b1db763fe9 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -118,6 +118,8 @@ func (api *API) firstUser(rw http.ResponseWriter, r *http.Request) { // @Success 201 {object} codersdk.CreateFirstUserResponse // @Router /users/first [post] func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) { + // The first user can also be created via oidc, so if making changes to the flow, + // ensure that the oidc flow is also updated. ctx := r.Context() var createUser codersdk.CreateFirstUserRequest if !httpapi.Read(ctx, rw, r, &createUser) { @@ -198,6 +200,7 @@ func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) { OrganizationIDs: []uuid.UUID{defaultOrg.ID}, }, LoginType: database.LoginTypePassword, + RBACRoles: []string{rbac.RoleOwner().String()}, accountCreatorName: "coder", }) if err != nil { @@ -225,23 +228,6 @@ func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) { Users: []telemetry.User{telemetryUser}, }) - // TODO: @emyrk this currently happens outside the database tx used to create - // the user. Maybe I add this ability to grant roles in the createUser api - // and add some rbac bypass when calling api functions this way?? - // Add the admin role to this first user. - //nolint:gocritic // needed to create first user - _, err = api.Database.UpdateUserRoles(dbauthz.AsSystemRestricted(ctx), database.UpdateUserRolesParams{ - GrantedRoles: []string{rbac.RoleOwner().String()}, - ID: user.ID, - }) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error updating user's roles.", - Detail: err.Error(), - }) - return - } - httpapi.Write(ctx, rw, http.StatusCreated, codersdk.CreateFirstUserResponse{ UserID: user.ID, OrganizationID: defaultOrg.ID, @@ -1351,6 +1337,7 @@ type CreateUserRequest struct { LoginType database.LoginType SkipNotifications bool accountCreatorName string + RBACRoles []string } func (api *API) CreateUser(ctx context.Context, store database.Store, req CreateUserRequest) (database.User, error) { @@ -1360,6 +1347,13 @@ func (api *API) CreateUser(ctx context.Context, store database.Store, req Create return database.User{}, xerrors.Errorf("invalid username %q: %w", req.Username, usernameValid) } + // If the caller didn't specify rbac roles, default to + // a member of the site. + rbacRoles := []string{} + if req.RBACRoles != nil { + rbacRoles = req.RBACRoles + } + var user database.User err := store.InTx(func(tx database.Store) error { orgRoles := make([]string, 0) @@ -1376,10 +1370,9 @@ func (api *API) CreateUser(ctx context.Context, store database.Store, req Create CreatedAt: dbtime.Now(), UpdatedAt: dbtime.Now(), HashedPassword: []byte{}, - // All new users are defaulted to members of the site. - RBACRoles: []string{}, - LoginType: req.LoginType, - Status: status, + RBACRoles: rbacRoles, + LoginType: req.LoginType, + Status: status, } // If a user signs up with OAuth, they can have no password! if req.Password != "" { @@ -1437,6 +1430,10 @@ func (api *API) CreateUser(ctx context.Context, store database.Store, req Create } for _, u := range userAdmins { + if u.ID == user.ID { + // If the new user is an admin, don't notify them about themselves. + continue + } if _, err := api.NotificationsEnqueuer.EnqueueWithData( // nolint:gocritic // Need notifier actor to enqueue notifications dbauthz.AsNotifier(ctx), From 84a20d391dccc8cd2f056d0d8a7b0ebd35f855c1 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Fri, 21 Feb 2025 15:26:34 +0000 Subject: [PATCH 2/4] backend tests --- coderd/userauth_test.go | 64 +++++++++++++++++++++++++++++++++++------ 1 file changed, 55 insertions(+), 9 deletions(-) diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 9c32aefadc8aa..ee6ee957ba861 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -22,6 +22,7 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/atomic" "golang.org/x/oauth2" "golang.org/x/xerrors" @@ -254,11 +255,20 @@ func TestUserOAuth2Github(t *testing.T) { }) t.Run("BlockSignups", func(t *testing.T) { t.Parallel() + + db, ps := dbtestutil.NewDB(t) + + id := atomic.NewInt64(100) + login := atomic.NewString("testuser") + email := atomic.NewString("testuser@coder.com") + client := coderdtest.New(t, &coderdtest.Options{ + Database: db, + Pubsub: ps, GithubOAuth2Config: &coderd.GithubOAuth2Config{ OAuth2Config: &testutil.OAuth2Config{}, AllowOrganizations: []string{"coder"}, - ListOrganizationMemberships: func(ctx context.Context, client *http.Client) ([]*github.Membership, error) { + ListOrganizationMemberships: func(_ context.Context, _ *http.Client) ([]*github.Membership, error) { return []*github.Membership{{ State: &stateActive, Organization: &github.Organization{ @@ -266,16 +276,19 @@ func TestUserOAuth2Github(t *testing.T) { }, }}, nil }, - AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) { + AuthenticatedUser: func(_ context.Context, _ *http.Client) (*github.User, error) { + id := id.Load() + login := login.Load() return &github.User{ - ID: github.Int64(100), - Login: github.String("testuser"), + ID: &id, + Login: &login, Name: github.String("The Right Honorable Sir Test McUser"), }, nil }, - ListEmails: func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) { + ListEmails: func(_ context.Context, _ *http.Client) ([]*github.UserEmail, error) { + email := email.Load() return []*github.UserEmail{{ - Email: github.String("testuser@coder.com"), + Email: &email, Verified: github.Bool(true), Primary: github.Bool(true), }}, nil @@ -283,8 +296,23 @@ func TestUserOAuth2Github(t *testing.T) { }, }) + // The first user in a deployment with signups disabled will be allowed to sign up, + // but all the other users will not. resp := oauth2Callback(t, client) + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + + ctx := testutil.Context(t, testutil.WaitLong) + + // nolint:gocritic // Unit test + count, err := db.GetUserCount(dbauthz.AsSystemRestricted(ctx)) + require.NoError(t, err) + require.Equal(t, int64(1), count) + + id.Store(101) + email.Store("someotheruser@coder.com") + login.Store("someotheruser") + resp = oauth2Callback(t, client) require.Equal(t, http.StatusForbidden, resp.StatusCode) }) t.Run("MultiLoginNotAllowed", func(t *testing.T) { @@ -988,6 +1016,7 @@ func TestUserOIDC(t *testing.T) { IgnoreEmailVerified bool IgnoreUserInfo bool UseAccessToken bool + PrecreateFirstUser bool }{ { Name: "NoSub", @@ -1150,7 +1179,17 @@ func TestUserOIDC(t *testing.T) { "email_verified": true, "sub": uuid.NewString(), }, - StatusCode: http.StatusForbidden, + StatusCode: http.StatusForbidden, + PrecreateFirstUser: true, + }, + { + Name: "FirstSignup", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": true, + "sub": uuid.NewString(), + }, + StatusCode: http.StatusOK, }, { Name: "UsernameFromEmail", @@ -1443,6 +1482,15 @@ func TestUserOIDC(t *testing.T) { }) numLogs := len(auditor.AuditLogs()) + ctx := testutil.Context(t, testutil.WaitShort) + if tc.PrecreateFirstUser { + owner.CreateFirstUser(ctx, codersdk.CreateFirstUserRequest{ + Email: "precreated@coder.com", + Username: "precreated", + Password: "SomeSecurePassword!", + }) + } + client, resp := fake.AttemptLogin(t, owner, tc.IDTokenClaims) numLogs++ // add an audit log for login require.Equal(t, tc.StatusCode, resp.StatusCode) @@ -1450,8 +1498,6 @@ func TestUserOIDC(t *testing.T) { tc.AssertResponse(t, resp) } - ctx := testutil.Context(t, testutil.WaitShort) - if tc.AssertUser != nil { user, err := client.User(ctx, "me") require.NoError(t, err) From c767d2817c8165500063d60681e8ab2b7b4b5dce Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Wed, 19 Feb 2025 16:32:01 +0000 Subject: [PATCH 3/4] frontend changes --- site/src/pages/SetupPage/SetupPage.tsx | 6 +- site/src/pages/SetupPage/SetupPageView.tsx | 84 ++++++++++++++++------ 2 files changed, 66 insertions(+), 24 deletions(-) diff --git a/site/src/pages/SetupPage/SetupPage.tsx b/site/src/pages/SetupPage/SetupPage.tsx index 100c02e21334e..be81f966154ad 100644 --- a/site/src/pages/SetupPage/SetupPage.tsx +++ b/site/src/pages/SetupPage/SetupPage.tsx @@ -1,5 +1,5 @@ import { buildInfo } from "api/queries/buildInfo"; -import { createFirstUser } from "api/queries/users"; +import { authMethods, createFirstUser } from "api/queries/users"; import { Loader } from "components/Loader/Loader"; import { useAuthContext } from "contexts/auth/AuthProvider"; import { useEmbeddedMetadata } from "hooks/useEmbeddedMetadata"; @@ -19,6 +19,7 @@ export const SetupPage: FC = () => { isSignedIn, isSigningIn, } = useAuthContext(); + const authMethodsQuery = useQuery(authMethods()); const createFirstUserMutation = useMutation(createFirstUser()); const setupIsComplete = !isConfiguringTheFirstUser; const { metadata } = useEmbeddedMetadata(); @@ -34,7 +35,7 @@ export const SetupPage: FC = () => { }); }, [buildInfoQuery.data]); - if (isLoading) { + if (isLoading || authMethodsQuery.isLoading) { return ; } @@ -54,6 +55,7 @@ export const SetupPage: FC = () => { {pageTitle("Set up your account")} { diff --git a/site/src/pages/SetupPage/SetupPageView.tsx b/site/src/pages/SetupPage/SetupPageView.tsx index 3e4ddba46db33..5547518ef64a4 100644 --- a/site/src/pages/SetupPage/SetupPageView.tsx +++ b/site/src/pages/SetupPage/SetupPageView.tsx @@ -1,6 +1,8 @@ +import GitHubIcon from "@mui/icons-material/GitHub"; import LoadingButton from "@mui/lab/LoadingButton"; import AlertTitle from "@mui/material/AlertTitle"; import Autocomplete from "@mui/material/Autocomplete"; +import Button from "@mui/material/Button"; import Checkbox from "@mui/material/Checkbox"; import Link from "@mui/material/Link"; import MenuItem from "@mui/material/MenuItem"; @@ -15,8 +17,7 @@ import { PasswordField } from "components/PasswordField/PasswordField"; import { SignInLayout } from "components/SignInLayout/SignInLayout"; import { Stack } from "components/Stack/Stack"; import { type FormikContextType, useFormik } from "formik"; -import type { FC } from "react"; -import { useEffect } from "react"; +import { type ChangeEvent, type FC, useCallback } from "react"; import { docs } from "utils/docs"; import { getFormHelpers, @@ -33,7 +34,8 @@ export const Language = { emailInvalid: "Please enter a valid email address.", emailRequired: "Please enter an email address.", passwordRequired: "Please enter a password.", - create: "Create account", + create: "Continue with email", + githubCreate: "Continue with GitHub", welcomeMessage: <>Welcome to Coder, firstNameLabel: "First name", lastNameLabel: "Last name", @@ -50,13 +52,29 @@ export const Language = { developersRequired: "Please select the number of developers in your company.", }; +const usernameValidator = nameValidator(Language.usernameLabel); +const usernameFromEmail = (email: string): string => { + try { + const emailPrefix = email.split("@")[0]; + const username = emailPrefix.toLowerCase().replace(/[^a-z0-9]/g, "-"); + usernameValidator.validateSync(username); + return username; + } catch (error) { + console.warn( + "failed to automatically generate username, defaulting to 'admin'", + error, + ); + return "admin"; + } +}; + const validationSchema = Yup.object({ email: Yup.string() .trim() .email(Language.emailInvalid) .required(Language.emailRequired), password: Yup.string().required(Language.passwordRequired), - username: nameValidator(Language.usernameLabel), + username: usernameValidator, trial: Yup.bool(), trial_info: Yup.object().when("trial", { is: true, @@ -81,16 +99,23 @@ const numberOfDevelopersOptions = [ "2500+", ]; +const iconStyles = { + width: 16, + height: 16, +}; + export interface SetupPageViewProps { onSubmit: (firstUser: TypesGen.CreateFirstUserRequest) => void; error?: unknown; isLoading?: boolean; + authMethods: TypesGen.AuthMethods | undefined; } export const SetupPageView: FC = ({ onSubmit, error, isLoading, + authMethods, }) => { const form: FormikContextType = useFormik({ @@ -112,6 +137,10 @@ export const SetupPageView: FC = ({ }, validationSchema, onSubmit, + // With validate on blur set to true, the form lights up red whenever + // you click out of it. This is a bit jarring. We instead validate + // on submit and change. + validateOnBlur: false, }); const getFieldHelpers = getFormHelpers( form, @@ -142,23 +171,36 @@ export const SetupPageView: FC = ({ - - + {authMethods?.github.enabled && ( + <> + +
+
+
+ or +
+
+
+ + )} { + const email = event.target.value; + const username = usernameFromEmail(email); + form.setFieldValue("username", username); + onChangeTrimmed(form)(event as ChangeEvent); + }} autoComplete="email" fullWidth label={Language.emailLabel} @@ -340,9 +382,7 @@ export const SetupPageView: FC = ({ loading={isLoading} type="submit" data-testid="create" - size="large" - variant="contained" - color="primary" + size="xlarge" > {Language.create} From 3b3602592b82f01feba2c8bc2bda5d8beb41e1e8 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Fri, 21 Feb 2025 16:12:47 +0000 Subject: [PATCH 4/4] frontend tests --- site/e2e/setup/addUsersAndLicense.spec.ts | 1 - site/src/pages/SetupPage/SetupPage.test.tsx | 3 --- 2 files changed, 4 deletions(-) diff --git a/site/e2e/setup/addUsersAndLicense.spec.ts b/site/e2e/setup/addUsersAndLicense.spec.ts index bcaa8c9281cf8..784db4812aaa1 100644 --- a/site/e2e/setup/addUsersAndLicense.spec.ts +++ b/site/e2e/setup/addUsersAndLicense.spec.ts @@ -16,7 +16,6 @@ test("setup deployment", async ({ page }) => { } // Setup first user - await page.getByLabel(Language.usernameLabel).fill(users.admin.username); await page.getByLabel(Language.emailLabel).fill(users.admin.email); await page.getByLabel(Language.passwordLabel).fill(users.admin.password); await page.getByTestId("create").click(); diff --git a/site/src/pages/SetupPage/SetupPage.test.tsx b/site/src/pages/SetupPage/SetupPage.test.tsx index a088948623ff4..47cf1d58746e2 100644 --- a/site/src/pages/SetupPage/SetupPage.test.tsx +++ b/site/src/pages/SetupPage/SetupPage.test.tsx @@ -13,7 +13,6 @@ import { SetupPage } from "./SetupPage"; import { Language as PageViewLanguage } from "./SetupPageView"; const fillForm = async ({ - username = "someuser", email = "someone@coder.com", password = "password", }: { @@ -21,10 +20,8 @@ const fillForm = async ({ email?: string; password?: string; } = {}) => { - const usernameField = screen.getByLabelText(PageViewLanguage.usernameLabel); const emailField = screen.getByLabelText(PageViewLanguage.emailLabel); const passwordField = screen.getByLabelText(PageViewLanguage.passwordLabel); - await userEvent.type(usernameField, username); await userEvent.type(emailField, email); await userEvent.type(passwordField, password); const submitButton = screen.getByRole("button", {