diff --git a/src/renderer/components/GlobalEffects.test.tsx b/src/renderer/components/GlobalEffects.test.tsx index 827fb5433..a96e750fd 100644 --- a/src/renderer/components/GlobalEffects.test.tsx +++ b/src/renderer/components/GlobalEffects.test.tsx @@ -54,7 +54,7 @@ describe('renderer/components/GlobalEffects.tsx', () => { expect(useShortcutRegistrationStore.getState().shortcutRegistrationError).toBeNull(); }); - describe('migrateAccountTokens (startup)', () => { + describe('persistRotatedAccountTokens (startup)', () => { const refreshAccountSpy = vi .spyOn(authUtils, 'refreshAccount') .mockImplementation(async (account) => account); @@ -94,9 +94,8 @@ describe('renderer/components/GlobalEffects.tsx', () => { expect(useAccountsStore.getState().accounts[0]?.token).toBe(mockGitHubCloudAccount.token); }); - it('re-encrypts plaintext token (legacy migration) when decrypt throws', async () => { + it('does not re-encrypt or persist when decrypt throws', async () => { decryptValueSpy.mockRejectedValue(new Error('not encrypted')); - encryptValueSpy.mockResolvedValue('newly-encrypted'); await act(async () => { renderWithProviders(, { @@ -104,8 +103,8 @@ describe('renderer/components/GlobalEffects.tsx', () => { }); }); - expect(encryptValueSpy).toHaveBeenCalledWith(mockGitHubCloudAccount.token); - expect(useAccountsStore.getState().accounts[0]?.token).toBe('newly-encrypted'); + expect(encryptValueSpy).not.toHaveBeenCalled(); + expect(useAccountsStore.getState().accounts[0]?.token).toBe(mockGitHubCloudAccount.token); }); }); }); diff --git a/src/renderer/hooks/useAccounts.ts b/src/renderer/hooks/useAccounts.ts index 283f12b34..c167aaa1b 100644 --- a/src/renderer/hooks/useAccounts.ts +++ b/src/renderer/hooks/useAccounts.ts @@ -17,22 +17,21 @@ interface AccountsState { * Hook that keeps account details fresh. * * Refreshes all accounts on startup and on an hourly interval via TanStack - * Query. Token migration runs once, before the first refresh, so legacy or - * rotated tokens are re-encrypted ahead of any API calls. + * Query. Any keychain-rotated token ciphertext is persisted once, before the + * first refresh, so rotated tokens are stored ahead of any API calls. */ export const useAccounts = (): AccountsState => { const accounts = useAccountsStore((state) => state.accounts); - const hasMigratedTokensRef = useRef(false); + const hasPersistedRotatedTokensRef = useRef(false); const { refetch } = useQuery({ queryKey: accountsKeys.refresh(accounts.map(getAccountUUID)), queryFn: async () => { - // TODO - Remove token migration logic in future release - if (!hasMigratedTokensRef.current) { - await useAccountsStore.getState().migrateAccountTokens(); - hasMigratedTokensRef.current = true; + if (!hasPersistedRotatedTokensRef.current) { + await useAccountsStore.getState().persistRotatedAccountTokens(); + hasPersistedRotatedTokensRef.current = true; } const refreshAccount = useAccountsStore.getState().refreshAccount; diff --git a/src/renderer/stores/types.ts b/src/renderer/stores/types.ts index a227f7ba0..4db45bf7b 100644 --- a/src/renderer/stores/types.ts +++ b/src/renderer/stores/types.ts @@ -50,12 +50,12 @@ export interface AccountsActions { removeAccount: (account: Account) => void; /** - * Re-encrypts persisted account tokens when the OS keychain rotated keys, - * and encrypts any tokens persisted in plaintext by legacy versions. + * Persists account tokens re-encrypted after the OS keychain rotated keys. * - * TODO - Remove migration logic in future release + * Tokens are expected to already be encrypted at rest; plaintext tokens from + * pre-encryption releases are no longer migrated and require re-authentication. */ - migrateAccountTokens: () => Promise; + persistRotatedAccountTokens: () => Promise; /** * Checks if the user is logged in (has at least one account). diff --git a/src/renderer/stores/useAccountsStore.ts b/src/renderer/stores/useAccountsStore.ts index 07817df10..e8ee9d008 100644 --- a/src/renderer/stores/useAccountsStore.ts +++ b/src/renderer/stores/useAccountsStore.ts @@ -19,7 +19,7 @@ import { } from '../utils/auth/utils'; import { rendererLogWarn } from '../utils/core/logger'; import { getAdapter, isKnownForge } from '../utils/forges/registry'; -import { decryptValue, encryptValue } from '../utils/system/comms'; +import { decryptValue } from '../utils/system/comms'; import { DEFAULT_ACCOUNTS_STATE } from './defaults'; /** @@ -98,38 +98,35 @@ const useAccountsStore = create()( set({ accounts: updated.accounts }); }, - migrateAccountTokens: async () => { + persistRotatedAccountTokens: async () => { const accounts = get().accounts; - const migratedAccounts = await Promise.all( + const rotatedAccounts = await Promise.all( accounts.map(async (account) => { try { const { reEncryptedToken } = await decryptValue(account.token); if (reEncryptedToken) { return { ...account, token: reEncryptedToken as Token }; } - return account; } catch { + // Tokens are expected to already be encrypted at rest; plaintext + // tokens from pre-encryption releases are no longer migrated. + // Leave the account unchanged (the user must re-authenticate). rendererLogWarn( - 'migrateAccountTokens', - `token for account ${account.user?.login ?? account.hostname} could not be decrypted, re-encrypting`, + 'persistRotatedAccountTokens', + `token for account ${account.user?.login ?? account.hostname} could not be decrypted; the account may need to be re-authenticated`, ); - const encryptedToken = (await encryptValue(account.token)) as Token; - return { ...account, token: encryptedToken }; } + return account; }), ); - const tokensMigrated = migratedAccounts.some((migratedAccount) => { - const originalAccount = accounts.find( - (account) => getAccountUUID(account) === getAccountUUID(migratedAccount), - ); - - return !originalAccount || migratedAccount.token !== originalAccount.token; - }); + const tokensRotated = rotatedAccounts.some( + (account, index) => account.token !== accounts[index]?.token, + ); - if (tokensMigrated) { - set({ accounts: migratedAccounts }); + if (tokensRotated) { + set({ accounts: rotatedAccounts }); } },