fix(auth): network error may invalidate SSO token (#4582)

* auth: handle networking issues during sso validation

Problem:

We were having issues due to network issues,
such as no internet connected, the SSO auth token
would become invalid.

Solution:

We had some code that validated the token, but if it failed
due to network issues it would invalidate it.

Now, when we validate the token and there is a network error,
we will keep the state as-is instead of setting the connection
to invalid.

The user will instead see an error message indicating network issues.

Signed-off-by: Nikolas Komonen <[email protected]>

* rename: getToken -> getSsoToken

Imo this makes it easier to understand as I skim over
the code since 'SSO' aligns it with with the other naming
in this file.

Signed-off-by: Nikolas Komonen <[email protected]>

* refactor: remove unused parameter

This parameter is not used anywhere

Signed-off-by: Nikolas Komonen <[email protected]>

* add test + remove duplicate code

Signed-off-by: Nikolas Komonen <[email protected]>

---------

Signed-off-by: Nikolas Komonen <[email protected]>

Author: nkomonen-amazon

packages/core/src/auth/auth.ts

@@ -139,7 +139,7 @@ export class Auth implements AuthService, ConnectionManager {
         private readonly store: ProfileStore,
         private readonly iamProfileProvider = CredentialsProviderManager.getInstance(),
         private readonly createSsoClient = SsoClient.create.bind(SsoClient),
-        private readonly createTokenProvider = createFactoryFunction(SsoAccessTokenProvider)
+        private readonly createSsoTokenProvider = createFactoryFunction(SsoAccessTokenProvider)
     ) {}
 
     #activeConnection: Mutable<StatefulConnection> | undefined
@@ -169,7 +169,7 @@ export class Auth implements AuthService, ConnectionManager {
     public async reauthenticate({ id }: Pick<Connection, 'id'>): Promise<Connection> {
         const profile = this.store.getProfileOrThrow(id)
         if (profile.type === 'sso') {
-            const provider = this.getTokenProvider(id, profile)
+            const provider = this.getSsoTokenProvider(id, profile)
             await this.authenticate(id, () => provider.createToken())
 
             return this.getSsoConnection(id, profile)
@@ -184,14 +184,13 @@ export class Auth implements AuthService, ConnectionManager {
     public async useConnection({ id }: Pick<SsoConnection, 'id'>): Promise<SsoConnection>
     public async useConnection({ id }: Pick<IamConnection, 'id'>): Promise<IamConnection>
     public async useConnection({ id }: Pick<Connection, 'id'>): Promise<Connection> {
+        await this.refreshConnectionState({ id })
+
         const profile = this.store.getProfile(id)
         if (profile === undefined) {
             throw new Error(`Connection does not exist: ${id}`)
         }
-        getLogger().info(`auth: validating connection before using it: ${id}`)
-        const validated = await this.validateConnection(id, profile)
-        const conn =
-            validated.type === 'sso' ? this.getSsoConnection(id, validated) : this.getIamConnection(id, validated)
+        const conn = profile.type === 'sso' ? this.getSsoConnection(id, profile) : this.getIamConnection(id, profile)
 
         this.#activeConnection = conn
         this.#onDidChangeActiveConnection.fire(conn)
@@ -253,7 +252,7 @@ export class Auth implements AuthService, ConnectionManager {
                             this.store,
                             id,
                             profile,
-                            this.createSsoClient(profile.ssoRegion, this.getTokenProvider(id, profile))
+                            this.createSsoClient(profile.ssoRegion, this.getSsoTokenProvider(id, profile))
                         )
                     )
                         .catch(err => {
@@ -276,7 +275,7 @@ export class Auth implements AuthService, ConnectionManager {
         }
 
         const id = uuid.v4()
-        const tokenProvider = this.getTokenProvider(id, {
+        const tokenProvider = this.getSsoTokenProvider(id, {
             ...profile,
             metadata: { connectionState: 'unauthenticated' },
         })
@@ -344,10 +343,10 @@ export class Auth implements AuthService, ConnectionManager {
         if (profile.type === 'iam') {
             throw new ToolkitError('Auth: Cannot force expire an IAM connection')
         }
-        const provider = this.getTokenProvider(conn.id, profile)
+        const provider = this.getSsoTokenProvider(conn.id, profile)
         await provider.invalidate()
         // updates the state of the connection
-        await this.validateConnection(conn.id, profile)
+        await this.refreshConnectionState(conn)
     }
 
     public async getConnection(connection: Pick<Connection, 'id'>): Promise<Connection | undefined> {
@@ -460,7 +459,7 @@ export class Auth implements AuthService, ConnectionManager {
         const profile = this.store.getProfileOrThrow(id)
 
         if (profile.type === 'sso') {
-            const provider = this.getTokenProvider(id, profile)
+            const provider = this.getSsoTokenProvider(id, profile)
             const client = this.createSsoClient(profile.ssoRegion, provider)
 
             if (opt?.skipGlobalLogout !== true) {
@@ -506,7 +505,7 @@ export class Auth implements AuthService, ConnectionManager {
     private async validateConnection<T extends Profile>(id: Connection['id'], profile: StoredProfile<T>) {
         const runCheck = async () => {
             if (profile.type === 'sso') {
-                const provider = this.getTokenProvider(id, profile)
+                const provider = this.getSsoTokenProvider(id, profile)
                 if ((await provider.getToken()) === undefined) {
                     getLogger().info(`auth: Connection is not valid: ${id} `)
                     return this.updateConnectionState(id, 'invalid')
@@ -540,10 +539,13 @@ export class Auth implements AuthService, ConnectionManager {
             }
         }
 
-        return runCheck().catch(err => this.handleValidationError(id, err))
+        return runCheck().catch(err => this.handleSsoTokenError(id, err))
     }
 
-    private async handleValidationError(id: Connection['id'], err: unknown) {
+    private async handleSsoTokenError(id: Connection['id'], err: unknown) {
+        // Bubble-up networking issues so we don't treat the session as invalid
+        this.throwOnNetworkError(err)
+
         this.#validationErrors.set(id, UnknownError.cast(err))
         getLogger().info(`auth: Handling validation error of connection: ${id}`)
         return this.updateConnectionState(id, 'invalid')
@@ -579,7 +581,7 @@ export class Auth implements AuthService, ConnectionManager {
             throw new Error(`Source profile for "${id}" is not an SSO profile`)
         }
 
-        const tokenProvider = this.getTokenProvider(profile.ssoSession, sourceProfile)
+        const tokenProvider = this.getSsoTokenProvider(profile.ssoSession, sourceProfile)
         const credentialsProvider = new SsoCredentialsProvider(
             fromString(id),
             this.createSsoClient(sourceProfile.ssoRegion, tokenProvider),
@@ -630,7 +632,7 @@ export class Auth implements AuthService, ConnectionManager {
             : [identifier, createSsoProfile(region, startUrl, scopesCodeCatalyst)]
     }
 
-    private getTokenProvider(id: Connection['id'], profile: StoredProfile<SsoProfile>) {
+    private getSsoTokenProvider(id: Connection['id'], profile: StoredProfile<SsoProfile>) {
         // XXX: Use the token created by Dev Environments if and only if the profile is strictly
         // for CodeCatalyst, as indicated by its set of scopes. A consequence of these semantics is
         // that any profile will be coerced to use this token if that profile exclusively contains
@@ -642,7 +644,7 @@ export class Auth implements AuthService, ConnectionManager {
 
         const tokenIdentifier = shouldUseSoftwareStatement ? this.detectSsoSessionNameForCodeCatalyst() : id
 
-        return this.createTokenProvider(
+        return this.createSsoTokenProvider(
             {
                 identifier: tokenIdentifier,
                 startUrl: profile.startUrl,
@@ -671,7 +673,7 @@ export class Auth implements AuthService, ConnectionManager {
         id: Connection['id'],
         profile: StoredProfile<SsoProfile>
     ): SsoConnection & StatefulConnection {
-        const provider = this.getTokenProvider(id, profile)
+        const provider = this.getSsoTokenProvider(id, profile)
 
         return {
             id,
@@ -692,7 +694,7 @@ export class Auth implements AuthService, ConnectionManager {
 
             return result
         } catch (err) {
-            await this.handleValidationError(id, err)
+            await this.handleSsoTokenError(id, err)
             throw err
         }
     }
@@ -717,18 +719,29 @@ export class Auth implements AuthService, ConnectionManager {
     private async _getToken(id: Connection['id'], provider: SsoAccessTokenProvider): Promise<SsoToken> {
         const token = await provider.getToken().catch(err => {
             // Bubble-up networking issues so we don't treat the session as invalid
-            if (isNetworkError(err)) {
-                throw new ToolkitError('Failed to refresh connection due to networking issues', {
-                    cause: err,
-                })
-            }
+            this.throwOnNetworkError(err)
 
             this.#validationErrors.set(id, err)
         })
 
         return token ?? this.handleInvalidCredentials(id, () => provider.createToken())
     }
 
+    /**
+     * Auth processes can fail if there are network issues, and we do not
+     * want to intepret these failures as invalid/expired auth tokens.
+     *
+     * We use this to check if the given error is network related and then
+     * throw, expecting the caller to not change the state of the connection.
+     */
+    private throwOnNetworkError(e: unknown) {
+        if (isNetworkError(e)) {
+            throw new ToolkitError('Failed to update connection due to networking issues', {
+                cause: e,
+            })
+        }
+    }
+
     private readonly getCredentials = keyedDebounce(this._getCredentials.bind(this))
     private async _getCredentials(id: Connection['id'], provider: CredentialsProvider): Promise<Credentials> {
         const credentials = await this.getCachedCredentials(provider)

packages/core/src/auth/sso/ssoAccessTokenProvider.ts

@@ -108,9 +108,9 @@ export class SsoAccessTokenProvider {
         }
     }
 
-    public async createToken(identityProvider?: (token: SsoToken) => Promise<string>): Promise<SsoToken> {
+    public async createToken(): Promise<SsoToken> {
         const access = await this.runFlow()
-        const identity = (await identityProvider?.(access.token)) ?? this.tokenCacheKey
+        const identity = this.tokenCacheKey
         await this.cache.token.save(identity, access)
         await setSessionCreationDate(this.tokenCacheKey, new Date())
 

packages/core/src/shared/errors.ts

@@ -527,7 +527,7 @@ export class PermissionsError extends ToolkitError {
     }
 }
 
-export function isNetworkError(err?: unknown) {
+export function isNetworkError(err?: unknown): err is Error & { code: string } {
     if (!hasCode(err)) {
         return false
     }

packages/core/src/test/credentials/auth.test.ts

@@ -240,6 +240,19 @@ describe('Auth', function () {
             assert.strictEqual(actual.cause, expected)
             assert.strictEqual(auth.getConnectionState(conn), 'valid')
         })
+
+        it('connection is not invalidated when networking issue during connection refresh', async function () {
+            const networkError = new ToolkitError('test', { code: 'ETIMEDOUT' })
+            const expectedError = new ToolkitError('Failed to update connection due to networking issues', {
+                cause: networkError,
+            })
+            const conn = await auth.createConnection(ssoProfile)
+            auth.getTestTokenProvider(conn)?.getToken.rejects(networkError)
+            const actual = await auth.refreshConnectionState(conn).catch(e => e)
+            assert.ok(actual instanceof ToolkitError)
+            assert.deepStrictEqual(actual, expectedError)
+            assert.strictEqual(auth.getConnectionState(conn), 'valid')
+        })
     })
 
     describe('Linked Connections', function () {