diff --git a/src/commands/git-profile/module.ts b/src/commands/git-profile/module.ts index 2603eabd..b3f03ffe 100644 --- a/src/commands/git-profile/module.ts +++ b/src/commands/git-profile/module.ts @@ -15,7 +15,8 @@ class Module extends IModule { .description("Commands related to Git settings"); const gitProfileCommand = gitCommand.command("profile") - .description("Manage Git profiles required to use git-related operations."); + .description("Manage Git profiles required to use git-related operations.") + .skipProfileLoading(); gitProfileCommand.command("list") .description("Command to list all stored Git profiles") diff --git a/src/commands/profile/module.ts b/src/commands/profile/module.ts index 164097ee..d7174154 100644 --- a/src/commands/profile/module.ts +++ b/src/commands/profile/module.ts @@ -12,7 +12,8 @@ class Module extends IModule { public register(context: Context, configurator: Configurator): void { const command = configurator.command("profile") - .description("Manage profiles required to access a system."); + .description("Manage profiles required to access a system.") + .skipProfileLoading(); command.command("list") .description("Command to list all stored profiles") diff --git a/src/content-cli.ts b/src/content-cli.ts index c0749714..5ca76fc5 100644 --- a/src/content-cli.ts +++ b/src/content-cli.ts @@ -2,7 +2,7 @@ import semverSatisfies = require("semver/functions/satisfies"); import { Command } from "commander"; -import { Configurator, IModuleConstructor, ModuleHandler } from "./core/command/module-handler"; +import { Configurator, IModuleConstructor, ModuleHandler, shouldLoadProfile } from "./core/command/module-handler"; import { Context } from "./core/command/cli-context"; import { VersionUtils } from "./core/utils/version"; import { logger } from "./core/utils/logger"; @@ -63,6 +63,14 @@ export function createProgram(context: Context, opts: CreateProgramOptions = {}) return program; } +export function loadProfileOnDemand(program: Command, context: Context): void { + program.hook("preAction", async (_program, actionCommand) => { + if (shouldLoadProfile(actionCommand)) { + await context.init(); + } + }); +} + /** * To support the legacy command structure, we have to configure some root commands * that the individual modules will extend. @@ -103,9 +111,8 @@ async function run(): Promise { } const context = new Context(globalOpts); - await context.init(); - const program = createProgram(context, { devMode: !!globalOpts.dev }); + loadProfileOnDemand(program, context); try { await program.parseAsync(process.argv); diff --git a/src/core/command/module-handler.ts b/src/core/command/module-handler.ts index 88a650f4..5bfbb655 100644 --- a/src/core/command/module-handler.ts +++ b/src/core/command/module-handler.ts @@ -123,6 +123,15 @@ export class ModuleHandler { type CommandHandler = (context: Context, command: Command, options: OptionValues) => Promise; +export function shouldLoadProfile(command: Command): boolean { + for (let cmd = command; cmd; cmd = cmd.parent) { + if ((cmd as any).skipsProfileLoading) { + return false; + } + } + return true; +} + /** * Allows the creation of root level commands. */ @@ -209,6 +218,11 @@ export class CommandConfig { return this; } + public skipProfileLoading(): this { + (this.cmd as any).skipsProfileLoading = true; + return this; + } + public action(handler: CommandHandler): void { this.cmd.action(async (): Promise => { try { diff --git a/src/core/profile/profile.service.ts b/src/core/profile/profile.service.ts index 27b01846..1305dce0 100644 --- a/src/core/profile/profile.service.ts +++ b/src/core/profile/profile.service.ts @@ -19,7 +19,6 @@ const expiryBuffer = 5000; const OAUTH_SCOPES = ["studio", "package-manager", "integration.data-pools", "action-engine.projects"]; /** Device code fallback: try without action-engine.projects if all 4 scopes fail. */ const DEVICE_CODE_SCOPES_WITHOUT_ACTION_ENGINE = ["studio", "package-manager", "integration.data-pools"]; - export interface Config { defaultProfile: string; } @@ -263,9 +262,9 @@ export class ProfileService { if (!this.isProfileExpired(profile, expiryBuffer)) { return; } - const issuer = await Issuer.discover(profile.team); - if (profile.type === ProfileType.DEVICE_CODE) { - try { + try { + const issuer = await Issuer.discover(profile.team); + if (profile.type === ProfileType.DEVICE_CODE) { const oauthClient = new issuer.Client({ client_id: "content-cli", token_endpoint_auth_method: "none", @@ -274,12 +273,7 @@ export class ProfileService { profile.apiToken = tokenSet.access_token; profile.expiresAt = tokenSet.expires_at; profile.refreshToken = tokenSet.refresh_token; - } catch (err) { - logger.error(new FatalError("The profile cannot be refreshed. Please retry or recreate profile.")); - } - } - else { - try { + } else { const oauthClient = new issuer.Client({ client_id: profile.clientId, client_secret: profile.clientSecret, @@ -291,14 +285,21 @@ export class ProfileService { }); profile.apiToken = tokenSet.access_token; profile.expiresAt = tokenSet.expires_at; - } catch (err) { - logger.error(new FatalError("The profile cannot be refreshed. Please retry or recreate profile.")); } + } catch (err) { + throw this.unrefreshableProfile(profile.name, err); } await this.storeProfile(profile); } + private unrefreshableProfile(profileName: string, err: unknown): Error { + const cause = err instanceof Error ? err.message : "an unrecognised error"; + logger.debug(`Refreshing the profile ${profileName} failed: ${cause}`); + logger.warn(`The profile ${profileName} cannot be refreshed. Retry, or recreate it.`); + return new Error(`The profile ${profileName} cannot be refreshed.`); + } + private getProfileEnvVariables(): any { return { teamUrl: this.getBaseTeamUrl(process.env.TEAM_URL), diff --git a/tests/commands/profile/profile-recovery.spec.ts b/tests/commands/profile/profile-recovery.spec.ts new file mode 100644 index 00000000..e1afb909 --- /dev/null +++ b/tests/commands/profile/profile-recovery.spec.ts @@ -0,0 +1,93 @@ +import Module = require("../../../src/commands/profile/module"); +import { createProgram, loadProfileOnDemand } from "../../../src/content-cli"; +import { ProfileCommandService } from "../../../src/commands/profile/profile-command.service"; +import { Context } from "../../../src/core/command/cli-context"; +import { GitProfileService } from "../../../src/core/git-profile/git-profile.service"; +import { ProfileService } from "../../../src/core/profile/profile.service"; + +jest.mock("../../../src/commands/profile/profile-command.service"); + +describe("Profile commands when the default profile cannot be refreshed", () => { + + let exitSpy: jest.SpyInstance; + let findProfile: jest.SpyInstance; + let listProfiles: jest.Mock; + let createProfile: jest.Mock; + let defaultProfile: jest.Mock; + let secureProfile: jest.Mock; + + beforeEach(() => { + exitSpy = jest.spyOn(process, "exit").mockImplementation((() => undefined) as never); + + jest.spyOn(GitProfileService.prototype, "getDefaultProfile").mockReturnValue(null); + jest.spyOn(GitProfileService.prototype, "findProfile").mockRejectedValue(new Error("No profile was found")); + jest.spyOn(ProfileService.prototype, "getDefaultProfile").mockReturnValue("expired-profile"); + findProfile = jest.spyOn(ProfileService.prototype, "findProfile") + .mockRejectedValue("The profile expired-profile couldn't be resolved."); + + listProfiles = jest.fn().mockResolvedValue(undefined); + createProfile = jest.fn().mockResolvedValue(undefined); + defaultProfile = jest.fn().mockResolvedValue(undefined); + secureProfile = jest.fn().mockResolvedValue(undefined); + (ProfileCommandService as jest.MockedClass).mockImplementation(() => ({ + listProfiles, + createProfile, + makeDefaultProfile: defaultProfile, + secureProfile, + }) as unknown as ProfileCommandService); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + async function runProfileCommand(args: string[]): Promise { + const context = new Context({}); + + const program = createProgram(context, { modules: [Module] }); + loadProfileOnDemand(program, context); + program.exitOverride(); + await program.parseAsync(["node", "content-cli", ...args]); + + return context; + } + + it("should run profile list without loading a profile", async () => { + await runProfileCommand(["profile", "list"]); + + expect(listProfiles).toHaveBeenCalled(); + expect(findProfile).not.toHaveBeenCalled(); + expect(exitSpy).not.toHaveBeenCalled(); + }); + + it("should run profile create without loading a profile", async () => { + await runProfileCommand(["profile", "create", "--setAsDefault"]); + + expect(createProfile).toHaveBeenCalledWith(true); + expect(findProfile).not.toHaveBeenCalled(); + expect(exitSpy).not.toHaveBeenCalled(); + }); + + it("should run profile default without loading a profile", async () => { + await runProfileCommand(["profile", "default", "replacement"]); + + expect(defaultProfile).toHaveBeenCalledWith("replacement"); + expect(findProfile).not.toHaveBeenCalled(); + expect(exitSpy).not.toHaveBeenCalled(); + }); + + it("should run profile secure without loading a profile", async () => { + await runProfileCommand(["profile", "secure", "broken"]); + + expect(secureProfile).toHaveBeenCalledWith("broken"); + expect(findProfile).not.toHaveBeenCalled(); + expect(exitSpy).not.toHaveBeenCalled(); + }); + + it("should leave the context without a profile so content commands still fail", async () => { + const context = await runProfileCommand(["profile", "list"]); + + expect(context.profile).toBeUndefined(); + expect(() => context.httpClient).toThrow("No profile provided"); + }); +}); diff --git a/tests/core/command/profile-on-demand.spec.ts b/tests/core/command/profile-on-demand.spec.ts new file mode 100644 index 00000000..39d20079 --- /dev/null +++ b/tests/core/command/profile-on-demand.spec.ts @@ -0,0 +1,79 @@ +import { Command } from "commander"; +import { createProgram, loadProfileOnDemand } from "../../../src/content-cli"; +import { Context } from "../../../src/core/command/cli-context"; +import { Configurator, IModule, shouldLoadProfile } from "../../../src/core/command/module-handler"; + +class ProfiledModule extends IModule { + public register(context: Context, configurator: Configurator): void { + configurator.command("content") + .command("show") + .action(async () => undefined); + } +} + +class ProfileFreeModule extends IModule { + public register(context: Context, configurator: Configurator): void { + configurator.command("settings") + .skipProfileLoading() + .command("show") + .action(async () => undefined); + } +} + +describe("shouldLoadProfile", () => { + + it("should load a profile for an unmarked command", () => { + const command = new Command("export"); + + expect(shouldLoadProfile(command)).toBe(true); + }); + + it("should not load a profile for a marked command", () => { + const command = new Command("profile"); + (command as any).skipsProfileLoading = true; + + expect(shouldLoadProfile(command)).toBe(false); + }); + + it("should inherit the opt-out from an ancestor", () => { + const parent = new Command("profile"); + (parent as any).skipsProfileLoading = true; + const child = parent.command("list"); + + expect(shouldLoadProfile(child)).toBe(false); + }); +}); + +describe("loadProfileOnDemand", () => { + + let context: Context; + let init: jest.SpyInstance; + + beforeEach(() => { + context = new Context({}); + init = jest.spyOn(context, "init").mockResolvedValue(undefined); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + async function run(modules: any[], args: string[]): Promise { + const program = createProgram(context, { modules }); + loadProfileOnDemand(program, context); + program.exitOverride(); + await program.parseAsync(["node", "content-cli", ...args]); + } + + it("should load the profile for a command that needs one", async () => { + await run([ProfiledModule], ["content", "show"]); + + expect(init).toHaveBeenCalledTimes(1); + }); + + it("should not load the profile for a command that opted out", async () => { + await run([ProfileFreeModule], ["settings", "show"]); + + expect(init).not.toHaveBeenCalled(); + }); +}); diff --git a/tests/core/profile/profile-refresh-exit.spec.ts b/tests/core/profile/profile-refresh-exit.spec.ts new file mode 100644 index 00000000..ebe4e477 --- /dev/null +++ b/tests/core/profile/profile-refresh-exit.spec.ts @@ -0,0 +1,65 @@ +const mockIssuerDiscover = jest.fn(); + +jest.mock("openid-client", () => ({ + Issuer: { discover: mockIssuerDiscover }, +})); + +import { AuthenticationType, Profile, ProfileType } from "../../../src/core/profile/profile.interface"; +import { ProfileService } from "../../../src/core/profile/profile.service"; + +describe("ProfileService - refreshProfile keeps the process alive", () => { + + let profileService: ProfileService; + let exitSpy: jest.SpyInstance; + let storeSpy: jest.SpyInstance; + + const expiredProfile = (): Profile => ({ + name: "broken", + team: "https://example.invalid", + apiToken: "stale-token", + refreshToken: "revoked-refresh-token", + authenticationType: AuthenticationType.BEARER, + type: ProfileType.DEVICE_CODE, + expiresAt: Math.floor(Date.now() / 1000) - 10, + }); + + beforeEach(() => { + profileService = new ProfileService(); + exitSpy = jest.spyOn(process, "exit").mockImplementation((() => undefined) as never); + storeSpy = jest.spyOn(profileService, "storeProfile").mockImplementation(async () => {}); + }); + + afterEach(() => { + jest.restoreAllMocks(); + mockIssuerDiscover.mockReset(); + }); + + it("should reject without exiting when the token endpoint rejects the refresh token", async () => { + mockIssuerDiscover.mockResolvedValue({ + Client: jest.fn().mockImplementation(() => ({ + refresh: jest.fn().mockRejectedValue(new Error("invalid_grant")), + })), + }); + + await expect(profileService.refreshProfile(expiredProfile())) + .rejects.toThrow("The profile broken cannot be refreshed."); + + expect(exitSpy).not.toHaveBeenCalled(); + expect(storeSpy).not.toHaveBeenCalled(); + }); + + it("should reject without exiting when the team is unreachable", async () => { + const dnsFailure = Object.assign(new Error("getaddrinfo ENOTFOUND example.invalid"), { + errno: -3008, + code: "ENOTFOUND", + syscall: "getaddrinfo", + }); + mockIssuerDiscover.mockRejectedValue(dnsFailure); + + await expect(profileService.refreshProfile(expiredProfile())) + .rejects.toThrow("The profile broken cannot be refreshed."); + + expect(exitSpy).not.toHaveBeenCalled(); + expect(storeSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/tests/core/profile/profile.service.spec.ts b/tests/core/profile/profile.service.spec.ts index 07dc59bc..d5c888d6 100644 --- a/tests/core/profile/profile.service.spec.ts +++ b/tests/core/profile/profile.service.spec.ts @@ -28,7 +28,7 @@ jest.mock("openid-client", () => ({ })); jest.mock("../../../src/core/utils/logger", () => ({ - logger: { error: jest.fn(), info: jest.fn() }, + logger: { error: jest.fn(), info: jest.fn(), debug: jest.fn(), warn: jest.fn() }, FatalError: class FatalError extends Error { constructor(m: string) { super(m); @@ -37,6 +37,7 @@ jest.mock("../../../src/core/utils/logger", () => ({ }, })); +import { logger } from "../../../src/core/utils/logger"; import { ProfileService } from "../../../src/core/profile/profile.service"; import { Dirent } from "node:fs"; @@ -435,6 +436,50 @@ describe("ProfileService - findProfile", () => { }); }); + describe("when the stored profile cannot be refreshed", () => { + it("should reject instead of resolving an unrefreshed profile", async () => { + const profileName = "expired-profile"; + const mockProfile: Profile = { + name: profileName, + team: "https://example.celonis.cloud", + apiToken: "expired-token", + authenticationType: AuthenticationType.BEARER, + type: ProfileType.DEVICE_CODE + }; + + jest.spyOn(fs, "readFileSync").mockReturnValue(JSON.stringify(mockProfile)); + jest.spyOn(profileService, "refreshProfile") + .mockRejectedValue(new Error(`The profile ${profileName} cannot be refreshed.`)); + + await expect(profileService.findProfile(profileName)) + .rejects.toEqual(`The profile ${profileName} couldn't be resolved.`); + }); + + it("should reject when secrets come from the keychain", async () => { + const profileName = "secure-expired-profile"; + const mockProfile: Profile = { + name: profileName, + team: "https://example.celonis.cloud", + apiToken: "expired-token", + authenticationType: AuthenticationType.BEARER, + type: ProfileType.DEVICE_CODE, + secretsStoredSecurely: true + }; + + jest.spyOn(fs, "readFileSync").mockReturnValue(JSON.stringify(mockProfile)); + mockGetSecrets.mockResolvedValue({ + apiToken: "expired-token", + refreshToken: "revoked-refresh-token", + clientSecret: undefined + }); + jest.spyOn(profileService, "refreshProfile") + .mockRejectedValue(new Error(`The profile ${profileName} cannot be refreshed.`)); + + await expect(profileService.findProfile(profileName)) + .rejects.toEqual(`The profile ${profileName} couldn't be resolved.`); + }); + }); + describe("when profileName is provided but profile file does not exist", () => { it("should reject with error message", async () => { const profileName = "non-existent-profile"; @@ -1178,6 +1223,80 @@ describe("ProfileService - refreshProfile", () => { expect(profile.expiresAt).toBe(newTokenSet.expires_at); expect(storeSpy).toHaveBeenCalledWith(profile); }); + + it("should reject with the recovery command when a device code refresh fails", async () => { + const profile: Profile = { + name: "device-code-profile", + team: "https://example.com", + apiToken: "old-token", + authenticationType: AuthenticationType.BEARER, + type: ProfileType.DEVICE_CODE, + refreshToken: "revoked-refresh-token", + expiresAt: Math.floor(Date.now() / 1000) - 10, + }; + mockIssuerDiscover.mockResolvedValue({ + Client: jest.fn().mockImplementation(() => ({ + refresh: jest.fn().mockRejectedValue(new Error("invalid_grant")), + })), + }); + const storeSpy = jest.spyOn(profileService, "storeProfile").mockImplementation(async () => {}); + + await expect(profileService.refreshProfile(profile)) + .rejects.toThrow("The profile device-code-profile cannot be refreshed."); + expect(logger.warn).toHaveBeenCalledWith("The profile device-code-profile cannot be refreshed. Retry, or recreate it."); + expect(storeSpy).not.toHaveBeenCalled(); + expect(logger.error).not.toHaveBeenCalled(); + }); + + it("should reject with the recovery command when a client credentials refresh fails", async () => { + const profile: Profile = { + name: "client-credentials-profile", + team: "https://example.com", + apiToken: "old-token", + authenticationType: AuthenticationType.BEARER, + type: ProfileType.CLIENT_CREDENTIALS, + clientId: "id", + clientSecret: "secret", + scopes: ["studio"], + clientAuthenticationMethod: "client_secret_basic", + expiresAt: Math.floor(Date.now() / 1000) - 10, + }; + mockIssuerDiscover.mockResolvedValue({ + Client: jest.fn().mockImplementation(() => ({ + grant: jest.fn().mockRejectedValue(new Error("invalid_client")), + })), + }); + const storeSpy = jest.spyOn(profileService, "storeProfile").mockImplementation(async () => {}); + + await expect(profileService.refreshProfile(profile)) + .rejects.toThrow("The profile client-credentials-profile cannot be refreshed."); + expect(logger.warn).toHaveBeenCalledWith("The profile client-credentials-profile cannot be refreshed. Retry, or recreate it."); + expect(storeSpy).not.toHaveBeenCalled(); + expect(logger.error).not.toHaveBeenCalled(); + }); + + it("should keep the stale token on the profile when the refresh fails", async () => { + const profile: Profile = { + name: "device-code-profile", + team: "https://example.com", + apiToken: "old-token", + authenticationType: AuthenticationType.BEARER, + type: ProfileType.DEVICE_CODE, + refreshToken: "revoked-refresh-token", + expiresAt: Math.floor(Date.now() / 1000) - 10, + }; + mockIssuerDiscover.mockResolvedValue({ + Client: jest.fn().mockImplementation(() => ({ + refresh: jest.fn().mockRejectedValue(new Error("invalid_grant")), + })), + }); + jest.spyOn(profileService, "storeProfile").mockImplementation(async () => {}); + + await expect(profileService.refreshProfile(profile)).rejects.toThrow("cannot be refreshed"); + + expect(profile.apiToken).toBe("old-token"); + expect(profile.refreshToken).toBe("revoked-refresh-token"); + }); }); describe("Profile Service - Store Profile", () => {