Add phone verification #12258

svenefftinge · 2022-08-22T09:14:02Z

Description

This PR adds phone number verification before untrusted users can start a workspace.

A user is considered trusted if

Their GitHub account is older than 30 days, or
Their GitLab account confirmed_at event is older than 30 days, or
Their Gitpod account has been created before 2022-08-24 (i.e. existing users)

Related Issue(s)

Fixes #11339

See also https://github.com/gitpod-io/ops/pull/4149

How to test

Test 1

Create a fresh GitHub account and sign in (use admin to unblock).
You should need to do phone number verification before you can start your first workspace.
Once the number is verified you should be able to work as usual.

Test 2

Use a GitHub account older than 30 days.
You should be able to start a workspace without verification.

Test 1 ++

delete the lastVerificationTimestamp in your user's additional Data.
if the account had started a workspace before, you should not need to verify
if the account doesn't have any workspaces, you should be asked to verify

Release Notes

Added phone number verification in SaaS, to help mitigate abuse.

Documentation

Werft options:

/werft with-preview

svenefftinge · 2022-08-22T09:41:30Z

/werft run with-clean-slate-deployment=true

svenefftinge · 2022-08-22T10:09:20Z

/werft run with-clean-slate-deployment=true

👍 started the job as gitpod-build-sefftinge-add-phone-verification-11339.22
(with .werft/ from main)

gtsiolis

Left some early feedback on the UX changes below. 🥯

components/dashboard/src/start/VerifyModal.tsx

gtsiolis · 2022-08-23T11:52:33Z

components/dashboard/src/start/VerifyModal.tsx

+                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


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

gtsiolis · 2022-08-23T15:07:25Z

Looking at this now! 👀

gtsiolis

@svenefftinge I've left another round of comments. 🥊

I could not go through all the steps of the flow. 😭

components/dashboard/src/start/VerifyModal.tsx

gtsiolis · 2022-08-24T09:49:43Z

Looking at this now! 👀

gtsiolis

Alright! Left a few general comments and questions below. 🔮

Most of them can be ignored or used for opening follow up issues out of the scope of these changes. 🛹

Thanks @svenefftinge for pushing this forward! 🏀

components/dashboard/src/start/VerifyModal.tsx

gtsiolis · 2022-08-24T11:44:12Z

components/dashboard/src/start/VerifyModal.tsx

+                visible={true}
+            >
+                <Alert type="message" className="mt-2">
+                    Your account has been successfully verified.


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

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>

components/dashboard/src/start/VerifyModal.tsx

gtsiolis · 2022-08-24T11:58:23Z

components/dashboard/src/start/VerifyModal.tsx

+                onClose={continueStartWorkspace}
+                closeable={false}
+                onEnter={continueStartWorkspace}
+                title="User Validation Successful"


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

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

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?

components/dashboard/src/start/VerifyModal.tsx

svenefftinge · 2022-08-24T14:39:33Z

I'll hide this behind a feature flag so we can carefully enable it in prod.

svenefftinge · 2022-08-24T18:30:12Z

/werft run

👍 started the job as gitpod-build-sefftinge-add-phone-verification-11339.33
(with .werft/ from main)

corneliusludmann

It seems that this PR does not have changes in Team Self-Hosted owned files anymore. However, from self-hosted perspective, it looks good so far.

‼️ Hint: You should rebase shortly before you merge this and let golden files of the installer render tests be re-generated since there are new tests in main already that will let fail the main build after merging. Sorry for the inconvenience.

AlexTugarev

The account verification works nicely 🎉

I left some comment, and I'm happy to discuss the additionalData comment 👍🏻
Ah, the only real issue so far is that old GH account are not count as verified, because the check is not performed after signup, but rather after the following re-login.

AlexTugarev · 2022-08-25T17:01:35Z

components/server/src/auth/rate-limiter.ts

