Skip to content

Add phone verification #12258

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 29, 2022
Merged

Add phone verification #12258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions components/dashboard/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"query-string": "^7.1.1",
"react": "^17.0.1",
"react-dom": "^17.0.1",
"react-intl-tel-input": "^8.2.0",
"react-router-dom": "^5.2.0",
"xterm": "^4.11.0",
"xterm-addon-fit": "^0.5.0"
Expand Down
1 change: 1 addition & 0 deletions components/dashboard/src/admin/UserDetail.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ export default function UserDetail(p: { user: User }) {
<h3>{user.fullName}</h3>
{user.blocked ? <Label text="Blocked" color="red" /> : null}{" "}
{user.markedDeleted ? <Label text="Deleted" color="red" /> : null}
{user.lastVerificationTime ? <Label text="Verified" color="green" /> : null}
</div>
<p>
{user.identities
Expand Down
9 changes: 9 additions & 0 deletions components/dashboard/src/components/Alert.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@ import { ReactComponent as Exclamation } from "../images/exclamation.svg";
import { ReactComponent as Exclamation2 } from "../images/exclamation2.svg";
import { ReactComponent as InfoSvg } from "../images/info.svg";
import { ReactComponent as XSvg } from "../images/x.svg";
import { ReactComponent as Check } from "../images/check-circle.svg";

export type AlertType =
// Green
| "success"
// Yellow
| "warning"
// Gray alert
Expand Down Expand Up @@ -40,6 +43,12 @@ interface AlertInfo {
}

const infoMap: Record<AlertType, AlertInfo> = {
success: {
bgCls: "bg-green-100 dark:bg-green-800",
txtCls: "text-green-700 dark:text-green-50",
icon: <Check className="w-4 h-4"></Check>,
iconColor: "text-green-700 dark:text-green-100",
},
warning: {
bgCls: "bg-yellow-100 dark:bg-yellow-700",
txtCls: "text-yellow-600 dark:text-yellow-50",
Expand Down
3 changes: 3 additions & 0 deletions components/dashboard/src/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@

textarea,
input[type="text"],
input[type="tel"],
input[type="number"],
input[type="search"],
input[type="password"],
Expand All @@ -87,12 +88,14 @@
}
textarea::placeholder,
input[type="text"]::placeholder,
input[type="tel"]::placeholder,
input[type="number"]::placeholder,
input[type="search"]::placeholder,
input[type="password"]::placeholder {
@apply text-gray-400 dark:text-gray-500;
}
input[type="text"].error,
input[type="tel"].error,
input[type="number"].error,
input[type="search"].error,
input[type="password"].error,
Expand Down
3 changes: 3 additions & 0 deletions components/dashboard/src/start/StartPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@
* See License-AGPL.txt in the project root for license information.
*/

import { ErrorCodes } from "@gitpod/gitpod-protocol/lib/messaging/error";
import { useEffect } from "react";
import Alert from "../components/Alert";
import gitpodIconUA from "../icons/gitpod.svg";
import { gitpodHostUrl } from "../service/service";
import { VerifyModal } from "./VerifyModal";

export enum StartPhase {
Checking = 0,
Expand Down Expand Up @@ -106,6 +108,7 @@ export function StartPage(props: StartPageProps) {
{typeof phase === "number" && phase < StartPhase.IdeReady && (
<ProgressBar phase={phase} error={!!error} />
)}
{error && error.code === ErrorCodes.NEEDS_VERIFICATION && <VerifyModal />}
{error && <StartError error={error} />}
{props.children}
{props.showLatestIdeWarning && (
Expand Down
241 changes: 241 additions & 0 deletions components/dashboard/src/start/VerifyModal.tsx
View file
Open in desktop
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
Copy link
Contributor

@gtsiolis gtsiolis Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

aren't the two limits you posted below what you are asking for here?

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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
To use Gitpod you'll need to validate your account with your phone number. This is required to
To use Gitpod you'll need to validate your account with a mobile phone number. This is required to

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">
Copy link
Contributor

Choose a reason for hiding this comment

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

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. ⏱️

Screenshot 2022-08-24 at 1 36 25 PM

Copy link
Contributor

@gtsiolis gtsiolis Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

issue(non-blocking): I've ran into this error but should be fine to ignore. Twilio probably hates me by now. 😇

Screenshot 2022-08-24 at 2 30 49 PM

{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. */}
<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"
buttons={
<div>
<button className="ml-2" disabled={!isTokenFilled()} onClick={verifyToken}>
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 cursor-not-allowed which is something to consider when we build the button component, see relevant docs.

Validate Account
Copy link
Contributor

Choose a reason for hiding this comment

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

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

<button
className="flex items-center space-x-2"
disabled={isRerunningPrebuild}
onClick={rerunPrebuild}
>
{isRerunningPrebuild && (
<img className="h-4 w-4 animate-spin filter brightness-150" src={Spinner} />
)}
<span>Rerun Prebuild ({prebuild?.info.branch})</span>
</button>

Re-posting the verification in progress design from #11339 (comment) for visibility:

Verification (Progress)
UserValidationModalVerificationInProgress

</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}>
Copy link
Contributor

Choose a reason for hiding this comment

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

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. 💭

&larr; 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>
<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"
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Latest Release on Workspace Start Validation on Workspace Start
Screenshot 2022-08-24 at 2 52 07 PM (2) Screenshot 2022-08-24 at 2 51 18 PM (2)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think an explicit, "you are verified" message with a 'continue' button is clearer. Especially because we are reloading the page.

Copy link
Contributor

@gtsiolis gtsiolis Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Alert / Info (IMPLEMENTED) Alert / Success (NOT IMPLEMENTED) Alert / Success (NOT IMPLEMENTED) 🌔
Frame 381 Frame 382 Frame 385

If you'd like to add this here, here's the icon (SVG) and the colors used for light and dark themes:

LIGHT THEME ⛅

Background: green-100
Icon: green-500
Text: green-700

<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 🌔

Background: green-800
Icon: green-50
Text: green-100

<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>
);
}
}
18 changes: 18 additions & 0 deletions components/dashboard/src/start/phone-input.css
View file
Open in desktop
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;
}
12 changes: 12 additions & 0 deletions components/gitpod-db/src/typeorm/entity/db-user.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,16 @@ export class DBUser implements User {
transformer: Transformer.MAP_EMPTY_STR_TO_UNDEFINED,
})
usageAttributionId?: string;

@Column({
default: "",
transformer: Transformer.MAP_EMPTY_STR_TO_UNDEFINED,
})
lastVerificationTime?: string;

@Column({
default: "",
transformer: Transformer.MAP_EMPTY_STR_TO_UNDEFINED,
})
verificationPhoneNumber?: string;
}
29 changes: 29 additions & 0 deletions components/gitpod-db/src/typeorm/migration/1661519441407-PhoneVerification.ts
View file
Open in desktop
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> {}
}
Loading