[server, db] AuthProviderEntry: Introduce oauthRevision to avoid repeated materialization of encrypted data

geropl · roboquat · commit 07e013ecc280 · 2022-02-11T16:44:33.000+01:00
diff --git a/components/dashboard/src/service/service-mock.ts b/components/dashboard/src/service/service-mock.ts
@@ -192,6 +192,7 @@ const gitpodServiceMock = createServiceMock({
                 "clientId": "clientid-123",
                 "clientSecret": "redacted"
             },
+            "oauthRevision": "some-revision",
             "deleted": false
         }]
     },
diff --git a/components/gitpod-db/BUILD.yaml b/components/gitpod-db/BUILD.yaml
@@ -16,19 +16,8 @@ packages:
   - name: migrations
     type: yarn
     srcs:
-      - "src/typeorm/migration/**/*.ts"
-      - "src/typeorm/migrate-migrations-0_2_0.ts"
-      - "src/typeorm/entity/*.ts"
-      - "src/typeorm/ormconfig.ts"
-      - "src/typeorm/typeorm.ts"
-      - "src/typeorm/naming-strategy.ts"
-      - "src/typeorm/user-db-impl.ts"
-      - "src/typeorm/transformer.ts"
-      - "src/config.ts"
-      - "src/wait-for-db.ts"
-      - "src/migrate-migrations.ts"
-      - "src/user-db.ts"
-      - "package.json"
+      - "src/**/*.ts"
+      - package.json
     deps:
       - components/gitpod-protocol:lib
     config:
@@ -64,6 +53,7 @@ packages:
       - DB_PORT=23306
       - DB_USER=root
       - DB_PASSWORD=test
+      - DB_ENCRYPTION_KEYS=[{"name":"general","version":1,"primary":true,"material":"5vRrp0H4oRgdkPnX1qQcS54Q0xggr6iyho42IQ1rO+c="}]
     ephemeral: true
     config:
       commands:
diff --git a/components/gitpod-db/src/auth-provider-entry-db.ts b/components/gitpod-db/src/auth-provider-entry-db.ts
@@ -5,15 +5,21 @@
  */
 
 import { AuthProviderEntry as AuthProviderEntry } from "@gitpod/gitpod-protocol";
+import { createHash } from "crypto";
 
 export const AuthProviderEntryDB = Symbol('AuthProviderEntryDB');
 
 export interface AuthProviderEntryDB {
-    storeAuthProvider(ap: AuthProviderEntry): Promise<AuthProviderEntry>;
+    storeAuthProvider(ap: AuthProviderEntry, updateOAuthRevision: boolean): Promise<AuthProviderEntry>;
 
     delete(ap: AuthProviderEntry): Promise<void>;
 
-    findAll(): Promise<AuthProviderEntry[]>;
+    findAll(exceptOAuthRevisions?: string[]): Promise<AuthProviderEntry[]>;
+    findAllHosts(): Promise<string[]>;
     findByHost(host: string): Promise<AuthProviderEntry | undefined>;
     findByUserId(userId: string): Promise<AuthProviderEntry[]>;
 }
