Address PR review feedback

- Convert CliAuth getters to methods (getInstanceName, getBaseUrl) so
  options can be added in the future
- Remove StoredInstance from cli-node public API, hiding instance details
- Move secretStore to cli-internal for re-use, refactoring from fs-extra
  to node:fs
- Add shared getAuthInstanceService helper in cli-internal for
  constructing secret-store service keys
- Define StoredInstance locally in cli-module-auth instead of importing
  from cli-node
- Update all consumers and tests for the new method-based API

Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>
Made-with: Cursor

packages/cli-module-auth/package.json

@@ -19,6 +19,7 @@
   "license": "Apache-2.0",
   "main": "src/index.ts",
   "types": "src/index.ts",
+  "bin": "bin/backstage-cli-module-auth",
   "files": [
     "dist",
     "bin"
@@ -50,6 +51,5 @@
   },
   "optionalDependencies": {
     "keytar": "^7.9.0"
-  },
-  "bin": "bin/backstage-cli-module-auth"
+  }
 }

packages/cli-module-auth/src/commands/login.ts

@@ -27,7 +27,7 @@ import {
   getInstanceByName,
   StoredInstance,
 } from '../lib/storage';
-import { getSecretStore } from '../lib/secretStore';
+import { getSecretStore, getAuthInstanceService } from '@internal/cli';
 import crypto from 'node:crypto';
 import fs from 'fs-extra';
 import path from 'node:path';
