Skip to content

Commit 282ad7e

Browse files
authored
fix(auth): retain original validation exception (#3291)
## Problem Some of the validation logic happens 'passively', so issues aren't propagated to callers ## Solution Store the errors and chain them when handling invalid connections
1 parent 79fc42c commit 282ad7e

File tree

6 files changed

+122
-52
lines changed

6 files changed

+122
-52
lines changed
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
{
2+
"type": "Bug Fix",
3+
"description": "auth: switching to a connection that just expired shows an error"
4+
}

src/credentials/auth.ts

Lines changed: 80 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import { Commands } from '../shared/vscode/commands2'
1717
import { createQuickPick, DataQuickPickItem, showQuickPick } from '../shared/ui/pickerPrompter'
1818
import { isValidResponse } from '../shared/wizards/wizard'
1919
import { CancellationError } from '../shared/utilities/timeoutUtils'
20-
import { ToolkitError, UnknownError } from '../shared/errors'
20+
import { formatError, ToolkitError, UnknownError } from '../shared/errors'
2121
import { getCache } from './sso/cache'
2222
import { createFactoryFunction, Mutable } from '../shared/utilities/tsUtils'
2323
import { builderIdStartUrl, SsoToken } from './sso/model'
@@ -38,6 +38,7 @@ import { getCodeCatalystDevEnvId } from '../shared/vscode/env'
3838
import { getConfigFilename } from './sharedCredentials'
3939
import { authHelpUrl } from '../shared/constants'
4040
import { getDependentAuths } from './secondaryAuth'
41+
import { DevSettings } from '../shared/settings'
4142

4243
export const ssoScope = 'sso:account:access'
4344
export const codecatalystScopes = ['codecatalyst:read_write']
@@ -301,7 +302,8 @@ interface ConnectionStateChangeEvent {
301302
}
302303

303304
export class Auth implements AuthService, ConnectionManager {
304-
private readonly ssoCache = getCache()
305+
readonly #ssoCache = getCache()
306+
readonly #validationErrors = new Map<Connection['id'], Error>()
305307
readonly #onDidChangeActiveConnection = new vscode.EventEmitter<StatefulConnection | undefined>()
306308
readonly #onDidChangeConnectionState = new vscode.EventEmitter<ConnectionStateChangeEvent>()
307309
public readonly onDidChangeActiveConnection = this.#onDidChangeActiveConnection.event
@@ -455,6 +457,10 @@ export class Auth implements AuthService, ConnectionManager {
455457
return this.store.getProfile(connection.id)?.metadata.connectionState
456458
}
457459

460+
public getInvalidationReason(connection: Pick<Connection, 'id'>): Error | undefined {
461+
return this.#validationErrors.get(connection.id)
462+
}
463+
458464
/**
459465
* Attempts to remove all auth state related to the connection.
460466
*
@@ -487,6 +493,10 @@ export class Auth implements AuthService, ConnectionManager {
487493
}
488494

489495
const profile = await this.store.updateProfile(id, { connectionState })
496+
if (connectionState !== 'invalid') {
497+
this.#validationErrors.delete(id)
498+
}
499+
490500
if (this.#activeConnection?.id === id) {
491501
this.#activeConnection.state = connectionState
492502
this.#onDidChangeActiveConnection.fire(this.#activeConnection)
@@ -497,16 +507,16 @@ export class Auth implements AuthService, ConnectionManager {
497507
}
498508

499509
private async validateConnection<T extends Profile>(id: Connection['id'], profile: StoredProfile<T>) {
500-
if (profile.type === 'sso') {
501-
const provider = this.getTokenProvider(id, profile)
502-
if ((await provider.getToken()) === undefined) {
503-
return this.updateConnectionState(id, 'invalid')
510+
const runCheck = async () => {
511+
if (profile.type === 'sso') {
512+
const provider = this.getTokenProvider(id, profile)
513+
if ((await provider.getToken()) === undefined) {
514+
return this.updateConnectionState(id, 'invalid')
515+
} else {
516+
return this.updateConnectionState(id, 'valid')
517+
}
504518
} else {
505-
return this.updateConnectionState(id, 'valid')
506-
}
507-
} else {
508-
const provider = await this.getCredentialsProvider(id)
509-
try {
519+
const provider = await this.getCredentialsProvider(id)
510520
const credentials = await this.getCachedCredentials(provider)
511521
if (credentials !== undefined) {
512522
return this.updateConnectionState(id, 'valid')
@@ -517,10 +527,16 @@ export class Auth implements AuthService, ConnectionManager {
517527
} else {
518528
return this.updateConnectionState(id, 'invalid')
519529
}
520-
} catch {
521-
return this.updateConnectionState(id, 'invalid')
522530
}
523531
}
532+
533+
return runCheck().catch(err => this.handleValidationError(id, err))
534+
}
535+
536+
private async handleValidationError(id: Connection['id'], err: unknown) {
537+
this.#validationErrors.set(id, UnknownError.cast(err))
538+
539+
return this.updateConnectionState(id, 'invalid')
524540
}
525541

526542
private async getCredentialsProvider(id: Connection['id']) {
@@ -564,7 +580,7 @@ export class Auth implements AuthService, ConnectionManager {
564580
scopes: profile.scopes,
565581
region: profile.ssoRegion,
566582
},
567-
this.ssoCache
583+
this.#ssoCache
568584
)
569585
}
570586

@@ -612,7 +628,7 @@ export class Auth implements AuthService, ConnectionManager {
612628

613629
return result
614630
} catch (err) {
615-
await this.updateConnectionState(id, 'invalid')
631+
await this.handleValidationError(id, err)
616632
throw err
617633
}
618634
}
@@ -635,7 +651,9 @@ export class Auth implements AuthService, ConnectionManager {
635651

636652
private readonly getToken = keyedDebounce(this._getToken.bind(this))
637653
private async _getToken(id: Connection['id'], provider: SsoAccessTokenProvider): Promise<SsoToken> {
638-
const token = await provider.getToken()
654+
const token = await provider.getToken().catch(err => {
655+
this.#validationErrors.set(id, err)
656+
})
639657

640658
return token ?? this.handleInvalidCredentials(id, () => provider.createToken())
641659
}
@@ -659,6 +677,7 @@ export class Auth implements AuthService, ConnectionManager {
659677
if (previousState === 'invalid') {
660678
throw new ToolkitError('Connection is invalid or expired. Try logging in again.', {
661679
code: 'InvalidConnection',
680+
cause: this.#validationErrors.get(id),
662681
})
663682
}
664683
// TODO: cancellable notification?
@@ -669,6 +688,7 @@ export class Auth implements AuthService, ConnectionManager {
669688
throw new ToolkitError('User cancelled login', {
670689
cancelled: true,
671690
code: 'InvalidConnection',
691+
cause: this.#validationErrors.get(id),
672692
})
673693
}
674694
}
@@ -1003,47 +1023,57 @@ export function createConnectionPrompter(auth: Auth, type?: 'iam' | 'sso') {
10031023

10041024
function toPickerItem(conn: Connection): DataQuickPickItem<Connection> {
10051025
const state = auth.getConnectionState(conn)
1006-
if (state !== 'valid') {
1026+
if (state === 'valid') {
10071027
return {
10081028
data: conn,
1009-
invalidSelection: true,
1010-
label: codicon`${getIcon('vscode-error')} ${conn.label}`,
1011-
description:
1012-
state === 'authenticating'
1013-
? 'authenticating...'
1014-
: localize(
1015-
'aws.auth.promptConnection.expired.description',
1016-
'Expired or Invalid, select to authenticate'
1017-
),
1018-
onClick:
1019-
state !== 'authenticating'
1020-
? async () => {
1021-
// XXX: this is hack because only 1 picker can be shown at a time
1022-
// Some legacy auth providers will show a picker, hiding this one
1023-
// If we detect this then we'll jump straight into using the connection
1024-
let hidden = false
1025-
const sub = prompter.quickPick.onDidHide(() => {
1026-
hidden = true
1027-
sub.dispose()
1028-
})
1029-
const newConn = await reauthCommand.execute(auth, conn)
1030-
if (hidden && newConn && auth.getConnectionState(newConn) === 'valid') {
1031-
await auth.useConnection(newConn)
1032-
} else {
1033-
await prompter.clearAndLoadItems(loadItems())
1034-
prompter.selectItems(
1035-
...prompter.quickPick.items.filter(i => i.label.includes(conn.label))
1036-
)
1037-
}
1038-
}
1039-
: undefined,
1029+
label: codicon`${getConnectionIcon(conn)} ${conn.label}`,
1030+
description: getConnectionDescription(conn),
1031+
}
1032+
}
1033+
1034+
const getDetail = () => {
1035+
if (!DevSettings.instance.get('renderDebugDetails', false)) {
1036+
return
10401037
}
1038+
1039+
const err = auth.getInvalidationReason(conn)
1040+
return err ? formatError(err) : undefined
10411041
}
10421042

10431043
return {
1044+
detail: getDetail(),
10441045
data: conn,
1045-
label: codicon`${getConnectionIcon(conn)} ${conn.label}`,
1046-
description: getConnectionDescription(conn),
1046+
invalidSelection: true,
1047+
label: codicon`${getIcon('vscode-error')} ${conn.label}`,
1048+
description:
1049+
state === 'authenticating'
1050+
? 'authenticating...'
1051+
: localize(
1052+
'aws.auth.promptConnection.expired.description',
1053+
'Expired or Invalid, select to authenticate'
1054+
),
1055+
onClick:
1056+
state !== 'authenticating'
1057+
? async () => {
1058+
// XXX: this is hack because only 1 picker can be shown at a time
1059+
// Some legacy auth providers will show a picker, hiding this one
1060+
// If we detect this then we'll jump straight into using the connection
1061+
let hidden = false
1062+
const sub = prompter.quickPick.onDidHide(() => {
1063+
hidden = true
1064+
sub.dispose()
1065+
})
1066+
const newConn = await reauthCommand.execute(auth, conn)
1067+
if (hidden && newConn && auth.getConnectionState(newConn) === 'valid') {
1068+
await auth.useConnection(newConn)
1069+
} else {
1070+
await prompter.clearAndLoadItems(loadItems())
1071+
prompter.selectItems(
1072+
...prompter.quickPick.items.filter(i => i.label.includes(conn.label))
1073+
)
1074+
}
1075+
}
1076+
: undefined,
10471077
}
10481078
}
10491079

src/credentials/sso/ssoAccessTokenProvider.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,10 @@ import { hasProps, hasStringProps, RequiredProps, selectFrom } from '../../share
1515
import { CancellationError, sleep } from '../../shared/utilities/timeoutUtils'
1616
import { OidcClient } from './clients'
1717
import { loadOr } from '../../shared/utilities/cacheUtils'
18-
import { getTelemetryReason, getTelemetryResult, isClientFault, ToolkitError } from '../../shared/errors'
18+
import { getRequestId, getTelemetryReason, getTelemetryResult, isClientFault, ToolkitError } from '../../shared/errors'
1919
import { getLogger } from '../../shared/logger'
2020
import { telemetry } from '../../shared/telemetry/telemetry'
21+
import { DevSettings } from '../../shared/settings'
2122

2223
const clientRegistrationType = 'public'
2324
const deviceGrantType = 'urn:ietf:params:oauth:grant-type:device_code'
@@ -122,7 +123,12 @@ export class SsoAccessTokenProvider {
122123

123124
return refreshed
124125
} catch (err) {
125-
telemetry.aws_refreshCredentials.emit({
126+
const span = telemetry.aws_refreshCredentials
127+
if (DevSettings.instance.get('reportRequestIds', false)) {
128+
span.record({ requestId: getRequestId(err) } as any)
129+
}
130+
131+
span.emit({
126132
result: getTelemetryResult(err),
127133
reason: getTelemetryReason(err),
128134
sessionDuration: getSessionDuration(this.tokenCacheKey),

src/shared/errors.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -297,3 +297,13 @@ export function isUserCancelledError(error: unknown): boolean {
297297
export function isClientFault(error: ServiceException): boolean {
298298
return error.$fault === 'client' && !(isThrottlingError(error) || isTransientError(error))
299299
}
300+
301+
export function getRequestId(error: unknown): string | undefined {
302+
if (isAwsError(error)) {
303+
return error.requestId
304+
}
305+
306+
if (error instanceof ServiceException) {
307+
return error.$metadata.requestId
308+
}
309+
}

src/shared/settings.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -544,6 +544,8 @@ const devSettings = {
544544
forceInstallTools: Boolean,
545545
telemetryEndpoint: String,
546546
telemetryUserPool: String,
547+
reportRequestIds: Boolean,
548+
renderDebugDetails: Boolean,
547549
endpoints: Record(String, String),
548550
cawsStage: String,
549551
}

src/test/credentials/auth.test.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,15 @@ describe('Auth', function () {
170170
await auth.useConnection(conn)
171171
assert.strictEqual(events.emits[0]?.id, conn.id)
172172
})
173+
174+
it('sets the active connection even when the underlying provider throws', async function () {
175+
const err = new Error('test')
176+
const conn = await auth.createConnection(ssoProfile)
177+
tokenProviders.get(conn.id)?.getToken.rejects(err)
178+
await auth.useConnection(conn)
179+
assert.strictEqual(auth.activeConnection?.id, conn.id)
180+
assert.strictEqual(auth.getInvalidationReason(conn), err)
181+
})
173182
})
174183

175184
it('can login and fires an event', async function () {
@@ -236,6 +245,15 @@ describe('Auth', function () {
236245

237246
assert.strictEqual(auth.activeConnection?.state, 'invalid')
238247
})
248+
249+
it('chains errors when handling invalid connections', async function () {
250+
const err1 = new ToolkitError('test', { code: 'test' })
251+
const conn = await auth.createConnection(ssoProfile)
252+
tokenProviders.get(conn.id)?.getToken.rejects(err1)
253+
const err2 = await runExpiredGetTokenFlow(conn, /no/i).catch(e => e)
254+
assert.ok(err2 instanceof ToolkitError)
255+
assert.strictEqual(err2.cause, err1)
256+
})
239257
})
240258

241259
describe('AuthNode', function () {

0 commit comments

Comments
 (0)