From b1828a2f274b5a4f997dfba142ef574f17dbc5c9 Mon Sep 17 00:00:00 2001 From: Matti Nannt Date: Thu, 18 Sep 2025 12:24:58 +0200 Subject: [PATCH] feat: support IAM role authentication for S3 storage - Update IS_STORAGE_CONFIGURED to only require S3_REGION and S3_BUCKET_NAME - Make S3_ACCESS_KEY and S3_SECRET_KEY optional in S3 client creation - Allow AWS SDK to use IAM roles, instance profiles, and credential chains - Maintain backward compatibility with explicit credentials and MinIO - Update tests to reflect new IAM role authentication behavior BREAKING CHANGE: IS_STORAGE_CONFIGURED now returns true with only region and bucket configured --- apps/web/lib/constants.ts | 5 +- packages/storage/src/client.test.ts | 124 +++++++++++++++++++++------- packages/storage/src/client.ts | 24 ++++-- 3 files changed, 116 insertions(+), 37 deletions(-) diff --git a/apps/web/lib/constants.ts b/apps/web/lib/constants.ts index 7090d93292..82cc979b36 100644 --- a/apps/web/lib/constants.ts +++ b/apps/web/lib/constants.ts @@ -114,7 +114,10 @@ export const MAX_FILE_UPLOAD_SIZES = { standard: 1024 * 1024 * 10, // 10MB big: 1024 * 1024 * 1024, // 1GB } as const; -export const IS_STORAGE_CONFIGURED = Boolean(S3_ACCESS_KEY && S3_SECRET_KEY && S3_REGION && S3_BUCKET_NAME); +// Storage is considered configured if we have the minimum required settings: +// - S3_REGION and S3_BUCKET_NAME are always required +// - S3_ACCESS_KEY and S3_SECRET_KEY are optional (for IAM role-based authentication) +export const IS_STORAGE_CONFIGURED = Boolean(S3_REGION && S3_BUCKET_NAME); // Colors for Survey Bg export const SURVEY_BG_COLORS = [ diff --git a/packages/storage/src/client.test.ts b/packages/storage/src/client.test.ts index d90aaa6bda..d8745c3713 100644 --- a/packages/storage/src/client.test.ts +++ b/packages/storage/src/client.test.ts @@ -82,8 +82,8 @@ describe("client.ts", () => { } }); - test("should return error when access key is missing", async () => { - // Mock constants with missing access key + test("should create S3 client without credentials (IAM role authentication)", async () => { + // Mock constants with missing access key (IAM role scenario) vi.doMock("./constants", () => ({ ...mockConstants, S3_ACCESS_KEY: undefined, @@ -93,14 +93,20 @@ describe("client.ts", () => { const result = createS3ClientFromEnv(); - expect(result.ok).toBe(false); - if (!result.ok) { - expect(result.error.code).toBe("s3_credentials_error"); + expect(mockS3Client).toHaveBeenCalledWith({ + region: mockConstants.S3_REGION, + endpoint: mockConstants.S3_ENDPOINT_URL, + forcePathStyle: mockConstants.S3_FORCE_PATH_STYLE, + }); + + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.data).toBeDefined(); } }); - test("should return error when secret key is missing", async () => { - // Mock constants with missing secret key + test("should create S3 client without secret key (IAM role authentication)", async () => { + // Mock constants with missing secret key (IAM role scenario) vi.doMock("./constants", () => ({ ...mockConstants, S3_SECRET_KEY: undefined, @@ -110,14 +116,20 @@ describe("client.ts", () => { const result = createS3ClientFromEnv(); - expect(result.ok).toBe(false); - if (!result.ok) { - expect(result.error.code).toBe("s3_credentials_error"); + expect(mockS3Client).toHaveBeenCalledWith({ + region: mockConstants.S3_REGION, + endpoint: mockConstants.S3_ENDPOINT_URL, + forcePathStyle: mockConstants.S3_FORCE_PATH_STYLE, + }); + + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.data).toBeDefined(); } }); - test("should return error when both credentials are missing", async () => { - // Mock constants with no credentials + test("should create S3 client without any credentials (IAM role authentication)", async () => { + // Mock constants with no credentials (full IAM role scenario) vi.doMock("./constants", () => ({ ...mockConstants, S3_ACCESS_KEY: undefined, @@ -128,14 +140,20 @@ describe("client.ts", () => { const result = createS3ClientFromEnv(); - expect(result.ok).toBe(false); - if (!result.ok) { - expect(result.error.code).toBe("s3_credentials_error"); + expect(mockS3Client).toHaveBeenCalledWith({ + region: mockConstants.S3_REGION, + endpoint: mockConstants.S3_ENDPOINT_URL, + forcePathStyle: mockConstants.S3_FORCE_PATH_STYLE, + }); + + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.data).toBeDefined(); } }); - test("should return error when credentials are empty strings", async () => { - // Mock constants with empty string credentials + test("should create S3 client with empty string credentials (IAM role authentication)", async () => { + // Mock constants with empty string credentials (treated as undefined) vi.doMock("./constants", () => ({ ...mockConstants, S3_ACCESS_KEY: "", @@ -146,14 +164,20 @@ describe("client.ts", () => { const result = createS3ClientFromEnv(); - expect(result.ok).toBe(false); - if (!result.ok) { - expect(result.error.code).toBe("s3_credentials_error"); + expect(mockS3Client).toHaveBeenCalledWith({ + region: mockConstants.S3_REGION, + endpoint: mockConstants.S3_ENDPOINT_URL, + forcePathStyle: mockConstants.S3_FORCE_PATH_STYLE, + }); + + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.data).toBeDefined(); } }); - test("should return error when mixed empty and undefined credentials", async () => { - // Mock constants with mixed empty and undefined + test("should create S3 client with mixed empty and undefined credentials (IAM role authentication)", async () => { + // Mock constants with mixed empty and undefined (both treated as missing) vi.doMock("./constants", () => ({ ...mockConstants, S3_ACCESS_KEY: "", @@ -164,9 +188,15 @@ describe("client.ts", () => { const result = createS3ClientFromEnv(); - expect(result.ok).toBe(false); - if (!result.ok) { - expect(result.error.code).toBe("s3_credentials_error"); + expect(mockS3Client).toHaveBeenCalledWith({ + region: mockConstants.S3_REGION, + endpoint: mockConstants.S3_ENDPOINT_URL, + forcePathStyle: mockConstants.S3_FORCE_PATH_STYLE, + }); + + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.data).toBeDefined(); } }); @@ -197,6 +227,40 @@ describe("client.ts", () => { } }); + test("should return error when region is missing", async () => { + // Mock constants with missing region + vi.doMock("./constants", () => ({ + ...mockConstants, + S3_REGION: undefined, + })); + + const { createS3ClientFromEnv } = await import("./client"); + + const result = createS3ClientFromEnv(); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.error.code).toBe("s3_credentials_error"); + } + }); + + test("should return error when bucket name is missing", async () => { + // Mock constants with missing bucket name + vi.doMock("./constants", () => ({ + ...mockConstants, + S3_BUCKET_NAME: undefined, + })); + + const { createS3ClientFromEnv } = await import("./client"); + + const result = createS3ClientFromEnv(); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.error.code).toBe("s3_credentials_error"); + } + }); + test("should return unknown error when S3Client constructor throws", async () => { // Provide valid credentials so we reach the constructor path vi.doMock("./constants", () => ({ @@ -254,11 +318,11 @@ describe("client.ts", () => { }); test("should return undefined when creating from env fails and no client provided", async () => { - // Mock constants with missing credentials + // Mock constants with missing required fields (region and bucket) vi.doMock("./constants", () => ({ ...mockConstants, - S3_ACCESS_KEY: undefined, - S3_SECRET_KEY: undefined, + S3_REGION: undefined, + S3_BUCKET_NAME: undefined, })); const { createS3Client } = await import("./client"); @@ -290,8 +354,8 @@ describe("client.ts", () => { test("returns undefined when env is invalid and does not construct client", async () => { vi.doMock("./constants", () => ({ ...mockConstants, - S3_ACCESS_KEY: undefined, - S3_SECRET_KEY: undefined, + S3_REGION: undefined, + S3_BUCKET_NAME: undefined, })); const { getCachedS3Client } = await import("./client"); diff --git a/packages/storage/src/client.ts b/packages/storage/src/client.ts index 7b7268cf12..b1cdf33f94 100644 --- a/packages/storage/src/client.ts +++ b/packages/storage/src/client.ts @@ -1,4 +1,4 @@ -import { S3Client } from "@aws-sdk/client-s3"; +import { S3Client, type S3ClientConfig } from "@aws-sdk/client-s3"; import { logger } from "@formbricks/logger"; import { type Result, type StorageError, StorageErrorCode, err, ok } from "../types/error"; import { @@ -19,19 +19,31 @@ let cachedS3Client: S3Client | undefined; */ export const createS3ClientFromEnv = (): Result => { try { - if (!S3_ACCESS_KEY || !S3_SECRET_KEY || !S3_BUCKET_NAME || !S3_REGION) { - logger.error("S3 Client: S3 credentials are not set"); + // S3_REGION and S3_BUCKET_NAME are always required + if (!S3_BUCKET_NAME || !S3_REGION) { + logger.error("S3 Client: S3_REGION and S3_BUCKET_NAME are required"); return err({ code: StorageErrorCode.S3CredentialsError, }); } - const s3ClientInstance = new S3Client({ - credentials: { accessKeyId: S3_ACCESS_KEY, secretAccessKey: S3_SECRET_KEY }, + // Build S3 client configuration + const s3Config: S3ClientConfig = { region: S3_REGION, endpoint: S3_ENDPOINT_URL, forcePathStyle: S3_FORCE_PATH_STYLE, - }); + }; + + // Only add credentials if both access key and secret key are provided + // This allows the AWS SDK to use IAM roles, instance profiles, or other credential providers + if (S3_ACCESS_KEY && S3_SECRET_KEY) { + s3Config.credentials = { + accessKeyId: S3_ACCESS_KEY, + secretAccessKey: S3_SECRET_KEY, + }; + } + + const s3ClientInstance = new S3Client(s3Config); return ok(s3ClientInstance); } catch (error) {