Merge pull request #1084 from ral-facilities/bugfix/no-autologin-erro…

…rs-dls-#1081 #1081 - add autoLogin setting to control whether we attempt autoLogin
ral-facilities · Jun 9, 2022 · 9d5c7a5 · 9d5c7a5
2 parents 6223ec1 + da1024d
commit 9d5c7a5
Show file tree

Hide file tree

Showing 10 changed files with 120 additions and 119 deletions.
diff --git a/cypress/integration/login.spec.ts b/cypress/integration/login.spec.ts
@@ -279,7 +279,7 @@ describe('Login', () => {
     });
   });
 
-  describe('autoLogin', () => {
+  describe('autoLogin on', () => {
     // Define responses for login attempts
     let verifyResponse: { statusCode: Number; body: string };
     let loginResponse: { statusCode: Number; body: string };
@@ -303,6 +303,7 @@ describe('Login', () => {
         'ui-strings': 'res/default.json',
         'auth-provider': 'icat',
         authUrl: 'http://localhost:8000',
+        autoLogin: true,
         'help-tour-steps': [],
       });
       cy.intercept('/authenticators', [
@@ -425,4 +426,31 @@ describe('Login', () => {
       cy.contains('div', 'Demo Plugin').should('be.visible');
     });
   });
+
+  describe('autoLogin off', () => {
+    beforeEach(() => {
+      cy.intercept('/settings.json', {
+        plugins: [
+          {
+            name: 'demo_plugin',
+            src: '/plugins/e2e-plugin/main.js',
+            enable: true,
+            location: 'main',
+          },
+        ],
+        'ui-strings': 'res/default.json',
+        'auth-provider': 'icat',
+        authUrl: 'http://localhost:8000',
+        autoLogin: false,
+        'help-tour-steps': [],
+      });
+    });
+
+    it('should not attempt to auto login in if autoLogin setting is set to false', () => {
+      cy.visit('/plugin1');
+      cy.get('#demo_plugin').should('not.exist');
+      cy.contains('h1', 'Sign in').should('be.visible');
+      cy.contains('Unable to create anonymous session').should('not.exist');
+    });
+  });
 });
diff --git a/public/settings.example.json b/public/settings.example.json
@@ -1,7 +1,11 @@
 {
   "title": "a cool title",
   "logo": "",
-  "navigationDrawerLogo": { "light": "", "dark": "", "altTxt": "" },
+  "navigationDrawerLogo": {
+    "light": "",
+    "dark": "",
+    "altTxt": ""
+  },
   "adminPageDefaultTab": "maintenance",
   "features": {
     "showHelpPageButton": true,
@@ -52,6 +56,7 @@
   "ui-strings": "res/default.json",
   "auth-provider": "jwt",
   "authUrl": "http://localhost:3000",
+  "autoLogin": true,
   "ga-tracking-id": "UA-XXXX-Y",
   "contactUsAccessibilityFormUrl": ""
 }
diff --git a/src/authentication/icatAuthProvider.test.tsx b/src/authentication/icatAuthProvider.test.tsx
@@ -28,7 +28,8 @@ describe('ICAT auth provider', () => {
 
     icatAuthProvider = new ICATAuthProvider(
       'mnemonic',
-      'http://localhost:8000'
+      'http://localhost:8000',
+      true
     );
     ReactGA.initialize('test id', { testMode: true, titleCase: false });
     (parseJwt as jest.Mock).mockImplementation(
@@ -44,7 +45,11 @@ describe('ICAT auth provider', () => {
   });
 
   it('should set the mnemonic to empty string if none is provided (after autologin)', async () => {
-    icatAuthProvider = new ICATAuthProvider(undefined, 'http://localhost:8000');
+    icatAuthProvider = new ICATAuthProvider(
+      undefined,
+      'http://localhost:8000',
+      true
+    );
     await icatAuthProvider.autoLogin();
     expect(icatAuthProvider.mnemonic).toBe('');
   });
@@ -177,7 +182,11 @@ describe('ICAT auth provider', () => {
     // ensure token is null
     window.localStorage.__proto__.getItem = jest.fn().mockReturnValue(null);
 
-    icatAuthProvider = new ICATAuthProvider(undefined, 'http://localhost:8000');
+    icatAuthProvider = new ICATAuthProvider(
+      undefined,
+      'http://localhost:8000',
+      true
+    );
     expect(icatAuthProvider.autoLogin).toBeDefined();
 
     await icatAuthProvider.autoLogin();
@@ -216,7 +225,11 @@ describe('ICAT auth provider', () => {
     // ensure token is null
     window.localStorage.__proto__.getItem = jest.fn().mockReturnValue(null);
 
-    icatAuthProvider = new ICATAuthProvider(undefined, 'http://localhost:8000');
+    icatAuthProvider = new ICATAuthProvider(
+      undefined,
+      'http://localhost:8000',
+      true
+    );
     expect(icatAuthProvider.autoLogin).toBeDefined();
 
     await icatAuthProvider.autoLogin().catch(() => {
@@ -244,12 +257,22 @@ describe('ICAT auth provider', () => {
   it('should set autologin to resolved promise if mnemonic is set', async () => {
     icatAuthProvider = new ICATAuthProvider(
       'mnemonic',
-      'http://localhost:8000'
+      'http://localhost:8000',
+      true
     );
     expect(icatAuthProvider.autoLogin()).toBeDefined();
     return expect(icatAuthProvider.autoLogin()).resolves;
   });
 
+  it('should not define autoLogin function if autoLogin arg is false', async () => {
+    icatAuthProvider = new ICATAuthProvider(
+      undefined,
+      'http://localhost:8000',
+      false
+    );
+    expect(icatAuthProvider.autoLogin).toBeUndefined();
+  });
+
   it('should call api to verify token', async () => {
     (mockAxios.post as jest.Mock).mockImplementation(() => Promise.resolve());
 

diff --git a/src/authentication/icatAuthProvider.tsx b/src/authentication/icatAuthProvider.tsx
@@ -7,16 +7,13 @@ import { MaintenanceState } from '../state/scigateway.types';
 export default class ICATAuthProvider extends BaseAuthProvider {
   public mnemonic: string;
 
-  public constructor(
-    mnemonic: string | undefined,
-    authUrl: string | undefined
-  ) {
+  public constructor(mnemonic?: string, authUrl?: string, autoLogin?: boolean) {
     super(authUrl);
     this.mnemonic = mnemonic || '';
-    this.autoLogin = this.autoLogin.bind(this);
+    this.autoLogin = autoLogin ? this.autoLoginFunc.bind(this) : undefined;
   }
 
-  public autoLogin(): Promise<void> {
+  private autoLoginFunc = (): Promise<void> => {
     const prevMnemonic = this.mnemonic;
     this.mnemonic = 'anon';
     return this.logIn('', '')
@@ -28,7 +25,10 @@ export default class ICATAuthProvider extends BaseAuthProvider {
       .finally(() => {
         this.mnemonic = prevMnemonic;
       });
-  }
+  };
+
+  // this has to be defined in the constructor to know whether it should exist or not
+  public autoLogin;
 
   public logIn(username: string, password: string): Promise<void> {
     if (this.isLoggedIn() && localStorage.getItem('autoLogin') !== 'true') {

diff --git a/src/loginPage/loginPage.component.test.tsx b/src/loginPage/loginPage.component.test.tsx
@@ -450,7 +450,6 @@ describe('Login page component', () => {
       'new username',
       'new password',
       undefined,
-      undefined,
     ]);
 
     simulateUsernameInput.instance().value = 'new username 2';
@@ -471,7 +470,6 @@ describe('Login page component', () => {
       'new username 2',
       'new password 2',
       undefined,
-      undefined,
     ]);
   });
 
@@ -514,15 +512,13 @@ describe('Login page component', () => {
       '',
       '?token=test_token',
       undefined,
-      undefined,
     ]);
   });
 
   it('on submit verification method should be called when logs in via keyless authenticator', async () => {
     const mockLoginfn = jest.fn();
     props.verifyUsernameAndPassword = mockLoginfn;
     props.auth.provider.mnemonic = 'nokeys';
-    props.auth.provider.authUrl = 'http://example.com';
 
     (axios.get as jest.Mock).mockImplementation(() =>
       Promise.resolve({
@@ -550,12 +546,7 @@ describe('Login page component', () => {
 
     expect(mockLoginfn.mock.calls.length).toEqual(1);
 
-    expect(mockLoginfn.mock.calls[0]).toEqual([
-      '',
-      '',
-      'nokeys',
-      'http://example.com',
-    ]);
+    expect(mockLoginfn.mock.calls[0]).toEqual(['', '', 'nokeys']);
 
     wrapper
       .find(AnonLoginScreen)
@@ -565,19 +556,13 @@ describe('Login page component', () => {
 
     expect(mockLoginfn.mock.calls.length).toEqual(2);
 
-    expect(mockLoginfn.mock.calls[1]).toEqual([
-      '',
-      '',
-      'nokeys',
-      'http://example.com',
-    ]);
+    expect(mockLoginfn.mock.calls[1]).toEqual(['', '', 'nokeys']);
   });
 
   it('verifyUsernameAndPassword action should be sent when the verifyUsernameAndPassword function is called', async () => {
     state.scigateway.authorisation.provider.redirectUrl = 'test redirect';
     state.router.location.search = '?token=test_token';
     state.scigateway.authorisation.provider.mnemonic = 'nokeys';
-    state.scigateway.authorisation.provider.authUrl = 'http://example.com';
 
     (axios.get as jest.Mock).mockImplementation(() =>
       Promise.resolve({

diff --git a/src/loginPage/loginPage.component.tsx b/src/loginPage/loginPage.component.tsx
@@ -149,8 +149,7 @@ interface LoginPageDispatchProps {
   verifyUsernameAndPassword: (
     username: string,
     password: string,
-    mnemonic?: string,
-    authUrl?: string
+    mnemonic?: string
   ) => Promise<void>;
   resetAuthState: () => Action;
 }
@@ -192,7 +191,6 @@ export const RedirectLoginScreen = (
 export const CredentialsLoginScreen = (
   props: CombinedLoginProps & {
     mnemonic?: string;
-    authUrl?: string;
   }
 ): React.ReactElement => {
   const [username, setUsername] = useState<string>('');
@@ -211,12 +209,7 @@ export const CredentialsLoginScreen = (
           e.key === 'Enter' &&
           isInputValid()
         ) {
-          props.verifyUsernameAndPassword(
-            username,
-            password,
-            props.mnemonic,
-            props.authUrl
-          );
+          props.verifyUsernameAndPassword(username, password, props.mnemonic);
         }
       }}
     >
@@ -260,12 +253,7 @@ export const CredentialsLoginScreen = (
         className={props.classes.button}
         disabled={!isInputValid() || props.auth.loading}
         onClick={() => {
-          props.verifyUsernameAndPassword(
-            username,
-            password,
-            props.mnemonic,
-            props.authUrl
-          );
+          props.verifyUsernameAndPassword(username, password, props.mnemonic);
         }}
       >
         <Typography
@@ -299,7 +287,6 @@ export const CredentialsLoginScreen = (
 export const AnonLoginScreen = (
   props: CombinedLoginProps & {
     mnemonic?: string;
-    authUrl?: string;
   }
 ): React.ReactElement => {
   const [t] = useTranslation();
@@ -308,12 +295,7 @@ export const AnonLoginScreen = (
       className={props.classes.root}
       onKeyPress={(e) => {
         if (e.key === 'Enter') {
-          props.verifyUsernameAndPassword(
-            '',
-            '',
-            props.mnemonic,
-            props.authUrl
-          );
+          props.verifyUsernameAndPassword('', '', props.mnemonic);
         }
       }}
     >
@@ -332,12 +314,7 @@ export const AnonLoginScreen = (
         color="primary"
         className={props.classes.button}
         onClick={() => {
-          props.verifyUsernameAndPassword(
-            '',
-            '',
-            props.mnemonic,
-            props.authUrl
-          );
+          props.verifyUsernameAndPassword('', '', props.mnemonic);
         }}
       >
         <Typography color="inherit" noWrap style={{ marginTop: 3 }}>
@@ -452,12 +429,7 @@ const LoginPageComponent = (props: CombinedLoginProps): React.ReactElement => {
       !props.auth.failedToLogin
     ) {
       if (props.location.search) {
-        props.verifyUsernameAndPassword(
-          '',
-          props.location.search,
-          mnemonic,
-          authUrl
-        );
+        props.verifyUsernameAndPassword('', props.location.search, mnemonic);
       }
     }
   });
@@ -472,13 +444,7 @@ const LoginPageComponent = (props: CombinedLoginProps): React.ReactElement => {
   let LoginScreen: React.ReactElement | null = null;
 
   if (typeof mnemonic === 'undefined') {
-    LoginScreen = (
-      <CredentialsLoginScreen
-        {...props}
-        mnemonic={mnemonic}
-        authUrl={authUrl}
-      />
-    );
+    LoginScreen = <CredentialsLoginScreen {...props} mnemonic={mnemonic} />;
 
     if (props.auth.provider.redirectUrl) {
       LoginScreen = <RedirectLoginScreen {...props} />;
@@ -491,9 +457,7 @@ const LoginPageComponent = (props: CombinedLoginProps): React.ReactElement => {
       )
     ) {
       // anon
-      LoginScreen = (
-        <AnonLoginScreen {...props} mnemonic={mnemonic} authUrl={authUrl} />
-      );
+      LoginScreen = <AnonLoginScreen {...props} mnemonic={mnemonic} />;
     } else if (
       mnemonics.find(
         (authenticator) =>
@@ -503,13 +467,7 @@ const LoginPageComponent = (props: CombinedLoginProps): React.ReactElement => {
       )
     ) {
       // user/pass
-      LoginScreen = (
-        <CredentialsLoginScreen
-          {...props}
-          mnemonic={mnemonic}
-          authUrl={authUrl}
-        />
-      );
+      LoginScreen = <CredentialsLoginScreen {...props} mnemonic={mnemonic} />;
     } else if (
       mnemonics.find(
         (authenticator) =>
@@ -570,17 +528,8 @@ const mapDispatchToProps = (
   verifyUsernameAndPassword: (
     username: string,
     password: string,
-    mnemonic: string | undefined,
-    authUrl?: string
-  ) =>
-    dispatch(
-      verifyUsernameAndPassword(
-        username.trim(),
-        password,
-        mnemonic,
-        authUrl ?? ''
-      )
-    ),
+    mnemonic: string | undefined
+  ) => dispatch(verifyUsernameAndPassword(username.trim(), password, mnemonic)),
   resetAuthState: () => dispatch(resetAuthState()),
 });