fix(authentication-client): Do not cache authentication errors (#2892)

daffl · web-flow · commit cc4e76726fce · 2022-11-24T15:57:03.000-08:00
diff --git a/packages/authentication-client/src/core.ts b/packages/authentication-client/src/core.ts
@@ -134,7 +134,7 @@ export class AuthenticationClient {
       return type === 'logout' ? promise : promise.then(() => Promise.reject(error))
     }
 
-    return Promise.reject(error)
+    return this.reset().then(() => Promise.reject(error))
   }
 
   reAuthenticate(force = false, strategy?: string): Promise<AuthenticationResult> {
diff --git a/packages/authentication-client/test/index.test.ts b/packages/authentication-client/test/index.test.ts
@@ -17,29 +17,29 @@ describe('@feathersjs/authentication-client', () => {
 
     app.configure(client())
     app.use('/authentication', {
-      create(data: any) {
+      async create(data: any) {
         if (data.error) {
-          return Promise.reject(new Error('Did not work'))
+          throw new Error('Did not work')
         }
 
-        return Promise.resolve({
+        return {
           accessToken,
           data,
           user
-        })
+        }
       },
 
-      remove(id) {
+      async remove(id) {
         if (!app.get('authentication')) {
           throw new NotAuthenticated('Not logged in')
         }
 
-        return Promise.resolve({ id })
+        return { id }
       }
     })
     app.use('dummy', {
-      find(params) {
-        return Promise.resolve(params)
+      async find(params) {
+        return params
       }
     })
   })
@@ -147,6 +147,16 @@ describe('@feathersjs/authentication-client', () => {
     assert.strictEqual(at, accessToken)
   })
 
+  it('resets after any error (#1947)', async () => {
+    await assert.rejects(() => app.authenticate({ strategy: 'testing', error: true }), {
+      message: 'Did not work'
+    })
+
+    const found = await app.service('dummy').find()
+
+    assert.deepStrictEqual(found, {})
+  })
+
   it('logout when not logged in without error', async () => {
     const result = await app.logout()
 
diff --git a/packages/authentication-client/test/integration/commons.ts b/packages/authentication-client/test/integration/commons.ts
@@ -11,119 +11,108 @@ export default (
     let client: Application
     let user: any
 
-    before(() =>
-      getApp()
-        .service('users')
-        .create({
+    before(
+      async () =>
+        (user = await getApp().service('users').create({
           email,
           password
-        })
-        .then((result: any) => {
-          user = result
-        })
+        }))
     )
 
     beforeEach(() => {
       client = getClient()
     })
 
-    it('authenticates with local strategy', () => {
-      return client
-        .authenticate({
-          strategy: 'local',
-          email,
-          password
-        })
-        .then((result) => {
-          assert.ok(result.accessToken)
-          assert.strictEqual(result.authentication.strategy, 'local')
-          assert.strictEqual(result.user.email, email)
-        })
+    after(async () => {
+      await getApp().service('users').remove(user.id)
     })
 
-    it('authentication with wrong credentials fails, does not maintain state', () => {
-      return client
-        .authenticate({
-          strategy: 'local',
-          email,
-          password: 'blabla'
-        })
-        .then(() => assert.fail('Should never get here'))
-        .catch((error) => {
-          assert.strictEqual(error.name, 'NotAuthenticated')
-          assert.strictEqual(error.message, 'Invalid login')
-          assert.ok(!client.get('authentication'), 'Reset client state')
-        })
+    it('authenticates with local strategy', async () => {
+      const result = await client.authenticate({
+        strategy: 'local',
+        email,
+        password
+      })
+
+      assert.ok(result.accessToken)
+      assert.strictEqual(result.authentication.strategy, 'local')
+      assert.strictEqual(result.user.email, email)
     })
 
-    it('errors when not authenticated', () => {
-      return client
-        .service('dummy')
-        .find()
-        .then(() => assert.fail('Should never get here'))
-        .catch((error: any) => {
-          assert.strictEqual(error.name, 'NotAuthenticated')
-          assert.strictEqual(error.code, 401)
-          assert.strictEqual(error.message, 'Not authenticated')
-        })
+    it('authentication with wrong credentials fails, does not maintain state', async () => {
+      await assert.rejects(
+        () =>
+          client.authenticate({
+            strategy: 'local',
+            email,
+            password: 'blabla'
+          }),
+        {
+          name: 'NotAuthenticated',
+          message: 'Invalid login'
+        }
+      )
+      assert.ok(!client.get('authentication'), 'Reset client state')
     })
 
-    it('authenticates and allows access', () => {
-      return client
-        .authenticate({
-          strategy: 'local',
-          email,
-          password
-        })
-        .then(() => client.service('dummy').find())
-        .then((result) => {
-          assert.strictEqual(result.provider, provider)
-          assert.ok(result.authentication)
-          assert.ok(result.authentication.payload)
-          assert.strictEqual(result.user.email, user.email)
-          assert.strictEqual(result.user.id, user.id)
-        })
+    it('errors when not authenticated', async () => {
+      await assert.rejects(() => client.service('dummy').find(), {
+        name: 'NotAuthenticated',
+        code: 401,
+        message: 'Not authenticated'
+      })
     })
 
-    it('re-authenticates', () => {
-      return client
-        .authenticate({
-          strategy: 'local',
-          email,
-          password
-        })
-        .then(() => client.authentication.reset())
-        .then(() => client.authenticate())
-        .then(() => client.service('dummy').find())
-        .then((result) => {
-          assert.strictEqual(result.provider, provider)
-          assert.ok(result.authentication)
-          assert.ok(result.authentication.payload)
-          assert.strictEqual(result.user.email, user.email)
-          assert.strictEqual(result.user.id, user.id)
-        })
+    it('authenticates and allows access', async () => {
+      await client.authenticate({
+        strategy: 'local',
+        email,
+        password
+      })
+      const result = await client.service('dummy').find()
+
+      assert.strictEqual(result.provider, provider)
+      assert.ok(result.authentication)
+      assert.ok(result.authentication.payload)
+      assert.strictEqual(result.user.email, user.email)
+      assert.strictEqual(result.user.id, user.id)
     })
 
-    it('after logout does not allow subsequent access', () => {
-      return client
-        .authenticate({
-          strategy: 'local',
-          email,
-          password
-        })
-        .then(() => client.logout())
-        .then((result) => {
-          assert.ok(result.accessToken)
-          assert.ok(result.user)
-
-          return client.service('dummy').find()
-        })
-        .then(() => assert.fail('Should never get here'))
-        .catch((error) => {
-          assert.strictEqual(error.name, 'NotAuthenticated')
-          assert.strictEqual(error.code, 401)
-          assert.strictEqual(error.message, 'Not authenticated')
-        })
+    it('re-authenticates', async () => {
+      await client.authenticate({
+        strategy: 'local',
+        email,
+        password
+      })
+
+      client.authentication.reset()
+      client.authenticate()
+      const result = await client.service('dummy').find()
+
+      assert.strictEqual(result.provider, provider)
+      assert.ok(result.authentication)
+      assert.ok(result.authentication.payload)
+      assert.strictEqual(result.user.email, user.email)
+      assert.strictEqual(result.user.id, user.id)
+    })
+
+    it('after logout does not allow subsequent access', async () => {
+      await client.authenticate({
+        strategy: 'local',
+        email,
+        password
+      })
+
+      const result = await client.logout()
+
+      assert.ok(result!.accessToken)
+      assert.ok(result!.user)
+
+      assert.rejects(() => client.service('dummy').find(), {
+        name: 'NotAuthenticated',
+        code: 401,
+        message: 'Not authenticated'
+      })
     })
   })
 }

Original file line number	Diff line number	Diff line change
`@@ -134,7 +134,7 @@ export class AuthenticationClient {`
`134`	`134`	`return type === 'logout' ? promise : promise.then(() => Promise.reject(error))`
`135`	`135`	`}`
`136`	`136`
`137`		`- return Promise.reject(error)`
	`137`	`+ return this.reset().then(() => Promise.reject(error))`
`138`	`138`	`}`
`139`	`139`
`140`	`140`	`reAuthenticate(force = false, strategy?: string): Promise<AuthenticationResult> {`