Add phone verification #11346

svenefftinge · 2022-07-13T12:24:28Z

Description

Adds phone number verification to make abusing Gitpod harder

Related Issue(s)

Fixes #11339

How to test

Release Notes

Documentation

Werft options:

/werft with-preview

roboquat · 2022-07-13T12:24:30Z

@svenefftinge: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

werft-gitpod-dev-com · 2022-07-13T12:24:51Z

started the job as gitpod-build-sefftinge-add-phone-verification-11339.1 because the annotations in the pull request description changed
(with .werft/ from main)

components/server/src/config.ts

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

geropl · 2022-07-14T09:04:39Z

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

+        if (!user.additionalData) {
+            user.additionalData = {};
+        }
+        if (!user.additionalData?.profile) {


nit: this cascade could be wrapped in a User.updateProfile(partial: Partial<User.Profile>) method.

geropl · 2022-07-14T09:05:24Z

install/installer/pkg/components/server/deployment.go

+
+			volumes = append(volumes,
+				corev1.Volume{
+					Name: "stripe-config",


copy-n-paste: twilio-config

stale · 2022-07-30T17:56:08Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

gitguardian · 2022-08-09T08:38:42Z

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.

^{_{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!}}

svenefftinge · 2022-08-19T14:28:49Z

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

+        if (!this.config.twilioConfig) {
+            return false;
+        }
+        // if (!!user.additionalData?.lastVerificationTime) {


I have disabled this code, so it is easier to test multiple times. After a first review I would enable the code again to test the functionality.

svenefftinge · 2022-08-19T14:30:19Z

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

+    //     // we only want to ask new users, so if a user has ever started workspaces before, we trust them.
+    //     // there might be a few false positives because all workspaces might have been deleted.
+    //     const workspaces = await this.workspaceDB.findWorkspacesByUser(user.id);
+    //     return workspaces.length !== 0;


@geropl I think it makes sense to check for usage instead?

Jep, that could work as an indicator, with a threshold > what we see from abusers.

lucasvaltl · 2022-08-20T14:23:09Z

I'm assuming this is only "turned on" in SaaS?

geropl · 2022-08-22T06:17:55Z

/hold Due to this comment.

@svenefftinge While (un-)commenting this part of the code is not harmful we (WebApp) decided after a recent incident that we hide all "dev commits" behind Feature Flags: either the whole feature, or the "dev" part.

geropl · 2022-08-22T06:18:10Z

Starting to review now.

svenefftinge · 2022-08-22T06:20:23Z

Thanks for the hold. FWIW the logic for "trusted" if going to get a bit more complex as we don't want to ask every new user. I'll try to judge trustworthiness also based on age of GitHub/GitLab account. The rest is ready for review.

svenefftinge · 2022-08-22T06:20:52Z

/werft run

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

geropl · 2022-08-22T06:20:59Z

FWIW the logic for "trusted" if going to get a bit more complex as we don't want to ask every new user.

Jep! I guess that's also something we need/want to fine-tune as we go.

svenefftinge · 2022-08-22T06:22:11Z

I'm spinning up a preview env and will update the env var on the server deployment, so trillio works.

geropl · 2022-08-22T06:43:53Z

components/gitpod-protocol/src/util/logging.ts

@@ -6,7 +6,7 @@

 const inspect: (object: any) => string = require("util").inspect; // undefined in frontend

-let plainLogging: boolean = false; // set to true during development to get non JSON output
+let plainLogging: boolean = true; // set to true during development to get non JSON output


This should also be part of the "dev commit". Ideally, we should not need this, at all.

☁️ I think we had a de-jsonifier in the past. Could we use that together with yarn telepresence to avoid the need for this code change? Or maybe set an env var, or similar?

Yes, I'll remove it as well, of course.

geropl · 2022-08-22T06:46:35Z

components/gitpod-protocol/src/gitpod-service.ts

@@ -85,6 +85,9 @@ export interface GitpodServer extends JsonRpcServer<GitpodClient>, AdminServer,
    getLoggedInUser(): Promise<User>;
    getTerms(): Promise<Terms>;
    updateLoggedInUser(user: Partial<User>): Promise<User>;
+    needsVerification(): Promise<boolean>;


Do we need the method needsVerification()? I don't see it being used in the frontend (anymore). Feels like a leftover from an ealier version?

geropl · 2022-08-22T06:47:32Z

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


geropl · 2022-08-22T06:50:22Z

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

+    }
+
+    public async sendPhoneNumberVerificationToken(ctx: TraceContext, phoneNumber: string): Promise<void> {
+        this.checkUser("sendPhoneNumberVerificationToken");


This whole concern feels like it should be part of the EE code-base, and this methods should be empty stubs (like

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

Line 2744 in 7f2ab4d

throw new ResponseError(ErrorCodes.EE_FEATURE, `Admin support is implemented in Gitpod's Enterprise Edition`);

).

What is the reasoning you believe it should be EE? I have no concerns about making this open-source as I don't see how it could be something we want to monetize.

Mainly because I thought it would not be that interesting. But have no hard feelings here!

💡 Then the binding should move to the other container: #11346 (comment)

We should align on a general strategy / view so this is easier to decide for everyone. My take is to put as much as possible into the open-source version, so we have as little concerns as possible in the EE layer which makes code harder to grasp.

My take is to put as much as possible into the open-source version

That has been our strategy so far, yes, and I agree that it applies to phone verification as well. Put it into the "billing" basket at first, but it's unrelated. 💯 . Sorry for creating confusion here. 🧘

geropl · 2022-08-22T06:51:39Z

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

+import { UserDB, WorkspaceDB } from "@gitpod/gitpod-db/lib";
+
+@injectable()
+export class VerificationService {


Currently this class is part of the community edition, but only bound in the EE container.

As mentioned in GitpodServerImpl, I think it should move to the EE side entirely.

geropl · 2022-08-22T06:52:19Z

components/server/ee/src/container-module.ts

@@ -86,6 +87,7 @@ export const productionEEContainerModule = new ContainerModule((bind, unbind, is
    bind(BitbucketAppSupport).toSelf().inSingletonScope();
    bind(GitHubEnterpriseApp).toSelf().inSingletonScope();
    bind(BitbucketServerApp).toSelf().inSingletonScope();
+    bind(VerificationService).toSelf().inSingletonScope();


This was it's only bound for the EE edition. See other comments maybe adjust.

geropl · 2022-08-22T06:55:26Z

components/server/src/config.ts

@@ -240,6 +253,22 @@ export namespace ConfigFile {
                log.error("Could not load Stripe config", error);
            }
        }
+        let twilioConfig: Config["twilioConfig"];
+        log.info("loading twilio config");
+        if (!!env.TWILIO_CONFIG) {


Why the fallback with the env var? If it's for dev purposes, I think the same can be achieved with pushing and manipulating a Secret.

☁️ We invested a bit of effort last year to move to a central config file which is far easier to reason about (not part of config file? Does not influence the config).

svenefftinge · 2022-08-22T06:58:00Z

I have updated the server deployment, but noticed that there are still styling issues with the phone input. Strangely they are not there when running in dev mode 🙄

geropl · 2022-08-22T07:47:45Z

I have updated the server deployment, but noticed that there are still styling issues with the phone input.

Reminds me of last week, where a werft job "missed" to pick-up the "with-preview" config: build was green, but was still looking at old code.

Update: last job worked, though.

roboquat added the do-not-merge/work-in-progress label Jul 13, 2022

roboquat added do-not-merge/release-note-label-needed size/L labels Jul 13, 2022

gtsiolis mentioned this pull request Jul 13, 2022

Add Phone Verification for new Users #11339

Closed

svenefftinge force-pushed the sefftinge/add-phone-verification-11339 branch 2 times, most recently from 6d50167 to b1626e3 Compare July 14, 2022 08:42

geropl reviewed Jul 14, 2022

View reviewed changes

components/server/src/config.ts Outdated Show resolved Hide resolved

geropl reviewed Jul 14, 2022

View reviewed changes

components/server/src/auth/phone-verification-service.ts Outdated Show resolved Hide resolved

geropl reviewed Jul 14, 2022

View reviewed changes

svenefftinge force-pushed the sefftinge/add-phone-verification-11339 branch from b1626e3 to 783f480 Compare July 14, 2022 17:25

roboquat added size/XL and removed size/L labels Jul 14, 2022

stale bot added the meta: stale This issue/PR is stale and will be closed soon label Jul 30, 2022

svenefftinge removed the meta: stale This issue/PR is stale and will be closed soon label Aug 8, 2022

svenefftinge force-pushed the sefftinge/add-phone-verification-11339 branch from 783f480 to 8d2c434 Compare August 9, 2022 07:09

roboquat added size/XXL and removed size/XL labels Aug 9, 2022

svenefftinge force-pushed the sefftinge/add-phone-verification-11339 branch 2 times, most recently from 59c58c3 to cb6fefd Compare August 9, 2022 08:38

svenefftinge force-pushed the sefftinge/add-phone-verification-11339 branch 2 times, most recently from c214fa3 to e6664fc Compare August 16, 2022 07:21

roboquat added size/XL and removed size/XXL labels Aug 16, 2022

svenefftinge force-pushed the sefftinge/add-phone-verification-11339 branch 2 times, most recently from f157d5d to 887c0a2 Compare August 19, 2022 13:22

svenefftinge requested review from a team August 19, 2022 14:27

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

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

svenefftinge commented Aug 19, 2022

View reviewed changes

mrsimonemms approved these changes Aug 19, 2022

View reviewed changes

svenefftinge commented Aug 19, 2022

View reviewed changes

svenefftinge force-pushed the sefftinge/add-phone-verification-11339 branch from 806714c to 7f2ab4d Compare August 19, 2022 14:44

roboquat added the do-not-merge/hold label Aug 22, 2022

geropl reviewed Aug 22, 2022

View reviewed changes

svenefftinge merged commit 135bc0f into main Aug 22, 2022

svenefftinge force-pushed the sefftinge/add-phone-verification-11339 branch from 7f2ab4d to 135bc0f Compare August 22, 2022 08:43

svenefftinge deleted the sefftinge/add-phone-verification-11339 branch August 22, 2022 08:43

svenefftinge mentioned this pull request Aug 22, 2022

Add phone verification #12258

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add phone verification #11346

Add phone verification #11346

svenefftinge commented Jul 13, 2022 •

edited

Loading

roboquat commented Jul 13, 2022

werft-gitpod-dev-com bot commented Jul 13, 2022

geropl Jul 14, 2022

geropl Jul 14, 2022

stale bot commented Jul 30, 2022

gitguardian bot commented Aug 9, 2022 •

edited

Loading

svenefftinge Aug 19, 2022 •

edited

Loading

svenefftinge Aug 19, 2022

geropl Aug 19, 2022

lucasvaltl commented Aug 20, 2022

geropl commented Aug 22, 2022

geropl commented Aug 22, 2022

svenefftinge commented Aug 22, 2022

svenefftinge commented Aug 22, 2022 •

edited by werft-gitpod-dev-com bot

Loading

geropl commented Aug 22, 2022

svenefftinge commented Aug 22, 2022

geropl Aug 22, 2022

svenefftinge Aug 22, 2022

geropl Aug 22, 2022

geropl Aug 22, 2022

geropl Aug 22, 2022

svenefftinge Aug 22, 2022

geropl Aug 22, 2022

svenefftinge Aug 22, 2022

geropl Aug 22, 2022

geropl Aug 22, 2022

geropl Aug 22, 2022

geropl Aug 22, 2022

svenefftinge commented Aug 22, 2022 •

edited by werft-gitpod-dev-com bot

Loading

geropl commented Aug 22, 2022

Add phone verification #11346

Add phone verification #11346

Conversation

svenefftinge commented Jul 13, 2022 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

Werft options:

roboquat commented Jul 13, 2022

werft-gitpod-dev-com bot commented Jul 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jul 30, 2022

gitguardian bot commented Aug 9, 2022 • edited Loading

️✅ There are no secrets present in this pull request anymore.

svenefftinge Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasvaltl commented Aug 20, 2022

geropl commented Aug 22, 2022

geropl commented Aug 22, 2022

svenefftinge commented Aug 22, 2022

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

geropl commented Aug 22, 2022

svenefftinge commented Aug 22, 2022

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

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

geropl commented Aug 22, 2022

svenefftinge commented Jul 13, 2022 •

edited

Loading

gitguardian bot commented Aug 9, 2022 •

edited

Loading

svenefftinge Aug 19, 2022 •

edited

Loading

svenefftinge commented Aug 22, 2022 •

edited by werft-gitpod-dev-com bot

Loading

svenefftinge commented Aug 22, 2022 •

edited by werft-gitpod-dev-com bot

Loading