Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent email from being altered #1863

Merged
merged 6 commits into from
May 27, 2022
Merged

fix: prevent email from being altered #1863

merged 6 commits into from
May 27, 2022

Conversation

f0ssel
Copy link
Contributor

@f0ssel f0ssel commented May 27, 2022

Fixes: #1490
Fixes: #1725

Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice if you removed the frontend in this PR too. It'd be good to build the habit of not having broken states in our main deployment.

I'm happy to help if you encounter any issues. That's not a blocker though, just a suggestion!

@BrunoQuaresma
Copy link
Collaborator

BrunoQuaresma commented May 27, 2022

@f0ssel feel free to do the FE if you want but if not, which is 100% ok, could you create a ticket for this and add the FE label, and make it Community MVP, please?

@f0ssel
Copy link
Contributor Author

f0ssel commented May 27, 2022

💯 I would not merge this as is, frontend will be same PR

@f0ssel f0ssel requested a review from a team as a code owner May 27, 2022 18:48
@f0ssel
Copy link
Contributor Author

f0ssel commented May 27, 2022

@BrunoQuaresma This is ready for frontend review

@greyscaled
Copy link
Contributor

Will still need to make the frontend field readonly and only send the username in the update payload.

FE ticket is here: #1725

@@ -59,6 +56,7 @@ export const AccountForm: React.FC<AccountFormProps> = ({
fullWidth
label={Language.emailLabel}
variant="outlined"
disabled={true}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
disabled={true}
disabled

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we should remove the getFieldHelpers("email") and onChange and autoComplete here.

At a minimum, can you remove the autoComplete (doesn't make much sense for a disabled field).

Copy link
Contributor

@greyscaled greyscaled left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a patch I'd prefer to be applied to the FE code. It cleans up the form so that email is read-only and not associated with the Formik form.

diff --git a/site/src/components/SettingsAccountForm/SettingsAccountForm.tsx b/site/src/components/SettingsAccountForm/SettingsAccountForm.tsx
index 3773b307..4c90deb6 100644
--- a/site/src/components/SettingsAccountForm/SettingsAccountForm.tsx
+++ b/site/src/components/SettingsAccountForm/SettingsAccountForm.tsx
@@ -8,7 +8,6 @@ import { LoadingButton } from "../LoadingButton/LoadingButton"
 import { Stack } from "../Stack/Stack"
 
 interface AccountFormValues {
-  email: string
   username: string
 }
 
@@ -23,7 +22,9 @@ const validationSchema = Yup.object({
 })
 
 export type AccountFormErrors = FormikErrors<AccountFormValues>
+
 export interface AccountFormProps {
+  email: string
   isLoading: boolean
   initialValues: AccountFormValues
   onSubmit: (values: AccountFormValues) => void
@@ -32,6 +33,7 @@ export interface AccountFormProps {
 }
 
 export const AccountForm: React.FC<AccountFormProps> = ({
+  email,
   isLoading,
   onSubmit,
   initialValues,
@@ -49,15 +51,7 @@ export const AccountForm: React.FC<AccountFormProps> = ({
     <>
       <form onSubmit={form.handleSubmit}>
         <Stack>
-          <TextField
-            {...getFieldHelpers("email")}
-            onChange={onChangeTrimmed(form)}
-            autoComplete="email"
-            fullWidth
-            label={Language.emailLabel}
-            variant="outlined"
-            disabled={true}
-          />
+          <TextField disabled fullWidth label={Language.emailLabel} value={email} variant="outlined" />
           <TextField
             {...getFieldHelpers("username")}
             onChange={onChangeTrimmed(form)}
diff --git a/site/src/pages/SettingsPages/AccountPage/AccountPage.tsx b/site/src/pages/SettingsPages/AccountPage/AccountPage.tsx
index 5f9cf428..5954ac64 100644
--- a/site/src/pages/SettingsPages/AccountPage/AccountPage.tsx
+++ b/site/src/pages/SettingsPages/AccountPage/AccountPage.tsx
@@ -26,10 +26,11 @@ export const AccountPage: React.FC = () => {
   return (
     <Section title={Language.title}>
       <AccountForm
+        email={me.email}
         error={hasUnknownError ? Language.unknownError : undefined}
         formErrors={formErrors}
         isLoading={authState.matches("signedIn.profile.updatingProfile")}
-        initialValues={{ username: me.username, email: me.email }}
+        initialValues={{ username: me.username }}
         onSubmit={(data) => {
           authSend({
             type: "UPDATE_PROFILE",

@f0ssel f0ssel enabled auto-merge (squash) May 27, 2022 22:22
@f0ssel f0ssel merged commit 5598ac0 into main May 27, 2022
@f0ssel f0ssel deleted the f0ssel/github-auth-lock branch May 27, 2022 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants