From 6f720cbfe78f3d8f49d378b24ac2f2038b2d4ec6 Mon Sep 17 00:00:00 2001 From: kenneth Date: Sun, 12 Apr 2026 13:50:11 +0000 Subject: [PATCH] fix: unified source config + credentials Accept optional credentials in PUT /api/sources/:sourceId so the dashboard can send config and credentials in a single request, eliminating the race condition between parallel config/credential updates that left sources uninitialized until server restart. The existing /credentials endpoint is preserved for independent credential updates. Co-authored-by: Ona --- .../src/components/source-config-panel.tsx | 30 ++--- apps/admin-dashboard/src/lib/api.ts | 4 +- .../src/session/user-session-manager.test.ts | 121 ++++++++++++++++++ .../src/session/user-session-manager.ts | 28 +++- apps/aelis-backend/src/sources/http.test.ts | 36 ++++++ apps/aelis-backend/src/sources/http.ts | 8 +- 6 files changed, 201 insertions(+), 26 deletions(-) diff --git a/apps/admin-dashboard/src/components/source-config-panel.tsx b/apps/admin-dashboard/src/components/source-config-panel.tsx index ed5f63f..25790f0 100644 --- a/apps/admin-dashboard/src/components/source-config-panel.tsx +++ b/apps/admin-dashboard/src/components/source-config-panel.tsx @@ -20,13 +20,7 @@ import { import { Separator } from "@/components/ui/separator" import { Switch } from "@/components/ui/switch" import { Tooltip, TooltipContent, TooltipTrigger } from "@/components/ui/tooltip" -import { - fetchSourceConfig, - pushLocation, - replaceSource, - updateProviderConfig, - updateSourceCredentials, -} from "@/lib/api" +import { fetchSourceConfig, pushLocation, replaceSource, updateProviderConfig } from "@/lib/api" interface SourceConfigPanelProps { source: SourceDefinition @@ -80,23 +74,21 @@ export function SourceConfigPanel({ source, onUpdate }: SourceConfigPanelProps) const saveMutation = useMutation({ mutationFn: async () => { - const promises: Promise[] = [ - replaceSource(source.id, { enabled, config: getUserConfig() }), - ] - const credentialFields = getCredentialFields() const hasCredentials = Object.values(credentialFields).some( (v) => typeof v === "string" && v.length > 0, ) - if (hasCredentials) { - if (source.perUserCredentials) { - promises.push(updateSourceCredentials(source.id, credentialFields)) - } else { - promises.push(updateProviderConfig(source.id, { credentials: credentialFields })) - } - } - await Promise.all(promises) + await replaceSource(source.id, { + enabled, + config: getUserConfig(), + ...(hasCredentials && source.perUserCredentials ? { credentials: credentialFields } : {}), + }) + + // For non-per-user credentials (provider-level), still use the admin endpoint. + if (hasCredentials && !source.perUserCredentials) { + await updateProviderConfig(source.id, { credentials: credentialFields }) + } }, onSuccess() { setDirty({}) diff --git a/apps/admin-dashboard/src/lib/api.ts b/apps/admin-dashboard/src/lib/api.ts index d47d19a..fd57a2c 100644 --- a/apps/admin-dashboard/src/lib/api.ts +++ b/apps/admin-dashboard/src/lib/api.ts @@ -114,7 +114,7 @@ const sourceDefinitions: SourceDefinition[] = [ timeZone: { type: "string", label: "Timezone", - description: "IANA timezone for determining \"today\" (e.g. Europe/London). Defaults to UTC.", + description: 'IANA timezone for determining "today" (e.g. Europe/London). Defaults to UTC.', }, }, }, @@ -174,7 +174,7 @@ export async function fetchConfigs(): Promise { export async function replaceSource( sourceId: string, - body: { enabled: boolean; config: unknown }, + body: { enabled: boolean; config: unknown; credentials?: Record }, ): Promise { const res = await fetch(`${serverBase()}/sources/${sourceId}`, { method: "PUT", diff --git a/apps/aelis-backend/src/session/user-session-manager.test.ts b/apps/aelis-backend/src/session/user-session-manager.test.ts index 5d08924..7f6f167 100644 --- a/apps/aelis-backend/src/session/user-session-manager.test.ts +++ b/apps/aelis-backend/src/session/user-session-manager.test.ts @@ -81,6 +81,9 @@ mock.module("../sources/user-sources.ts", () => ({ updatedAt: now, } }, + async upsertConfig(_sourceId: string, _data: { enabled: boolean; config: unknown }) { + // no-op for tests + }, async updateCredentials(sourceId: string, credentials: Buffer) { if (mockUpdateCredentialsError) { throw mockUpdateCredentialsError @@ -824,3 +827,121 @@ describe("UserSessionManager.updateSourceCredentials", () => { expect(factory).not.toHaveBeenCalled() }) }) + +describe("UserSessionManager.upsertSourceConfig", () => { + test("upserts config without credentials (existing behavior)", async () => { + setEnabledSources(["test"]) + const factory = mock(async () => createStubSource("test")) + const provider: FeedSourceProvider = { sourceId: "test", feedSourceForUser: factory } + const manager = new UserSessionManager({ + db: fakeDb, + providers: [provider], + credentialEncryptor: testEncryptor, + }) + + // Create a session first so we can verify the source is refreshed + await manager.getOrCreate("user-1") + + await manager.upsertSourceConfig("user-1", "test", { + enabled: true, + config: { key: "value" }, + }) + + // feedSourceForUser called once for session creation, once for upsert refresh + expect(factory).toHaveBeenCalledTimes(2) + // No credentials should have been persisted + expect(mockUpdateCredentialsCalls).toHaveLength(0) + }) + + test("upserts config with credentials — persists both and passes credentials to source", async () => { + setEnabledSources(["test"]) + let receivedCredentials: unknown = null + const factory = mock(async (_userId: string, _config: unknown, creds: unknown) => { + receivedCredentials = creds + return createStubSource("test") + }) + const provider: FeedSourceProvider = { sourceId: "test", feedSourceForUser: factory } + const manager = new UserSessionManager({ + db: fakeDb, + providers: [provider], + credentialEncryptor: testEncryptor, + }) + + // Create a session so the source refresh path runs + await manager.getOrCreate("user-1") + + const creds = { username: "alice", password: "s3cret" } + await manager.upsertSourceConfig("user-1", "test", { + enabled: true, + config: { serverUrl: "https://example.com" }, + credentials: creds, + }) + + // Credentials were encrypted and persisted + expect(mockUpdateCredentialsCalls).toHaveLength(1) + const decrypted = JSON.parse(testEncryptor.decrypt(mockUpdateCredentialsCalls[0]!.credentials)) + expect(decrypted).toEqual(creds) + + // feedSourceForUser received the provided credentials (not null) + expect(receivedCredentials).toEqual(creds) + }) + + test("upserts config with credentials adds source to session when not already present", async () => { + // Start with no enabled sources so the session is empty + setEnabledSources([]) + const factory = mock(async () => createStubSource("test")) + const provider: FeedSourceProvider = { sourceId: "test", feedSourceForUser: factory } + const manager = new UserSessionManager({ + db: fakeDb, + providers: [provider], + credentialEncryptor: testEncryptor, + }) + + const session = await manager.getOrCreate("user-1") + expect(session.hasSource("test")).toBe(false) + + // Set mockFindResult to undefined so find() returns a row (simulating the row was just created by upsertConfig) + await manager.upsertSourceConfig("user-1", "test", { + enabled: true, + config: {}, + credentials: { token: "abc" }, + }) + + // Source should now be in the session + expect(session.hasSource("test")).toBe(true) + expect(mockUpdateCredentialsCalls).toHaveLength(1) + }) + + test("throws CredentialStorageUnavailableError when credentials provided without encryptor", async () => { + setEnabledSources(["test"]) + const provider = createStubProvider("test") + const manager = new UserSessionManager({ + db: fakeDb, + providers: [provider], + // No credentialEncryptor + }) + + await expect( + manager.upsertSourceConfig("user-1", "test", { + enabled: true, + config: {}, + credentials: { token: "abc" }, + }), + ).rejects.toBeInstanceOf(CredentialStorageUnavailableError) + }) + + test("throws SourceNotFoundError for unknown provider", async () => { + const manager = new UserSessionManager({ + db: fakeDb, + providers: [], + credentialEncryptor: testEncryptor, + }) + + await expect( + manager.upsertSourceConfig("user-1", "unknown", { + enabled: true, + config: {}, + }), + ).rejects.toBeInstanceOf(SourceNotFoundError) + }) +}) diff --git a/apps/aelis-backend/src/session/user-session-manager.ts b/apps/aelis-backend/src/session/user-session-manager.ts index d9dcc86..10db38d 100644 --- a/apps/aelis-backend/src/session/user-session-manager.ts +++ b/apps/aelis-backend/src/session/user-session-manager.ts @@ -11,6 +11,7 @@ import type { FeedSourceProvider } from "./feed-source-provider.ts" import { CredentialStorageUnavailableError, InvalidSourceConfigError, + InvalidSourceCredentialsError, SourceNotFoundError, } from "../sources/errors.ts" import { sources } from "../sources/user-sources.ts" @@ -171,13 +172,18 @@ export class UserSessionManager { * inserts a new row if one doesn't exist and fully replaces config * (no merge). * + * When `credentials` is provided, they are encrypted and persisted + * alongside the config in the same flow, avoiding the race condition + * of separate config + credential requests. + * * @throws {SourceNotFoundError} if the sourceId has no registered provider * @throws {InvalidSourceConfigError} if config fails schema validation + * @throws {CredentialStorageUnavailableError} if credentials are provided but no encryptor is configured */ async upsertSourceConfig( userId: string, sourceId: string, - data: { enabled: boolean; config?: unknown }, + data: { enabled: boolean; config?: unknown; credentials?: unknown }, ): Promise { const provider = this.providers.get(sourceId) if (!provider) { @@ -191,6 +197,10 @@ export class UserSessionManager { } } + if (data.credentials !== undefined && !this.encryptor) { + throw new CredentialStorageUnavailableError() + } + const config = data.config ?? {} // Fetch existing row before upsert to capture credentials for session refresh. @@ -202,14 +212,24 @@ export class UserSessionManager { config, }) + // Persist credentials after the upsert so the row exists. + if (data.credentials !== undefined && this.encryptor) { + const encrypted = this.encryptor.encrypt(JSON.stringify(data.credentials)) + await sources(this.db, userId).updateCredentials(sourceId, encrypted) + } + const session = this.sessions.get(userId) if (session) { if (!data.enabled) { session.removeSource(sourceId) } else { - const credentials = existingRow?.credentials - ? this.decryptCredentials(existingRow.credentials) - : null + // Prefer the just-provided credentials over what was in the DB. + const credentials = + data.credentials !== undefined + ? data.credentials + : existingRow?.credentials + ? this.decryptCredentials(existingRow.credentials) + : null const source = await provider.feedSourceForUser(userId, config, credentials) if (session.hasSource(sourceId)) { session.replaceSource(sourceId, source) diff --git a/apps/aelis-backend/src/sources/http.test.ts b/apps/aelis-backend/src/sources/http.test.ts index 95af829..ec20799 100644 --- a/apps/aelis-backend/src/sources/http.test.ts +++ b/apps/aelis-backend/src/sources/http.test.ts @@ -738,6 +738,42 @@ describe("PUT /api/sources/:sourceId", () => { expect(res.status).toBe(204) }) + + test("returns 204 when credentials are included alongside config", async () => { + activeStore = createInMemoryStore() + const { app } = createAppWithEncryptor( + [createStubProvider("aelis.weather", weatherConfig)], + MOCK_USER_ID, + ) + + const res = await put(app, "aelis.weather", { + enabled: true, + config: { units: "metric" }, + credentials: { apiKey: "secret123" }, + }) + + expect(res.status).toBe(204) + const row = activeStore.rows.get(`${MOCK_USER_ID}:aelis.weather`) + expect(row).toBeDefined() + expect(row!.enabled).toBe(true) + expect(row!.config).toEqual({ units: "metric" }) + }) + + test("returns 503 when credentials are provided but no encryptor is configured", async () => { + activeStore = createInMemoryStore() + // createApp does NOT configure an encryptor + const { app } = createApp([createStubProvider("aelis.weather", weatherConfig)], MOCK_USER_ID) + + const res = await put(app, "aelis.weather", { + enabled: true, + config: { units: "metric" }, + credentials: { apiKey: "secret123" }, + }) + + expect(res.status).toBe(503) + const body = (await res.json()) as { error: string } + expect(body.error).toContain("not configured") + }) }) describe("PUT /api/sources/:sourceId/credentials", () => { diff --git a/apps/aelis-backend/src/sources/http.ts b/apps/aelis-backend/src/sources/http.ts index 76e4c61..5406a59 100644 --- a/apps/aelis-backend/src/sources/http.ts +++ b/apps/aelis-backend/src/sources/http.ts @@ -34,11 +34,13 @@ const ReplaceSourceConfigRequestBody = type({ "+": "reject", enabled: "boolean", config: "unknown", + "credentials?": "unknown", }) const ReplaceSourceConfigNoConfigRequestBody = type({ "+": "reject", enabled: "boolean", + "credentials?": "unknown", }) export function registerSourcesHttpHandlers( @@ -161,7 +163,7 @@ async function handleReplaceSource(c: Context) { return c.json({ error: parsed.summary }, 400) } - const { enabled } = parsed + const { enabled, credentials } = parsed const config = "config" in parsed ? parsed.config : undefined const user = c.get("user")! @@ -169,6 +171,7 @@ async function handleReplaceSource(c: Context) { await sessionManager.upsertSourceConfig(user.id, sourceId, { enabled, config, + credentials, }) } catch (err) { if (err instanceof SourceNotFoundError) { @@ -177,6 +180,9 @@ async function handleReplaceSource(c: Context) { if (err instanceof InvalidSourceConfigError) { return c.json({ error: err.message }, 400) } + if (err instanceof CredentialStorageUnavailableError) { + return c.json({ error: err.message }, 503) + } throw err }