fix(auth): client registration expiry not checked when creating token #3877

Problem:

In the scenario a client registration was expired and
SsoAccessTokenProvider.createToken() was called, the
expired client registration would be used to create
the new token.

This scenario was not expected to occurr since it was
assumed that getToken() would be called before createToken()
and getToken() would do the work of invalidating the
expired client registration so that createToken() would
not need to check if the client registration is expired.

Solution:

Since we cannot guarantee createToken() is always run
before getToken(), in createToken() we will verify
that the client registration is not expired, and if it is
we will create a new registration, then continue with the same
process as before.

Signed-off-by: nkomonen <[email protected]>

* add changelog item

Signed-off-by: nkomonen <[email protected]>

---------

Signed-off-by: nkomonen <[email protected]>

Author: nkomonen-amazon

.changes/next-release/Bug Fix-628d74cf-3fbf-4bdd-bf03-93d0992660bf.json

@@ -0,0 +1,4 @@
+{
+	"type": "Bug Fix",
+	"description": "BuilderID/IdentityCenter: Fix 'Invalid Client' error in the browser when re-authenticating"
+}

src/auth/sso/ssoAccessTokenProvider.ts

@@ -108,14 +108,15 @@ export class SsoAccessTokenProvider {
     }
 
     private async runFlow() {
-        const cacheKey = this.registrationCacheKey
-        const registration = await loadOr(this.cache.registration, cacheKey, () => this.registerClient())
-
+        const registration = await this.getValidatedClientRegistration()
         try {
             return await this.authorize(registration)
         } catch (err) {
             if (err instanceof SSOOIDCServiceException && isClientFault(err)) {
-                await this.cache.registration.clear(cacheKey, `client fault: SSOOIDCServiceException: ${err.message}`)
+                await this.cache.registration.clear(
+                    this.registrationCacheKey,
+                    `client fault: SSOOIDCServiceException: ${err.message}`
+                )
             }
 
             throw err
@@ -166,6 +167,7 @@ export class SsoAccessTokenProvider {
     }
 
     private async authorize(registration: ClientRegistration) {
+        // This will NOT throw on expired clientId/Secret, but WILL throw on invalid clientId/Secret
         const authorization = await this.oidc.startDeviceAuthorization({
             startUrl: this.profile.startUrl,
             clientId: registration.clientId,
@@ -187,6 +189,23 @@ export class SsoAccessTokenProvider {
         return this.formatToken(token, registration)
     }
 
+    /**
+     * If the registration already exists locally, it
+     * will be validated before being returned. Otherwise, a client registration is
+     * created and returned.
+     */
+    private async getValidatedClientRegistration(): Promise<ClientRegistration> {
+        const cacheKey = this.registrationCacheKey
+        const cachedRegistration = await this.cache.registration.load(cacheKey)
+
+        // Clear cached if registration is expired
+        if (cachedRegistration && isExpired(cachedRegistration)) {
+            await this.invalidate()
+        }
+
+        return loadOr(this.cache.registration, cacheKey, () => this.registerClient())
+    }
+
     private async registerClient(): Promise<ClientRegistration> {
         const companyName = getIdeProperties().company
         return this.oidc.registerClient({

src/test/credentials/sso/ssoAccessTokenProvider.test.ts

@@ -10,7 +10,7 @@ import { SsoAccessTokenProvider } from '../../../auth/sso/ssoAccessTokenProvider
 import { installFakeClock } from '../../testUtil'
 import { getCache } from '../../../auth/sso/cache'
 
-import { instance, mock, when, anything, reset } from '../../utilities/mockito'
+import { instance, mock, when, anything, reset, deepEqual } from '../../utilities/mockito'
 import { makeTemporaryToolkitFolder, tryRemoveFolder } from '../../../shared/filesystemUtilities'
 import { ClientRegistration, SsoToken, proceedToBrowser } from '../../../auth/sso/model'
 import { OidcClient } from '../../../auth/sso/clients'
@@ -199,13 +199,15 @@ describe('SsoAccessTokenProvider', function () {
             getOpenExternalStub().resolves(userClicked)
         }
 
-        function setupFlow() {
+        function setupFlow(opts?: { skipAuthorization: boolean }) {
             const token = createToken(hourInMs)
             const registration = createRegistration(hourInMs)
             const authorization = createAuthorization(hourInMs)
 
             when(oidcClient.registerClient(anything())).thenResolve(registration)
-            when(oidcClient.startDeviceAuthorization(anything())).thenResolve(authorization)
+            if (!opts?.skipAuthorization) {
+                when(oidcClient.startDeviceAuthorization(anything())).thenResolve(authorization)
+            }
             when(oidcClient.createToken(anything())).thenResolve(token)
 
             return { token, registration, authorization }
@@ -251,6 +253,49 @@ describe('SsoAccessTokenProvider', function () {
             await assert.rejects(resp, ToolkitError)
         })
 
+        /**
+         * Saves an expired client registration to the cache.
+         */
+        async function saveExpiredRegistrationToCache(): Promise<{
+            key: { region: string; scopes: string[] }
+            registration: ClientRegistration
+        }> {
+            const key = { region, scopes: [] }
+            const registration = {
+                clientId: 'myExpiredClientId',
+                clientSecret: 'myExpiredClientSecret',
+                expiresAt: new clock.Date(clock.Date.now() - 1), // expired date
+            }
+            await cache.registration.save(key, registration)
+            return { key, registration }
+        }
+
+        it('registers a new client registration if the existing client registration is expired', async function () {
+            const { token, registration: validRegistration, authorization } = setupFlow({ skipAuthorization: true })
+            stubOpen()
+
+            const { key: registrationKey, registration: expiredRegistration } = await saveExpiredRegistrationToCache()
+            // If we do not invalidate the expired registration, startDeviceAuthorization()
+            // will be given expired registration values. The following ensures we only
+            // return a value when startDeviceAuthorization() is given valid registration values.
+            when(
+                oidcClient.startDeviceAuthorization(
+                    deepEqual({
+                        startUrl: startUrl,
+                        clientId: validRegistration.clientId,
+                        clientSecret: validRegistration.clientSecret,
+                    })
+                )
+            ).thenResolve(authorization)
+
+            // sanity check we have expired registration in cache
+            assert.deepStrictEqual(await cache.registration.load(registrationKey), expiredRegistration)
+            const result = await sut.createToken()
+            // a valid registration should be cached since the previous was expired
+            assert.deepStrictEqual(await cache.registration.load(registrationKey), validRegistration)
+            assert.deepStrictEqual(result, { ...token, identity: startUrl }) // verify final result
+        })
+
         describe('Exceptions', function () {
             it('removes the client registration cache on client faults', async function () {
                 const exception = new UnauthorizedClientException({ message: '', $metadata: {} })