+
+export function hashOAuth(oauth: AuthProviderEntry["oauth"]): string {
+    return createHash('sha256').update(JSON.stringify(oauth)).digest('hex');
+}
diff --git a/components/gitpod-db/src/auth-provider-entry.spec.db.ts b/components/gitpod-db/src/auth-provider-entry.spec.db.ts
@@ -0,0 +1,94 @@
+/**
+ * Copyright (c) 2022 Gitpod GmbH. All rights reserved.
+ * Licensed under the Gitpod Enterprise Source Code License,
+ * See License.enterprise.txt in the project root folder.
+ */
+
+import * as chai from 'chai';
+import { suite, test, timeout } from 'mocha-typescript';
+import { testContainer } from './test-container';
+import { TypeORM } from './typeorm/typeorm';
+import { AuthProviderEntryDB } from '.';
+import { DBAuthProviderEntry } from './typeorm/entity/db-auth-provider-entry';
+import { DeepPartial } from '@gitpod/gitpod-protocol/lib/util/deep-partial';
+const expect = chai.expect;
+
+@suite @timeout(5000)
+export class AuthProviderEntryDBSpec {
+
+    typeORM = testContainer.get<TypeORM>(TypeORM);
+    db = testContainer.get<AuthProviderEntryDB>(AuthProviderEntryDB);
+
+    async before() {
+        await this.clear();
+    }
+
+    async after() {
+        await this.clear();
+    }
+
+    protected async clear() {
+        const connection = await this.typeORM.getConnection();
+        const manager = connection.manager;
+        await manager.clear(DBAuthProviderEntry);
+    }
+
+    protected authProvider(ap: DeepPartial<DBAuthProviderEntry> = {}): DBAuthProviderEntry {
+        const ownerId = "1234";
+        const host = "github.com";
+        return {
+            id: "0049b9d2-005f-43c2-a0ae-76377805d8b8",
+            host,
+            ownerId,
+            status: 'verified',
+            type: "GitHub",
+            oauthRevision: undefined,
+            deleted: false,
+            ...ap,
+            oauth: {
+                callBackUrl: "example.org/some/callback",
+                authorizationUrl: "example.org/some/auth",
+                settingsUrl: "example.org/settings",
+                configURL: "example.org/config",
+                clientId: "clientId",
+                clientSecret: "clientSecret",
+                tokenUrl: "example.org/get/token",
+                scope: "scope",
+                scopeSeparator: ",",
+                ...ap.oauth,
+                authorizationParams: {},
+            },
+        };
+    }
+
+    @test public async storeEmtpyOAuthRevision() {
+        const ap = this.authProvider();
+        await this.db.storeAuthProvider(ap, false);
+
+        const aap = await this.db.findByHost(ap.host);
+        expect(aap, "AuthProvider").to.deep.equal(ap);
+    }
+
+    @test public async findAll() {
+        const ap1 = this.authProvider({ id: "1", oauthRevision: "rev1" });
+        const ap2 = this.authProvider({ id: "2", oauthRevision: "rev2" });
+        await this.db.storeAuthProvider(ap1, false);
+        await this.db.storeAuthProvider(ap2, false);
+
+        const all = await this.db.findAll();
+        expect(all, "findAll([])").to.deep.equal([ap1, ap2]);
+        expect(await this.db.findAll([ap1.oauthRevision!, ap2.oauthRevision!]), "findAll([ap1, ap2])").to.be.empty;
+        expect(await this.db.findAll([ap1.oauthRevision!]), "findAll([ap1])").to.deep.equal([ap2]);
+    }
+
+    @test public async oauthRevision() {
+        const ap = this.authProvider({ id: "1" });
+        await this.db.storeAuthProvider(ap, true);
+
+        const loadedAp = await this.db.findByHost(ap.host);
+        expect(loadedAp, "findByHost()").to.deep.equal(ap);
+        expect(loadedAp?.oauthRevision, "findByHost()").to.equal("e05ea6fab8efcaba4b3246c2b2d3931af897c3bc2c1cf075c31614f0954f9840");
+    }
+}
+
+module.exports = AuthProviderEntryDBSpec
diff --git a/components/gitpod-db/src/typeorm/auth-provider-entry-db-impl.ts b/components/gitpod-db/src/typeorm/auth-provider-entry-db-impl.ts
@@ -11,6 +11,7 @@ import { AuthProviderEntry } from "@gitpod/gitpod-protocol";
 import { AuthProviderEntryDB } from "../auth-provider-entry-db";
 import { DBAuthProviderEntry } from "./entity/db-auth-provider-entry";
 import { DBIdentity } from "./entity/db-identity";
