fix: Only remove token on NotAuthenticated error in authentication client and handle error better (#1525)

Author: daffl

packages/authentication-client/src/core.ts

@@ -1,4 +1,4 @@
-import { NotAuthenticated } from '@feathersjs/errors';
+import { NotAuthenticated, FeathersError } from '@feathersjs/errors';
 import { Application } from '@feathersjs/feathers';
 import { AuthenticationRequest, AuthenticationResult } from '@feathersjs/authentication';
 import { Storage, StorageWrapper } from './storage';
@@ -118,6 +118,16 @@ export class AuthenticationClient {
     return Promise.resolve(null);
   }
 
+  handleError (error: FeathersError, type: 'authenticate'|'logout') {
+    if (error.code === 401 || error.code === 403) {
+      const promise = this.removeAccessToken().then(() => this.reset());
+
+      return type === 'logout' ? promise : promise.then(() => Promise.reject(error));
+    }
+
+    return Promise.reject(error);
+  }
+
   reAuthenticate (force: boolean = false): Promise<AuthenticationResult> {
     // Either returns the authentication state or
     // tries to re-authenticate with the stored JWT and strategy
@@ -133,9 +143,7 @@ export class AuthenticationClient {
           strategy: this.options.jwtStrategy,
           accessToken
         });
-      }).catch((error: Error) =>
-        this.removeAccessToken().then(() => Promise.reject(error))
-      );
+      });
     }
 
     return authPromise;
@@ -155,8 +163,8 @@ export class AuthenticationClient {
         this.app.emit('authenticated', authResult);
 
         return this.setAccessToken(accessToken).then(() => authResult);
-      }).catch((error: Error) =>
-        this.reset().then(() => Promise.reject(error))
+      }).catch((error: FeathersError) =>
+        this.handleError(error, 'authenticate')
       );
 
     this.app.set('authentication', promise);
@@ -166,20 +174,17 @@ export class AuthenticationClient {
 
   logout () {
     return Promise.resolve(this.app.get('authentication'))
-      .then(auth => {
-        if (!auth) {
-          return null;
-        }
-
-        return this.service.remove(null)
-          .then((authResult: AuthenticationResult) => this.removeAccessToken()
-            .then(() => this.reset())
-            .then(() => {
-              this.app.emit('logout', authResult);
-
-              return authResult;
-            })
-          );
-      });
+      .then(() => this.service.remove(null)
+      .then((authResult: AuthenticationResult) => this.removeAccessToken()
+        .then(() => this.reset())
+        .then(() => {
+          this.app.emit('logout', authResult);
+
+          return authResult;
+        })
+      ))
+      .catch((error: FeathersError) =>
+        this.handleError(error, 'logout')
+      );
   }
 }

packages/authentication-client/test/index.test.ts

@@ -2,6 +2,7 @@ import assert from 'assert';
 import feathers, { Application } from '@feathersjs/feathers';
 
 import client, { AuthenticationClient } from '../src';
+import { NotAuthenticated } from '@feathersjs/errors';
 
 describe('@feathersjs/authentication-client', () => {
   const accessToken = 'testing';
@@ -15,7 +16,11 @@ describe('@feathersjs/authentication-client', () => {
 
     app.configure(client());
     app.use('/authentication', {
-      create (data) {
+      create (data: any) {
+        if (data.error) {
+          return Promise.reject(new Error('Did not work'));
+        }
+
         return Promise.resolve({
           accessToken,
           data,
@@ -25,7 +30,7 @@ describe('@feathersjs/authentication-client', () => {
 
       remove (id) {
         if (!app.get('authentication')) {
-          throw new Error('Not logged in');
+          throw new NotAuthenticated('Not logged in');
         }
 
         return Promise.resolve({ id });
@@ -135,6 +140,15 @@ describe('@feathersjs/authentication-client', () => {
     });
   });
 
+  it('does not remove AccessToken on other errors', () => {
+    return app.authenticate({
+      strategy: 'testing'
+    }).then(() => app.authenticate({
+      strategy: 'testing'
+    })).then(() => app.authentication.getAccessToken())
+    .then(at => assert.strictEqual(at, accessToken));
+  });
+
   it('logout when not logged in without error', async () => {
     const result = await app.logout();