Skip to content

Commit

Permalink
feat(uninstall): Delete cached credentials during re-install (CODY-1043
Browse files Browse the repository at this point in the history
…) (#5819)

This is the VSCode half of
[CODY-1043](https://linear.app/sourcegraph/issue/CODY-1043/bug-account-credentials-are-cached-between-installs).

Unfortunately, we don't have access to any VSCode APIs during the
uninstall script; it is just a raw node process that VSCode kicks off
the next time VSCode is started after an uninstall (could be an
arbitrary time after uninstalling). So the way we collected telemetry
was to create files in the deactivation subroutine (which is run anytime
the extension is stopped, i.e. when it is deactivated, VSCode is closed,
the extension is upgraded or uninstalled) that stored all the config
needed to boot telemetry and then we read those resources in the
uninstall script.

Here we do basically the opposite. The uninstall script is run only when
the user explicitly uninstalls the extension and closes and re-opens
VSCode (importantly it does not run when the extension is upgraded). We
create a marker file in the uninstallation process which we can check
when the extension is initializing. When present, we delete the file and
perform any cleanup from the previous installation (currently just
deleting auth credentials and previous endpoints).

This also fixes some existing errors in the uninstall script (it isn't
working today because the client capabilities aren't initialized), adds
a check to the bundling step for the uninstaller that no vscode
dependencies are transitively included (similar to the vscode-shim) and
adds a re-install telemetry event.

## Test plan
Finally added an E2E test for the uninstall flow. It took forever, but
seeing as how it was broken for gosh knows how long and we didn't know
indicates that it was necessary.

To manually test it:
1. Build the VSIX bundle with `pnpm -C vscode _build:vsix_for_test`
2. Start up VSCode and uninstall the Cody extension (or use the
alternate editor, for instance I use VSCode Insiders day to day so I do
this testing in VSCode).
3. From the command palette run "Extension: Install from VSIX" and
select `vscode/dist/cody.e2e.vsix`. Log into Cody normally.
4. Close and reopen code. Verify that your credentials are cached.
5. Uninstall Cody (click on the gear icon on the extension view and
select uninstall).
6. Fully close Code (not just the open window but the whole process)
7. Re-open Code and re-install Cody from VSIX.
8. Verify that credentials are cleared and you are presented with a
fresh login screen.

## Changelog
deletes auth credentials and endpoint history when the extension is
reinstalled

---------

Co-authored-by: Beatrix <68532117+abeatrix@users.noreply.github.com>
  • Loading branch information
jamesmcnamara and abeatrix authored Oct 16, 2024
1 parent 2f56d2b commit e9a460e
Show file tree
Hide file tree
Showing 35 changed files with 600 additions and 306 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ interface CodyAgentServer {
fun ignore_test(params: Ignore_TestParams): CompletableFuture<Ignore_TestResult>
@JsonRequest("testing/ignore/overridePolicy")
fun testing_ignore_overridePolicy(params: ContextFilters?): CompletableFuture<Null?>
@JsonRequest("extension/reset")
fun extension_reset(params: Null?): CompletableFuture<Null?>

// =============
// Notifications
Expand Down
1 change: 0 additions & 1 deletion agent/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@
"csv-writer": "^1.6.0",
"dedent": "^0.7.0",
"easy-table": "^1.2.0",
"env-paths": "^3.0.0",
"fast-myers-diff": "^3.2.0",
"glob": "^11.0.0",
"js-levenshtein": "^1.1.6",
Expand Down
7 changes: 6 additions & 1 deletion agent/src/agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import type * as agent_protocol from '../../vscode/src/jsonrpc/agent-protocol'
import { mkdirSync, statSync } from 'node:fs'
import { PassThrough } from 'node:stream'
import type { Har } from '@pollyjs/persister'
import { codyPaths } from '@sourcegraph/cody-shared'
import { TESTING_TELEMETRY_EXPORTER } from '@sourcegraph/cody-shared/src/telemetry-v2/TelemetryRecorderProvider'
import { type TelemetryEventParameters, TestTelemetryExporter } from '@sourcegraph/telemetry'
import { copySync } from 'fs-extra'
Expand Down Expand Up @@ -76,7 +77,6 @@ import { AgentWorkspaceConfiguration } from './AgentWorkspaceConfiguration'
import { AgentWorkspaceDocuments } from './AgentWorkspaceDocuments'
import { registerNativeWebviewHandlers, resolveWebviewView } from './NativeWebview'
import type { PollyRequestError } from './cli/command-jsonrpc-stdio'
import { codyPaths } from './codyPaths'
import {
currentProtocolAuthStatus,
currentProtocolAuthStatusOrNotReadyYet,
Expand Down Expand Up @@ -967,6 +967,11 @@ export class Agent extends MessageHandler implements ExtensionClient {
}
})

this.registerAuthenticatedRequest('extension/reset', async () => {
await this.globalState?.reset()
return null
})

this.registerNotification('autocomplete/completionAccepted', async ({ completionID }) => {
const provider = await vscode_shim.completionProvider()
await provider.handleDidAcceptCompletionItem(completionID as CompletionItemID)
Expand Down
2 changes: 1 addition & 1 deletion agent/src/cli/command-auth/settings.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import fs from 'node:fs'

import path from 'node:path'
import { codyPaths } from '../../codyPaths'
import { codyPaths } from '@sourcegraph/cody-shared'

export interface Account {
// In most cases, the ID will be the same as the username. It's only when
Expand Down
3 changes: 1 addition & 2 deletions agent/src/cli/command-bench/command-bench.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { newAgentClient } from '../../agent'
import { exec } from 'node:child_process'
import fs from 'node:fs'
import { promisify } from 'node:util'
import { isDefined, modelsService, setClientCapabilities } from '@sourcegraph/cody-shared'
import { codyPaths, isDefined, modelsService, setClientCapabilities } from '@sourcegraph/cody-shared'
import { sleep } from '../../../../vscode/src/completions/utils'
import {
getConfiguration,
Expand All @@ -21,7 +21,6 @@ import { createOrUpdateTelemetryRecorderProvider } from '../../../../vscode/src/
import { startPollyRecording } from '../../../../vscode/src/testutils/polly'
import { dotcomCredentials } from '../../../../vscode/src/testutils/testing-credentials'
import { allClientCapabilitiesEnabled } from '../../allClientCapabilitiesEnabled'
import { codyPaths } from '../../codyPaths'
import { arrayOption, booleanOption, intOption } from './cli-parsers'
import { matchesGlobPatterns } from './matchesGlobPatterns'
import { evaluateAutocompleteStrategy } from './strategy-autocomplete'
Expand Down
25 changes: 3 additions & 22 deletions agent/src/esbuild.mjs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import fs from 'node:fs/promises'
import path from 'node:path'
import process from 'node:process'
// @ts-ignore this is not compiled by typescript so it can import files from outside the rootDir
import { detectForbiddenImportPlugin } from '../../lib/shared/esbuild.utils.mjs'

import { build } from 'esbuild'

Expand Down Expand Up @@ -65,7 +67,7 @@ async function buildAgent() {
'@sourcegraph/cody-shared/src': '@sourcegraph/cody-shared/src',
},
}
const res = await build(esbuildOptions)
await build(esbuildOptions)

// Copy all .wasm files to the dist/ directory
const distDir = path.join(process.cwd(), '..', 'vscode', 'dist')
Expand All @@ -83,24 +85,3 @@ async function buildAgent() {
await fs.copyFile(src, dest)
}
}

function detectForbiddenImportPlugin(allForbiddenModules) {
return {
name: 'detect-forbidden-import-plugin',
setup(build) {
build.onResolve({ filter: /.*/ }, args => {
for (const forbidden of allForbiddenModules) {
if (args.path === forbidden) {
throw new Error(`'${forbidden}' module is imported in file: ${args.importer}`)
}
}
args
})

build.onLoad({ filter: /.*/ }, async args => {
const contents = await fs.readFile(args.path, 'utf8')
return { contents, loader: 'default' }
})
},
}
}
12 changes: 7 additions & 5 deletions agent/src/global-state/AgentGlobalState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,10 @@ export class AgentGlobalState implements vscode.Memento {
}

public async reset(): Promise<void> {
if (this.db instanceof InMemoryDB) {
this.db.clear()
this.db.clear()

// HACK(sqs): Force `localStorage` to fire a change event.
await localStorage.delete('')
}
// HACK(sqs): Force `localStorage` to fire a change event.
await localStorage.delete('')
}

public keys(): readonly string[] {
Expand Down Expand Up @@ -91,6 +89,7 @@ interface DB {
get(key: string): any
set(key: string, value: any): void
keys(): readonly string[]
clear(): void
}

class InMemoryDB implements DB {
Expand Down Expand Up @@ -120,6 +119,9 @@ class LocalStorageDB implements DB {
const quota = 1024 * 1024 * 256 // 256 MB
this.storage = new LocalStorage(path.join(dir, `${ide}-globalState`), quota)
}
clear() {
this.storage.clear()
}

get(key: string): any {
const item = this.storage.getItem(key)
Expand Down
20 changes: 16 additions & 4 deletions agent/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,21 @@
"rootDir": ".",
"outDir": "dist",
"target": "es2019",
"allowJs": true,
"allowJs": false
},
"include": ["**/*", ".*", "package.json", "src/language-file-extensions.json"],
"exclude": ["dist", "bindings", "src/__tests__", "vitest.config.ts", "typehacks"],
"references": [{ "path": "../lib/shared" }, { "path": "../vscode" }],
"include": [
"**/*",
".*",
"package.json",
"src/language-file-extensions.json"
],
"exclude": [
"dist",
"bindings",
"src/__tests__",
"vitest.config.ts",
"typehacks",
"*.mjs"
],
"references": [{ "path": "../lib/shared" }, { "path": "../vscode" }]
}
22 changes: 22 additions & 0 deletions lib/shared/esbuild.utils.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import fs from 'node:fs/promises'

export function detectForbiddenImportPlugin(allForbiddenModules) {
return {
name: 'detect-forbidden-import-plugin',
setup(build) {
build.onResolve({ filter: /.*/ }, args => {
for (const forbidden of allForbiddenModules) {
if (args.path === forbidden) {
throw new Error(`'${forbidden}' module is imported in file: ${args.importer}`)
}
}
args
})

build.onLoad({ filter: /.*/ }, async args => {
const contents = await fs.readFile(args.path, 'utf8')
return { contents, loader: 'default' }
})
},
}
}
1 change: 1 addition & 0 deletions lib/shared/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"date-fns": "^2.30.0",
"dedent": "^0.7.0",
"diff": "^5.2.0",
"env-paths": "^2.2.1",
"immer": "^10.1.1",
"isomorphic-fetch": "^3.0.0",
"js-tiktoken": "^1.0.14",
Expand Down
6 changes: 2 additions & 4 deletions lib/shared/src/auth/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,14 @@ export interface UnauthenticatedAuthStatus {
}

export const AUTH_STATUS_FIXTURE_AUTHED: AuthenticatedAuthStatus = {
// this typecast is necessary to prevent codegen from becoming too specific
endpoint: 'https://example.com' as string,
endpoint: 'https://example.com',
authenticated: true,
username: 'alice',
pendingValidation: false,
}

export const AUTH_STATUS_FIXTURE_UNAUTHED: AuthStatus & { authenticated: false } = {
// this typecast is necessary to prevent codegen from becoming too specific
endpoint: 'https://example.com' as string,
endpoint: 'https://example.com',
authenticated: false,
pendingValidation: false,
}
Expand Down
File renamed without changes.
29 changes: 22 additions & 7 deletions lib/shared/src/configuration/resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ export interface ConfigurationInput {
clientConfiguration: ClientConfiguration
clientSecrets: ClientSecrets
clientState: ClientState
reinstall: {
isReinstalling(): Promise<boolean>
onReinstall(): Promise<void>
}
}

export interface ClientSecrets {
Expand All @@ -42,6 +46,7 @@ export type ResolvedConfiguration = ReadonlyDeep<{
configuration: ClientConfiguration
auth: AuthCredentials
clientState: ClientState
isReinstall: boolean
}>

/**
Expand All @@ -67,29 +72,39 @@ export type PickResolvedConfiguration<Keys extends KeysSpec> = {
: undefined
}

async function resolveConfiguration(input: ConfigurationInput): Promise<ResolvedConfiguration> {
async function resolveConfiguration({
clientConfiguration,
clientSecrets,
clientState,
reinstall: { isReinstalling, onReinstall },
}: ConfigurationInput): Promise<ResolvedConfiguration> {
const isReinstall = await isReinstalling()
if (isReinstall) {
await onReinstall()
}
// we allow for overriding the server endpoint from config if we haven't
// manually signed in somewhere else
const serverEndpoint = normalizeServerEndpointURL(
input.clientConfiguration.overrideServerEndpoint ||
(input.clientState.lastUsedEndpoint ?? DOTCOM_URL.toString())
clientConfiguration.overrideServerEndpoint ||
(clientState.lastUsedEndpoint ?? DOTCOM_URL.toString())
)

// We must not throw here, because that would result in the `resolvedConfig` observable
// terminating and all callers receiving no further config updates.
const loadTokenFn = () =>
input.clientSecrets.getToken(serverEndpoint).catch(error => {
clientSecrets.getToken(serverEndpoint).catch(error => {
logError(
'resolveConfiguration',
`Failed to get access token for endpoint ${serverEndpoint}: ${error}`
)
return null
})
const accessToken = input.clientConfiguration.overrideAuthToken || ((await loadTokenFn()) ?? null)
const accessToken = clientConfiguration.overrideAuthToken || ((await loadTokenFn()) ?? null)
return {
configuration: input.clientConfiguration,
clientState: input.clientState,
configuration: clientConfiguration,
clientState,
auth: { accessToken, serverEndpoint },
isReinstall,
}
}

Expand Down
1 change: 1 addition & 0 deletions lib/shared/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ export {
LARGE_FILE_WARNING_LABEL,
NO_SYMBOL_MATCHES_HELP_LABEL,
} from './codebase-context/messages'
export * from './codyPaths'
export type {
CodyCommand,
CodyCommandContext,
Expand Down
17 changes: 4 additions & 13 deletions lib/shared/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,9 @@
"module": "ESNext",
"rootDir": "src",
"outDir": "dist",
"lib": [
"ESNext",
"DOM",
"DOM.Iterable"
],
"moduleResolution": "Bundler",
"lib": ["ESNext", "DOM", "DOM.Iterable"],
"moduleResolution": "Bundler"
},
"include": [
"src",
],
"exclude": [
"dist",
"typehacks"
],
"include": ["src"],
"exclude": ["dist", "typehacks", "*.mjs"]
}
Loading

0 comments on commit e9a460e

Please sign in to comment.