@@ -321,7 +321,7 @@ async function persistInstance(options: {
   const { instanceName, backendBaseUrl, clientId, token } = options;
   const secretStore = await getSecretStore();
   await withMetadataLock(async () => {
-    const service = `backstage-cli:auth-instance:${instanceName}`;
+    const service = getAuthInstanceService(instanceName);
     await secretStore.set(service, 'accessToken', token.access_token);
     if (token.refresh_token) {
       await secretStore.set(service, 'refreshToken', token.refresh_token);

packages/cli-module-auth/src/commands/logout.ts

@@ -16,7 +16,7 @@
 
 import { cli } from 'cleye';
 import type { CliCommandContext } from '@backstage/cli-node';
-import { getSecretStore } from '../lib/secretStore';
+import { getSecretStore, getAuthInstanceService } from '@internal/cli';
 import {
   removeInstance,
   withMetadataLock,
@@ -47,7 +47,7 @@ export default async ({ args, info }: CliCommandContext) => {
   await withMetadataLock(async () => {
     const instance = await getInstanceByName(instanceName);
     const secretStore = await getSecretStore();
-    const service = `backstage-cli:auth-instance:${instanceName}`;
+    const service = getAuthInstanceService(instanceName);
     const refreshToken = (await secretStore.get(service, 'refreshToken')) ?? '';
 
     if (refreshToken) {

packages/cli-module-auth/src/commands/show.ts

@@ -38,7 +38,7 @@ export default async ({ args, info }: CliCommandContext) => {
   const auth = await CliAuth.create({ instanceName: instanceFlag });
   const accessToken = await auth.getAccessToken();
 
-  const authBase = new URL('/api/auth', auth.baseUrl)
+  const authBase = new URL('/api/auth', auth.getBaseUrl())
     .toString()
     .replace(/\/$/, '');
 

packages/cli-module-auth/src/lib/auth.test.ts

@@ -16,15 +16,15 @@
 
 import { accessTokenNeedsRefresh, refreshAccessToken } from './auth';
 import * as storage from './storage';
-import * as secretStore from './secretStore';
+import * as internalCli from '@internal/cli';
 import * as http from './http';
 
 jest.mock('./storage');
-jest.mock('./secretStore');
+jest.mock('@internal/cli');
 jest.mock('./http');
 
 const mockStorage = storage as jest.Mocked<typeof storage>;
-const mockSecretStore = secretStore as jest.Mocked<typeof secretStore>;
+const mockInternalCli = internalCli as jest.Mocked<typeof internalCli>;
 const mockHttp = http as jest.Mocked<typeof http>;
 
 describe('auth', () => {
@@ -95,7 +95,10 @@ describe('auth', () => {
 
     beforeEach(() => {
       jest.clearAllMocks();
-      mockSecretStore.getSecretStore.mockResolvedValue(mockSecretStoreInstance);
+      mockInternalCli.getSecretStore.mockResolvedValue(mockSecretStoreInstance);
+      mockInternalCli.getAuthInstanceService.mockImplementation(
+        (name: string) => `backstage-cli:auth-instance:${name}`,
+      );
     });
 
     it('should successfully refresh access token', async () => {

packages/cli-module-auth/src/lib/auth.ts

@@ -15,9 +15,13 @@
  */
 
 import { z } from 'zod/v3';
-import type { StoredInstance } from '@backstage/cli-node';
-import { upsertInstance, withMetadataLock, getInstanceByName } from './storage';
-import { getSecretStore } from './secretStore';
+import {
+  type StoredInstance,
+  upsertInstance,
+  withMetadataLock,
+  getInstanceByName,
+} from './storage';
+import { getSecretStore, getAuthInstanceService } from '@internal/cli';
 import { httpJson } from './http';
 
 const TokenResponseSchema = z.object({
@@ -40,7 +44,7 @@ export async function refreshAccessToken(
   return withMetadataLock(async () => {
     const instance = await getInstanceByName(instanceName);
 
-    const service = `backstage-cli:auth-instance:${instanceName}`;
+    const service = getAuthInstanceService(instanceName);
     const refreshToken = (await secretStore.get(service, 'refreshToken')) ?? '';
     if (!refreshToken) {
       throw new Error(

packages/cli-module-auth/src/lib/secretStore.test.ts

@@ -21,7 +21,7 @@ jest.mock('keytar', () => {
 import fs from 'fs-extra';
 import path from 'node:path';
 import { createMockDirectory } from '@backstage/backend-test-utils';
-import { getSecretStore, resetSecretStore } from './secretStore';
+import { getSecretStore, resetSecretStore } from '@internal/cli';
 
 const mockDir = createMockDirectory();
 

packages/cli-module-auth/src/lib/secretStore.ts

@@ -1,110 +0,0 @@
-/*
- * Copyright 2025 The Backstage Authors
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *     http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-import fs from 'fs-extra';
-import os from 'node:os';
-import path from 'node:path';
-
-type SecretStore = {
-  get(service: string, account: string): Promise<string | undefined>;
-  set(service: string, account: string, secret: string): Promise<void>;
-  delete(service: string, account: string): Promise<void>;
-};
-
-async function loadKeytar(): Promise<typeof import('keytar') | undefined> {
-  try {
-    // eslint-disable-next-line import/no-extraneous-dependencies, @backstage/no-undeclared-imports
-    const keytar = require('keytar') as typeof import('keytar');
-    if (keytar && typeof keytar.getPassword === 'function') {
-      return keytar;
-    }
-  } catch {
-    // keytar not available
-  }
-  return undefined;
-}
-
-class KeytarSecretStore implements SecretStore {
-  private readonly keytar: typeof import('keytar');
-  constructor(keytar: typeof import('keytar')) {
-    this.keytar = keytar;
-  }
-  async get(service: string, account: string): Promise<string | undefined> {
-    const result = await this.keytar.getPassword(service, account);
-    return result ?? undefined;
-  }
-  async set(service: string, account: string, secret: string): Promise<void> {
-    await this.keytar.setPassword(service, account, secret);
-  }
-  async delete(service: string, account: string): Promise<void> {
-    await this.keytar.deletePassword(service, account);
-  }
-}
-
-class FileSecretStore implements SecretStore {
-  private readonly baseDir: string;
-  constructor() {
-    const root =
-      process.env.XDG_DATA_HOME ||
-      (process.platform === 'win32'
-        ? process.env.APPDATA || path.join(os.homedir(), 'AppData', 'Roaming')
-        : path.join(os.homedir(), '.local', 'share'));
-    this.baseDir = path.join(root, 'backstage-cli', 'auth-secrets');
-  }
-  private filePath(service: string, account: string): string {
-    return path.join(
-      this.baseDir,
-      encodeURIComponent(service),
-      `${encodeURIComponent(account)}.secret`,
-    );
-  }
-  async get(service: string, account: string): Promise<string | undefined> {
-    const file = this.filePath(service, account);
-    if (!(await fs.pathExists(file))) return undefined;
-    return await fs.readFile(file, 'utf8');
-  }
-  async set(service: string, account: string, secret: string): Promise<void> {
-    const file = this.filePath(service, account);
-    await fs.ensureDir(path.dirname(file));
-    await fs.writeFile(file, secret, { encoding: 'utf8', mode: 0o600 });
-  }
-  async delete(service: string, account: string): Promise<void> {
-    const file = this.filePath(service, account);
-    await fs.remove(file);
-  }
-}
-
-let singleton: SecretStore | undefined;
-
-export async function getSecretStore(): Promise<SecretStore> {
-  if (!singleton) {
-    const keytar = await loadKeytar();
-    if (keytar) {
-      singleton = new KeytarSecretStore(keytar);
-    } else {
-      singleton = new FileSecretStore();
-    }
-  }
-  return singleton;
-}
-
-/**
- * Reset the singleton instance (for testing purposes only)
- * @internal
- */
-export function resetSecretStore(): void {
-  singleton = undefined;
-}

packages/cli-module-auth/src/lib/storage.ts

@@ -22,8 +22,15 @@ import lockfile from 'proper-lockfile';
 import YAML from 'yaml';
 import { z } from 'zod/v3';
 
-export type { StoredInstance } from '@backstage/cli-node';
-import type { StoredInstance } from '@backstage/cli-node';
+export type StoredInstance = {
+  name: string;
+  baseUrl: string;
+  clientId: string;
+  issuedAt: number;
+  accessTokenExpiresAt: number;
+  selected?: boolean;
+  config?: Record<string, unknown>;
+};
 
 const METADATA_FILE = 'auth-instances.yaml';