[audit log] Fix audit log persistence (#20054)

* [audit] Catch + log errors properly * [audit] Properly serialize BigInts * [public-api] Fix PublicApiConverter toAuditLog by re-using the BigIntToJson.replacer
gitpod-io · Jul 22, 2024 · a03a28e · a03a28e
1 parent 9089924
commit a03a28e
Show file tree

Hide file tree

Showing 8 changed files with 73 additions and 12 deletions.
diff --git a/components/gitpod-db/src/typeorm/entity/db-audit-log.ts b/components/gitpod-db/src/typeorm/entity/db-audit-log.ts
@@ -7,6 +7,8 @@
 import { Entity, Column, PrimaryColumn } from "typeorm";
 import { TypeORM } from "../typeorm";
 import { AuditLog } from "@gitpod/gitpod-protocol/lib/audit-log";
+import { BigIntToJson } from "@gitpod/gitpod-protocol/lib/util/stringify";
+import { Transformer } from "../transformer";
 
 @Entity()
 export class DBAuditLog implements AuditLog {
@@ -25,6 +27,9 @@ export class DBAuditLog implements AuditLog {
     @Column("varchar")
     action: string;
 
-    @Column("simple-json")
+    @Column({
+        type: "text", // it's JSON on DB, but we aim to disable the Typeorm-internal JSON-transformer we can't control and can't disable otherwise
+        transformer: Transformer.SIMPLE_JSON_CUSTOM([], BigIntToJson.replacer, BigIntToJson.reviver),
+    })
     args: object[];
 }
diff --git a/components/gitpod-db/src/typeorm/transformer.ts b/components/gitpod-db/src/typeorm/transformer.ts
@@ -81,6 +81,22 @@ export namespace Transformer {
         };
     };
 
+    export const SIMPLE_JSON_CUSTOM = (
+        defaultValue: any,
+        replacer?: (this: any, key: string, value: any) => any,
+        reviver?: (this: any, key: string, value: any) => any,
+    ) => {
+        return <ValueTransformer>{
+            to(value: any): any {
+                return JSON.stringify(value || defaultValue, replacer);
+            },
+            from(value: any): any {
+                // eslint-disable-next-line @typescript-eslint/no-unsafe-argument
+                return JSON.parse(value, reviver);
+            },
+        };
+    };
+
     export const encrypted = (encryptionServiceProvider: () => EncryptionService): ValueTransformer => {
         return {
             to(value: any): any {

diff --git a/components/gitpod-protocol/src/util/stringify.ts b/components/gitpod-protocol/src/util/stringify.ts
@@ -0,0 +1,22 @@
+/**
+ * Copyright (c) 2024 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.
+ */
+
+export namespace BigIntToJson {
+    export const BIGINT_KEY = "bigint:";
+    export function replacer(key: string, value: any) {
+        if (typeof value === "bigint") {
+            return `${BIGINT_KEY}${value.toString()}`;
+        }
+        return value;
+    }
+    export function reviver(key: string, value: any) {
+        if (typeof value === "string" && value.startsWith(BIGINT_KEY)) {
+            const v: string = value.substring(7);
+            return BigInt(v);
+        }
+        return value;
+    }
+}
diff --git a/components/public-api/typescript-common/src/public-api-converter.ts b/components/public-api/typescript-common/src/public-api-converter.ts
@@ -159,6 +159,7 @@ import {
     WorkspaceStatus_PrebuildResult,
     WorkspaceStatus_WorkspaceConditions,
 } from "@gitpod/public-api/lib/gitpod/v1/workspace_pb";
+import { BigIntToJson } from "@gitpod/gitpod-protocol/lib/util/stringify";
 import { getPrebuildLogPath } from "./prebuild-utils";
 import { InvalidGitpodYMLError, RepositoryNotFoundError, UnauthorizedRepositoryAccessError } from "./public-api-errors";
 const URL = require("url").URL || window.URL;
@@ -1604,7 +1605,7 @@ export class PublicAPIConverter {
             actorId: input.actorId,
             action: input.action,
             timestamp: this.toTimestamp(input.timestamp),
-            args: JSON.stringify(input.args),
+            args: JSON.stringify(input.args, BigIntToJson.replacer),
         });
     }
 }
diff --git a/components/server/src/api/server.ts b/components/server/src/api/server.ts
@@ -306,15 +306,11 @@ export class API {
                                     const promise = await apply<Promise<any>>();
                                     const result = await promise;
                                     if (subjectId) {
-                                        try {
-                                            self.auditLogService.recordAuditLog(
-                                                subjectId!.userId()!,
-                                                requestContext.requestMethod,
-                                                [args[0]],
-                                            );
-                                        } catch (error) {
-                                            log.error("Failed to record audit log", error);
-                                        }
+                                        self.auditLogService.asyncRecordAuditLog(
+                                            subjectId!.userId()!,
+                                            requestContext.requestMethod,
+                                            [args[0]],
+                                        );
                                     }
                                     done();
                                     return result;

diff --git a/components/server/src/audit/AuditLogService.spec.db.ts b/components/server/src/audit/AuditLogService.spec.db.ts
@@ -15,6 +15,7 @@ import { createTestContainer } from "../test/service-testing-container-module";
 import { UserService } from "../user/user-service";
 import { AuditLogService } from "./AuditLogService";
 import { Experiments } from "@gitpod/gitpod-protocol/lib/experiments/configcat-server";
+import { Timestamp } from "@bufbuild/protobuf";
 
 describe("AuditLogService", () => {
     let container: Container;
@@ -94,6 +95,22 @@ describe("AuditLogService", () => {
         expect(logs.length).to.eq(1);
     });
 
+    it("should record audit log containing timestamp args (like listWorkspaceSessions)", async () => {
+        const t1 = new Timestamp({ seconds: 100n, nanos: 0 });
+        await auditLogService.recordAuditLog(owner.id, "action1", [
+            {
+                organizationId: org.id,
+            },
+            t1,
+        ]);
+
+        const logs = await auditLogService.listAuditLogs(owner.id, org.id);
+
+        expect(logs.length).to.eq(1);
+        expect(logs[0].args.length).to.eq(2);
+        expect(logs[0].args[1]).to.deep.equal(t1);
+    });
+
     it("should list audit logs", async () => {
         await auditLogDB.recordAuditLog({
             id: v4(),

diff --git a/components/server/src/audit/AuditLogService.ts b/components/server/src/audit/AuditLogService.ts
@@ -23,6 +23,10 @@ export class AuditLogService {
         @inject(UserDB) private readonly dbUser: UserDB,
     ) {}
 
+    asyncRecordAuditLog(actorId: string, method: string, args: any[]): void {
+        this.recordAuditLog(actorId, method, args).catch((err) => log.error("failed to record audit log", err));
+    }
+
     async recordAuditLog(actorId: string, method: string, args: any[]): Promise<void> {
         if (
             !(await getExperimentsClientForBackend().getValueAsync("audit_logs", false, {

diff --git a/components/server/src/websocket/websocket-connection-manager.ts b/components/server/src/websocket/websocket-connection-manager.ts
@@ -404,7 +404,7 @@ class GitpodJsonRpcProxyFactory<T extends object> extends JsonRpcProxyFactory<T>
                     const result = await this.internalOnRequest(span, method, ...args);
                     if (userId) {
                         // omit the last argument, which is the cancellation token
-                        this.auditLogService.recordAuditLog(userId, method, args.slice(0, -1));
+                        this.auditLogService.asyncRecordAuditLog(userId, method, args.slice(0, -1));
                     }
                     return result;
                 } finally {