Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(amazonq): Use the correct state for Q in remote environment #5575

Merged
merged 2 commits into from
Sep 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,11 @@ describe('AuthUtil', async function () {
// Switch to unsupported connection
const cwAuthUpdatedConnection = captureEventOnce(authUtil.secondaryAuth.onDidChangeActiveConnection)
await auth.useConnection(unsupportedConn)
// This is triggered when the main Auth connection is switched
await cwAuthUpdatedConnection
// This is triggered by registerAuthListener() when it saves the previous active connection as a fallback.
// TODO in a refactor see if we can simplify multiple multiple triggers on the same event.
await captureEventOnce(authUtil.secondaryAuth.onDidChangeActiveConnection)

// Is using the fallback connection
assert.ok(authUtil.isConnected())
Expand Down
13 changes: 12 additions & 1 deletion packages/core/src/auth/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1022,9 +1022,20 @@ export class Auth implements AuthService, ConnectionManager {
}
}

/**
* Auth uses its own state, separate from the default {@link globalState}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really separate, but nested in the globalState.

* that is normally used throughout the codebase.
*
* IMPORTANT: Anything involving auth should ONLY use this state since the state
* can vary on certain conditions. So if you see something explicitly using
* globalState verify if it should actually be using that.
*/
public getStateMemento: () => vscode.Memento = () => Auth._getStateMemento()
private static _getStateMemento = once(() => getEnvironmentSpecificMemento())

static #instance: Auth | undefined
public static get instance() {
return (this.#instance ??= new Auth(new ProfileStore(getEnvironmentSpecificMemento())))
return (this.#instance ??= new Auth(new ProfileStore(Auth._getStateMemento())))
}

private getSsoProfileLabel(profile: SsoProfile) {
Expand Down
20 changes: 10 additions & 10 deletions packages/core/src/auth/secondaryAuth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
* SPDX-License-Identifier: Apache-2.0
*/

import globals from '../shared/extensionGlobals'

import * as vscode from 'vscode'
import { getLogger } from '../shared/logger'
import { cast, Optional } from '../shared/utilities/typeConstructors'
Expand All @@ -18,6 +16,7 @@ import { AuthStatus, telemetry } from '../shared/telemetry/telemetry'
import { asStringifiedStack } from '../shared/telemetry/spans'
import { withTelemetryContext } from '../shared/telemetry/util'
import { isNetworkError } from '../shared/errors'
import globals from '../shared/extensionGlobals'

export type ToolId = 'codecatalyst' | 'codewhisperer' | 'testId'

Expand Down Expand Up @@ -214,10 +213,14 @@ export class SecondaryAuth<T extends Connection = Connection> {
return !!this.activeConnection && this.auth.getConnectionState(this.activeConnection) === 'invalid'
}

public get state() {
return this.auth.getStateMemento()
}

public async saveConnection(conn: T) {
// TODO: fix this
// eslint-disable-next-line aws-toolkits/no-banned-usages
await globals.context.globalState.update(this.key, conn.id)
await this.state.update(this.key, conn.id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The eslint ignore comment can be removed

this.#savedConnection = conn
this.#onDidChangeActiveConnection.fire(this.activeConnection)
}
Expand Down Expand Up @@ -251,7 +254,7 @@ export class SecondaryAuth<T extends Connection = Connection> {
public async clearSavedConnection() {
// TODO: fix this
// eslint-disable-next-line aws-toolkits/no-banned-usages
await globals.context.globalState.update(this.key, undefined)
await this.state.update(this.key, undefined)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The eslint ignore comment can be removed

this.#savedConnection = undefined
this.#onDidChangeActiveConnection.fire(this.activeConnection)
}
Expand Down Expand Up @@ -326,9 +329,6 @@ export class SecondaryAuth<T extends Connection = Connection> {
* Provides telemetry if called by restoreConnection() (or another auth_modifyConnection context)
*/
private async _loadSavedConnection() {
// TODO: fix this
// eslint-disable-next-line aws-toolkits/no-banned-usages
const globalState = globals.context.globalState
const id = this.getStateConnectionId()
if (id === undefined) {
return
Expand All @@ -337,10 +337,10 @@ export class SecondaryAuth<T extends Connection = Connection> {
const conn = await this.auth.getConnection({ id })
if (conn === undefined) {
getLogger().warn(`auth (${this.toolId}): removing saved connection "${this.key}" as it no longer exists`)
await globalState.update(this.key, undefined)
await this.state.update(this.key, undefined)
} else if (!this.isUsable(conn)) {
getLogger().warn(`auth (${this.toolId}): saved connection "${this.key}" is not valid`)
await globalState.update(this.key, undefined)
await this.state.update(this.key, undefined)
} else {
const getAuthStatus = (state: ReturnType<typeof this.auth.getConnectionState>): AuthStatus => {
return state === 'invalid' ? 'expired' : 'connected'
Expand Down Expand Up @@ -377,7 +377,7 @@ export class SecondaryAuth<T extends Connection = Connection> {
}

private getStateConnectionId() {
return cast(globals.globalState.get(this.key), Optional(String))
return cast(this.state.get(this.key), Optional(String))
}
}

Expand Down
9 changes: 8 additions & 1 deletion packages/core/src/shared/utilities/mementos.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,14 @@ export function partition(memento: vscode.Memento, key: string): vscode.Memento
}
}

/** Avoids sharing globalState with remote vscode instances. */
/**
* Resolves the appropriate memento/state for the current runtime environment.
*
* Why?
* - In remote instances where we ssh in to them, we do not always want to share
* with the local globalState. We want certain functionality to be isolated to
* the remote instance.
*/
export function getEnvironmentSpecificMemento(): vscode.Memento {
if (!vscode.env.remoteName) {
// local compute: no further partitioning
Expand Down
8 changes: 7 additions & 1 deletion packages/core/src/test/credentials/secondaryAuth.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { createBuilderIdProfile, createTestAuth } from './testUtil'
import { Connection, createSsoProfile, hasScopes, isSsoConnection } from '../../auth/connection'
import assert from 'assert'
import globals from '../../shared/extensionGlobals'
import { waitUntil } from '../../shared/utilities/timeoutUtils'

describe('SecondaryAuth', function () {
let auth: ReturnType<typeof createTestAuth>
Expand Down Expand Up @@ -100,8 +101,13 @@ describe('SecondaryAuth', function () {
// delete SecondaryAuth
await auth.deleteConnection(conn)

// currently both Auth onDidChangeConnectionState and onDidDeleteConnection trigger
// and we need to wait for both of the callbacks defined through them in SecondaryAuth to complete.
// We know they all completed when SecondaryAuth.onDidChangeActiveConnection has been called twice
await waitUntil(async () => onDidChangeActiveConnection.callCount === 2, { interval: 10, timeout: 10000 })

// we fallback to the PrimaryAuth connection
assert.strictEqual(onDidChangeActiveConnection.called, true)
assert.strictEqual(onDidChangeActiveConnection.callCount, 2)
assert.deepStrictEqual(
{
id: secondaryAuth.activeConnection?.id,
Expand Down
Loading