Add phone verification #12258
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,241 @@ | ||||||||
| /** | ||||||||
| * Copyright (c) 2022 Gitpod GmbH. All rights reserved. | ||||||||
| * Licensed under the GNU Affero General Public License (AGPL). | ||||||||
| * See License-AGPL.txt in the project root for license information. | ||||||||
| */ | ||||||||
|
|
||||||||
| import { useState } from "react"; | ||||||||
| import Alert, { AlertType } from "../components/Alert"; | ||||||||
| import Modal from "../components/Modal"; | ||||||||
| import { getGitpodService } from "../service/service"; | ||||||||
| import PhoneInput from "react-intl-tel-input"; | ||||||||
| import "react-intl-tel-input/dist/main.css"; | ||||||||
| import "./phone-input.css"; | ||||||||
|
|
||||||||
| interface VerifyModalState { | ||||||||
| phoneNumber?: string; | ||||||||
| phoneNumberValid?: boolean; | ||||||||
| sent?: Date; | ||||||||
| sending?: boolean; | ||||||||
| message?: { | ||||||||
| type: AlertType; | ||||||||
| text: string; | ||||||||
| }; | ||||||||
| token?: string; | ||||||||
| verified?: boolean; | ||||||||
| } | ||||||||
|
|
||||||||
| export function VerifyModal() { | ||||||||
| const [state, setState] = useState<VerifyModalState>({}); | ||||||||
|
|
||||||||
| if (!state.sent) { | ||||||||
| const sendCode = async () => { | ||||||||
| try { | ||||||||
| setState({ | ||||||||
| ...state, | ||||||||
| message: undefined, | ||||||||
| sending: true, | ||||||||
| }); | ||||||||
| await getGitpodService().server.sendPhoneNumberVerificationToken(state.phoneNumber || ""); | ||||||||
| setState({ | ||||||||
| ...state, | ||||||||
| sending: false, | ||||||||
| sent: new Date(), | ||||||||
| }); | ||||||||
| return true; | ||||||||
| } catch (err) { | ||||||||
| setState({ | ||||||||
| sent: undefined, | ||||||||
| sending: false, | ||||||||
| message: { | ||||||||
| type: "error", | ||||||||
| text: err.toString(), | ||||||||
| }, | ||||||||
| }); | ||||||||
| return false; | ||||||||
| } | ||||||||
| }; | ||||||||
| return ( | ||||||||
| <Modal | ||||||||
| onClose={() => {}} | ||||||||
| closeable={false} | ||||||||
| onEnter={sendCode} | ||||||||
| title="User Validation Required" | ||||||||
| buttons={ | ||||||||
| <div> | ||||||||
| <button className="ml-2" disabled={!state.phoneNumberValid || state.sending} onClick={sendCode}> | ||||||||
| Send Code via SMS | ||||||||
gtsiolis marked this conversation as resolved.
Show resolved
Hide resolved
gtsiolis marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. issue(non-blocking): Sounds ok to leave this as is but I could trigger four (4) messages in a row. Could we easily disable or somehow prevent multiple requests here to avoid abuse or unnecessary charges in Twilio? There was a problem hiding this comment. aren't the two limits you posted below what you are asking for here? There was a problem hiding this comment. This is about limiting users for clicking the button multiple times for fun or abuse. I clicked 4 times in a row and got four SMS messages. Famous last words: Minor issue, few or no one will notice. 😈 |
||||||||
| </button> | ||||||||
| </div> | ||||||||
| } | ||||||||
| visible={true} | ||||||||
| > | ||||||||
| <Alert type="warning" className="mt-2"> | ||||||||
| To use Gitpod you'll need to validate your account with your phone number. This is required to | ||||||||
|
There was a problem hiding this comment. nitpick: Does it help us achieve our goal (abuse)? I'd rather go with a neutral tone but any option could suffice. Feel free to ignore this nitpick. 🙂
Suggested change
|
||||||||
| discourage and reduce abuse on Gitpod infrastructure. | ||||||||
| </Alert> | ||||||||
| <div className="text-gray-600 dark:text-gray-400 mt-2"> | ||||||||
| Enter a mobile phone number you would like to use to verify your account. | ||||||||
| </div> | ||||||||
| {state.message ? ( | ||||||||
| <Alert type={state.message.type} className="mt-4 py-3"> | ||||||||
|
There was a problem hiding this comment. issue: Testing is good enough only if you're reaching the limits, see screenshot below. 🙂 Ideally, we could allow users to go back and use a different mobile number when they reach an error like this but we could leave this would probably be an abuse use case and simply reloading should unblock them. Also, the limit seems to be automatically removed after some time. ⏱️
There was a problem hiding this comment. issue(non-blocking): I've ran into this error but should be fine to ignore. Twilio probably hates me by now. 😇
|
||||||||
| {state.message.text} | ||||||||
| </Alert> | ||||||||
| ) : ( | ||||||||
| <></> | ||||||||
| )} | ||||||||
| <div className="mt-4"> | ||||||||
| <h4>Mobile Phone Number</h4> | ||||||||
| {/* HACK: Below we are adding a dummy dom element that is not visible, to reference the classes so they are not removed by purgeCSS. */} | ||||||||
gtsiolis marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| <input type="tel" className="hidden intl-tel-input country-list" /> | ||||||||
| <PhoneInput | ||||||||
| autoFocus={true} | ||||||||
| containerClassName={"allow-dropdown w-full intl-tel-input"} | ||||||||
| inputClassName={"w-full"} | ||||||||
| allowDropdown={true} | ||||||||
| defaultCountry={""} | ||||||||
| autoHideDialCode={false} | ||||||||
| onPhoneNumberChange={(isValid, phoneNumberRaw, countryData) => { | ||||||||
| let phoneNumber = phoneNumberRaw; | ||||||||
| if (!phoneNumber.startsWith("+") && !phoneNumber.startsWith("00")) { | ||||||||
| phoneNumber = "+" + countryData.dialCode + phoneNumber; | ||||||||
| } | ||||||||
| setState({ | ||||||||
| ...state, | ||||||||
| phoneNumber, | ||||||||
| phoneNumberValid: isValid, | ||||||||
| }); | ||||||||
| }} | ||||||||
| /> | ||||||||
| </div> | ||||||||
| </Modal> | ||||||||
| ); | ||||||||
| } else if (!state.verified) { | ||||||||
| const isTokenFilled = () => { | ||||||||
| return state.token && /\d{6}/.test(state.token); | ||||||||
| }; | ||||||||
| const verifyToken = async () => { | ||||||||
| try { | ||||||||
| const verified = await getGitpodService().server.verifyPhoneNumberVerificationToken( | ||||||||
| state.phoneNumber!, | ||||||||
| state.token!, | ||||||||
| ); | ||||||||
| if (verified) { | ||||||||
| setState({ | ||||||||
| ...state, | ||||||||
| verified: true, | ||||||||
| message: undefined, | ||||||||
| }); | ||||||||
| } else { | ||||||||
| setState({ | ||||||||
| ...state, | ||||||||
| message: { | ||||||||
| type: "error", | ||||||||
| text: `Invalid verification code.`, | ||||||||
| }, | ||||||||
| }); | ||||||||
| } | ||||||||
| return verified; | ||||||||
| } catch (err) { | ||||||||
| setState({ | ||||||||
| sent: undefined, | ||||||||
| sending: false, | ||||||||
| message: { | ||||||||
| type: "error", | ||||||||
| text: err.toString(), | ||||||||
| }, | ||||||||
| }); | ||||||||
| return false; | ||||||||
| } | ||||||||
| }; | ||||||||
|
|
||||||||
| const reset = () => { | ||||||||
| setState({ | ||||||||
| ...state, | ||||||||
| sent: undefined, | ||||||||
| message: undefined, | ||||||||
| token: undefined, | ||||||||
| }); | ||||||||
| }; | ||||||||
| return ( | ||||||||
| <Modal | ||||||||
| onClose={() => {}} | ||||||||
| closeable={false} | ||||||||
| onEnter={verifyToken} | ||||||||
| title="User Validation Required" | ||||||||
gtsiolis marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| buttons={ | ||||||||
| <div> | ||||||||
| <button className="ml-2" disabled={!isTokenFilled()} onClick={verifyToken}> | ||||||||
|
There was a problem hiding this comment. issue(non-blocking): The disabled button state should not have a green color (primary, confirm) but a gray background color like the default button variant. Also, relevant to the comment about the button component. There was a problem hiding this comment. issue(non-blocking): The disabled button state should have the "not-allowed" cursor when hovered and not relying on the CSS class for disabling the interaction. Also, relevant to the comment about the button component. suggestion: Tailwind supports |
||||||||
| Validate Account | ||||||||
|
There was a problem hiding this comment. suggestion: I noticed we've skipped the verification in progress step from the initial design specs in #11339 (comment) which is fine, but what do you think of injecting a loading state inside the button here with the spinner icon? FWIW, we've used this pattern for running prebuilds in the past, see relevant diff below. thought: We could use the same pattern also for sending the SMS via code since we rely on an external service for this action. thought: The loading state of the button is another great example use case where a button component could be used. 💭 gitpod/components/dashboard/src/projects/Prebuild.tsx Lines 160 to 169 in 3133297 Re-posting the verification in progress design from #11339 (comment) for visibility:
|
||||||||
| </button> | ||||||||
| </div> | ||||||||
| } | ||||||||
| visible={true} | ||||||||
| > | ||||||||
| <Alert type="warning" className="mt-2"> | ||||||||
| To use Gitpod you'll need to validate your account with your phone number. This is required to | ||||||||
| discourage and reduce abuse on Gitpod infrastructure. | ||||||||
| </Alert> | ||||||||
| <div className="pt-4"> | ||||||||
| <button className="gp-link" onClick={reset}> | ||||||||
|
There was a problem hiding this comment. thought: Ideally, instead of relying on a custom class, this would use a link variant of a button component as this involves a reset action but let's keep this out of the scope here as we also don't have a button component yet. 💭 |
||||||||
| ← Use a different phone number | ||||||||
| </button> | ||||||||
| </div> | ||||||||
| <div className="text-gray-600 dark:text-gray-400 pt-4"> | ||||||||
| Enter the verification code we sent to {state.phoneNumber}.<br /> | ||||||||
| Having trouble?{" "} | ||||||||
| <a className="gp-link" href="https://www.gitpod.io/contact/support"> | ||||||||
| Contact support | ||||||||
| </a> | ||||||||
| </div> | ||||||||
| {state.message ? ( | ||||||||
| <Alert type={state.message.type} className="mt-4 py-3"> | ||||||||
| {state.message.text} | ||||||||
| </Alert> | ||||||||
| ) : ( | ||||||||
| <></> | ||||||||
| )} | ||||||||
| <div className="mt-4"> | ||||||||
| <h4>Verification Code</h4> | ||||||||
gtsiolis marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| <input | ||||||||
| autoFocus={true} | ||||||||
| className="w-full" | ||||||||
| type="text" | ||||||||
| placeholder="Enter code sent via SMS" | ||||||||
| onChange={(v) => { | ||||||||
| setState({ | ||||||||
| ...state, | ||||||||
| token: v.currentTarget.value, | ||||||||
| }); | ||||||||
| }} | ||||||||
| /> | ||||||||
| </div> | ||||||||
| </Modal> | ||||||||
| ); | ||||||||
| } else { | ||||||||
| const continueStartWorkspace = () => { | ||||||||
| window.location.reload(); | ||||||||
| return true; | ||||||||
| }; | ||||||||
| return ( | ||||||||
| <Modal | ||||||||
| onClose={continueStartWorkspace} | ||||||||
| closeable={false} | ||||||||
| onEnter={continueStartWorkspace} | ||||||||
| title="User Validation Successful" | ||||||||
gtsiolis marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. suggestion: Given it's not ideal to update the modal title based on the user action and taking into account the relevant discussion (internal), what do you think of moving the alert component on the loading screen directly where we later on show the warnings about the latest editor release and skip asking users to acknowledge the account validation?
There was a problem hiding this comment. I think an explicit, "you are verified" message with a 'continue' button is clearer. Especially because we are reloading the page. There was a problem hiding this comment. thought: It is certainly clearer and gives users the time to acknowledge this. 💯 question: What do you think of keeping modal, and updating the modal title to be the same on all cases, and rely on modal body (paragraph and alerts) to inform the user about validation errors or successful validation?
gtsiolis marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| buttons={ | ||||||||
| <div> | ||||||||
| <button className="ml-2" onClick={continueStartWorkspace}> | ||||||||
| Continue | ||||||||
| </button> | ||||||||
| </div> | ||||||||
| } | ||||||||
| visible={true} | ||||||||
| > | ||||||||
| <Alert type="success" className="mt-2"> | ||||||||
| Your account has been successfully verified. | ||||||||
|
There was a problem hiding this comment. suggestion: Now that we've tried this, we probably need a new alert component variant for successful messages like this that is using green-ish colors. Could be fine to leave this out of the scope of these changes for now. Your call.
If you'd like to add this here, here's the icon (SVG) and the colors used for light and dark themes: LIGHT THEME ⛅
<svg width="20" height="20" viewBox="0 0 20 20" fill="none" xmlns="http://www.w3.org/2000/svg">
<path fill-rule="evenodd" clip-rule="evenodd" d="M10 18C14.4183 18 18 14.4183 18 10C18 5.58172 14.4183 2 10 2C5.58172 2 2 5.58172 2 10C2 14.4183 5.58172 18 10 18ZM13.7071 8.70711C14.0976 8.31658 14.0976 7.68342 13.7071 7.29289C13.3166 6.90237 12.6834 6.90237 12.2929 7.29289L9 10.5858L7.70711 9.29289C7.31658 8.90237 6.68342 8.90237 6.29289 9.29289C5.90237 9.68342 5.90237 10.3166 6.29289 10.7071L8.29289 12.7071C8.68342 13.0976 9.31658 13.0976 9.70711 12.7071L13.7071 8.70711Z" fill="#84CC16"/>
</svg>DARK THEME 🌔
<svg width="20" height="20" viewBox="0 0 20 20" fill="none" xmlns="http://www.w3.org/2000/svg">
<path fill-rule="evenodd" clip-rule="evenodd" d="M10 18C14.4183 18 18 14.4183 18 10C18 5.58172 14.4183 2 10 2C5.58172 2 2 5.58172 2 10C2 14.4183 5.58172 18 10 18ZM13.7071 8.70711C14.0976 8.31658 14.0976 7.68342 13.7071 7.29289C13.3166 6.90237 12.6834 6.90237 12.2929 7.29289L9 10.5858L7.70711 9.29289C7.31658 8.90237 6.68342 8.90237 6.29289 9.29289C5.90237 9.68342 5.90237 10.3166 6.29289 10.7071L8.29289 12.7071C8.68342 13.0976 9.31658 13.0976 9.70711 12.7071L13.7071 8.70711Z" fill="#F7FEE7"/>
</svg> |
||||||||
| </Alert> | ||||||||
| </Modal> | ||||||||
| ); | ||||||||
| } | ||||||||
| } | ||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| /** | ||
| * Copyright (c) 2022 Gitpod GmbH. All rights reserved. | ||
| * Licensed under the GNU Affero General Public License (AGPL). | ||
| * See License-AGPL.txt in the project root for license information. | ||
| */ | ||
|
|
||
| .country-list { | ||
| width: 29rem !important; | ||
| } | ||
|
|
||
| input[type="tel"], | ||
| .country-list { | ||
| @apply block w-56 text-gray-600 dark:text-gray-400 bg-white dark:bg-gray-800 rounded-md border border-gray-300 dark:border-gray-500 focus:border-gray-400 dark:focus:border-gray-400 focus:ring-0; | ||
| } | ||
|
|
||
| input[type="tel"]::placeholder { | ||
| @apply text-gray-400 dark:text-gray-500; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| /** | ||
| * Copyright (c) 2022 Gitpod GmbH. All rights reserved. | ||
| * Licensed under the GNU Affero General Public License (AGPL). | ||
| * See License-AGPL.txt in the project root for license information. | ||
| */ | ||
|
|
||
| import { MigrationInterface, QueryRunner } from "typeorm"; | ||
| import { columnExists } from "./helper/helper"; | ||
|
|
||
| const D_B_USER = "d_b_user"; | ||
| const COL_VERIFICATIONTIME = "lastVerificationTime"; | ||
| const COL_PHONE_NUMBER = "verificationPhoneNumber"; | ||
|
|
||
| export class PhoneVerification1661519441407 implements MigrationInterface { | ||
| public async up(queryRunner: QueryRunner): Promise<void> { | ||
| if (!(await columnExists(queryRunner, D_B_USER, COL_VERIFICATIONTIME))) { | ||
| await queryRunner.query( | ||
| `ALTER TABLE ${D_B_USER} ADD COLUMN ${COL_VERIFICATIONTIME} varchar(30) NOT NULL DEFAULT '', ALGORITHM=INPLACE, LOCK=NONE `, | ||
| ); | ||
| } | ||
| if (!(await columnExists(queryRunner, D_B_USER, COL_PHONE_NUMBER))) { | ||
| await queryRunner.query( | ||
| `ALTER TABLE ${D_B_USER} ADD COLUMN ${COL_PHONE_NUMBER} varchar(30) NOT NULL DEFAULT '', ALGORITHM=INPLACE, LOCK=NONE `, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| public async down(queryRunner: QueryRunner): Promise<void> {} | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.