Skip to content

Commit 8298a92

Browse files
BrunoQuaresmaBrunoQuaresma
authored
fix(site): Fix login flow (coder#6294)
1 parent a32169c commit 8298a92

File tree

15 files changed

+273
-386
lines changed

15 files changed

+273
-386
lines changed

‎site/src/api/api.ts

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -101,9 +101,19 @@ export const logout = async (): Promise<void> => {
101101
await axios.post("/api/v2/users/logout")
102102
}
103103

104-
export const getUser = async (): Promise<TypesGen.User> => {
105-
const response = await axios.get<TypesGen.User>("/api/v2/users/me")
106-
return response.data
104+
export const getAuthenticatedUser = async (): Promise<
105+
TypesGen.User | undefined
106+
> => {
107+
try {
108+
const response = await axios.get<TypesGen.User>("/api/v2/users/me")
109+
return response.data
110+
} catch (error) {
111+
if (axios.isAxiosError(error) && error.response?.status === 401) {
112+
return undefined
113+
}
114+
115+
throw error
116+
}
107117
}
108118

109119
export const getAuthMethods = async (): Promise<TypesGen.AuthMethods> => {

‎site/src/components/RequireAuth/RequireAuth.tsx

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,12 @@ export const RequireAuth: FC = () => {
1313

1414
if (authState.matches("signedOut")) {
1515
return <Navigate to={navigateTo} state={{ isRedirect: !isHomePage }} />
16-
} else if (authState.matches("waitingForTheFirstUser")) {
16+
} else if (authState.matches("configuringTheFirstUser")) {
1717
return <Navigate to="/setup" />
18-
} else if (authState.hasTag("loading")) {
18+
} else if (
19+
authState.matches("loadingInitialAuthData") ||
20+
authState.matches("signingOut")
21+
) {
1922
return <FullScreenLoader />
2023
} else {
2124
return <Outlet />

‎site/src/components/SignInForm/OAuthSignInForm.tsx

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import { FC } from "react"
99
import { makeStyles } from "@material-ui/core/styles"
1010

1111
type OAuthSignInFormProps = {
12-
isLoading: boolean
12+
isSigningIn: boolean
1313
redirectTo: string
1414
authMethods?: AuthMethods
1515
}
@@ -22,7 +22,7 @@ const useStyles = makeStyles((theme) => ({
2222
}))
2323

2424
export const OAuthSignInForm: FC<OAuthSignInFormProps> = ({
25-
isLoading,
25+
isSigningIn,
2626
redirectTo,
2727
authMethods,
2828
}) => {
@@ -39,7 +39,7 @@ export const OAuthSignInForm: FC<OAuthSignInFormProps> = ({
3939
>
4040
<Button
4141
startIcon={<GitHubIcon className={styles.buttonIcon} />}
42-
disabled={isLoading}
42+
disabled={isSigningIn}
4343
fullWidth
4444
type="submit"
4545
variant="outlined"
@@ -68,7 +68,7 @@ export const OAuthSignInForm: FC<OAuthSignInFormProps> = ({
6868
<KeyIcon className={styles.buttonIcon} />
6969
)
7070
}
71-
disabled={isLoading}
71+
disabled={isSigningIn}
7272
fullWidth
7373
type="submit"
7474
variant="outlined"

‎site/src/components/SignInForm/PasswordSignInForm.tsx

Lines changed: 6 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,23 @@
11
import { Stack } from "../Stack/Stack"
2-
import { AlertBanner } from "../AlertBanner/AlertBanner"
32
import TextField from "@material-ui/core/TextField"
43
import { getFormHelpers, onChangeTrimmed } from "../../util/formUtils"
54
import { LoadingButton } from "../LoadingButton/LoadingButton"
6-
import { Language, LoginErrors } from "./SignInForm"
5+
import { Language } from "./SignInForm"
76
import { FormikContextType, FormikTouched, useFormik } from "formik"
87
import * as Yup from "yup"
98
import { FC } from "react"
109
import { BuiltInAuthFormValues } from "./SignInForm.types"
1110

1211
type PasswordSignInFormProps = {
13-
loginErrors: Partial<Record<LoginErrors, Error | unknown>>
1412
onSubmit: (credentials: { email: string; password: string }) => void
1513
initialTouched?: FormikTouched<BuiltInAuthFormValues>
16-
isLoading: boolean
14+
isSigningIn: boolean
1715
}
1816

1917
export const PasswordSignInForm: FC<PasswordSignInFormProps> = ({
20-
loginErrors,
2118
onSubmit,
2219
initialTouched,
23-
isLoading,
20+
isSigningIn,
2421
}) => {
2522
const validationSchema = Yup.object({
2623
email: Yup.string()
@@ -37,33 +34,14 @@ export const PasswordSignInForm: FC<PasswordSignInFormProps> = ({
3734
password: "",
3835
},
3936
validationSchema,
40-
// The email field has an autoFocus, but users may log in with a button click.
41-
// This is set to `false` in order to keep the autoFocus, validateOnChange
42-
// and Formik experience friendly. Validation will kick in onChange (any
43-
// field), or after a submission attempt.
44-
validateOnBlur: false,
4537
onSubmit,
4638
initialTouched,
4739
})
48-
const getFieldHelpers = getFormHelpers<BuiltInAuthFormValues>(
49-
form,
50-
loginErrors.authError,
51-
)
40+
const getFieldHelpers = getFormHelpers<BuiltInAuthFormValues>(form)
5241

5342
return (
5443
<form onSubmit={form.handleSubmit}>
5544
<Stack>
56-
{Object.keys(loginErrors).map(
57-
(errorKey: string) =>
58-
Boolean(loginErrors[errorKey as LoginErrors]) && (
59-
<AlertBanner
60-
key={errorKey}
61-
severity="error"
62-
error={loginErrors[errorKey as LoginErrors]}
63-
text={Language.errorMessages[errorKey as LoginErrors]}
64-
/>
65-
),
66-
)}
6745
<TextField
6846
{...getFieldHelpers("email")}
6947
onChange={onChangeTrimmed(form)}
@@ -85,12 +63,12 @@ export const PasswordSignInForm: FC<PasswordSignInFormProps> = ({
8563
/>
8664
<div>
8765
<LoadingButton
88-
loading={isLoading}
66+
loading={isSigningIn}
8967
fullWidth
9068
type="submit"
9169
variant="outlined"
9270
>
93-
{isLoading ? "" : Language.passwordSignIn}
71+
{isSigningIn ? "" : Language.passwordSignIn}
9472
</LoadingButton>
9573
</div>
9674
</Stack>

‎site/src/components/SignInForm/SignInForm.tsx

Lines changed: 14 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -11,24 +11,11 @@ import Button from "@material-ui/core/Button"
1111
import EmailIcon from "@material-ui/icons/EmailOutlined"
1212
import { AlertBanner } from "components/AlertBanner/AlertBanner"
1313

14-
export enum LoginErrors {
15-
AUTH_ERROR = "authError",
16-
GET_USER_ERROR = "getUserError",
17-
CHECK_PERMISSIONS_ERROR = "checkPermissionsError",
18-
GET_METHODS_ERROR = "getMethodsError",
19-
}
20-
2114
export const Language = {
2215
emailLabel: "Email",
2316
passwordLabel: "Password",
2417
emailInvalid: "Please enter a valid email address.",
2518
emailRequired: "Please enter an email address.",
26-
errorMessages: {
27-
[LoginErrors.AUTH_ERROR]: "Incorrect email or password.",
28-
[LoginErrors.GET_USER_ERROR]: "Failed to fetch user details.",
29-
[LoginErrors.CHECK_PERMISSIONS_ERROR]: "Unable to fetch user permissions.",
30-
[LoginErrors.GET_METHODS_ERROR]: "Unable to fetch auth methods.",
31-
},
3219
passwordSignIn: "Sign In",
3320
githubSignIn: "GitHub",
3421
oidcSignIn: "OpenID Connect",
@@ -49,6 +36,9 @@ const useStyles = makeStyles((theme) => ({
4936
fontWeight: 600,
5037
},
5138
},
39+
error: {
40+
marginBottom: theme.spacing(4),
41+
},
5242
divider: {
5343
paddingTop: theme.spacing(3),
5444
paddingBottom: theme.spacing(3),
@@ -75,9 +65,9 @@ const useStyles = makeStyles((theme) => ({
7565
}))
7666

7767
export interface SignInFormProps {
78-
isLoading: boolean
68+
isSigningIn: boolean
7969
redirectTo: string
80-
loginErrors: Partial<Record<LoginErrors, Error | unknown>>
70+
error?: unknown
8171
authMethods?: AuthMethods
8272
onSubmit: (credentials: { email: string; password: string }) => void
8373
// initialTouched is only used for testing the error state of the form.
@@ -87,20 +77,18 @@ export interface SignInFormProps {
8777
export const SignInForm: FC<React.PropsWithChildren<SignInFormProps>> = ({
8878
authMethods,
8979
redirectTo,
90-
isLoading,
91-
loginErrors,
80+
isSigningIn,
81+
error,
9282
onSubmit,
9383
initialTouched,
9484
}) => {
9585
const oAuthEnabled = Boolean(
9686
authMethods?.github.enabled || authMethods?.oidc.enabled,
9787
)
9888
const passwordEnabled = authMethods?.password.enabled ?? true
99-
10089
// Hide password auth by default if any OAuth method is enabled
10190
const [showPasswordAuth, setShowPasswordAuth] = useState(!oAuthEnabled)
10291
const styles = useStyles()
103-
10492
const commonTranslation = useTranslation("common")
10593
const loginPageTranslation = useTranslation("loginPage")
10694

@@ -110,12 +98,16 @@ export const SignInForm: FC<React.PropsWithChildren<SignInFormProps>> = ({
11098
{loginPageTranslation.t("signInTo")}{" "}
11199
<strong>{commonTranslation.t("coder")}</strong>
112100
</h1>
101+
<Maybe condition={error !== undefined}>
102+
<div className={styles.error}>
103+
<AlertBanner severity="error" error={error} />
104+
</div>
105+
</Maybe>
113106
<Maybe condition={passwordEnabled && showPasswordAuth}>
114107
<PasswordSignInForm
115-
loginErrors={loginErrors}
116108
onSubmit={onSubmit}
117109
initialTouched={initialTouched}
118-
isLoading={isLoading}
110+
isSigningIn={isSigningIn}
119111
/>
120112
</Maybe>
121113
<Maybe condition={passwordEnabled && showPasswordAuth && oAuthEnabled}>
@@ -127,7 +119,7 @@ export const SignInForm: FC<React.PropsWithChildren<SignInFormProps>> = ({
127119
</Maybe>
128120
<Maybe condition={oAuthEnabled}>
129121
<OAuthSignInForm
130-
isLoading={isLoading}
122+
isSigningIn={isSigningIn}
131123
redirectTo={redirectTo}
132124
authMethods={authMethods}
133125
/>

‎site/src/hooks/useMe.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,14 @@
11
import { User } from "api/typesGenerated"
22
import { useAuth } from "components/AuthProvider/AuthProvider"
3-
import { selectUser } from "xServices/auth/authSelectors"
3+
import { isAuthenticated } from "xServices/auth/authXService"
44

55
export const useMe = (): User => {
66
const [authState] = useAuth()
7-
const me = selectUser(authState)
7+
const { data } = authState.context
88

9-
if (!me) {
10-
throw new Error("User not found.")
9+
if (isAuthenticated(data)) {
10+
return data.user
1111
}
1212

13-
return me
13+
throw new Error("User is not authenticated")
1414
}

‎site/src/hooks/useOrganizationId.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
import { useAuth } from "components/AuthProvider/AuthProvider"
2-
import { selectOrgId } from "../xServices/auth/authSelectors"
2+
import { isAuthenticated } from "xServices/auth/authXService"
33

44
export const useOrganizationId = (): string => {
55
const [authState] = useAuth()
6-
const organizationId = selectOrgId(authState)
6+
const { data } = authState.context
77

8-
if (!organizationId) {
9-
throw new Error("No organization ID found")
8+
if (isAuthenticated(data)) {
9+
return data.user.organization_ids[0]
1010
}
1111

12-
return organizationId
12+
throw new Error("User is not authenticated")
1313
}

‎site/src/hooks/usePermissions.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
import { useAuth } from "components/AuthProvider/AuthProvider"
2-
import { AuthContext } from "xServices/auth/authXService"
2+
import { isAuthenticated, Permissions } from "xServices/auth/authXService"
33

4-
export const usePermissions = (): NonNullable<AuthContext["permissions"]> => {
4+
export const usePermissions = (): Permissions => {
55
const [authState] = useAuth()
6-
const { permissions } = authState.context
6+
const { data } = authState.context
77

8-
if (!permissions) {
9-
throw new Error("Permissions are not loaded yet.")
8+
if (isAuthenticated(data)) {
9+
return data.permissions
1010
}
1111

12-
return permissions
12+
throw new Error("User is not authenticated.")
1313
}

‎site/src/pages/LoginPage/LoginPage.test.tsx

Lines changed: 3 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -36,13 +36,11 @@ describe("LoginPage", () => {
3636

3737
it("shows an error message if SignIn fails", async () => {
3838
// Given
39+
const apiErrorMessage = "Something wrong happened"
3940
server.use(
4041
// Make login fail
4142
rest.post("/api/v2/users/login", async (req, res, ctx) => {
42-
return res(
43-
ctx.status(500),
44-
ctx.json({ message: Language.errorMessages.authError }),
45-
)
43+
return res(ctx.status(500), ctx.json({ message: apiErrorMessage }))
4644
}),
4745
)
4846

@@ -57,30 +55,10 @@ describe("LoginPage", () => {
5755
const signInButton = await screen.findByText(Language.passwordSignIn)
5856
fireEvent.click(signInButton)
5957

60-
// Then
61-
const errorMessage = await screen.findByText(
62-
Language.errorMessages.authError,
63-
)
64-
expect(errorMessage).toBeDefined()
65-
expect(history.location.pathname).toEqual("/login")
66-
})
67-
68-
it("shows an error if fetching auth methods fails", async () => {
69-
// Given
70-
const apiErrorMessage = "Unable to fetch methods"
71-
server.use(
72-
// Make login fail
73-
rest.get("/api/v2/users/authmethods", async (req, res, ctx) => {
74-
return res(ctx.status(500), ctx.json({ message: apiErrorMessage }))
75-
}),
76-
)
77-
78-
// When
79-
render(<LoginPage />)
80-
8158
// Then
8259
const errorMessage = await screen.findByText(apiErrorMessage)
8360
expect(errorMessage).toBeDefined()
61+
expect(history.location.pathname).toEqual("/login")
8462
})
8563

8664
it("shows github authentication when enabled", async () => {

‎site/src/pages/LoginPage/LoginPage.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ export const LoginPage: FC = () => {
1515

1616
if (authState.matches("signedIn")) {
1717
return <Navigate to={redirectTo} replace />
18-
} else if (authState.matches("waitingForTheFirstUser")) {
18+
} else if (authState.matches("configuringTheFirstUser")) {
1919
return <Navigate to="/setup" />
2020
} else {
2121
return (
@@ -27,7 +27,8 @@ export const LoginPage: FC = () => {
2727
</Helmet>
2828
<LoginPageView
2929
context={authState.context}
30-
isLoading={authState.hasTag("loading")}
30+
isLoading={authState.matches("loadingInitialAuthData")}
31+
isSigningIn={authState.matches("signingIn")}
3132
onSignIn={({ email, password }) => {
3233
authSend({ type: "SIGN_IN", email, password })
3334
}}

0 commit comments

Comments
 (0)