From 4c4440551371ae44654ce24acd52cdfa78a38843 Mon Sep 17 00:00:00 2001 From: Leo-Paul Goffic Date: Thu, 9 Jul 2026 17:17:45 +0200 Subject: [PATCH] fix: profile mangled following multiple logins --- cli/src/commands/login.ts | 12 +++ cli/src/commands/profile.ts | 18 ++-- cli/src/features/profile/model.ts | 36 ++++++++ cli/src/lib/profile-configure-core.ts | 26 +++--- cli/src/lib/profile-store.ts | 9 ++ cli/tests/oauth.test.ts | 123 ++++++++++++++++++++++---- tests/profile.test.ts | 22 +++++ 7 files changed, 206 insertions(+), 40 deletions(-) diff --git a/cli/src/commands/login.ts b/cli/src/commands/login.ts index a9e090a..ae4f65d 100644 --- a/cli/src/commands/login.ts +++ b/cli/src/commands/login.ts @@ -10,9 +10,11 @@ import type { WhoamiResponse } from "@/features/management/model.ts"; import { formatWhoamiPrincipalLine } from "@/features/management/render.ts"; import { configureRunClear } from "@/lib/profile-configure-core.ts"; import { + assertProfileHasNoEnvCredentials, createEmptyProfile, deriveProfileName, profileExists, + profileHasAnyAuthConfigured, resolveProfileName, setActiveProfile, updateProfile, @@ -59,6 +61,15 @@ function selectLoginProfile( return { profileName: currentProfile, profileAction: "replaced" }; } + // Sign into the current profile while it has no credentials of its own yet — a + // fresh `default` or a just-created empty profile. Only branch to a new profile + // once the current one is already authenticated, so a second login doesn't + // clobber an existing session. (Env credentials never reach here — login is + // refused up front while they are set.) + if (!profileHasAnyAuthConfigured(currentProfile)) { + return { profileName: currentProfile, profileAction: "unchanged" }; + } + const targetProfile = loginProfileName(whoami, environment, currentProfile); if (targetProfile === currentProfile) { return { profileName: targetProfile, profileAction: "unchanged" }; @@ -155,6 +166,7 @@ async function fetchLoginWhoami(oauthResponse: TokenResponse): Promise { + assertProfileHasNoEnvCredentials("altertable login"); assertInteractiveLogin(); applyControlPlaneOverride(args); diff --git a/cli/src/commands/profile.ts b/cli/src/commands/profile.ts index 286b056..008f63f 100644 --- a/cli/src/commands/profile.ts +++ b/cli/src/commands/profile.ts @@ -6,6 +6,7 @@ import { buildConfigureShowData, configureCredentialStatus, type ConfigureShowData, + type ProfileInspect, } from "@/features/profile/model.ts"; import type { ConfigureAuthPlane } from "@/lib/profile-status.ts"; import { configureVerify } from "@/lib/profile-status.ts"; @@ -42,6 +43,7 @@ import { } from "@/features/profile/render.ts"; import { renderShellExportView } from "@/ui/shell/render.ts"; import { + assertProfileHasNoEnvCredentials, createEmptyProfile, deleteProfile, getActiveProfileName, @@ -186,7 +188,7 @@ async function fetchProfileIdentity( const profileShowCommand = defineLocalCommand< { profileName: string; config: boolean }, - ProfileShowResult & { config: boolean } + ProfileInspect >({ id: "profile.show", localConfig: true, @@ -208,17 +210,13 @@ const profileShowCommand = defineLocalCommand< config: Boolean(args.config), }; }, - async local(input, context) { + async local(input) { const previous = getCliContext(); try { const next = { ...previous, profile: input.profileName }; setCliContext(next); refreshCliRuntimeContext(next); - const configuration = buildConfigureShowData(); - const identity = configureCredentialStatus().hasManagement - ? await fetchProfileIdentity(context) - : undefined; - return { configuration, identity, config: input.config }; + return inspectProfile(input.profileName); } finally { setCliContext(previous); refreshCliRuntimeContext(previous); @@ -227,8 +225,8 @@ const profileShowCommand = defineLocalCommand< present(result) { return { kind: "normalized", - data: profileShowToJson(result), - humanText: formatProfileShow(result, { config: result.config }), + data: { profile: result }, + humanText: formatProfileInspect(result), }; }, }); @@ -247,6 +245,7 @@ function createProfileUseCommand(id: string, name: string, hidden = false) { return requireProfileName(args.name); }, local(profileName) { + assertProfileHasNoEnvCredentials("Switching profiles"); setActiveProfile(profileName); return profileName; }, @@ -274,6 +273,7 @@ const profileSwitchCommand = defineLocalCommand({ return args.name ? requireProfileName(args.name) : undefined; }, async local(profileName) { + assertProfileHasNoEnvCredentials("Switching profiles"); if (!profileName) { if (isJsonOutput(getCliContext()) || getCliContext().agent || process.stdin.isTTY !== true) { throw new CliError("Interactive profile switch requires a TTY. Pass a profile name."); diff --git a/cli/src/features/profile/model.ts b/cli/src/features/profile/model.ts index eb9e1fd..52d67ec 100644 --- a/cli/src/features/profile/model.ts +++ b/cli/src/features/profile/model.ts @@ -12,6 +12,7 @@ import { import { assertSafeProfileName, DEFAULT_PROFILE_NAME, + FROM_ENV_PSEUDOPROFILE_NAME, type ProfileConfigKey, ensureProfileExists, ensureProfilesLayout, @@ -21,6 +22,7 @@ import { profileDir, profileExists, readProfileConfigRecord, + resolveProfileName, setActiveProfile, writeProfileConfig, } from "@/lib/profile-store.ts"; @@ -30,6 +32,7 @@ export { DEFAULT_PROFILE_NAME, ensureProfileExists, ensureProfilesLayout, + FROM_ENV_PSEUDOPROFILE_NAME, getActiveProfileName, profileConfigFile, profileExists, @@ -196,6 +199,11 @@ function profileAuth(name: string): ProfileAuth { }; } +export function profileHasAnyAuthConfigured(name: string): boolean { + const auth = profileAuth(name); + return auth.management !== "none" || auth.lakehouse !== "none"; +} + function readProfileSnapshot(name: string): ProfileSnapshot { return { config: readProfileConfigRecord(name), @@ -486,6 +494,34 @@ function hasStoredLakehouseCredentials(): boolean { return secretExists("lakehouse/basic-token") || secretExists("lakehouse/password"); } +export function hasCredentialsThroughEnv(): boolean { + return hasEnvManagementCredentials() || hasEnvLakehouseCredentials(); +} + +/** + * The profile identity currently in effect. Normally the active stored profile, + * but environment credentials override any stored profile for the whole process, + * so they surface as the reserved `_from_env` pseudo-profile. + */ +export function resolveActiveProfileName(): string { + return hasCredentialsThroughEnv() + ? FROM_ENV_PSEUDOPROFILE_NAME + : resolveProfileName(getCliContext().profile); +} + +/** + * Guard for commands that mutate stored-profile state (login, switching). While + * credentials come from the environment there is no stored profile to act on — + * the env vars pin the identity and would override any change — so refuse. + */ +export function assertProfileHasNoEnvCredentials(action: string): void { + if (resolveActiveProfileName() === FROM_ENV_PSEUDOPROFILE_NAME) { + throw new ConfigurationError( + `${action} is disabled while credentials come from the environment. Unset ALTERTABLE_API_KEY / ALTERTABLE_BASIC_AUTH_TOKEN / ALTERTABLE_LAKEHOUSE_USERNAME / ALTERTABLE_LAKEHOUSE_PASSWORD to manage stored profiles.`, + ); + } +} + export function configureCredentialStatus(): ConfigureCredentialStatus { return { hasManagement: diff --git a/cli/src/lib/profile-configure-core.ts b/cli/src/lib/profile-configure-core.ts index 96fb6ce..ffba642 100644 --- a/cli/src/lib/profile-configure-core.ts +++ b/cli/src/lib/profile-configure-core.ts @@ -26,7 +26,6 @@ export type ConfigureOptions = { dataPlaneUrl?: string; controlPlaneUrl?: string; profile?: string; - org?: string; show?: boolean; clear?: boolean; allowInsecureHttp?: boolean; @@ -112,7 +111,6 @@ export async function configureRunSet( const env = options.env ?? ""; const dataPlaneUrl = options.dataPlaneUrl ?? ""; const controlPlaneUrl = options.controlPlaneUrl ?? ""; - const org = options.org ?? ""; const allowInsecureHttp = options.allowInsecureHttp ?? false; const passwordFromArgv = @@ -146,18 +144,19 @@ export async function configureRunSet( apiKey = (await Bun.stdin.text()).replace(/\n$/, ""); } - const hasLakehouse = Boolean(user || password); - const hasToken = Boolean(basicToken); + const hasLakehouseCredentials = Boolean(user || password); + const hasLakehouseBasicToken = Boolean(basicToken); const hasApiKey = Boolean(apiKey); - const lakehouseMechanismCount = Number(hasLakehouse) + Number(hasToken); - const hasAnyCredential = hasLakehouse || hasToken || hasApiKey; + const lakehouseMechanismCount = + Number(hasLakehouseCredentials) + Number(hasLakehouseBasicToken); + const hasAnyCredential = hasLakehouseCredentials || hasLakehouseBasicToken || hasApiKey; if (lakehouseMechanismCount > 1) { throw new CliError( "Choose a single lakehouse authentication mechanism: --user/--password or --basic-token.", ); } - if (hasApiKey && (hasLakehouse || hasToken)) { + if (hasApiKey && (hasLakehouseCredentials || hasLakehouseBasicToken)) { throw new CliError( "Choose a single authentication mechanism per configure invocation: lakehouse credentials or --api-key.", ); @@ -173,7 +172,7 @@ export async function configureRunSet( "Nothing to configure. Use --user/--password, --basic-token, or --api-key --env .", ); } - if (hasLakehouse && (!user || !password)) { + if (hasLakehouseCredentials && (!user || !password)) { throw new CliError("--user and --password must be provided together."); } if (hasApiKey && !env) { @@ -184,14 +183,11 @@ export async function configureRunSet( configureClearManagementCredentials(); secretSet("api-key", apiKey, undefined, { fromArgv: apiKeyFromArgv }); configSet("api_key_env", env); - if (org) { - configSet("organization_slug", org); - } } - if (hasToken) { + if (hasLakehouseBasicToken) { configureClearLakehouseCredentials(); secretSet("lakehouse/basic-token", basicToken); - } else if (hasLakehouse) { + } else if (hasLakehouseCredentials) { configureClearLakehouseCredentials(); configSet("user", user); secretSet("lakehouse/password", password, undefined, { fromArgv: passwordFromArgv }); @@ -212,9 +208,9 @@ export async function configureRunSet( if (hasApiKey) { sink.writeMetadata([terminalMetadata(`Saved management API key for environment ${env}.`)]); - } else if (hasToken) { + } else if (hasLakehouseBasicToken) { sink.writeMetadata([terminalMetadata("Saved lakehouse Basic token.")]); - } else if (hasLakehouse) { + } else if (hasLakehouseCredentials) { sink.writeMetadata([terminalMetadata(`Saved lakehouse credentials for user ${user}.`)]); } else if (dataPlaneUrl) { sink.writeMetadata([terminalMetadata(`Saved data plane URL ${dataPlaneUrl}.`)]); diff --git a/cli/src/lib/profile-store.ts b/cli/src/lib/profile-store.ts index 1f0f378..a813730 100644 --- a/cli/src/lib/profile-store.ts +++ b/cli/src/lib/profile-store.ts @@ -5,6 +5,12 @@ import { ConfigurationError } from "@/lib/errors.ts"; export const DEFAULT_PROFILE_NAME = "default"; +// Reserved pseudo-profile: the identity in effect when credentials come from +// environment variables rather than a stored profile. Never a real directory. +export const FROM_ENV_PSEUDOPROFILE_NAME = "_from_env"; + +const RESERVED_PROFILE_NAMES = new Set([FROM_ENV_PSEUDOPROFILE_NAME]); + export const PROFILE_CONFIG_KEYS = [ "user", "api_key_env", @@ -52,6 +58,9 @@ export function assertSafeProfileName(name: string): void { `Invalid profile name: ${name}. Use only letters, digits, dots, underscores, and hyphens.`, ); } + if (RESERVED_PROFILE_NAMES.has(name)) { + throw new ConfigurationError(`Profile name is reserved: ${name}`); + } const resolvedProfileDir = resolve(join(profilesDir(), name)); const resolvedProfilesRoot = resolve(profilesDir()); diff --git a/cli/tests/oauth.test.ts b/cli/tests/oauth.test.ts index b44ce5b..8b87a01 100644 --- a/cli/tests/oauth.test.ts +++ b/cli/tests/oauth.test.ts @@ -7,7 +7,9 @@ import { configGet } from "@/lib/config.ts"; import { setCliContext } from "@/context.ts"; import { parseCallback, buildAuthorizeUrl, startLoopbackServer } from "@/lib/oauth-flow.ts"; import { storeOAuthTokens, getStoredAccessToken, clearOAuthTokens } from "@/lib/oauth-profile.ts"; -import { getActiveProfileName, profileExists } from "@/lib/profile-store.ts"; +import { getActiveProfileName, profileExists, setActiveProfile } from "@/lib/profile-store.ts"; +import { createEmptyProfile } from "@/features/profile/model.ts"; +import { secretSet } from "@/lib/secrets.ts"; let testHome = ""; const TEST_PRINCIPAL = { @@ -29,6 +31,10 @@ afterEach(() => { delete process.env.ALTERTABLE_SECRET_BACKEND; delete process.env.ALTERTABLE_MANAGEMENT_API_BASE; delete process.env.ALTERTABLE_ALLOW_INSECURE_HTTP; + delete process.env.ALTERTABLE_API_KEY; + delete process.env.ALTERTABLE_BASIC_AUTH_TOKEN; + delete process.env.ALTERTABLE_LAKEHOUSE_USERNAME; + delete process.env.ALTERTABLE_LAKEHOUSE_PASSWORD; }); describe("resolveOAuthBase", () => { @@ -151,6 +157,10 @@ import { applyControlPlaneOverride, storeLoginProfileMetadata, } from "@/commands/login.ts"; +import { + assertProfileHasNoEnvCredentials, + resolveActiveProfileName, +} from "@/features/profile/model.ts"; import { ConfigurationError } from "@/lib/errors.ts"; import { configureRunClear } from "@/lib/profile-configure-core.ts"; import { getOutputSink } from "@/lib/runtime.ts"; @@ -173,26 +183,26 @@ describe("resolveWhoamiEnvironment", () => { }); describe("login profile metadata", () => { - test("stores environment and organization from whoami", () => { - const metadata = storeLoginProfileMetadata( - { - principal: TEST_PRINCIPAL, - organization: { name: "Altertable", slug: "altertable" }, - authentication_scope: "environment", - environment_slug: "production", - }, - {}, - ); + const WHOAMI = { + principal: TEST_PRINCIPAL, + organization: { name: "Altertable", slug: "altertable" }, + authentication_scope: "environment", + environment_slug: "production", + } as const; + + // `altertable login` on a fresh install signs into the unauthenticated `default` + // profile and stores the whoami identity there, rather than deriving a new one. + test("lands in the current default profile when it is unauthenticated", () => { + const metadata = storeLoginProfileMetadata(WHOAMI, {}); storeOAuthTokens({ access_token: "acc", refresh_token: "ref", expires_in: 3600 }); expect(metadata).toEqual({ environment: "production", - profileName: "altertable_production", - profileAction: "created", + profileName: "default", + profileAction: "unchanged", }); - expect(profileExists("default")).toBe(true); - expect(profileExists("altertable_production")).toBe(true); - expect(getActiveProfileName()).toBe("altertable_production"); + expect(getActiveProfileName()).toBe("default"); + expect(profileExists("altertable_production")).toBe(false); expect(configGet("api_key_env")).toBe("production"); expect(configGet("organization_slug")).toBe("altertable"); expect(configGet("organization_name")).toBe("Altertable"); @@ -201,6 +211,87 @@ describe("login profile metadata", () => { expect(getStoredAccessToken()).toBe("acc"); }); + // A second `altertable login` must not clobber the now-authenticated `default`; + // it derives and switches to a fresh profile instead. + test("creates a separate profile when the current one is already authenticated", () => { + storeLoginProfileMetadata(WHOAMI, {}); + storeOAuthTokens({ access_token: "acc", refresh_token: "ref", expires_in: 3600 }); + + const metadata = storeLoginProfileMetadata(WHOAMI, {}); + + expect(metadata).toEqual({ + environment: "production", + profileName: "altertable_production", + profileAction: "created", + }); + expect(profileExists("default")).toBe(true); + expect(profileExists("altertable_production")).toBe(true); + expect(getActiveProfileName()).toBe("altertable_production"); + }); + + // `altertable profile create new` (which switches to `new`) followed by + // `altertable login` signs into `new`, since it has not been configured yet. + test("lands in a freshly created, unconfigured profile", () => { + createEmptyProfile("new"); + setActiveProfile("new"); + + const metadata = storeLoginProfileMetadata(WHOAMI, {}); + + expect(metadata).toEqual({ + environment: "production", + profileName: "new", + profileAction: "unchanged", + }); + expect(getActiveProfileName()).toBe("new"); + expect(profileExists("altertable_production")).toBe(false); + }); + + // Lakehouse-only credentials still count as "authenticated" — a login must not + // overwrite them, so it branches to a new profile. + test("treats a profile with only lakehouse credentials as authenticated", () => { + secretSet("lakehouse/password", "s_lake"); + + const metadata = storeLoginProfileMetadata(WHOAMI, {}); + + expect(metadata).toEqual({ + environment: "production", + profileName: "altertable_production", + profileAction: "created", + }); + expect(getActiveProfileName()).toBe("altertable_production"); + }); + + // Credentials supplied via environment variables aren't a stored profile; the + // active identity is the reserved `_from_env` pseudo-profile. + test("resolves the active identity to reserved _from_env when a credential env var is set", () => { + expect(resolveActiveProfileName()).toBe("default"); + + process.env.ALTERTABLE_API_KEY = "atm_env"; + expect(resolveActiveProfileName()).toBe("_from_env"); + delete process.env.ALTERTABLE_API_KEY; + + process.env.ALTERTABLE_BASIC_AUTH_TOKEN = "dG9rZW4="; + expect(resolveActiveProfileName()).toBe("_from_env"); + delete process.env.ALTERTABLE_BASIC_AUTH_TOKEN; + + process.env.ALTERTABLE_LAKEHOUSE_USERNAME = "u"; + process.env.ALTERTABLE_LAKEHOUSE_PASSWORD = "p"; + expect(resolveActiveProfileName()).toBe("_from_env"); + }); + + // Environment credentials pin the identity, so stored-profile controls (login, + // switching) are locked while they are set. + test("locks stored-profile controls while credentials come from the environment", () => { + expect(() => assertProfileHasNoEnvCredentials("altertable login")).not.toThrow(); + + process.env.ALTERTABLE_API_KEY = "atm_env"; + expect(() => assertProfileHasNoEnvCredentials("altertable login")).toThrow(ConfigurationError); + }); + + test("_from_env is a reserved profile name that cannot be created", () => { + expect(() => createEmptyProfile("_from_env")).toThrow(); + }); + test("can replace the current profile login session", () => { const metadata = storeLoginProfileMetadata( { diff --git a/tests/profile.test.ts b/tests/profile.test.ts index 35b2c7d..e88c135 100644 --- a/tests/profile.test.ts +++ b/tests/profile.test.ts @@ -73,6 +73,28 @@ describe("profile switching", () => { result = await workspace.runCommand("altertable profile list"); expect(result.stdout).not.toContain("globex_dev"); }); + + // Environment credentials pin the identity to `_from_env`, so login and profile + // switching are disabled — they would only mutate stored state the env overrides. + test("environment credentials disable login and profile switching", async () => { + const login = await workspace.runCommand("altertable login", { + env: { ALTERTABLE_API_KEY: "atm_env" }, + }); + expect(login.exitCode).not.toBe(0); + expect(login.stderr).toContain("disabled while credentials come from the environment"); + + const use = await workspace.runCommand("altertable profile use acme_staging", { + env: { ALTERTABLE_API_KEY: "atm_env" }, + }); + expect(use.exitCode).not.toBe(0); + expect(use.stderr).toContain("disabled while credentials come from the environment"); + + const switched = await workspace.runCommand("altertable profile switch acme_staging", { + env: { ALTERTABLE_LAKEHOUSE_USERNAME: "u", ALTERTABLE_LAKEHOUSE_PASSWORD: "p" }, + }); + expect(switched.exitCode).not.toBe(0); + expect(switched.stderr).toContain("disabled while credentials come from the environment"); + }); }); async function seedAcmeProfiles(workspace: TestWorkspace): Promise {