@@ -51,6 +51,8 @@ function getConfig(config: RateLimiterConfig): RateLimiterConfig {
        getLoggedInUser: { group: "default", points: 1 },
        getTerms: { group: "default", points: 1 },
        updateLoggedInUser: { group: "default", points: 1 },
+        sendPhoneNumberVerificationToken: { group: "default", points: 1 },


does this mean it would be allowed to call it full 1k per interval?
if the external service imposes a rate limit on our registration, we'd be exposed to random DoS attacks, no?
in that case let's introduce a config with a reasonable limitation of several tries per 10s – similar to create workspace and the like.

I guess this also applies to verifyPhoneNumberVerificationToken.

Ah, yes. Probably worth limiting this per user on our side, although there is a rate limit per phone number on twilio as well.

I'm introducing a new category phoneVerification with 10 points per 10 seconds.

AlexTugarev · 2022-08-25T17:28:07Z

components/server/src/user/user-deletion-service.ts

@@ -66,6 +66,9 @@ export class UserDeletionService {
            this.deleteIdentities(user);
            await this.deleteTokens(db, user);
            user.markedDeleted = true;
+            if (user.additionalData) {
+                delete user.additionalData.lastVerificationTime;


I guess we'd need to delete the phone number as well.

I intentionally left it in order to follow up in case the user did something bad.

That we definitely need to check with data privacy. To prevent recreating accounts it would be sufficient to store a hash of the phone number, no?

Ok, I'll store the hash instead and introduce d_b_blocked_phone_hashesthat we update on user block/unblock. Also will check that table on first sign up.

Update: I'm leaving the blocked_phone_hashes out of this PR, as it makes it even bigger (it already is big).
But I'm moving the two properties into d_b_user.

AlexTugarev · 2022-08-25T17:57:56Z

components/server/src/user/user-service.ts

@@ -419,6 +422,11 @@ export class UserService {
        user.name = user.name || authUser.name || authUser.primaryEmail;
        user.avatarUrl = user.avatarUrl || authUser.avatarUrl;

+        if (authUser.created_at && isDateSmaller(authUser.created_at, daysBefore(new Date().toISOString(), 30))) {


updateUserOnLogin is not called on signups. that means, a new Gitpod account for an old GH account is not considered trusted.
for a complete picture, I think this should not be updated on logins (but rather on signups,) otherwise they might just create a pile of GH account and start using them after a while.

Good catch! 🙏

Let's move this check to createUser, that should fix Test 2.

moved it to the callback that is passed to createUser because I needed access to the authUser object. Also looked like a good pace for it otherwise. Fine?

AlexTugarev · 2022-08-25T18:04:19Z

components/server/src/workspace/gitpod-server-impl.ts

        for (const p of allowedFields) {
            if (p in partialUser) {
                (user[p] as any) = partialUser[p];
            }
        }
+        // special treatment of nested additionalData
+        if (partialUser.additionalData) {


Preferably, let's not use a field dedicated for user preferences/settings for something that actually controls the wheels or breaks. That check seems to work fine, I've tried to break without success, but I'd prefer not to have a this guarded places in a field which otherwise is free to change for users.

There is an option that we can use to add field to tables without the downside of locking tables, we just need to add NOT NULL DEFAULT '', ALGORITHM=INPLACE, LOCK=NONE for the alter statement.

As we used this alternative the last time(s) discussed this matter, I'd prefer to stick to one pattern 🙏🏻

Which discussion do you refer to?

To avoid implementing the checks for nested attributes in the additionalData bucket we've moved the attribution override to User in #11522. The validation necessary is to disallow users setting the attribution target to a team id they are not a member of.

AlexTugarev · 2022-08-25T18:06:16Z

components/server/ee/src/billing/entitlement-service.ts


    async mayStartWorkspace(
        user: User,
        date: Date = new Date(),
        runningInstances: Promise<WorkspaceInstance[]>,
    ): Promise<MayStartWorkspaceResult> {
        try {
+            const verification = await this.verificationService.needsVerification(user);


Nice integration! Fits in perfectly.

That reminds me of the ToS flow integration, which intercepted the signup, but it would actually be better to use this kind of hook.

AlexTugarev · 2022-08-25T18:39:11Z

components/server/src/auth/verification-service.ts

+        if (!this.verifyService) {
+            throw new Error("No verification service configured.");
+        }
+        const verification_check = await this.verifyService.verificationChecks.create({


try-catch, otherwise it's handled as internal server error whenever one uses the happy finger on the send button.

I don't get this comment. Why is there always an internal server error?

as this is not wrapped in a ResponseError it'll be logged automatically as an internal server error, thus it's loud on the logs, and it seems not to be rendered properly on frontend, at least I couldn't see twillios rate limiting error rendered on frontend.

It is fine to be loud on the logs. It's not a user error when an error is thrown but something with the system. The error is thrown to frontend and I'll make it so that the user gets feedback about the error.

kylos101 · 2022-08-25T20:24:52Z

👋 @svenefftinge, would it be possible to include bitbucket with this too, or perhaps in a follow-on PR? I ask because of this thread.

svenefftinge · 2022-08-26T02:37:31Z

The logic is opt-out. So all bitbucket users need to verify.

svenefftinge · 2022-08-26T14:50:28Z

/werft run

👍 started the job as gitpod-build-sefftinge-add-phone-verification-11339.37
(with .werft/ from main)

jldec · 2022-08-27T10:52:54Z

/werft run with-clean-slate-deployment=true

👍 started the job as gitpod-build-sefftinge-add-phone-verification-11339.38
(with .werft/ from main)

jldec · 2022-08-27T11:22:56Z

/werft run with-clean-slate-deployment=true

👍 started the job as gitpod-build-sefftinge-add-phone-verification-11339.40
(with .werft/ from main)

AlexTugarev · 2022-08-29T06:55:29Z

components/server/src/user/user-deletion-service.ts

@@ -148,6 +149,9 @@ export class UserDeletionService {
        user.avatarUrl = "deleted-avatarUrl";
        user.fullName = "deleted-fullName";
        user.name = "deleted-Name";
+        if (user.verificationPhoneNumber) {
+            user.verificationPhoneNumber = "deleted-verificationPhoneNumber";


ER_DATA_TOO_LONG: Data too long for column 'verificationPhoneNumber'

we're almost there, just 2 chars too long.

what did you type in?

ah, got it.

That happens on deletion of an account, which actually breaks the process.

deleted-phoneNumber with a length of 20 should work just fine.

AlexTugarev

LGTM!
Tested works as advertised.

Logic for Test 1 ++ isn't there is code, which I think is not needed at all ;-)

svenefftinge requested review from a team August 22, 2022 09:14

roboquat added the release-note label Aug 22, 2022

svenefftinge marked this pull request as draft August 22, 2022 09:14

roboquat added the do-not-merge/work-in-progress label Aug 22, 2022

github-actions bot added team: delivery Issue belongs to the self-hosted team team: webapp Issue belongs to the WebApp team labels Aug 22, 2022

roboquat added the size/XXL label Aug 22, 2022

svenefftinge force-pushed the sefftinge/add-phone-verification-11339 branch 2 times, most recently from a603f6a to 7908ff0 Compare August 22, 2022 09:53

svenefftinge force-pushed the sefftinge/add-phone-verification-11339 branch 3 times, most recently from 971d89a to a87f052 Compare August 23, 2022 10:41

gtsiolis reviewed Aug 23, 2022

View reviewed changes

svenefftinge force-pushed the sefftinge/add-phone-verification-11339 branch 2 times, most recently from 0924951 to 6ecdbfa Compare August 23, 2022 14:03

gtsiolis reviewed Aug 23, 2022

View reviewed changes

svenefftinge force-pushed the sefftinge/add-phone-verification-11339 branch from 6ecdbfa to a3bcdf3 Compare August 24, 2022 08:44

gtsiolis reviewed Aug 24, 2022

View reviewed changes

svenefftinge force-pushed the sefftinge/add-phone-verification-11339 branch 3 times, most recently from b6d31bc to 8fa474f Compare August 24, 2022 14:25

svenefftinge marked this pull request as ready for review August 24, 2022 14:38

roboquat removed the do-not-merge/work-in-progress label Aug 24, 2022

svenefftinge force-pushed the sefftinge/add-phone-verification-11339 branch from 8fa474f to 979777c Compare August 24, 2022 17:00

AlexTugarev self-assigned this Aug 25, 2022

corneliusludmann approved these changes Aug 25, 2022

View reviewed changes

AlexTugarev reviewed Aug 25, 2022

View reviewed changes

svenefftinge force-pushed the sefftinge/add-phone-verification-11339 branch 2 times, most recently from 8e00464 to 3dc9ac6 Compare August 26, 2022 14:24

jldec mentioned this pull request Aug 27, 2022

Add Phone Verification for new Users #11339

Closed

AlexTugarev reviewed Aug 29, 2022

View reviewed changes

Add phone verification

1044b42

svenefftinge force-pushed the sefftinge/add-phone-verification-11339 branch from 3dc9ac6 to 1044b42 Compare August 29, 2022 07:17

AlexTugarev approved these changes Aug 29, 2022

View reviewed changes

roboquat merged commit 79b75ab into main Aug 29, 2022

roboquat deleted the sefftinge/add-phone-verification-11339 branch August 29, 2022 07:36

roboquat added the deployed: webapp Meta team change is running in production label Aug 29, 2022

gtsiolis mentioned this pull request Sep 11, 2022

Add link to support on phone verification #12852

Merged

1 task

jldec mentioned this pull request Sep 12, 2022

Limit phone number re-use for verification #12883

Closed

gtsiolis mentioned this pull request Feb 16, 2023

Extract button component #16441

Closed

	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

Add phone verification #12258

Add phone verification #12258

Conversation

svenefftinge commented Aug 22, 2022 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

Werft options:

svenefftinge commented Aug 22, 2022

svenefftinge commented Aug 22, 2022 • edited by werft-gitpod-dev-com bot Loading

gtsiolis left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtsiolis commented Aug 23, 2022 • edited by werft-gitpod-dev-com bot Loading

gtsiolis left a comment

Choose a reason for hiding this comment

gtsiolis commented Aug 24, 2022 • edited by werft-gitpod-dev-com bot Loading

gtsiolis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LIGHT THEME ⛅

DARK THEME 🌔

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtsiolis Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

svenefftinge commented Aug 24, 2022

svenefftinge commented Aug 24, 2022 • edited by werft-gitpod-dev-com bot Loading

corneliusludmann left a comment

Choose a reason for hiding this comment

AlexTugarev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexTugarev Aug 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylos101 commented Aug 25, 2022

svenefftinge commented Aug 26, 2022

svenefftinge commented Aug 26, 2022 • edited by werft-gitpod-dev-com bot Loading

jldec commented Aug 27, 2022 • edited by werft-gitpod-dev-com bot Loading

jldec commented Aug 27, 2022 • edited by werft-gitpod-dev-com bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexTugarev left a comment

Choose a reason for hiding this comment

svenefftinge commented Aug 22, 2022 •

edited

Loading

svenefftinge commented Aug 22, 2022 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis left a comment •

edited

Loading

gtsiolis commented Aug 23, 2022 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis commented Aug 24, 2022 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis Aug 24, 2022 •

edited

Loading

svenefftinge commented Aug 24, 2022 •

edited by werft-gitpod-dev-com bot

Loading

AlexTugarev Aug 26, 2022 •

edited

Loading

svenefftinge commented Aug 26, 2022 •

edited by werft-gitpod-dev-com bot

Loading

jldec commented Aug 27, 2022 •

edited by werft-gitpod-dev-com bot

Loading

jldec commented Aug 27, 2022 •

edited by werft-gitpod-dev-com bot

Loading