+import { createHash } from "crypto";
 
 @injectable()
 export class AuthProviderEntryDBImpl implements AuthProviderEntryDB {
@@ -28,8 +29,11 @@ export class AuthProviderEntryDBImpl implements AuthProviderEntryDB {
         return (await this.getEntityManager()).getRepository<DBIdentity>(DBIdentity);
     }
 
-    async storeAuthProvider(ap: AuthProviderEntry): Promise<AuthProviderEntry> {
+    async storeAuthProvider(ap: AuthProviderEntry, updateOAuthRevision: boolean): Promise<AuthProviderEntry> {
         const repo = await this.getAuthProviderRepo();
+        if (updateOAuthRevision) {
+            (ap.oauthRevision as any) = this.oauthContentHash(ap.oauth);
+        }
         return repo.save(ap);
     }
 
@@ -45,13 +49,29 @@ export class AuthProviderEntryDBImpl implements AuthProviderEntryDB {
         await repo.update({ id }, { deleted: true });
     }
 
-    async findAll(): Promise<AuthProviderEntry[]> {
+    async findAll(exceptOAuthRevisions: string[] = []): Promise<AuthProviderEntry[]> {
+        exceptOAuthRevisions = exceptOAuthRevisions.filter(r => r !== "");  // never filter out '' which means "undefined" in the DB
+
         const repo = await this.getAuthProviderRepo();
-        const query = repo.createQueryBuilder('auth_provider')
+        let query = repo.createQueryBuilder('auth_provider')
             .where('auth_provider.deleted != true');
+        if (exceptOAuthRevisions.length > 0) {
+            query = query.andWhere('auth_provider.oauthRevision NOT IN (:...exceptOAuthRevisions)', { exceptOAuthRevisions });
+        }
         return query.getMany();
     }
 
+    async findAllHosts(): Promise<string[]> {
+        const hostField: keyof DBAuthProviderEntry = "host";
+
+        const repo = await this.getAuthProviderRepo();
+        const query = repo.createQueryBuilder('auth_provider')
+            .select(hostField)
+            .where('auth_provider.deleted != true');
+        const result = (await query.execute()) as Pick<DBAuthProviderEntry, "host">[];
+        return result.map(r => r.host);
+    }
+
     async findByHost(host: string): Promise<AuthProviderEntry | undefined> {
         const repo = await this.getAuthProviderRepo();
         const query = repo.createQueryBuilder('auth_provider')
@@ -68,4 +88,8 @@ export class AuthProviderEntryDBImpl implements AuthProviderEntryDB {
         return query.getMany();
     }
 
+    protected oauthContentHash(oauth: AuthProviderEntry["oauth"]): string {
+        const result = createHash('sha256').update(JSON.stringify(oauth)).digest('hex');
+        return result;
+    }
 }
diff --git a/components/gitpod-db/src/typeorm/entity/db-auth-provider-entry.ts b/components/gitpod-db/src/typeorm/entity/db-auth-provider-entry.ts
@@ -4,7 +4,7 @@
  * See License-AGPL.txt in the project root for license information.
  */
 
-import { PrimaryColumn, Column, Entity } from "typeorm";
+import { PrimaryColumn, Column, Entity, Index } from "typeorm";
 import { TypeORM } from "../typeorm";
 import { AuthProviderEntry, OAuth2Config } from "@gitpod/gitpod-protocol";
 import { Transformer } from "../transformer";
@@ -37,6 +37,13 @@ export class DBAuthProviderEntry implements AuthProviderEntry {
     })
     oauth: OAuth2Config;
 
+    @Index("ind_oauthRevision")
+    @Column({
+        default: '',
+        transformer: Transformer.MAP_EMPTY_STR_TO_UNDEFINED,
+    })
+    oauthRevision?: string;
+
     @Column()
     deleted?: boolean;
 }
diff --git a/components/gitpod-db/src/typeorm/migration/1643986994402-OAuthRevision.ts b/components/gitpod-db/src/typeorm/migration/1643986994402-OAuthRevision.ts
@@ -0,0 +1,52 @@
+/**
+ * Copyright (c) 2021 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 { AuthProviderEntry } from "@gitpod/gitpod-protocol";
+import { MigrationInterface, QueryRunner } from "typeorm";
+import { dbContainerModule } from "../../container-module";
+import { columnExists, indexExists } from "./helper/helper";
+import { Container } from 'inversify';
+import { AuthProviderEntryDB } from "../../auth-provider-entry-db";
+import { UserDB } from "../../user-db";
+
+const TABLE_NAME = "d_b_auth_provider_entry";
+const COLUMN_NAME: keyof AuthProviderEntry = "oauthRevision";
+const INDEX_NAME = "ind_oauthRevision";
+
+export class OAuthRevision1643986994402 implements MigrationInterface {
+
+    public async up(queryRunner: QueryRunner): Promise<void> {
+        // create new column
+        if (!(await columnExists(queryRunner, TABLE_NAME, COLUMN_NAME))) {
+            await queryRunner.query(`ALTER TABLE ${TABLE_NAME} ADD COLUMN ${COLUMN_NAME} varchar(128) NOT NULL DEFAULT ''`);
+        }
+
+        // create index on said column
+        if (!(await indexExists(queryRunner, TABLE_NAME, INDEX_NAME))) {
+            await queryRunner.query(`CREATE INDEX ${INDEX_NAME} ON ${TABLE_NAME} (${COLUMN_NAME})`);
+        }
+
+        // to update all oauthRevisions we need to load all providers (to decrypt them) and
+        // write them back using the DB implementation (which does the calculation for us)
+        const container = new Container();
+        container.load(dbContainerModule);
+
+        container.get<UserDB>(UserDB);  // initializes encryptionProvider as side effect
+        const db = container.get<AuthProviderEntryDB>(AuthProviderEntryDB);
+        const allProviders = await db.findAll([]);
+        const writes: Promise<AuthProviderEntry>[] = [];
+        for (const provider of allProviders) {
+            writes.push(db.storeAuthProvider(provider, true));
+        }
+        await Promise.all(writes);
+    }
+
+    public async down(queryRunner: QueryRunner): Promise<void> {
+        await queryRunner.query(`ALTER TABLE ${TABLE_NAME} DROP INDEX ${INDEX_NAME}`);
+        await queryRunner.query(`ALTER TABLE ${TABLE_NAME} DROP COLUMN ${COLUMN_NAME}`);
+    }
+
+}
diff --git a/components/gitpod-protocol/src/protocol.ts b/components/gitpod-protocol/src/protocol.ts
@@ -1171,6 +1171,8 @@ export interface AuthProviderEntry {
     readonly status: AuthProviderEntry.Status;
 
     readonly oauth: OAuth2Config;
+    /** A random string that is to change whenever oauth changes (enforced on DB level) */
+    readonly oauthRevision?: string;
 }
 
 export interface OAuth2Config {
diff --git a/components/server/src/auth/auth-provider-service.ts b/components/server/src/auth/auth-provider-service.ts
@@ -26,8 +26,8 @@ export class AuthProviderService {
     /**
      * Returns all auth providers.
      */
-    async getAllAuthProviders(): Promise<AuthProviderParams[]> {
-        const all = await this.authProviderDB.findAll();
+    async getAllAuthProviders(exceptOAuthRevisions: string[] = []): Promise<AuthProviderParams[]> {
+        const all = await this.authProviderDB.findAll(exceptOAuthRevisions);
         const transformed = all.map(this.toAuthProviderParams.bind(this));
 
         // as a precaution, let's remove duplicates
@@ -43,6 +43,10 @@ export class AuthProviderService {
         return Array.from(unique.values());
     }
 
+    async getAllAuthProviderHosts(): Promise<string[]> {
+        return this.authProviderDB.findAllHosts();
+    }
+
     protected toAuthProviderParams = (oap: AuthProviderEntry) => <AuthProviderParams>{
         ...oap,
         host: oap.host.toLowerCase(),
@@ -82,13 +86,14 @@ export class AuthProviderService {
             }
 
             // update config on demand
+            const oauth = {
+                ...existing.oauth,
+                clientId: entry.clientId,
+                clientSecret: entry.clientSecret || existing.oauth.clientSecret, // FE may send empty ("") if not changed
+            };
             authProvider = {
                 ...existing,
-                oauth: {
-                    ...existing.oauth,
-                    clientId: entry.clientId,
-                    clientSecret: entry.clientSecret || existing.oauth.clientSecret, // FE may send empty ("") if not changed
-                },
+                oauth,
                 status: "pending",
             }
         } else {
@@ -98,24 +103,25 @@ export class AuthProviderService {
             }
             authProvider = this.initializeNewProvider(entry);
         }
-        return await this.authProviderDB.storeAuthProvider(authProvider as AuthProviderEntry);
+        return await this.authProviderDB.storeAuthProvider(authProvider as AuthProviderEntry, true);
     }
     protected initializeNewProvider(newEntry: AuthProviderEntry.NewEntry): AuthProviderEntry {
         const { host, type, clientId, clientSecret } = newEntry;
         const urls = type === "GitHub" ? githubUrls(host) : (type === "GitLab" ? gitlabUrls(host) : undefined);
         if (!urls) {
             throw new Error("Unexpected service type.");
         }
-        return <AuthProviderEntry>{
+        const oauth: AuthProviderEntry["oauth"] = {
+            ...urls,
+            callBackUrl: this.callbackUrl(host),
+            clientId: clientId!,
+            clientSecret: clientSecret!,
+        };
+        return {
             ...newEntry,
             id: uuidv4(),
             type,
-            oauth: {
-                ...urls,
-                callBackUrl: this.callbackUrl(host),
-                clientId,
-                clientSecret,
-            },
+            oauth,
             status: "pending",
         };
     }
@@ -136,7 +142,7 @@ export class AuthProviderService {
                     ownerId: ownerId,
                     status: "verified"
                 };
-                await this.authProviderDB.storeAuthProvider(ap);
+                await this.authProviderDB.storeAuthProvider(ap, true);
             } else {
                 log.warn("Failed to find the AuthProviderEntry to be activated.", { params, id, ap });
             }
diff --git a/components/server/src/auth/host-context-provider-impl.ts b/components/server/src/auth/host-context-provider-impl.ts

Original file line number	Diff line number	Diff line change
`@@ -1171,6 +1171,8 @@ export interface AuthProviderEntry {`
`1171`	`1171`	`readonly status: AuthProviderEntry.Status;`
`1172`	`1172`
`1173`	`1173`	`readonly oauth: OAuth2Config;`
	`1174`	`+ /** A random string that is to change whenever oauth changes (enforced on DB level) */`
	`1175`	`+ readonly oauthRevision?: string;`
`1174`	`1176`	`}`
`1175`	`1177`
`1176`	`1178`	`export interface OAuth2Config {`