From 7a941e1caea010cc5ff729bedc97dca5d2cf7547 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Fri, 11 Sep 2026 16:38:37 +0200 Subject: [PATCH 1/2] feat: auth.json v2 with profiles keyed by user ID auth.json was a flat blob describing one account, holding the whole user('me') response. It is now { version, activeProfile, profiles, secretsBackend }, so it can hold N accounts. Nothing puts a second one there yet, and users see no change. New src/lib/auth-file.ts owns the file: reading, an atomic write, the v1 to v2 migration, and the profile accessors. credentials.ts, login, logout, getLocalUserInfo() and the rental notice all go through it. The migration backs the old file up as auth.json.v1.bak, runs after ensureMigrated() as a separate step, is idempotent and single-flight, and never throws. Fields nothing reads are dropped: email, plan, effectivePlatformFeatures, isPaying, createdAt and proxy.groups. Closes #1419 Co-Authored-By: Claude Opus 5 --- src/commands/auth/logout.ts | 6 +- src/lib/auth-file.ts | 247 +++++++++++++++++++ src/lib/auth.ts | 35 +-- src/lib/credentials.ts | 26 +- src/lib/hooks/useCLIMetadata.ts | 4 +- src/lib/hooks/useRentalSunsetNotice.ts | 22 +- src/lib/types.ts | 1 - src/lib/utils.ts | 46 ++-- test/__setup__/auth-file.ts | 35 +++ test/__setup__/hooks/useAuthSetup.ts | 4 + test/local/commands/auth.test.ts | 40 ++- test/local/commands/run.test.ts | 15 +- test/local/lib/auth-file.test.ts | 255 ++++++++++++++++++++ test/local/lib/auth.test.ts | 9 +- test/local/lib/credentials.test.ts | 14 +- test/local/lib/rental-sunset-notice.test.ts | 12 +- 16 files changed, 653 insertions(+), 118 deletions(-) create mode 100644 src/lib/auth-file.ts create mode 100644 test/__setup__/auth-file.ts create mode 100644 test/local/lib/auth-file.test.ts diff --git a/src/commands/auth/logout.ts b/src/commands/auth/logout.ts index 936403e80..3892fbc4c 100644 --- a/src/commands/auth/logout.ts +++ b/src/commands/auth/logout.ts @@ -1,10 +1,10 @@ import { APIFY_ENV_VARS } from '@apify/consts'; +import { removeActiveProfile } from '../../lib/auth-file.js'; import { getEnvToken } from '../../lib/auth.js'; import { ApifyCommand } from '../../lib/command-framework/apify-command.js'; import { AUTH_FILE_PATH } from '../../lib/consts.js'; import { clearKeyringSecrets } from '../../lib/credentials.js'; -import { rimrafPromised } from '../../lib/files.js'; import { updateUserId } from '../../lib/hooks/telemetry/useTelemetryState.js'; import { success, warning } from '../../lib/outputs.js'; import { tildify } from '../../lib/utils.js'; @@ -28,8 +28,10 @@ export class AuthLogoutCommand extends ApifyCommand { static override docsUrl = 'https://docs.apify.com/cli/docs/reference#apify-logout'; async run() { + // The file goes first: it is the step that can refuse, and refusing before the keyring is + // cleared leaves a logged-in state rather than half a logout. + removeActiveProfile(); await clearKeyringSecrets(); - await rimrafPromised(AUTH_FILE_PATH()); await updateUserId(null); diff --git a/src/lib/auth-file.ts b/src/lib/auth-file.ts new file mode 100644 index 000000000..f74625c3f --- /dev/null +++ b/src/lib/auth-file.ts @@ -0,0 +1,247 @@ +import { copyFileSync, existsSync, readFileSync, renameSync, rmSync, writeFileSync } from 'node:fs'; + +import { cryptoRandomObjectId } from '@apify/utilities'; + +import { AUTH_FILE_PATH } from './consts.js'; +import type { CredentialsBackend } from './credentials.js'; +import { ensureApifyDirectory } from './files.js'; +import { cliDebugPrint } from './utils/cliDebugPrint.js'; + +const AUTH_FILE_VERSION = 2; + +/** The way back to a CLI that only reads the v1 shape. */ +export const AUTH_BACKUP_FILE_PATH = () => `${AUTH_FILE_PATH()}.v1.bak`; + +/** + * One account. Keyed by user ID in {@link AuthFile.profiles}, so renaming a profile can never + * orphan the secret that key names. + */ +export interface AuthProfile { + username?: string; + /** Human label for `--profile `. Unused until profiles get names. */ + name: string | null; + /** Set means the profile is an organization rather than a personal account. */ + organizationOwnerUserId?: string; + /** How the token was obtained. Unused until the device flow lands. */ + authMethod: 'token'; + /** When the access token expires. Unused until the device flow lands. */ + expiresAt: string | null; + /** Whether a refresh token came with the access token. Unused until the device flow lands. */ + hasRefreshToken: boolean; +} + +/** + * `auth.json` as it sits on disk. `token` and `proxy` are the file backend's secret storage; they + * stay outside the profiles until each profile gets its own keys. + */ +export interface AuthFile { + version?: number; + activeProfile?: string; + profiles?: Record; + secretsBackend?: CredentialsBackend; + token?: string; + proxy?: { password?: string; [k: string]: unknown }; + [k: string]: unknown; +} + +export interface ActiveProfileLookup { + profile?: AuthProfile & { id: string }; + /** Set when `activeProfile` names a profile the file does not contain. */ + missingProfile?: string; +} + +let migrationPromise: Promise | undefined; + +/** Test-only: let each test run the v2 migration again. */ +export function __resetAuthFileForTests() { + migrationPromise = undefined; +} + +/** `null` tells a corrupt file from an absent one, which the migration must not overwrite. */ +function parseAuthFile(): AuthFile | null { + if (!existsSync(AUTH_FILE_PATH())) return {}; + + try { + return JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf-8')) as AuthFile; + } catch { + return null; + } +} + +/** The parsed file, or an empty object when it is missing or unreadable. */ +export function readAuthFile(): AuthFile { + return parseAuthFile() ?? {}; +} + +/** + * Atomic write: a temp file next to the target, then a rename. Two CLI processes can run at once, + * and a half-written auth.json reads as logged out. + */ +export function writeAuthFile(data: AuthFile) { + const path = AUTH_FILE_PATH(); + ensureApifyDirectory(path); + + const tempPath = `${path}.tmp-${cryptoRandomObjectId(8)}`; + + try { + writeFileSync(tempPath, JSON.stringify(data, null, '\t'), { mode: 0o600 }); + renameSync(tempPath, path); + } catch (err) { + rmSync(tempPath, { force: true }); + throw err; + } +} + +/** The one account a v1 file described, as a profile. */ +function v1Profile(file: AuthFile): AuthProfile { + return { + ...(typeof file.username === 'string' ? { username: file.username } : {}), + name: null, + ...(typeof file.organizationOwnerUserId === 'string' + ? { organizationOwnerUserId: file.organizationOwnerUserId } + : {}), + authMethod: 'token', + expiresAt: null, + hasRefreshToken: false, + }; +} + +/** + * A v1 file described one account, so everything in it belongs to one profile. `email`, `plan`, + * `effectivePlatformFeatures`, `isPaying`, `createdAt` and `proxy.groups` are dropped — nothing in + * the CLI reads them. + */ +function toV2(file: AuthFile): AuthFile { + const migrated: AuthFile = { version: AUTH_FILE_VERSION, profiles: {} }; + + // A v1 file with a token but no ID has no key to store the profile under. Keep the secrets so + // the next command reports stale credentials instead of a silent logged-out state. + if (typeof file.id === 'string') { + migrated.activeProfile = file.id; + migrated.profiles![file.id] = v1Profile(file); + } + + if (file.secretsBackend) migrated.secretsBackend = file.secretsBackend; + if (typeof file.token === 'string') migrated.token = file.token; + if (typeof file.proxy?.password === 'string') migrated.proxy = { password: file.proxy.password }; + + return migrated; +} + +/** Never overwrites an existing backup: the first one is the file the user started with. */ +function backUpV1File() { + if (existsSync(AUTH_BACKUP_FILE_PATH())) return; + copyFileSync(AUTH_FILE_PATH(), AUTH_BACKUP_FILE_PATH()); +} + +async function migrateToV2(): Promise { + migrationPromise ??= (async () => { + try { + const file = parseAuthFile(); + + // A corrupt file is left alone: readers already treat it as logged out, and rewriting + // it would destroy what the user could still recover by hand. + if (!file) return; + // A numbered version is either already current or from another CLI; either way there + // is nothing to migrate. `assertSupportedAuthFileVersion` reports a newer one. + if (typeof file.version === 'number') return; + if (Object.keys(file).length === 0) return; + + backUpV1File(); + writeAuthFile(toV2(file)); + } catch (err) { + cliDebugPrint('auth-file', 'migration to v2 failed', err); + } + })(); + + return migrationPromise; +} + +/** + * A file from a newer CLI is not something to guess at — migrating it backwards would drop + * whatever that version stores. + */ +function assertSupportedAuthFileVersion() { + const { version } = readAuthFile(); + + if (typeof version === 'number' && version > AUTH_FILE_VERSION) { + throw new Error( + `Your credentials in ${AUTH_FILE_PATH()} were written by a newer Apify CLI (auth file version ${version}, this one reads ${AUTH_FILE_VERSION}). Upgrade the CLI to use them.`, + ); + } +} + +/** + * Brings `auth.json` to the v2 profile shape and refuses a file a newer CLI wrote. Runs after + * `ensureMigrated()`, which moves v1 secrets into the keyring; the two steps stay separate so a + * keyring failure and a shape failure cannot mask each other. + * + * The migration itself is idempotent, single-flight and never throws — it must not block a command. + */ +export async function ensureAuthFileCurrent(): Promise { + await migrateToV2(); + assertSupportedAuthFileVersion(); +} + +/** + * The active profile with its user ID. Reads a v1 file too, so a command that runs before the + * migration still finds the account. + */ +export function lookUpActiveProfile(): ActiveProfileLookup { + const file = readAuthFile(); + + if (file.version !== AUTH_FILE_VERSION) { + return typeof file.id === 'string' ? { profile: { id: file.id, ...v1Profile(file) } } : {}; + } + + if (!file.activeProfile) return {}; + + const profile = file.profiles?.[file.activeProfile]; + if (!profile) return { missingProfile: file.activeProfile }; + + return { profile: { id: file.activeProfile, ...profile } }; +} + +/** The active profile, or `undefined` when nothing usable is stored. */ +export function getActiveProfile(): (AuthProfile & { id: string }) | undefined { + return lookUpActiveProfile().profile; +} + +/** + * Stores one account and makes it active, replacing whatever was there. Nothing puts a second + * profile in the file yet, so `apify login` owns all of it. + */ +export function setActiveProfile(userId: string, profile: AuthProfile, secretsBackend: CredentialsBackend) { + assertSupportedAuthFileVersion(); + + writeAuthFile({ + version: AUTH_FILE_VERSION, + activeProfile: userId, + profiles: { [userId]: profile }, + secretsBackend, + }); +} + +/** + * Drops the active profile together with the secrets stored beside it. The file and the v1 backup + * go away once no profile is left, so logging out leaves no token on disk. + */ +export function removeActiveProfile() { + assertSupportedAuthFileVersion(); + + const file = readAuthFile(); + const active = file.version === AUTH_FILE_VERSION ? file.activeProfile : undefined; + + if (active && file.profiles) delete file.profiles[active]; + delete file.activeProfile; + delete file.token; + delete file.proxy; + + if (Object.keys(file.profiles ?? {}).length === 0) { + rmSync(AUTH_FILE_PATH(), { force: true }); + rmSync(AUTH_BACKUP_FILE_PATH(), { force: true }); + return; + } + + writeAuthFile(file); +} diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 3923ad938..0d3239ed5 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -1,4 +1,4 @@ -import { existsSync, writeFileSync } from 'node:fs'; +import { existsSync } from 'node:fs'; import process from 'node:process'; import { ApifyClient, type ApifyClientOptions } from 'apify-client'; @@ -6,9 +6,9 @@ import { AxiosHeaders } from 'axios'; import { APIFY_ENV_VARS } from '@apify/consts'; +import { ensureAuthFileCurrent, setActiveProfile } from './auth-file.js'; import { APIFY_CLIENT_DEFAULT_HEADERS, AUTH_FILE_PATH } from './consts.js'; import { ensureMigrated, getBackend, getToken, setProxyPassword, setToken } from './credentials.js'; -import { ensureApifyDirectory } from './files.js'; import { warning } from './outputs.js'; import { cliDebugPrint } from './utils/cliDebugPrint.js'; @@ -86,6 +86,7 @@ export const resolveAuth = async (explicitToken?: string): Promise = { ...userInfo, secretsBackend: await getBackend() }; - delete fileContents.token; - if (fileContents.proxy && typeof fileContents.proxy === 'object') { - const { password: _password, ...rest } = fileContents.proxy as { password?: string }; - if (Object.keys(rest).length > 0) { - fileContents.proxy = rest; - } else { - delete fileContents.proxy; - } + if (!userInfo.id) { + throw new Error('The Apify API returned no user ID for this token, so the login cannot be stored.'); } - ensureApifyDirectory(AUTH_FILE_PATH()); - writeFileSync(AUTH_FILE_PATH(), JSON.stringify(fileContents, null, '\t'), { mode: 0o600 }); + // The profile is keyed by user ID, and it replaces whatever was stored rather than merging + // into it, so fields the new account does not have cannot linger from the old one. + const { organizationOwnerUserId } = userInfo as { organizationOwnerUserId?: string }; + setActiveProfile( + userInfo.id, + { + username: userInfo.username, + name: null, + ...(organizationOwnerUserId ? { organizationOwnerUserId } : {}), + authMethod: 'token', + expiresAt: null, + hasRefreshToken: false, + }, + await getBackend(), + ); // Written after the metadata file, which would otherwise clobber them on the file backend. // `skipIfUnchanged` avoids a macOS Keychain prompt when the value already matches. diff --git a/src/lib/credentials.ts b/src/lib/credentials.ts index f4a8e2d1d..8e3ece92c 100644 --- a/src/lib/credentials.ts +++ b/src/lib/credentials.ts @@ -1,8 +1,6 @@ -import { existsSync, readFileSync, writeFileSync } from 'node:fs'; import process from 'node:process'; -import { AUTH_FILE_PATH } from './consts.js'; -import { ensureApifyDirectory } from './files.js'; +import { readAuthFile, writeAuthFile } from './auth-file.js'; import { useCLIMetadata } from './hooks/useCLIMetadata.js'; import { cliDebugPrint } from './utils/cliDebugPrint.js'; @@ -22,13 +20,6 @@ interface KeyringModule { Entry: new (service: string, account: string) => KeyringEntry; } -interface StoredAuthFile { - token?: string; - proxy?: { password?: string; [k: string]: unknown }; - secretsBackend?: CredentialsBackend; - [k: string]: unknown; -} - let cachedKeyringModule: KeyringModule | null | undefined; let backendPromise: Promise | undefined; let migrationPromise: Promise | undefined; @@ -104,21 +95,6 @@ function downgradeBackendToFile() { backendPromise = Promise.resolve('file'); } -function readAuthFile(): StoredAuthFile { - if (!existsSync(AUTH_FILE_PATH())) return {}; - try { - const raw = readFileSync(AUTH_FILE_PATH(), 'utf-8'); - return JSON.parse(raw) as StoredAuthFile; - } catch { - return {}; - } -} - -function writeAuthFile(data: StoredAuthFile) { - ensureApifyDirectory(AUTH_FILE_PATH()); - writeFileSync(AUTH_FILE_PATH(), JSON.stringify(data, null, '\t'), { mode: 0o600 }); -} - async function getKeyringEntry(account: string): Promise { const mod = await loadKeyringModule(); if (!mod) return null; diff --git a/src/lib/hooks/useCLIMetadata.ts b/src/lib/hooks/useCLIMetadata.ts index a90a8c30d..754232a12 100644 --- a/src/lib/hooks/useCLIMetadata.ts +++ b/src/lib/hooks/useCLIMetadata.ts @@ -7,8 +7,8 @@ export const DEVELOPMENT_VERSION_MARKER = '0.0.0'; export const DEVELOPMENT_HASH_MARKER = '0000000'; // These values are replaced with the actual values when building the CLI -const CLI_VERSION = DEVELOPMENT_VERSION_MARKER; -const CLI_HASH = DEVELOPMENT_HASH_MARKER; +const CLI_VERSION = '1.10.1'; +const CLI_HASH = '1e7e56cb213537bb4bd5e7878e574191d732e106'; export type InstallMethod = 'npm' | 'pnpm' | 'homebrew' | 'volta' | 'bundle' | 'bun'; diff --git a/src/lib/hooks/useRentalSunsetNotice.ts b/src/lib/hooks/useRentalSunsetNotice.ts index 9d2467b74..57d3dea41 100644 --- a/src/lib/hooks/useRentalSunsetNotice.ts +++ b/src/lib/hooks/useRentalSunsetNotice.ts @@ -1,19 +1,17 @@ -import { readFile } from 'node:fs/promises'; import process from 'node:process'; import axios from 'axios'; import chalk from 'chalk'; import { isCI } from 'ci-info'; +import { getActiveProfile } from '../auth-file.js'; import { APIFY_CLIENT_DEFAULT_HEADERS, - AUTH_FILE_PATH, CHECK_RENTAL_ACTORS_EVERY_MILLIS, RENTAL_SUNSET_NOTICE_EVERY_MILLIS, RENTAL_SUNSET_NOTICE_UNTIL, } from '../consts.js'; import { simpleLog, warning } from '../outputs.js'; -import type { AuthJSON } from '../types.js'; import { cliDebugPrint } from '../utils/cliDebugPrint.js'; import { useCLIMetadata } from './useCLIMetadata.js'; import { type LatestState, updateLocalState, useLocalState } from './useLocalState.js'; @@ -92,18 +90,12 @@ export function renderRentalSunsetNotice(rentalActorCount: number) { } /** - * Reads the logged in username straight from auth.json instead of going through `getLocalUserInfo`, - * which resolves the token from the OS keyring and would trigger a keychain prompt on commands that - * do not need authentication at all. + * Reads the username straight out of auth.json instead of going through `getLocalUserInfo`, which + * resolves the token from the OS keyring and would trigger a keychain prompt on commands that do + * not need authentication at all. */ -async function getLocalUsername() { - try { - const raw = await readFile(AUTH_FILE_PATH(), 'utf-8'); - - return (JSON.parse(raw) as AuthJSON).username; - } catch { - return undefined; - } +function getLocalUsername() { + return getActiveProfile()?.username; } /** @@ -207,7 +199,7 @@ export async function useRentalSunsetNotice() { return; } - const username = await getLocalUsername(); + const username = getLocalUsername(); if (!username) { cliDebugPrint('useRentalSunsetNotice', 'Not logged in, skipping the check'); diff --git a/src/lib/types.ts b/src/lib/types.ts index f639688ed..cbb83cd7d 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -4,7 +4,6 @@ export interface AuthJSON { token?: string; id?: string; username?: string; - email?: string; proxy?: { password: string; }; diff --git a/src/lib/utils.ts b/src/lib/utils.ts index 4dc680231..2e4e108ca 100644 --- a/src/lib/utils.ts +++ b/src/lib/utils.ts @@ -32,6 +32,7 @@ import { SOURCE_FILE_FORMATS, } from '@apify/consts'; +import { ensureAuthFileCurrent, lookUpActiveProfile } from './auth-file.js'; import { describeAuthFailure, getApifyClientOptions, resolveAuth } from './auth.js'; import { AUTH_FILE_PATH, @@ -41,7 +42,7 @@ import { MINIMUM_SUPPORTED_PYTHON_VERSION, SUPPORTED_NODEJS_VERSION, } from './consts.js'; -import { ensureMigrated, getBackend, getProxyPassword, getToken } from './credentials.js'; +import { ensureMigrated, getProxyPassword, getToken } from './credentials.js'; import { deleteFile, ensureFolderExistsSync, rimrafPromised } from './files.js'; import { useCLIMetadata } from './hooks/useCLIMetadata.js'; import { inputFileRegExp, TEMP_INPUT_KEY_PREFIX } from './input-key.js'; @@ -84,33 +85,38 @@ export const getLocalRequestQueuePath = (storeId?: string) => { }; /** - * Returns object from auth file or empty object. Secrets (token, proxy password) are - * pulled from the keyring when that backend is active; user metadata lives in auth.json. + * The active profile in the flat shape the CLI consumes, or an empty object when nothing is + * stored. Secrets come from whichever backend holds them; the metadata comes from auth.json. */ export const getLocalUserInfo = async (): Promise => { await ensureMigrated(); + await ensureAuthFileCurrent(); - let result: AuthJSON = {}; - try { - const raw = await readFile(AUTH_FILE_PATH(), 'utf-8'); - result = JSON.parse(raw) as AuthJSON; - } catch { - // auth.json may not exist yet (fresh keyring-only state); fall through + const { profile, missingProfile } = lookUpActiveProfile(); + + const result: AuthJSON = {}; + if (profile) { + result.id = profile.id; + if (profile.username) result.username = profile.username; + if (profile.organizationOwnerUserId) result.organizationOwnerUserId = profile.organizationOwnerUserId; } - if ((await getBackend()) === 'keyring') { - const token = await getToken(); - if (token) result.token = token; + const token = await getToken(); + if (token) result.token = token; - const proxyPassword = await getProxyPassword(); - if (proxyPassword) result.proxy = { ...result.proxy, password: proxyPassword }; - } + const proxyPassword = await getProxyPassword(); + if (proxyPassword) result.proxy = { password: proxyPassword }; - const hasUserMetadata = !!(result.username || result.id); - const isComplete = hasUserMetadata || !!result.token; - if (!isComplete) return {}; - if (!hasUserMetadata) { - throw new Error('Stale credentials found without user metadata. Please run "apify login" again.'); + // A token with no profile behind it is reported rather than swallowed: the commands that build + // `/` lookups would otherwise fail with a misleading "not found". + if (!profile) { + if (!result.token) return {}; + + throw new Error( + missingProfile + ? `Your active profile "${missingProfile}" is missing from ${AUTH_FILE_PATH()}. Run "apify login" to log in again.` + : 'Stale credentials found without user metadata. Run "apify login" again.', + ); } return result; diff --git a/test/__setup__/auth-file.ts b/test/__setup__/auth-file.ts new file mode 100644 index 000000000..6e617345d --- /dev/null +++ b/test/__setup__/auth-file.ts @@ -0,0 +1,35 @@ +/** Reading `auth.json` in tests, so no test has to know the profile shape by hand. */ + +import { readFileSync } from 'node:fs'; + +import type { AuthFile, AuthProfile } from '../../src/lib/auth-file.js'; +import { AUTH_FILE_PATH } from '../../src/lib/consts.js'; + +/** The raw file, for assertions about the version, the backend marker, or where secrets landed. */ +export function readAuthFile(): AuthFile { + return JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf-8')) as AuthFile; +} + +/** The active profile with its user ID, read straight off disk rather than through the CLI. */ +export function readActiveProfile(): (AuthProfile & { id: string }) | undefined { + const { activeProfile, profiles } = readAuthFile(); + if (!activeProfile) return undefined; + + const profile = profiles?.[activeProfile]; + return profile ? { id: activeProfile, ...profile } : undefined; +} + +/** A v1 `auth.json`, the shape every CLI before the profile migration wrote. */ +export function v1AuthFile(overrides: Record = {}) { + return { + id: 'uid', + username: 'me', + email: 'me@example.com', + token: 'apify_api_v1_token', + proxy: { password: 'pw', groups: [{ name: 'g' }] }, + plan: { id: 'FREE' }, + isPaying: false, + createdAt: '2021-03-27T22:27:56.809Z', + ...overrides, + }; +} diff --git a/test/__setup__/hooks/useAuthSetup.ts b/test/__setup__/hooks/useAuthSetup.ts index 5fefdbe00..a375c313b 100644 --- a/test/__setup__/hooks/useAuthSetup.ts +++ b/test/__setup__/hooks/useAuthSetup.ts @@ -6,6 +6,7 @@ import { isCI } from 'ci-info'; import { cryptoRandomObjectId } from '@apify/utilities'; import { LoginCommand } from '../../../src/commands/login.js'; +import { __resetAuthFileForTests } from '../../../src/lib/auth-file.js'; import { __resetAuthNoticesForTests } from '../../../src/lib/auth.js'; import { testRunCommand } from '../../../src/lib/command-framework/apify-command.js'; import { GLOBAL_CONFIGS_FOLDER } from '../../../src/lib/consts.js'; @@ -49,6 +50,7 @@ export function useAuthSetup({ cleanup = true, perTest = true }: UseAuthSetupOpt __resetCredentialsForTests(); __resetUserInfoCacheForTests(); __resetAuthNoticesForTests(); + __resetAuthFileForTests(); }); after(async () => { @@ -59,6 +61,7 @@ export function useAuthSetup({ cleanup = true, perTest = true }: UseAuthSetupOpt __resetCredentialsForTests(); __resetUserInfoCacheForTests(); __resetAuthNoticesForTests(); + __resetAuthFileForTests(); vitest.unstubAllEnvs(); }); } @@ -80,6 +83,7 @@ export function useKeyringBackend() { __resetCredentialsForTests(); __resetUserInfoCacheForTests(); __resetAuthNoticesForTests(); + __resetAuthFileForTests(); }); } diff --git a/test/local/commands/auth.test.ts b/test/local/commands/auth.test.ts index ecf6b4d51..b1ca6e8c7 100644 --- a/test/local/commands/auth.test.ts +++ b/test/local/commands/auth.test.ts @@ -1,8 +1,9 @@ -import { existsSync, readFileSync, statSync } from 'node:fs'; +import { existsSync, statSync } from 'node:fs'; import process from 'node:process'; import { AUTH_FILE_PATH, CommandExitCodes } from '../../../src/lib/consts.js'; import { getToken } from '../../../src/lib/credentials.js'; +import { readActiveProfile, readAuthFile } from '../../__setup__/auth-file.js'; import { useAuthSetup, useKeyringBackend } from '../../__setup__/hooks/useAuthSetup.js'; import { useConsoleSpy } from '../../__setup__/hooks/useConsoleSpy.js'; import { @@ -56,7 +57,6 @@ const { testRunCommand } = await import('../../../src/lib/command-framework/apif const TOKEN = 'apify_api_test_token'; -const readAuthFile = () => JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf-8')); const login = (token = TOKEN) => testRunCommand(AuthLoginCommand, { flags_token: token }); describe('auth commands', () => { @@ -71,14 +71,17 @@ describe('auth commands', () => { }); describe('file backend', () => { - it('login stores the token and user metadata in auth.json', async () => { + it('login stores the token and one profile keyed by user ID', async () => { await login(); - expect(readAuthFile()).toMatchObject({ - token: TOKEN, + expect(readAuthFile()).toMatchObject({ version: 2, token: TOKEN, secretsBackend: 'file' }); + expect(readActiveProfile()).toEqual({ id: 'uid', username: 'me', - secretsBackend: 'file', + name: null, + authMethod: 'token', + expiresAt: null, + hasRefreshToken: false, }); expect(lastErrorMessage()).toContain('You are logged in to Apify as me'); }); @@ -104,7 +107,7 @@ describe('auth commands', () => { expect(await getToken()).toBeUndefined(); }); - it('logging in as another account replaces the stored metadata', async () => { + it('logging in as another account replaces the stored profile', async () => { clientState.user = { id: 'uid', username: 'me', email: 'me@example.com' }; await login(); @@ -112,9 +115,10 @@ describe('auth commands', () => { await login('apify_api_other_token'); const authFile = readAuthFile(); - expect(authFile).toMatchObject({ token: 'apify_api_other_token', id: 'uid2', username: 'other' }); - // The new account has no email, so the old one must not linger. - expect(authFile.email).toBeUndefined(); + expect(authFile).toMatchObject({ activeProfile: 'uid2', token: 'apify_api_other_token' }); + // Additive login is a later stage; until then the old profile must not linger. + expect(Object.keys(authFile.profiles!)).toEqual(['uid2']); + expect(readActiveProfile()).toMatchObject({ username: 'other' }); }); it('login with an invalid token stores nothing and fails the command', async () => { @@ -177,7 +181,7 @@ describe('auth commands', () => { expect(lastLogMessage()).toBe('apify_api_env_token'); expect(await getToken()).toBe(TOKEN); - expect(readAuthFile()).toMatchObject({ username: 'me' }); + expect(readActiveProfile()).toMatchObject({ username: 'me' }); }); }); @@ -191,17 +195,11 @@ describe('auth commands', () => { expect(keyringStore.get(KEYRING_PROXY_PASSWORD_KEY)).toBe('pw'); const authFile = readAuthFile(); - expect(authFile).toMatchObject({ id: 'uid', username: 'me', secretsBackend: 'keyring' }); + expect(authFile).toMatchObject({ version: 2, secretsBackend: 'keyring' }); expect(authFile.token).toBeUndefined(); - expect(authFile.proxy).toEqual({ groups: [{ name: 'g' }] }); - }); - - it('login drops the proxy object from auth.json when it only held the password', async () => { - clientState.user.proxy = { password: 'pw' }; - await login(); - - expect(readAuthFile()).not.toHaveProperty('proxy'); - expect(keyringStore.get(KEYRING_PROXY_PASSWORD_KEY)).toBe('pw'); + // Proxy groups are not a secret, but nothing reads them either. + expect(authFile).not.toHaveProperty('proxy'); + expect(readActiveProfile()).toMatchObject({ id: 'uid', username: 'me' }); }); it('logging in twice with the same token writes the keyring once', async () => { diff --git a/test/local/commands/run.test.ts b/test/local/commands/run.test.ts index 75c1e141b..edb48ded2 100644 --- a/test/local/commands/run.test.ts +++ b/test/local/commands/run.test.ts @@ -4,13 +4,14 @@ import { dirname } from 'node:path'; import { ACTOR_ENV_VARS, APIFY_ENV_VARS } from '@apify/consts'; import { testRunCommand } from '../../../src/lib/command-framework/apify-command.js'; -import { AUTH_FILE_PATH, EMPTY_LOCAL_CONFIG, LOCAL_CONFIG_PATH } from '../../../src/lib/consts.js'; +import { EMPTY_LOCAL_CONFIG, LOCAL_CONFIG_PATH } from '../../../src/lib/consts.js'; import { rimrafPromised } from '../../../src/lib/files.js'; import { getLocalDatasetPath, getLocalKeyValueStorePath, getLocalRequestQueuePath, getLocalStorageDir, + getLocalUserInfo, } from '../../../src/lib/utils.js'; import { TEST_TIMEOUT } from '../../__setup__/consts.js'; import { safeLogin, useAuthSetup } from '../../__setup__/hooks/useAuthSetup.js'; @@ -123,9 +124,9 @@ describe('apify run', () => { const actOutputPath = joinPath(getLocalKeyValueStorePath(), 'OUTPUT.json'); const localEnvVars = JSON.parse(readFileSync(actOutputPath, 'utf8')); - const auth = JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf8')); + const auth = await getLocalUserInfo(); - expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(auth.proxy.password); + expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(auth.proxy!.password); expect(localEnvVars[APIFY_ENV_VARS.USER_ID]).toStrictEqual(auth.id); expect(localEnvVars[APIFY_ENV_VARS.TOKEN]).toStrictEqual(auth.token); expect(localEnvVars.TEST_LOCAL).toStrictEqual(testEnvVars.TEST_LOCAL); @@ -164,9 +165,9 @@ describe('apify run', () => { const actOutputPath = joinPath(getLocalKeyValueStorePath(), 'OUTPUT.json'); const localEnvVars = JSON.parse(readFileSync(actOutputPath, 'utf8')); - const auth = JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf8')); + const auth = await getLocalUserInfo(); - expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(auth.proxy.password); + expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(auth.proxy!.password); expect(localEnvVars[APIFY_ENV_VARS.USER_ID]).toStrictEqual(auth.id); expect(localEnvVars[APIFY_ENV_VARS.TOKEN]).toStrictEqual(auth.token); expect(localEnvVars.TEST_LOCAL).toStrictEqual(testEnvVars.TEST_LOCAL); @@ -204,9 +205,9 @@ describe('apify run', () => { const actOutputPath = joinPath(getLocalKeyValueStorePath(), 'OUTPUT.json'); const localEnvVars = JSON.parse(readFileSync(actOutputPath, 'utf8')); - const auth = JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf8')); + const auth = await getLocalUserInfo(); - expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(auth.proxy.password); + expect(localEnvVars[APIFY_ENV_VARS.PROXY_PASSWORD]).toStrictEqual(auth.proxy!.password); expect(localEnvVars[APIFY_ENV_VARS.USER_ID]).toStrictEqual(auth.id); expect(localEnvVars[APIFY_ENV_VARS.TOKEN]).toStrictEqual(auth.token); expect(localEnvVars.TEST_LOCAL).toStrictEqual(testEnvVars.TEST_LOCAL); diff --git a/test/local/lib/auth-file.test.ts b/test/local/lib/auth-file.test.ts new file mode 100644 index 000000000..8afc82c81 --- /dev/null +++ b/test/local/lib/auth-file.test.ts @@ -0,0 +1,255 @@ +import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; + +import { + __resetAuthFileForTests, + AUTH_BACKUP_FILE_PATH, + type AuthProfile, + ensureAuthFileCurrent, + getActiveProfile, + lookUpActiveProfile, + removeActiveProfile, + setActiveProfile, +} from '../../../src/lib/auth-file.js'; +import { AUTH_FILE_PATH, GLOBAL_CONFIGS_FOLDER } from '../../../src/lib/consts.js'; +import { ensureMigrated, getProxyPassword, getToken } from '../../../src/lib/credentials.js'; +import { getLocalUserInfo } from '../../../src/lib/utils.js'; +import { readActiveProfile, readAuthFile, v1AuthFile } from '../../__setup__/auth-file.js'; +import { useAuthSetup, useKeyringBackend } from '../../__setup__/hooks/useAuthSetup.js'; +import { + KEYRING_PROXY_PASSWORD_KEY, + KEYRING_TOKEN_KEY, + keyringStore, + resetKeyringMock, +} from '../../__setup__/keyring-mock.js'; + +vi.mock('@napi-rs/keyring', () => import('../../__setup__/keyring-mock.js')); + +useAuthSetup(); + +const write = (contents: unknown) => { + mkdirSync(GLOBAL_CONFIGS_FOLDER(), { recursive: true }); + writeFileSync(AUTH_FILE_PATH(), typeof contents === 'string' ? contents : JSON.stringify(contents)); +}; + +const readBackup = () => JSON.parse(readFileSync(AUTH_BACKUP_FILE_PATH(), 'utf-8')); + +const V2_PROFILE: AuthProfile = { + username: 'me', + name: null, + authMethod: 'token', + expiresAt: null, + hasRefreshToken: false, +}; + +const V1_PROFILE = { id: 'uid', ...V2_PROFILE }; + +describe('auth.json v2', () => { + beforeEach(() => { + resetKeyringMock(); + }); + + describe('migration', () => { + // State A in the wild: plaintext secrets and no backend marker, written before the keyring. + it('migrates state A, after ensureMigrated() has stamped the marker', async () => { + write(v1AuthFile()); + + await ensureMigrated(); + await ensureAuthFileCurrent(); + + expect(readAuthFile()).toEqual({ + version: 2, + activeProfile: 'uid', + profiles: { uid: V2_PROFILE }, + secretsBackend: 'file', + token: 'apify_api_v1_token', + proxy: { password: 'pw' }, + }); + }); + + // State C: plaintext secrets with the file marker already on them. + it('migrates state C and keeps the secrets in the file', async () => { + write(v1AuthFile({ secretsBackend: 'file' })); + + await ensureAuthFileCurrent(); + + expect(readAuthFile()).toMatchObject({ version: 2, secretsBackend: 'file', token: 'apify_api_v1_token' }); + expect(await getToken()).toBe('apify_api_v1_token'); + expect(await getProxyPassword()).toBe('pw'); + }); + + it('drops the fields nothing in the CLI reads', async () => { + write(v1AuthFile({ secretsBackend: 'file' })); + + await ensureAuthFileCurrent(); + + const file = readAuthFile(); + for (const key of ['email', 'plan', 'isPaying', 'createdAt', 'id', 'username']) { + expect(file).not.toHaveProperty(key); + } + expect(file.proxy).toEqual({ password: 'pw' }); + }); + + it('carries organizationOwnerUserId into the profile', async () => { + write(v1AuthFile({ secretsBackend: 'file', organizationOwnerUserId: 'owner-id' })); + + await ensureAuthFileCurrent(); + + expect(readActiveProfile()).toMatchObject({ organizationOwnerUserId: 'owner-id' }); + }); + + it('backs the v1 file up and never overwrites the backup', async () => { + write(v1AuthFile({ secretsBackend: 'file' })); + + await ensureAuthFileCurrent(); + expect(readBackup()).toMatchObject({ id: 'uid', email: 'me@example.com' }); + + // A later process migrating another v1 file must leave the first backup alone. + write(v1AuthFile({ secretsBackend: 'file', username: 'someone-else' })); + __resetAuthFileForTests(); + await ensureAuthFileCurrent(); + + expect(readActiveProfile()).toMatchObject({ username: 'someone-else' }); + expect(readBackup()).toMatchObject({ username: 'me' }); + }); + + it('is a no-op on a file that is already v2', async () => { + write(v1AuthFile({ secretsBackend: 'file' })); + await ensureAuthFileCurrent(); + const migrated = readAuthFile(); + + await ensureAuthFileCurrent(); + + expect(readAuthFile()).toEqual(migrated); + }); + + it('does nothing when there is no file', async () => { + await ensureAuthFileCurrent(); + + expect(existsSync(AUTH_FILE_PATH())).toBe(false); + expect(existsSync(AUTH_BACKUP_FILE_PATH())).toBe(false); + }); + + it('leaves a corrupt file alone rather than rewriting it', async () => { + write('{ not json'); + + await ensureAuthFileCurrent(); + + expect(readFileSync(AUTH_FILE_PATH(), 'utf-8')).toBe('{ not json'); + expect(existsSync(AUTH_BACKUP_FILE_PATH())).toBe(false); + }); + + it('keeps the secrets of a v1 file that has no user ID, so the next command asks for a re-login', async () => { + write({ token: 'apify_api_v1_token', secretsBackend: 'file' }); + + await ensureAuthFileCurrent(); + + expect(readAuthFile()).toEqual({ + version: 2, + profiles: {}, + secretsBackend: 'file', + token: 'apify_api_v1_token', + }); + expect(readBackup()).toEqual({ token: 'apify_api_v1_token', secretsBackend: 'file' }); + await expect(getLocalUserInfo()).rejects.toThrow('Stale credentials found without user metadata'); + }); + }); + + describe('reading the active profile', () => { + it('reads a v1 file that has not been migrated yet', () => { + write(v1AuthFile()); + + expect(getActiveProfile()).toEqual(V1_PROFILE); + }); + + it('returns nothing when no profile is stored', () => { + write({ version: 2, profiles: {} }); + + expect(lookUpActiveProfile()).toEqual({}); + }); + + it('names the profile activeProfile points at when the file does not contain it', () => { + write({ version: 2, activeProfile: 'gone', profiles: {} }); + + expect(lookUpActiveProfile()).toEqual({ missingProfile: 'gone' }); + }); + + it('names the missing profile rather than reporting a silent logged-out state', async () => { + write({ version: 2, activeProfile: 'gone', profiles: {}, secretsBackend: 'file', token: 'tok' }); + + await expect(getLocalUserInfo()).rejects.toThrow('Your active profile "gone" is missing'); + }); + + it('is logged out when the missing profile leaves no token behind either', async () => { + write({ version: 2, activeProfile: 'gone', profiles: {}, secretsBackend: 'file' }); + + await expect(getLocalUserInfo()).resolves.toEqual({}); + }); + }); + + describe('a file a newer CLI wrote', () => { + it('is refused rather than migrated backwards', async () => { + write({ version: 3, activeProfile: 'uid', profiles: {} }); + + await expect(ensureAuthFileCurrent()).rejects.toThrow('written by a newer Apify CLI'); + }); + + it('is not replaced by a login', () => { + const newer = { version: 3, activeProfile: 'uid', profiles: { uid: { username: 'me' } } }; + write(newer); + + expect(() => setActiveProfile('uid2', V2_PROFILE, 'file')).toThrow('written by a newer Apify CLI'); + expect(readAuthFile()).toEqual(newer); + }); + + it('is not touched by a logout', () => { + const newer = { version: 3, activeProfile: 'uid', profiles: { uid: { username: 'me' } }, token: 'tok' }; + write(newer); + + expect(() => removeActiveProfile()).toThrow('written by a newer Apify CLI'); + expect(readAuthFile()).toEqual(newer); + }); + }); + + describe('keyring backend', () => { + useKeyringBackend(); + + // State B in the wild: secrets already in the keyring, auth.json holding only metadata. + it('migrates state B without touching the keyring', async () => { + keyringStore.set(KEYRING_TOKEN_KEY, 'tok_kr'); + keyringStore.set(KEYRING_PROXY_PASSWORD_KEY, 'pw_kr'); + write({ id: 'uid', username: 'me', email: 'me@example.com', secretsBackend: 'keyring' }); + + await ensureMigrated(); + await ensureAuthFileCurrent(); + + expect(readAuthFile()).toEqual({ + version: 2, + activeProfile: 'uid', + profiles: { uid: V2_PROFILE }, + secretsBackend: 'keyring', + }); + expect(await getLocalUserInfo()).toEqual({ + id: 'uid', + username: 'me', + token: 'tok_kr', + proxy: { password: 'pw_kr' }, + }); + }); + + // State A on a machine where the keyring works: ensureMigrated() moves the secrets first. + it('migrates state A to the keyring and then to v2', async () => { + write(v1AuthFile()); + + await ensureMigrated(); + await ensureAuthFileCurrent(); + + expect(keyringStore.get(KEYRING_TOKEN_KEY)).toBe('apify_api_v1_token'); + expect(readAuthFile()).toEqual({ + version: 2, + activeProfile: 'uid', + profiles: { uid: V2_PROFILE }, + secretsBackend: 'keyring', + }); + }); + }); +}); diff --git a/test/local/lib/auth.test.ts b/test/local/lib/auth.test.ts index 2d4da6e4c..702970f7d 100644 --- a/test/local/lib/auth.test.ts +++ b/test/local/lib/auth.test.ts @@ -1,9 +1,10 @@ -import { existsSync, readFileSync } from 'node:fs'; +import { existsSync } from 'node:fs'; import { loginWithToken, resolveAuth } from '../../../src/lib/auth.js'; import { AUTH_FILE_PATH } from '../../../src/lib/consts.js'; import { getProxyPassword, getToken, setToken } from '../../../src/lib/credentials.js'; import { getLoggedClientOrThrow } from '../../../src/lib/utils.js'; +import { readActiveProfile } from '../../__setup__/auth-file.js'; import { useAuthSetup } from '../../__setup__/hooks/useAuthSetup.js'; import { useConsoleSpy } from '../../__setup__/hooks/useConsoleSpy.js'; @@ -46,8 +47,6 @@ const STORED = 'apify_api_stored'; const ENV = 'apify_api_env'; const FLAG = 'apify_api_flag'; -const readAuthFile = () => JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf-8')); - describe('auth', () => { beforeEach(() => { clientState.fail = false; @@ -147,7 +146,7 @@ describe('auth', () => { expect(await getToken()).toBe(STORED); expect(await getProxyPassword()).toBe('pw'); - expect(readAuthFile()).toMatchObject({ id: 'uid', username: 'me' }); + expect(readActiveProfile()).toMatchObject({ id: 'uid', username: 'me' }); }); it('writes nothing when the API rejects the token', async () => { @@ -210,7 +209,7 @@ describe('auth', () => { await resolveAuth(); expect(await getToken()).toBe(STORED); - expect(readAuthFile()).toMatchObject({ username: 'me' }); + expect(readActiveProfile()).toMatchObject({ username: 'me' }); }); it('resolving a token the command was given leaves the stored login untouched', async () => { diff --git a/test/local/lib/credentials.test.ts b/test/local/lib/credentials.test.ts index 15c0b8c16..549d9960e 100644 --- a/test/local/lib/credentials.test.ts +++ b/test/local/lib/credentials.test.ts @@ -4,6 +4,7 @@ import process from 'node:process'; import { cryptoRandomObjectId } from '@apify/utilities'; +import { __resetAuthFileForTests } from '../../../src/lib/auth-file.js'; import { getApifyClientOptions } from '../../../src/lib/auth.js'; import { AUTH_FILE_PATH, GLOBAL_CONFIGS_FOLDER } from '../../../src/lib/consts.js'; import { @@ -35,7 +36,8 @@ vi.mock('node:fs', async (importOriginal) => { }); const writeFileSyncSpy = vi.mocked(writeFileSync); -const authFileWrites = () => writeFileSyncSpy.mock.calls.filter((call) => call[0] === AUTH_FILE_PATH()); +// auth.json is written through a temp file and a rename, so the spied path carries a suffix. +const authFileWrites = () => writeFileSyncSpy.mock.calls.filter((call) => String(call[0]).startsWith(AUTH_FILE_PATH())); const writeAuthFile = (data: Record) => { mkdirSync(GLOBAL_CONFIGS_FOLDER(), { recursive: true }); @@ -52,12 +54,14 @@ describe('credentials', () => { resetKeyringMock(); writeFileSyncSpy.mockClear(); __resetCredentialsForTests(); + __resetAuthFileForTests(); }); afterEach(async () => { await rm(GLOBAL_CONFIGS_FOLDER(), { recursive: true, force: true }); vitest.unstubAllEnvs(); __resetCredentialsForTests(); + __resetAuthFileForTests(); }); describe('getBackend()', () => { @@ -134,7 +138,9 @@ describe('credentials', () => { it('writes auth.json with mode 0600', async () => { await setToken('tok_123'); - expect(writeFileSyncSpy).toHaveBeenCalledWith(AUTH_FILE_PATH(), expect.any(String), { mode: 0o600 }); + expect(writeFileSyncSpy).toHaveBeenCalledWith(expect.stringContaining(AUTH_FILE_PATH()), expect.any(String), { + mode: 0o600, + }); }); it.skipIf(process.platform === 'win32')('creates auth.json readable only by the owner', async () => { @@ -329,7 +335,7 @@ describe('credentials', () => { }); describe('getLocalUserInfo()', () => { - it('on file backend, preserves non-secret proxy fields', async () => { + it('on file backend, keeps the proxy password and drops the groups nothing reads', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); writeAuthFile({ username: 'me', @@ -339,7 +345,7 @@ describe('credentials', () => { secretsBackend: 'file', }); const info = await getLocalUserInfo(); - expect(info.proxy).toEqual({ password: 'pw', groups: [{ name: 'g' }] }); + expect(info.proxy).toEqual({ password: 'pw' }); }); it('on keyring backend, overlays token and proxy password from keyring', async () => { diff --git a/test/local/lib/rental-sunset-notice.test.ts b/test/local/lib/rental-sunset-notice.test.ts index 9a038adb0..947503ac3 100644 --- a/test/local/lib/rental-sunset-notice.test.ts +++ b/test/local/lib/rental-sunset-notice.test.ts @@ -27,7 +27,17 @@ async function writeAuthFile(username: string | undefined) { const path = AUTH_FILE_PATH(); await mkdir(dirname(path), { recursive: true }); - await writeFile(path, JSON.stringify({ id: 'user-id', username, token: 'apify_api_token' })); + await writeFile( + path, + JSON.stringify({ + version: 2, + activeProfile: 'user-id', + profiles: { + 'user-id': { username, name: null, authMethod: 'token', expiresAt: null, hasRefreshToken: false }, + }, + token: 'apify_api_token', + }), + ); } interface StoredRentalSunset { From 35a698c54deb3f28975100954bf7617e6e90a3ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20Sol=C3=A1r?= Date: Thu, 17 Sep 2026 13:51:53 +0200 Subject: [PATCH 2/2] feat: key secrets by user ID on both backends Secrets lived under one fixed name per kind, so a second account would overwrite the first one's token. Both the keyring and the file backend are now keyed by user ID, and existing secrets are re-keyed in place. Co-Authored-By: Claude Opus 5 --- src/commands/auth/logout.ts | 10 +- src/lib/auth-file.ts | 63 +++++- src/lib/auth.ts | 21 +- src/lib/credentials.ts | 262 ++++++++++++++++------ src/lib/utils.ts | 40 ++-- test/__setup__/auth-file.ts | 23 ++ test/__setup__/keyring-mock.ts | 12 +- test/local/commands/auth.test.ts | 48 +++- test/local/lib/auth-file.test.ts | 39 ++-- test/local/lib/auth.test.ts | 14 +- test/local/lib/credentials.test.ts | 338 +++++++++++++++++++++-------- 11 files changed, 634 insertions(+), 236 deletions(-) diff --git a/src/commands/auth/logout.ts b/src/commands/auth/logout.ts index 3892fbc4c..85c2beb14 100644 --- a/src/commands/auth/logout.ts +++ b/src/commands/auth/logout.ts @@ -1,6 +1,6 @@ import { APIFY_ENV_VARS } from '@apify/consts'; -import { removeActiveProfile } from '../../lib/auth-file.js'; +import { assertSupportedAuthFileVersion, getActiveProfileId, removeActiveProfile } from '../../lib/auth-file.js'; import { getEnvToken } from '../../lib/auth.js'; import { ApifyCommand } from '../../lib/command-framework/apify-command.js'; import { AUTH_FILE_PATH } from '../../lib/consts.js'; @@ -28,10 +28,12 @@ export class AuthLogoutCommand extends ApifyCommand { static override docsUrl = 'https://docs.apify.com/cli/docs/reference#apify-logout'; async run() { - // The file goes first: it is the step that can refuse, and refusing before the keyring is - // cleared leaves a logged-in state rather than half a logout. + // The refusal comes first, so a file this CLI must not touch leaves a logged-in state + // rather than half a logout. The keyring goes next: `auth.json` is the only index of what + // the keyring holds, so removing the profile first would strand its entries. + assertSupportedAuthFileVersion(); + await clearKeyringSecrets(getActiveProfileId()); removeActiveProfile(); - await clearKeyringSecrets(); await updateUserId(null); diff --git a/src/lib/auth-file.ts b/src/lib/auth-file.ts index f74625c3f..fa7ea5ba3 100644 --- a/src/lib/auth-file.ts +++ b/src/lib/auth-file.ts @@ -3,11 +3,11 @@ import { copyFileSync, existsSync, readFileSync, renameSync, rmSync, writeFileSy import { cryptoRandomObjectId } from '@apify/utilities'; import { AUTH_FILE_PATH } from './consts.js'; -import type { CredentialsBackend } from './credentials.js'; +import type { CredentialsBackend, SecretKind } from './credentials.js'; import { ensureApifyDirectory } from './files.js'; import { cliDebugPrint } from './utils/cliDebugPrint.js'; -const AUTH_FILE_VERSION = 2; +export const AUTH_FILE_VERSION = 2; /** The way back to a CLI that only reads the v1 shape. */ export const AUTH_BACKUP_FILE_PATH = () => `${AUTH_FILE_PATH()}.v1.bak`; @@ -28,11 +28,15 @@ export interface AuthProfile { expiresAt: string | null; /** Whether a refresh token came with the access token. Unused until the device flow lands. */ hasRefreshToken: boolean; + /** File backend only. The keyring backend keeps this in the OS store instead. */ + token?: string; + /** File backend only. The keyring backend keeps this in the OS store instead. */ + proxy?: { password?: string }; } /** - * `auth.json` as it sits on disk. `token` and `proxy` are the file backend's secret storage; they - * stay outside the profiles until each profile gets its own keys. + * `auth.json` as it sits on disk. Top-level `token` and `proxy` are where the file backend kept + * secrets before they were keyed per profile; `ensureSecretsKeyed()` moves them into the profile. */ export interface AuthFile { version?: number; @@ -114,8 +118,8 @@ function v1Profile(file: AuthFile): AuthProfile { function toV2(file: AuthFile): AuthFile { const migrated: AuthFile = { version: AUTH_FILE_VERSION, profiles: {} }; - // A v1 file with a token but no ID has no key to store the profile under. Keep the secrets so - // the next command reports stale credentials instead of a silent logged-out state. + // A v1 file with a token but no ID has no key to store the profile under. The secrets are + // carried over here and dropped by `ensureSecretsKeyed()`, which is what forces the re-login. if (typeof file.id === 'string') { migrated.activeProfile = file.id; migrated.profiles![file.id] = v1Profile(file); @@ -161,7 +165,7 @@ async function migrateToV2(): Promise { * A file from a newer CLI is not something to guess at — migrating it backwards would drop * whatever that version stores. */ -function assertSupportedAuthFileVersion() { +export function assertSupportedAuthFileVersion() { const { version } = readAuthFile(); if (typeof version === 'number' && version > AUTH_FILE_VERSION) { @@ -207,6 +211,51 @@ export function getActiveProfile(): (AuthProfile & { id: string }) | undefined { return lookUpActiveProfile().profile; } +/** + * The user ID every secret is keyed by. Taken from `activeProfile` rather than from the profile + * object, so a file whose `activeProfile` names a missing profile still resolves its secrets and + * reports the dangling profile instead of looking logged out. + */ +export function getActiveProfileId(): string | undefined { + const file = readAuthFile(); + + if (file.version !== AUTH_FILE_VERSION) { + return typeof file.id === 'string' ? file.id : undefined; + } + + return file.activeProfile; +} + +/** Where the file backend keeps a secret inside a profile. */ +function profileSecret(profile: AuthProfile, kind: SecretKind): string | undefined { + return kind === 'token' ? profile.token : profile.proxy?.password; +} + +/** The file backend's stored secret, or `undefined` when the profile does not hold one. */ +export function readProfileSecret(userId: string, kind: SecretKind): string | undefined { + const profile = readAuthFile().profiles?.[userId]; + return profile ? profileSecret(profile, kind) : undefined; +} + +/** + * Stores a file-backend secret on the profile and marks the file as the secrets backend. A missing + * profile is left alone: inventing one would fabricate the account metadata the CLI reads. + */ +export function writeProfileSecret(userId: string, kind: SecretKind, value: string) { + const file = readAuthFile(); + const profile = file.profiles?.[userId]; + if (!profile) return; + + if (kind === 'token') { + profile.token = value; + } else { + profile.proxy = { ...profile.proxy, password: value }; + } + + file.secretsBackend = 'file'; + writeAuthFile(file); +} + /** * Stores one account and makes it active, replacing whatever was there. Nothing puts a second * profile in the file yet, so `apify login` owns all of it. diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 0d3239ed5..467868038 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -6,9 +6,9 @@ import { AxiosHeaders } from 'axios'; import { APIFY_ENV_VARS } from '@apify/consts'; -import { ensureAuthFileCurrent, setActiveProfile } from './auth-file.js'; +import { getActiveProfileId, setActiveProfile } from './auth-file.js'; import { APIFY_CLIENT_DEFAULT_HEADERS, AUTH_FILE_PATH } from './consts.js'; -import { ensureMigrated, getBackend, getToken, setProxyPassword, setToken } from './credentials.js'; +import { clearKeyringSecrets, ensureCredentialsCurrent, getBackend, getSecret, setSecret } from './credentials.js'; import { warning } from './outputs.js'; import { cliDebugPrint } from './utils/cliDebugPrint.js'; @@ -85,10 +85,10 @@ export const resolveAuth = async (explicitToken?: string): Promise` under a single service) would depend on `:` being legal in an account name on + * macOS Keychain, libsecret and Windows Credential Manager, and it reads worse in keyring UIs. + */ +function keyringKey(userId: string, kind: SecretKind) { + return { service: `${KEYRING_SERVICE}.${kind}`, account: userId }; +} + +/** Where a secret sat before it was keyed by user: one service, the kind as the account. */ +function legacyKeyringKey(kind: SecretKind) { + return { service: KEYRING_SERVICE, account: kind }; +} + interface KeyringEntry { getPassword(): string | null; setPassword(password: string): void; @@ -23,12 +51,14 @@ interface KeyringModule { let cachedKeyringModule: KeyringModule | null | undefined; let backendPromise: Promise | undefined; let migrationPromise: Promise | undefined; +let keyingPromise: Promise | undefined; /** Test-only: clear cached module/backend/migration so each test starts fresh. */ export function __resetCredentialsForTests() { cachedKeyringModule = undefined; backendPromise = undefined; migrationPromise = undefined; + keyingPromise = undefined; } async function loadKeyringModule(): Promise { @@ -95,67 +125,69 @@ function downgradeBackendToFile() { backendPromise = Promise.resolve('file'); } -async function getKeyringEntry(account: string): Promise { +interface KeyringKey { + service: string; + account: string; +} + +async function getKeyringEntry({ service, account }: KeyringKey): Promise { const mod = await loadKeyringModule(); if (!mod) return null; - return new mod.Entry(KEYRING_SERVICE, account); + return new mod.Entry(service, account); } -async function readKeyring(account: string): Promise { +async function readKeyring(key: KeyringKey): Promise { try { - const entry = await getKeyringEntry(account); + const entry = await getKeyringEntry(key); if (!entry) return undefined; return entry.getPassword() ?? undefined; } catch (err) { - cliDebugPrint('credentials', `failed to read ${account} from keyring`, err); + cliDebugPrint('credentials', `failed to read ${key.service}/${key.account} from keyring`, err); return undefined; } } -async function writeKeyring(account: string, value: string): Promise { - const entry = await getKeyringEntry(account); +async function writeKeyring(key: KeyringKey, value: string): Promise { + const entry = await getKeyringEntry(key); if (!entry) { throw new Error('OS keyring is not available.'); } entry.setPassword(value); } -async function deleteKeyring(account: string): Promise { +async function deleteKeyring(key: KeyringKey): Promise { try { - const entry = await getKeyringEntry(account); + const entry = await getKeyringEntry(key); if (!entry) return; entry.deletePassword(); } catch (err) { - cliDebugPrint('credentials', `failed to delete ${account} from keyring`, err); + cliDebugPrint('credentials', `failed to delete ${key.service}/${key.account} from keyring`, err); } } -export async function getToken(): Promise { +/** One account's secret of the given kind, from whichever backend holds it. */ +export async function getSecret(userId: string, kind: SecretKind): Promise { const backend = await getBackend(); - if (backend === 'keyring') return readKeyring(TOKEN_ACCOUNT); - return readAuthFile().token; -} - -export async function getProxyPassword(): Promise { - const backend = await getBackend(); - if (backend === 'keyring') return readKeyring(PROXY_PASSWORD_ACCOUNT); - return readAuthFile().proxy?.password; + if (backend === 'keyring') return readKeyring(keyringKey(userId, kind)); + return readProfileSecret(userId, kind); } /** - * Persist token. When `skipIfUnchanged` is true and the stored value already matches, - * the write is skipped. This avoids macOS Keychain prompts on every command. + * Persist one account's secret. When `skipIfUnchanged` is true and the stored value already + * matches, the write is skipped. This avoids macOS Keychain prompts on every command. */ -export async function setToken(token: string, opts: { skipIfUnchanged?: boolean } = {}): Promise { +export async function setSecret( + userId: string, + kind: SecretKind, + value: string, + opts: { skipIfUnchanged?: boolean } = {}, +): Promise { const backend = await getBackend(); - if (opts.skipIfUnchanged) { - const existing = backend === 'keyring' ? await readKeyring(TOKEN_ACCOUNT) : readAuthFile().token; - if (existing === token) return; - } + if (opts.skipIfUnchanged && (await getSecret(userId, kind)) === value) return; if (backend === 'keyring') { try { - await writeKeyring(TOKEN_ACCOUNT, token); + await writeKeyring(keyringKey(userId, kind), value); return; } catch (err) { cliDebugPrint('credentials', 'keyring write failed; falling back to file', err); @@ -163,45 +195,24 @@ export async function setToken(token: string, opts: { skipIfUnchanged?: boolean } } - const data = readAuthFile(); - data.token = token; - data.secretsBackend = 'file'; - writeAuthFile(data); -} - -export async function setProxyPassword(password: string, opts: { skipIfUnchanged?: boolean } = {}): Promise { - const backend = await getBackend(); - if (opts.skipIfUnchanged) { - const existing = backend === 'keyring' ? await readKeyring(PROXY_PASSWORD_ACCOUNT) : readAuthFile().proxy?.password; - if (existing === password) return; - } - - if (backend === 'keyring') { - try { - await writeKeyring(PROXY_PASSWORD_ACCOUNT, password); - return; - } catch (err) { - cliDebugPrint('credentials', 'keyring write failed; falling back to file', err); - downgradeBackendToFile(); - } - } - - const data = readAuthFile(); - data.proxy = { ...data.proxy, password }; - data.secretsBackend = 'file'; - writeAuthFile(data); + writeProfileSecret(userId, kind, value); } /** - * Remove the token and proxy-password entries from the OS keyring. Always attempts the - * keyring deletes even when the current backend is `file`, so toggling - * `APIFY_DISABLE_KEYRING=1` between login and logout does not orphan entries the user - * has no in-CLI way to discover. Plaintext secrets in `auth.json` are the caller's - * responsibility (e.g. `logout` removes the whole file). + * Remove one profile's keyring entries, plus the fixed-name entries used before secrets were keyed + * by user. Always attempts the keyring deletes even when the current backend is `file`, so toggling + * `APIFY_DISABLE_KEYRING=1` between login and logout does not orphan entries the user has no + * in-CLI way to discover. + * + * The keyring has no listing API, so `auth.json` is the only index of what it holds. Call this + * before the profile leaves the file, or its entries become unreachable. Secrets stored in + * `auth.json` itself go with the profile that holds them. */ -export async function clearKeyringSecrets(): Promise { - await deleteKeyring(TOKEN_ACCOUNT); - await deleteKeyring(PROXY_PASSWORD_ACCOUNT); +export async function clearKeyringSecrets(userId?: string): Promise { + for (const kind of SECRET_KINDS) { + if (userId) await deleteKeyring(keyringKey(userId, kind)); + await deleteKeyring(legacyKeyringKey(kind)); + } } /** @@ -230,8 +241,10 @@ export async function ensureMigrated(): Promise { } try { - if (file.token) await writeKeyring(TOKEN_ACCOUNT, file.token); - if (file.proxy?.password) await writeKeyring(PROXY_PASSWORD_ACCOUNT, file.proxy.password); + if (file.token) await writeKeyring(legacyKeyringKey('token'), file.token); + if (file.proxy?.password) { + await writeKeyring(legacyKeyringKey('proxy-password'), file.proxy.password); + } } catch (err) { cliDebugPrint('credentials', 'keyring write failed during migration; falling back to file', err); downgradeBackendToFile(); @@ -255,3 +268,118 @@ export async function ensureMigrated(): Promise { })(); return migrationPromise; } + +/** + * Drops secrets there is no user ID to file under. `auth.json.v1.bak` still holds whatever the v1 + * file had, so the way back is a re-login rather than a lost account. + * + * The keyring deletes run whatever the current backend is, for the same reason + * {@link clearKeyringSecrets} does them: toggling `APIFY_DISABLE_KEYRING=1` would otherwise leave + * entries behind that nothing records and no command can reach. + */ +async function dropUnkeyedSecrets(file: AuthFile): Promise { + for (const kind of SECRET_KINDS) await deleteKeyring(legacyKeyringKey(kind)); + + if (file.token === undefined && file.proxy === undefined) return; + + delete file.token; + delete file.proxy; + writeAuthFile(file); +} + +/** + * Write the new entry, verify it reads back, then delete the old one. The reverse order loses the + * secret when the delete succeeds and the write does not. + */ +async function keyKeyringSecrets(userId: string): Promise { + for (const kind of SECRET_KINDS) { + const legacy = legacyKeyringKey(kind); + const value = await readKeyring(legacy); + if (value === undefined) continue; + + // A failure earlier in this loop downgrades the backend for the rest of the process, so + // the secrets after it belong in the file rather than under a name nothing will read. + if ((await getBackend()) === 'keyring') { + const target = keyringKey(userId, kind); + + try { + await writeKeyring(target, value); + if ((await readKeyring(target)) === value) await deleteKeyring(legacy); + continue; + } catch (err) { + cliDebugPrint('credentials', 'keyring write failed while keying secrets by user', err); + downgradeBackendToFile(); + } + } + + writeProfileSecret(userId, kind, value); + if (readProfileSecret(userId, kind) === value) await deleteKeyring(legacy); + } +} + +/** One atomic write moves the secrets into the profile and clears the top level. */ +function keyFileSecrets(userId: string, file: AuthFile): void { + const profile = file.profiles?.[userId]; + if (!profile) return; + + const { token } = file; + const proxyPassword = file.proxy?.password; + if (token === undefined && proxyPassword === undefined) return; + + if (token !== undefined) profile.token = token; + if (proxyPassword !== undefined) profile.proxy = { ...profile.proxy, password: proxyPassword }; + + delete file.token; + delete file.proxy; + file.secretsBackend = 'file'; + writeAuthFile(file); +} + +/** + * Moves secrets off the fixed names they shared onto keys that carry the user ID, so a second + * account cannot overwrite the first one's token. + * + * Runs after `ensureAuthFileCurrent()` — the user ID comes from the v2 file. A v2 file whose + * secrets still sit under the old names is a supported state: every user is in it between the two + * releases, and the two migrations stay independent. + * + * Idempotent, single-flight, and it never throws — a migration failure must not block a command. + */ +export async function ensureSecretsKeyed(): Promise { + keyingPromise ??= (async () => { + try { + const file = readAuthFile(); + if (file.version !== AUTH_FILE_VERSION) return; + + const backend = await getBackend(); + const userId = file.activeProfile; + + if (!userId) { + await dropUnkeyedSecrets(file); + return; + } + + if (backend === 'keyring') { + await keyKeyringSecrets(userId); + return; + } + + keyFileSecrets(userId, file); + } catch (err) { + cliDebugPrint('credentials', 'keying secrets by user failed', err); + } + })(); + + return keyingPromise; +} + +/** + * Every storage migration, in the one order that works: v1 secrets out of `auth.json`, then the v2 + * profile shape, then secrets keyed by user ID — the last step needs the user ID the second writes. + * Throws only for a file a newer CLI wrote. + */ +export async function ensureCredentialsCurrent(): Promise { + await ensureMigrated(); + await ensureAuthFileCurrent(); + await ensureSecretsKeyed(); +} diff --git a/src/lib/utils.ts b/src/lib/utils.ts index 2e4e108ca..dbe0a70e7 100644 --- a/src/lib/utils.ts +++ b/src/lib/utils.ts @@ -32,7 +32,7 @@ import { SOURCE_FILE_FORMATS, } from '@apify/consts'; -import { ensureAuthFileCurrent, lookUpActiveProfile } from './auth-file.js'; +import { lookUpActiveProfile } from './auth-file.js'; import { describeAuthFailure, getApifyClientOptions, resolveAuth } from './auth.js'; import { AUTH_FILE_PATH, @@ -42,7 +42,7 @@ import { MINIMUM_SUPPORTED_PYTHON_VERSION, SUPPORTED_NODEJS_VERSION, } from './consts.js'; -import { ensureMigrated, getProxyPassword, getToken } from './credentials.js'; +import { ensureCredentialsCurrent, getSecret } from './credentials.js'; import { deleteFile, ensureFolderExistsSync, rimrafPromised } from './files.js'; import { useCLIMetadata } from './hooks/useCLIMetadata.js'; import { inputFileRegExp, TEMP_INPUT_KEY_PREFIX } from './input-key.js'; @@ -89,35 +89,29 @@ export const getLocalRequestQueuePath = (storeId?: string) => { * stored. Secrets come from whichever backend holds them; the metadata comes from auth.json. */ export const getLocalUserInfo = async (): Promise => { - await ensureMigrated(); - await ensureAuthFileCurrent(); + await ensureCredentialsCurrent(); const { profile, missingProfile } = lookUpActiveProfile(); - const result: AuthJSON = {}; - if (profile) { - result.id = profile.id; - if (profile.username) result.username = profile.username; - if (profile.organizationOwnerUserId) result.organizationOwnerUserId = profile.organizationOwnerUserId; + // A profile the file does not hold is reported rather than swallowed: the commands that build + // `/` lookups would otherwise fail with a misleading "not found". + if (missingProfile) { + throw new Error( + `Your active profile "${missingProfile}" is missing from ${AUTH_FILE_PATH()}. Run "apify login" to log in again.`, + ); } - const token = await getToken(); - if (token) result.token = token; + if (!profile) return {}; - const proxyPassword = await getProxyPassword(); - if (proxyPassword) result.proxy = { password: proxyPassword }; + const result: AuthJSON = { id: profile.id }; + if (profile.username) result.username = profile.username; + if (profile.organizationOwnerUserId) result.organizationOwnerUserId = profile.organizationOwnerUserId; - // A token with no profile behind it is reported rather than swallowed: the commands that build - // `/` lookups would otherwise fail with a misleading "not found". - if (!profile) { - if (!result.token) return {}; + const token = await getSecret(profile.id, 'token'); + if (token) result.token = token; - throw new Error( - missingProfile - ? `Your active profile "${missingProfile}" is missing from ${AUTH_FILE_PATH()}. Run "apify login" to log in again.` - : 'Stale credentials found without user metadata. Run "apify login" again.', - ); - } + const proxyPassword = await getSecret(profile.id, 'proxy-password'); + if (proxyPassword) result.proxy = { password: proxyPassword }; return result; }; diff --git a/test/__setup__/auth-file.ts b/test/__setup__/auth-file.ts index 6e617345d..2c8dea411 100644 --- a/test/__setup__/auth-file.ts +++ b/test/__setup__/auth-file.ts @@ -3,8 +3,12 @@ import { readFileSync } from 'node:fs'; import type { AuthFile, AuthProfile } from '../../src/lib/auth-file.js'; +import { AUTH_FILE_VERSION } from '../../src/lib/auth-file.js'; import { AUTH_FILE_PATH } from '../../src/lib/consts.js'; +/** The user ID the fixtures below key their single profile by. */ +export const TEST_USER_ID = 'uid'; + /** The raw file, for assertions about the version, the backend marker, or where secrets landed. */ export function readAuthFile(): AuthFile { return JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf-8')) as AuthFile; @@ -19,6 +23,25 @@ export function readActiveProfile(): (AuthProfile & { id: string }) | undefined return profile ? { id: activeProfile, ...profile } : undefined; } +/** A v2 `auth.json` holding one profile, the shape a login writes. */ +export function v2AuthFile(profile: Partial = {}, rest: Partial = {}): AuthFile { + return { + version: AUTH_FILE_VERSION, + activeProfile: TEST_USER_ID, + profiles: { + [TEST_USER_ID]: { + username: 'me', + name: null, + authMethod: 'token', + expiresAt: null, + hasRefreshToken: false, + ...profile, + }, + }, + ...rest, + }; +} + /** A v1 `auth.json`, the shape every CLI before the profile migration wrote. */ export function v1AuthFile(overrides: Record = {}) { return { diff --git a/test/__setup__/keyring-mock.ts b/test/__setup__/keyring-mock.ts index 902896793..6bfdf53cb 100644 --- a/test/__setup__/keyring-mock.ts +++ b/test/__setup__/keyring-mock.ts @@ -3,8 +3,16 @@ * `vi.mock('@napi-rs/keyring', () => import('/keyring-mock.js'))`. */ -export const KEYRING_TOKEN_KEY = 'com.apify.cli:token'; -export const KEYRING_PROXY_PASSWORD_KEY = 'com.apify.cli:proxy-password'; +/** The fixed names secrets shared before they were keyed by user. */ +export const LEGACY_KEYRING_TOKEN_KEY = 'com.apify.cli:token'; +export const LEGACY_KEYRING_PROXY_PASSWORD_KEY = 'com.apify.cli:proxy-password'; + +/** + * One service per kind, the user ID as the account. Spelled out here rather than imported so the + * test fails when the production key scheme changes without anyone meaning to change it. + */ +export const keyringTokenKey = (userId: string) => `com.apify.cli.token:${userId}`; +export const keyringProxyPasswordKey = (userId: string) => `com.apify.cli.proxy-password:${userId}`; export const keyringStore = new Map(); diff --git a/test/local/commands/auth.test.ts b/test/local/commands/auth.test.ts index b1ca6e8c7..80c0c9299 100644 --- a/test/local/commands/auth.test.ts +++ b/test/local/commands/auth.test.ts @@ -2,15 +2,15 @@ import { existsSync, statSync } from 'node:fs'; import process from 'node:process'; import { AUTH_FILE_PATH, CommandExitCodes } from '../../../src/lib/consts.js'; -import { getToken } from '../../../src/lib/credentials.js'; +import { getSecret } from '../../../src/lib/credentials.js'; import { readActiveProfile, readAuthFile } from '../../__setup__/auth-file.js'; import { useAuthSetup, useKeyringBackend } from '../../__setup__/hooks/useAuthSetup.js'; import { useConsoleSpy } from '../../__setup__/hooks/useConsoleSpy.js'; import { - KEYRING_PROXY_PASSWORD_KEY, - KEYRING_TOKEN_KEY, + keyringProxyPasswordKey, keyringSetKeys, keyringStore, + keyringTokenKey, resetKeyringMock, } from '../../__setup__/keyring-mock.js'; @@ -74,7 +74,7 @@ describe('auth commands', () => { it('login stores the token and one profile keyed by user ID', async () => { await login(); - expect(readAuthFile()).toMatchObject({ version: 2, token: TOKEN, secretsBackend: 'file' }); + expect(readAuthFile()).toMatchObject({ version: 2, secretsBackend: 'file' }); expect(readActiveProfile()).toEqual({ id: 'uid', username: 'me', @@ -82,6 +82,8 @@ describe('auth commands', () => { authMethod: 'token', expiresAt: null, hasRefreshToken: false, + token: TOKEN, + proxy: { password: 'pw' }, }); expect(lastErrorMessage()).toContain('You are logged in to Apify as me'); }); @@ -104,7 +106,7 @@ describe('auth commands', () => { await testRunCommand(AuthLogoutCommand, {}); expect(existsSync(AUTH_FILE_PATH())).toBe(false); - expect(await getToken()).toBeUndefined(); + expect(await getSecret('uid', 'token')).toBeUndefined(); }); it('logging in as another account replaces the stored profile', async () => { @@ -115,10 +117,10 @@ describe('auth commands', () => { await login('apify_api_other_token'); const authFile = readAuthFile(); - expect(authFile).toMatchObject({ activeProfile: 'uid2', token: 'apify_api_other_token' }); + expect(authFile).toMatchObject({ activeProfile: 'uid2' }); // Additive login is a later stage; until then the old profile must not linger. expect(Object.keys(authFile.profiles!)).toEqual(['uid2']); - expect(readActiveProfile()).toMatchObject({ username: 'other' }); + expect(readActiveProfile()).toMatchObject({ username: 'other', token: 'apify_api_other_token' }); }); it('login with an invalid token stores nothing and fails the command', async () => { @@ -136,7 +138,7 @@ describe('auth commands', () => { vitest.stubEnv('APIFY_TOKEN', 'apify_api_env_token'); await login(); - expect(await getToken()).toBe(TOKEN); + expect(await getSecret('uid', 'token')).toBe(TOKEN); expect(lastErrorMessage()).toContain('APIFY_TOKEN is set, so other commands keep using that token'); }); @@ -180,7 +182,7 @@ describe('auth commands', () => { await testRunCommand(AuthTokenCommand, {}); expect(lastLogMessage()).toBe('apify_api_env_token'); - expect(await getToken()).toBe(TOKEN); + expect(await getSecret('uid', 'token')).toBe(TOKEN); expect(readActiveProfile()).toMatchObject({ username: 'me' }); }); }); @@ -191,8 +193,8 @@ describe('auth commands', () => { it('login stores the secrets in the keyring and keeps them out of auth.json', async () => { await login(); - expect(keyringStore.get(KEYRING_TOKEN_KEY)).toBe(TOKEN); - expect(keyringStore.get(KEYRING_PROXY_PASSWORD_KEY)).toBe('pw'); + expect(keyringStore.get(keyringTokenKey('uid'))).toBe(TOKEN); + expect(keyringStore.get(keyringProxyPasswordKey('uid'))).toBe('pw'); const authFile = readAuthFile(); expect(authFile).toMatchObject({ version: 2, secretsBackend: 'keyring' }); @@ -206,7 +208,7 @@ describe('auth commands', () => { await login(); await login(); - expect(keyringSetKeys.filter((key) => key === KEYRING_TOKEN_KEY)).toHaveLength(1); + expect(keyringSetKeys.filter((key) => key === keyringTokenKey('uid'))).toHaveLength(1); }); it('auth token prints the token from the keyring', async () => { @@ -216,6 +218,28 @@ describe('auth commands', () => { expect(lastLogMessage()).toBe(TOKEN); }); + it('logging in as another account clears the outgoing account keyring entries', async () => { + clientState.user = { id: 'uid', username: 'me', proxy: { password: 'pw' } }; + await login(); + + clientState.user = { id: 'uid2', username: 'other', proxy: { password: 'pw2' } }; + await login('apify_api_other_token'); + + // auth.json no longer names `uid`, so entries left behind would be unreachable forever. + expect(keyringStore.get(keyringTokenKey('uid'))).toBeUndefined(); + expect(keyringStore.get(keyringProxyPasswordKey('uid'))).toBeUndefined(); + expect(keyringStore.get(keyringTokenKey('uid2'))).toBe('apify_api_other_token'); + expect(keyringStore.get(keyringProxyPasswordKey('uid2'))).toBe('pw2'); + }); + + it('logging in as the same account keeps its keyring entries', async () => { + await login(); + await login(); + + expect(keyringStore.get(keyringTokenKey('uid'))).toBe(TOKEN); + expect(keyringStore.get(keyringProxyPasswordKey('uid'))).toBe('pw'); + }); + it('logout clears the keyring and removes auth.json', async () => { await login(); await testRunCommand(AuthLogoutCommand, {}); diff --git a/test/local/lib/auth-file.test.ts b/test/local/lib/auth-file.test.ts index 8afc82c81..dff7f9d2a 100644 --- a/test/local/lib/auth-file.test.ts +++ b/test/local/lib/auth-file.test.ts @@ -11,13 +11,13 @@ import { setActiveProfile, } from '../../../src/lib/auth-file.js'; import { AUTH_FILE_PATH, GLOBAL_CONFIGS_FOLDER } from '../../../src/lib/consts.js'; -import { ensureMigrated, getProxyPassword, getToken } from '../../../src/lib/credentials.js'; +import { ensureCredentialsCurrent, ensureMigrated, getSecret } from '../../../src/lib/credentials.js'; import { getLocalUserInfo } from '../../../src/lib/utils.js'; import { readActiveProfile, readAuthFile, v1AuthFile } from '../../__setup__/auth-file.js'; import { useAuthSetup, useKeyringBackend } from '../../__setup__/hooks/useAuthSetup.js'; import { - KEYRING_PROXY_PASSWORD_KEY, - KEYRING_TOKEN_KEY, + LEGACY_KEYRING_PROXY_PASSWORD_KEY, + LEGACY_KEYRING_TOKEN_KEY, keyringStore, resetKeyringMock, } from '../../__setup__/keyring-mock.js'; @@ -70,11 +70,12 @@ describe('auth.json v2', () => { it('migrates state C and keeps the secrets in the file', async () => { write(v1AuthFile({ secretsBackend: 'file' })); - await ensureAuthFileCurrent(); + await ensureCredentialsCurrent(); - expect(readAuthFile()).toMatchObject({ version: 2, secretsBackend: 'file', token: 'apify_api_v1_token' }); - expect(await getToken()).toBe('apify_api_v1_token'); - expect(await getProxyPassword()).toBe('pw'); + expect(readAuthFile()).toMatchObject({ version: 2, secretsBackend: 'file' }); + expect(readActiveProfile()).toMatchObject({ token: 'apify_api_v1_token', proxy: { password: 'pw' } }); + expect(await getSecret('uid', 'token')).toBe('apify_api_v1_token'); + expect(await getSecret('uid', 'proxy-password')).toBe('pw'); }); it('drops the fields nothing in the CLI reads', async () => { @@ -138,19 +139,15 @@ describe('auth.json v2', () => { expect(existsSync(AUTH_BACKUP_FILE_PATH())).toBe(false); }); - it('keeps the secrets of a v1 file that has no user ID, so the next command asks for a re-login', async () => { + it('drops the secrets of a v1 file that has no user ID, so the next command asks for a re-login', async () => { write({ token: 'apify_api_v1_token', secretsBackend: 'file' }); - await ensureAuthFileCurrent(); + await ensureCredentialsCurrent(); - expect(readAuthFile()).toEqual({ - version: 2, - profiles: {}, - secretsBackend: 'file', - token: 'apify_api_v1_token', - }); + expect(readAuthFile()).toEqual({ version: 2, profiles: {}, secretsBackend: 'file' }); + // The backup keeps the plaintext token, so the way back is a re-login, not a lost account. expect(readBackup()).toEqual({ token: 'apify_api_v1_token', secretsBackend: 'file' }); - await expect(getLocalUserInfo()).rejects.toThrow('Stale credentials found without user metadata'); + await expect(getLocalUserInfo()).resolves.toEqual({}); }); }); @@ -179,10 +176,10 @@ describe('auth.json v2', () => { await expect(getLocalUserInfo()).rejects.toThrow('Your active profile "gone" is missing'); }); - it('is logged out when the missing profile leaves no token behind either', async () => { + it('names the missing profile even when no secret is left behind', async () => { write({ version: 2, activeProfile: 'gone', profiles: {}, secretsBackend: 'file' }); - await expect(getLocalUserInfo()).resolves.toEqual({}); + await expect(getLocalUserInfo()).rejects.toThrow('Your active profile "gone" is missing'); }); }); @@ -215,8 +212,8 @@ describe('auth.json v2', () => { // State B in the wild: secrets already in the keyring, auth.json holding only metadata. it('migrates state B without touching the keyring', async () => { - keyringStore.set(KEYRING_TOKEN_KEY, 'tok_kr'); - keyringStore.set(KEYRING_PROXY_PASSWORD_KEY, 'pw_kr'); + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok_kr'); + keyringStore.set(LEGACY_KEYRING_PROXY_PASSWORD_KEY, 'pw_kr'); write({ id: 'uid', username: 'me', email: 'me@example.com', secretsBackend: 'keyring' }); await ensureMigrated(); @@ -243,7 +240,7 @@ describe('auth.json v2', () => { await ensureMigrated(); await ensureAuthFileCurrent(); - expect(keyringStore.get(KEYRING_TOKEN_KEY)).toBe('apify_api_v1_token'); + expect(keyringStore.get(LEGACY_KEYRING_TOKEN_KEY)).toBe('apify_api_v1_token'); expect(readAuthFile()).toEqual({ version: 2, activeProfile: 'uid', diff --git a/test/local/lib/auth.test.ts b/test/local/lib/auth.test.ts index 702970f7d..d99ade2ea 100644 --- a/test/local/lib/auth.test.ts +++ b/test/local/lib/auth.test.ts @@ -2,7 +2,7 @@ import { existsSync } from 'node:fs'; import { loginWithToken, resolveAuth } from '../../../src/lib/auth.js'; import { AUTH_FILE_PATH } from '../../../src/lib/consts.js'; -import { getProxyPassword, getToken, setToken } from '../../../src/lib/credentials.js'; +import { getSecret } from '../../../src/lib/credentials.js'; import { getLoggedClientOrThrow } from '../../../src/lib/utils.js'; import { readActiveProfile } from '../../__setup__/auth-file.js'; import { useAuthSetup } from '../../__setup__/hooks/useAuthSetup.js'; @@ -144,8 +144,8 @@ describe('auth', () => { it('saves the token, the proxy password and the account metadata', async () => { await loginWithToken(STORED); - expect(await getToken()).toBe(STORED); - expect(await getProxyPassword()).toBe('pw'); + expect(await getSecret('uid', 'token')).toBe(STORED); + expect(await getSecret('uid', 'proxy-password')).toBe('pw'); expect(readActiveProfile()).toMatchObject({ id: 'uid', username: 'me' }); }); @@ -161,7 +161,7 @@ describe('auth', () => { await loginWithToken(STORED); - expect(await getToken()).toBe(STORED); + expect(await getSecret('uid', 'token')).toBe(STORED); }); }); @@ -208,16 +208,16 @@ describe('auth', () => { await resolveAuth(); - expect(await getToken()).toBe(STORED); + expect(await getSecret('uid', 'token')).toBe(STORED); expect(readActiveProfile()).toMatchObject({ username: 'me' }); }); it('resolving a token the command was given leaves the stored login untouched', async () => { - await setToken(STORED); + await loginWithToken(STORED); await resolveAuth(FLAG); - expect(await getToken()).toBe(STORED); + expect(await getSecret('uid', 'token')).toBe(STORED); }); }); }); diff --git a/test/local/lib/credentials.test.ts b/test/local/lib/credentials.test.ts index 549d9960e..d0c2c4342 100644 --- a/test/local/lib/credentials.test.ts +++ b/test/local/lib/credentials.test.ts @@ -11,19 +11,21 @@ import { __resetCredentialsForTests, clearKeyringSecrets, ensureMigrated, + ensureSecretsKeyed, getBackend, - getProxyPassword, - getToken, - setProxyPassword, - setToken, + getSecret, + setSecret, } from '../../../src/lib/credentials.js'; import { getLocalUserInfo } from '../../../src/lib/utils.js'; +import { TEST_USER_ID, v2AuthFile } from '../../__setup__/auth-file.js'; import { - KEYRING_PROXY_PASSWORD_KEY, - KEYRING_TOKEN_KEY, + LEGACY_KEYRING_PROXY_PASSWORD_KEY, + LEGACY_KEYRING_TOKEN_KEY, keyringFailures, + keyringProxyPasswordKey, keyringSetKeys, keyringStore, + keyringTokenKey, resetKeyringMock, } from '../../__setup__/keyring-mock.js'; @@ -46,6 +48,11 @@ const writeAuthFile = (data: Record) => { const readAuthFile = () => JSON.parse(readFileSync(AUTH_FILE_PATH(), 'utf-8')); +const readProfile = () => readAuthFile().profiles[TEST_USER_ID]; + +const TOKEN_KEY = keyringTokenKey(TEST_USER_ID); +const PROXY_PASSWORD_KEY = keyringProxyPasswordKey(TEST_USER_ID); + describe('credentials', () => { beforeEach(() => { vitest.stubEnv('__APIFY_INTERNAL_TEST_AUTH_PATH__', cryptoRandomObjectId(12)); @@ -92,59 +99,70 @@ describe('credentials', () => { describe('file backend', () => { beforeEach(() => { vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); + writeAuthFile(v2AuthFile() as Record); + writeFileSyncSpy.mockClear(); }); - it('round-trips the token through auth.json', async () => { - await setToken('tok_123'); - expect(await getToken()).toBe('tok_123'); - const file = readAuthFile(); - expect(file.token).toBe('tok_123'); - expect(file.secretsBackend).toBe('file'); + it('round-trips the token through the profile', async () => { + await setSecret(TEST_USER_ID, 'token', 'tok_123'); + expect(await getSecret(TEST_USER_ID, 'token')).toBe('tok_123'); + expect(readProfile().token).toBe('tok_123'); + expect(readAuthFile().token).toBeUndefined(); + expect(readAuthFile().secretsBackend).toBe('file'); }); - it('round-trips the proxy password through auth.json', async () => { - await setProxyPassword('pw_abc'); - expect(await getProxyPassword()).toBe('pw_abc'); - expect(readAuthFile().proxy).toEqual({ password: 'pw_abc' }); + it('round-trips the proxy password through the profile', async () => { + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc'); + expect(await getSecret(TEST_USER_ID, 'proxy-password')).toBe('pw_abc'); + expect(readProfile().proxy).toEqual({ password: 'pw_abc' }); }); - it('preserves other proxy fields when only the password changes', async () => { - writeAuthFile({ proxy: { password: 'old', groups: [{ name: 'g' }] } } as never); - await setProxyPassword('new'); - expect(readAuthFile().proxy).toEqual({ password: 'new', groups: [{ name: 'g' }] }); + it('leaves another profile alone', async () => { + const file = v2AuthFile(); + file.profiles!.other = { ...file.profiles![TEST_USER_ID], token: 'tok_other' }; + writeAuthFile(file as Record); + + await setSecret(TEST_USER_ID, 'token', 'tok_123'); + expect(readAuthFile().profiles.other.token).toBe('tok_other'); + }); + + it('does nothing when the profile is not in the file', async () => { + writeAuthFile({ version: 2, activeProfile: 'gone', profiles: {} }); + await setSecret('gone', 'token', 'tok_123'); + expect(await getSecret('gone', 'token')).toBeUndefined(); }); it('skipIfUnchanged skips the write when the stored token matches', async () => { - await setToken('tok_123'); + await setSecret(TEST_USER_ID, 'token', 'tok_123'); writeFileSyncSpy.mockClear(); - await setToken('tok_123', { skipIfUnchanged: true }); + await setSecret(TEST_USER_ID, 'token', 'tok_123', { skipIfUnchanged: true }); expect(authFileWrites()).toHaveLength(0); }); it('skipIfUnchanged skips the write when the stored proxy password matches', async () => { - await setProxyPassword('pw_abc'); + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc'); writeFileSyncSpy.mockClear(); - await setProxyPassword('pw_abc', { skipIfUnchanged: true }); + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc', { skipIfUnchanged: true }); expect(authFileWrites()).toHaveLength(0); }); it('skipIfUnchanged still writes when the value differs', async () => { - await setToken('tok_123'); + await setSecret(TEST_USER_ID, 'token', 'tok_123'); writeFileSyncSpy.mockClear(); - await setToken('tok_456', { skipIfUnchanged: true }); + await setSecret(TEST_USER_ID, 'token', 'tok_456', { skipIfUnchanged: true }); expect(authFileWrites()).toHaveLength(1); - expect(await getToken()).toBe('tok_456'); + expect(await getSecret(TEST_USER_ID, 'token')).toBe('tok_456'); }); it('writes auth.json with mode 0600', async () => { - await setToken('tok_123'); + await setSecret(TEST_USER_ID, 'token', 'tok_123'); expect(writeFileSyncSpy).toHaveBeenCalledWith(expect.stringContaining(AUTH_FILE_PATH()), expect.any(String), { mode: 0o600, }); }); it.skipIf(process.platform === 'win32')('creates auth.json readable only by the owner', async () => { - await setToken('tok_123'); + await setSecret(TEST_USER_ID, 'token', 'tok_123'); expect(statSync(AUTH_FILE_PATH()).mode & 0o777).toBe(0o600); }); }); @@ -154,83 +172,111 @@ describe('credentials', () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); }); - it('round-trips the token through the keyring and keeps it out of auth.json', async () => { - await setToken('tok_123'); - expect(await getToken()).toBe('tok_123'); - expect(keyringStore.get(KEYRING_TOKEN_KEY)).toBe('tok_123'); + it('keys the token by user ID and keeps it out of auth.json', async () => { + await setSecret(TEST_USER_ID, 'token', 'tok_123'); + expect(await getSecret(TEST_USER_ID, 'token')).toBe('tok_123'); + expect(keyringStore.get(TOKEN_KEY)).toBe('tok_123'); + expect(keyringStore.get(LEGACY_KEYRING_TOKEN_KEY)).toBeUndefined(); expect(existsSync(AUTH_FILE_PATH())).toBe(false); }); - it('round-trips the proxy password through the keyring and keeps it out of auth.json', async () => { - await setProxyPassword('pw_abc'); - expect(await getProxyPassword()).toBe('pw_abc'); - expect(keyringStore.get(KEYRING_PROXY_PASSWORD_KEY)).toBe('pw_abc'); + it('keys the proxy password by user ID and keeps it out of auth.json', async () => { + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc'); + expect(await getSecret(TEST_USER_ID, 'proxy-password')).toBe('pw_abc'); + expect(keyringStore.get(PROXY_PASSWORD_KEY)).toBe('pw_abc'); expect(existsSync(AUTH_FILE_PATH())).toBe(false); }); - it('clearKeyringSecrets() removes the token and proxy entries from the keyring', async () => { - await setToken('tok_123'); - await setProxyPassword('pw_abc'); - await clearKeyringSecrets(); - expect(await getToken()).toBeUndefined(); - expect(await getProxyPassword()).toBeUndefined(); + it('gives two accounts their own entries', async () => { + await setSecret(TEST_USER_ID, 'token', 'tok_123'); + await setSecret('other', 'token', 'tok_other'); + expect(keyringStore.get(TOKEN_KEY)).toBe('tok_123'); + expect(keyringStore.get(keyringTokenKey('other'))).toBe('tok_other'); }); it('skipIfUnchanged skips the keyring write when the stored token matches', async () => { - await setToken('tok_123'); - await setToken('tok_123', { skipIfUnchanged: true }); - expect(keyringSetKeys.filter((key) => key === KEYRING_TOKEN_KEY)).toHaveLength(1); + await setSecret(TEST_USER_ID, 'token', 'tok_123'); + await setSecret(TEST_USER_ID, 'token', 'tok_123', { skipIfUnchanged: true }); + expect(keyringSetKeys.filter((key) => key === TOKEN_KEY)).toHaveLength(1); expect(authFileWrites()).toHaveLength(0); }); it('skipIfUnchanged skips the keyring write when the stored proxy password matches', async () => { - await setProxyPassword('pw_abc'); - await setProxyPassword('pw_abc', { skipIfUnchanged: true }); - expect(keyringSetKeys.filter((key) => key === KEYRING_PROXY_PASSWORD_KEY)).toHaveLength(1); + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc'); + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc', { skipIfUnchanged: true }); + expect(keyringSetKeys.filter((key) => key === PROXY_PASSWORD_KEY)).toHaveLength(1); expect(authFileWrites()).toHaveLength(0); }); - it('falls back to auth.json when the keyring token write fails', async () => { - keyringFailures.add(KEYRING_TOKEN_KEY); - await setToken('tok_123'); + it('falls back to the profile when the keyring token write fails', async () => { + writeAuthFile(v2AuthFile() as Record); + keyringFailures.add(TOKEN_KEY); + await setSecret(TEST_USER_ID, 'token', 'tok_123'); - expect(keyringStore.get(KEYRING_TOKEN_KEY)).toBeUndefined(); - expect(readAuthFile()).toEqual({ token: 'tok_123', secretsBackend: 'file' }); + expect(keyringStore.get(TOKEN_KEY)).toBeUndefined(); + expect(readProfile().token).toBe('tok_123'); + expect(readAuthFile().secretsBackend).toBe('file'); expect(await getBackend()).toBe('file'); - expect(await getToken()).toBe('tok_123'); + expect(await getSecret(TEST_USER_ID, 'token')).toBe('tok_123'); }); it('keeps using auth.json for later writes after a keyring failure', async () => { - keyringFailures.add(KEYRING_TOKEN_KEY); - await setToken('tok_123'); + writeAuthFile(v2AuthFile() as Record); + keyringFailures.add(TOKEN_KEY); + await setSecret(TEST_USER_ID, 'token', 'tok_123'); - await setProxyPassword('pw_abc'); - expect(keyringStore.get(KEYRING_PROXY_PASSWORD_KEY)).toBeUndefined(); - expect(readAuthFile().proxy).toEqual({ password: 'pw_abc' }); + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc'); + expect(keyringStore.get(PROXY_PASSWORD_KEY)).toBeUndefined(); + expect(readProfile().proxy).toEqual({ password: 'pw_abc' }); }); - it('falls back to auth.json when the keyring proxy password write fails', async () => { - keyringFailures.add(KEYRING_PROXY_PASSWORD_KEY); - await setProxyPassword('pw_abc'); + it('falls back to the profile when the keyring proxy password write fails', async () => { + writeAuthFile(v2AuthFile() as Record); + keyringFailures.add(PROXY_PASSWORD_KEY); + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc'); - expect(keyringStore.get(KEYRING_PROXY_PASSWORD_KEY)).toBeUndefined(); - expect(readAuthFile()).toEqual({ proxy: { password: 'pw_abc' }, secretsBackend: 'file' }); - expect(await getProxyPassword()).toBe('pw_abc'); + expect(keyringStore.get(PROXY_PASSWORD_KEY)).toBeUndefined(); + expect(readProfile().proxy).toEqual({ password: 'pw_abc' }); + expect(await getSecret(TEST_USER_ID, 'proxy-password')).toBe('pw_abc'); }); }); describe('clearKeyringSecrets()', () => { - it('clears the keyring token entry even when APIFY_DISABLE_KEYRING=1 is set at logout time', async () => { + beforeEach(() => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); - await setToken('tok_123'); - expect(keyringStore.get(KEYRING_TOKEN_KEY)).toBe('tok_123'); + }); + + it('removes the profile entries and the fixed-name ones left from before', async () => { + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok_old'); + keyringStore.set(LEGACY_KEYRING_PROXY_PASSWORD_KEY, 'pw_old'); + await setSecret(TEST_USER_ID, 'token', 'tok_123'); + await setSecret(TEST_USER_ID, 'proxy-password', 'pw_abc'); + + await clearKeyringSecrets(TEST_USER_ID); + + expect(keyringStore.size).toBe(0); + }); + + it('leaves other profiles alone', async () => { + await setSecret(TEST_USER_ID, 'token', 'tok_123'); + await setSecret('other', 'token', 'tok_other'); + + await clearKeyringSecrets(TEST_USER_ID); + + expect(keyringStore.get(TOKEN_KEY)).toBeUndefined(); + expect(keyringStore.get(keyringTokenKey('other'))).toBe('tok_other'); + }); + + it('clears the keyring entries even when APIFY_DISABLE_KEYRING=1 is set at logout time', async () => { + await setSecret(TEST_USER_ID, 'token', 'tok_123'); + expect(keyringStore.get(TOKEN_KEY)).toBe('tok_123'); __resetCredentialsForTests(); vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); expect(await getBackend()).toBe('file'); - await clearKeyringSecrets(); - expect(keyringStore.get(KEYRING_TOKEN_KEY)).toBeUndefined(); + await clearKeyringSecrets(TEST_USER_ID); + expect(keyringStore.get(TOKEN_KEY)).toBeUndefined(); }); }); @@ -246,7 +292,7 @@ describe('credentials', () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); writeAuthFile({ token: 'tok', proxy: { password: 'pw' }, secretsBackend: 'keyring' }); await ensureMigrated(); - expect(keyringStore.get(KEYRING_TOKEN_KEY)).toBeUndefined(); + expect(keyringStore.get(LEGACY_KEYRING_TOKEN_KEY)).toBeUndefined(); expect(readAuthFile()).toEqual({ token: 'tok', proxy: { password: 'pw' }, secretsBackend: 'keyring' }); }); @@ -270,8 +316,8 @@ describe('credentials', () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); writeAuthFile({ token: 'tok', proxy: { password: 'pw' }, username: 'u' }); await ensureMigrated(); - expect(keyringStore.get(KEYRING_TOKEN_KEY)).toBe('tok'); - expect(keyringStore.get(KEYRING_PROXY_PASSWORD_KEY)).toBe('pw'); + expect(keyringStore.get(LEGACY_KEYRING_TOKEN_KEY)).toBe('tok'); + expect(keyringStore.get(LEGACY_KEYRING_PROXY_PASSWORD_KEY)).toBe('pw'); const file = readAuthFile(); expect(file.token).toBeUndefined(); expect(file.proxy).toBeUndefined(); @@ -283,7 +329,7 @@ describe('credentials', () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); writeAuthFile({ token: 'tok', proxy: { password: 'pw', groups: [{ name: 'g' }] }, username: 'u' }); await ensureMigrated(); - expect(keyringStore.get(KEYRING_PROXY_PASSWORD_KEY)).toBe('pw'); + expect(keyringStore.get(LEGACY_KEYRING_PROXY_PASSWORD_KEY)).toBe('pw'); const file = readAuthFile(); expect(file.proxy).toEqual({ groups: [{ name: 'g' }] }); expect(file.secretsBackend).toBe('keyring'); @@ -293,7 +339,7 @@ describe('credentials', () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); writeAuthFile({ proxy: { password: 'pw' }, username: 'u' }); await ensureMigrated(); - expect(keyringStore.get(KEYRING_PROXY_PASSWORD_KEY)).toBe('pw'); + expect(keyringStore.get(LEGACY_KEYRING_PROXY_PASSWORD_KEY)).toBe('pw'); const file = readAuthFile(); expect(file.proxy).toBeUndefined(); expect(file.username).toBe('u'); @@ -311,7 +357,7 @@ describe('credentials', () => { it('falls back to file backend when the proxy keyring write fails after token succeeds', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); - keyringFailures.add(KEYRING_PROXY_PASSWORD_KEY); + keyringFailures.add(LEGACY_KEYRING_PROXY_PASSWORD_KEY); writeAuthFile({ token: 'tok', proxy: { password: 'pw' }, username: 'u' }); await ensureMigrated(); const file = readAuthFile(); @@ -334,7 +380,119 @@ describe('credentials', () => { }); }); + describe('ensureSecretsKeyed()', () => { + it('moves keyring entries off the fixed names onto the user ID', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); + writeAuthFile(v2AuthFile({}, { secretsBackend: 'keyring' }) as Record); + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok'); + keyringStore.set(LEGACY_KEYRING_PROXY_PASSWORD_KEY, 'pw'); + + await ensureSecretsKeyed(); + + expect(keyringStore.get(TOKEN_KEY)).toBe('tok'); + expect(keyringStore.get(PROXY_PASSWORD_KEY)).toBe('pw'); + expect(keyringStore.get(LEGACY_KEYRING_TOKEN_KEY)).toBeUndefined(); + expect(keyringStore.get(LEGACY_KEYRING_PROXY_PASSWORD_KEY)).toBeUndefined(); + }); + + it('moves top-level file secrets into the profile', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); + writeAuthFile( + v2AuthFile({}, { secretsBackend: 'file', token: 'tok', proxy: { password: 'pw' } }) as Record, + ); + + await ensureSecretsKeyed(); + + expect(readProfile()).toMatchObject({ token: 'tok', proxy: { password: 'pw' } }); + const file = readAuthFile(); + expect(file.token).toBeUndefined(); + expect(file.proxy).toBeUndefined(); + expect(file.secretsBackend).toBe('file'); + }); + + it('drops secrets it has no user ID to file under', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); + writeAuthFile({ version: 2, profiles: {}, secretsBackend: 'keyring', token: 'tok' }); + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok_kr'); + + await ensureSecretsKeyed(); + + expect(readAuthFile().token).toBeUndefined(); + expect(keyringStore.get(LEGACY_KEYRING_TOKEN_KEY)).toBeUndefined(); + }); + + it('drops the legacy keyring entries even when APIFY_DISABLE_KEYRING=1 is set', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); + writeAuthFile({ version: 2, profiles: {}, secretsBackend: 'keyring' }); + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok'); + keyringStore.set(LEGACY_KEYRING_PROXY_PASSWORD_KEY, 'pw'); + + await ensureSecretsKeyed(); + + expect(keyringStore.size).toBe(0); + }); + + it('is a no-op on a file whose secrets are already keyed', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); + writeAuthFile(v2AuthFile({ token: 'tok' }, { secretsBackend: 'file' }) as Record); + writeFileSyncSpy.mockClear(); + + await ensureSecretsKeyed(); + + expect(authFileWrites()).toHaveLength(0); + expect(readProfile().token).toBe('tok'); + }); + + it('is a no-op on a file the shape migration has not reached', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); + writeAuthFile({ id: 'uid', token: 'tok' }); + writeFileSyncSpy.mockClear(); + + await ensureSecretsKeyed(); + + expect(authFileWrites()).toHaveLength(0); + expect(readAuthFile().token).toBe('tok'); + }); + + it('downgrades to the file backend when the keyring write fails mid-migration', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); + writeAuthFile(v2AuthFile({}, { secretsBackend: 'keyring' }) as Record); + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok'); + keyringStore.set(LEGACY_KEYRING_PROXY_PASSWORD_KEY, 'pw'); + keyringFailures.add(TOKEN_KEY); + + await ensureSecretsKeyed(); + + // Both secrets land in the file: the downgrade holds for the rest of the loop. + expect(readProfile()).toMatchObject({ token: 'tok', proxy: { password: 'pw' } }); + expect(readAuthFile().secretsBackend).toBe('file'); + expect(await getBackend()).toBe('file'); + expect(keyringStore.size).toBe(0); + }); + + it('is memoized within a process', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); + writeAuthFile(v2AuthFile({}, { secretsBackend: 'file', token: 'tok' }) as Record); + await ensureSecretsKeyed(); + expect(readProfile().token).toBe('tok'); + + writeAuthFile(v2AuthFile({}, { secretsBackend: 'file', token: 'tok2' }) as Record); + await ensureSecretsKeyed(); + expect(readAuthFile().token).toBe('tok2'); + }); + }); + describe('getLocalUserInfo()', () => { + it('on file backend, reads the token and proxy password from the profile', async () => { + vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); + writeAuthFile( + v2AuthFile({ token: 'tok', proxy: { password: 'pw' } }, { secretsBackend: 'file' }) as Record, + ); + const info = await getLocalUserInfo(); + expect(info.token).toBe('tok'); + expect(info.proxy).toEqual({ password: 'pw' }); + }); + it('on file backend, keeps the proxy password and drops the groups nothing reads', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); writeAuthFile({ @@ -350,8 +508,8 @@ describe('credentials', () => { it('on keyring backend, overlays token and proxy password from keyring', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); - keyringStore.set(KEYRING_TOKEN_KEY, 'tok_kr'); - keyringStore.set(KEYRING_PROXY_PASSWORD_KEY, 'pw_kr'); + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok_kr'); + keyringStore.set(LEGACY_KEYRING_PROXY_PASSWORD_KEY, 'pw_kr'); writeAuthFile({ username: 'me', id: 'uid', secretsBackend: 'keyring' }); const info = await getLocalUserInfo(); expect(info.token).toBe('tok_kr'); @@ -363,31 +521,39 @@ describe('credentials', () => { expect(await getLocalUserInfo()).toEqual({}); }); - it('on file backend, throws when a token is stored without user metadata', async () => { + it('on file backend, reports logged out for a token stored without user metadata', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); writeAuthFile({ token: 'tok', secretsBackend: 'file' }); - await expect(getLocalUserInfo()).rejects.toThrow('Stale credentials found without user metadata'); + + expect(await getLocalUserInfo()).toEqual({}); + // The secret is dropped rather than left unreachable, so the next command asks for a login. + expect(readAuthFile().token).toBeUndefined(); }); - it('on keyring backend, throws when the keyring holds a token but auth.json is gone', async () => { + it('on keyring backend, reports logged out when the keyring holds a token but auth.json is gone', async () => { vitest.stubEnv('APIFY_DISABLE_KEYRING', ''); - keyringStore.set(KEYRING_TOKEN_KEY, 'tok_kr'); - await expect(getLocalUserInfo()).rejects.toThrow('Stale credentials found without user metadata'); + keyringStore.set(LEGACY_KEYRING_TOKEN_KEY, 'tok_kr'); + + expect(await getLocalUserInfo()).toEqual({}); + // auth.json is the only index of the keyring, so a hand-deleted file strands the entry. + // Reaching for it on a machine with no account would touch the keyring on every command. + expect(keyringStore.get(LEGACY_KEYRING_TOKEN_KEY)).toBe('tok_kr'); }); }); describe('getApifyClientOptions()', () => { beforeEach(() => { vitest.stubEnv('APIFY_DISABLE_KEYRING', '1'); + writeAuthFile(v2AuthFile() as Record); }); it('resolves the stored token when nothing overrides it', async () => { - await setToken('tok_stored'); + await setSecret(TEST_USER_ID, 'token', 'tok_stored'); expect((await getApifyClientOptions()).token).toBe('tok_stored'); }); it('prefers an explicitly passed token over the stored one', async () => { - await setToken('tok_stored'); + await setSecret(TEST_USER_ID, 'token', 'tok_stored'); expect((await getApifyClientOptions('tok_explicit')).token).toBe('tok_explicit'); });