Revert "[CSTPER-144] Fixed issue with authorization request encountered while logging-in with external idp"

This reverts commit 73a9fe16
2025-10-07 18:14:17 +00:00 · 2021-07-19 17:54:02 +02:00
parent 98cecd3e84
commit fbde0cbad9
8 changed files with 48 additions and 206 deletions
--- a/src/app/core/auth/auth.actions.ts
+++ b/src/app/core/auth/auth.actions.ts
@@ -294,13 +294,10 @@ export class ResetAuthenticationMessagesAction implements Action {
 export class RetrieveAuthMethodsAction implements Action {
  public type: string = AuthActionTypes.RETRIEVE_AUTH_METHODS;

-  payload: {
-    status: AuthStatus;
-    blocking: boolean;
-  };
+  payload: AuthStatus;

-  constructor(status: AuthStatus, blocking: boolean) {
-    this.payload = { status, blocking };
+  constructor(authStatus: AuthStatus) {
+    this.payload = authStatus;
  }
 }

@@ -311,14 +308,10 @@ export class RetrieveAuthMethodsAction implements Action {
 */
 export class RetrieveAuthMethodsSuccessAction implements Action {
  public type: string = AuthActionTypes.RETRIEVE_AUTH_METHODS_SUCCESS;
+  payload: AuthMethod[];

-  payload: {
-    authMethods: AuthMethod[];
-    blocking: boolean;
-  };
-
-  constructor(authMethods: AuthMethod[], blocking: boolean ) {
-    this.payload = { authMethods, blocking };
+  constructor(authMethods: AuthMethod[] ) {
+    this.payload = authMethods;
  }
 }

@@ -329,12 +322,6 @@ export class RetrieveAuthMethodsSuccessAction implements Action {
 */
 export class RetrieveAuthMethodsErrorAction implements Action {
  public type: string = AuthActionTypes.RETRIEVE_AUTH_METHODS_ERROR;
-
-  payload: boolean;
-
-  constructor(blocking: boolean) {
-    this.payload = blocking;
-  }
 }

 /**
--- a/src/app/core/auth/auth.effects.spec.ts
+++ b/src/app/core/auth/auth.effects.spec.ts
@@ -43,12 +43,10 @@ describe('AuthEffects', () => {
  let initialState;
  let token;
  let store: MockStore<AppState>;
-  let authStatus;

  function init() {
    authServiceStub = new AuthServiceStub();
    token = authServiceStub.getToken();
-    authStatus = Object.assign(new AuthStatus(), {});
    initialState = {
      core: {
        auth: {
@@ -219,38 +217,16 @@ describe('AuthEffects', () => {
        expect(authEffects.checkTokenCookie$).toBeObservable(expected);
      });

-      describe('on CSR', () => {
-        it('should return a RETRIEVE_AUTH_METHODS action in response to a CHECK_AUTHENTICATION_TOKEN_COOKIE action when authenticated is false', () => {
-          spyOn((authEffects as any).authService, 'checkAuthenticationCookie').and.returnValue(
-            observableOf(
-              { authenticated: false })
-          );
-          spyOn((authEffects as any).authService, 'getRetrieveAuthMethodsAction').and.returnValue(
-            new RetrieveAuthMethodsAction({ authenticated: false } as AuthStatus, false)
-          );
-          actions = hot('--a-', { a: { type: AuthActionTypes.CHECK_AUTHENTICATION_TOKEN_COOKIE } });
+      it('should return a RETRIEVE_AUTH_METHODS action in response to a CHECK_AUTHENTICATION_TOKEN_COOKIE action when authenticated is false', () => {
+        spyOn((authEffects as any).authService, 'checkAuthenticationCookie').and.returnValue(
+          observableOf(
+            { authenticated: false })
+        );
+        actions = hot('--a-', { a: { type: AuthActionTypes.CHECK_AUTHENTICATION_TOKEN_COOKIE } });

-          const expected = cold('--b-', { b: new RetrieveAuthMethodsAction({ authenticated: false } as AuthStatus, false) });
+        const expected = cold('--b-', { b: new RetrieveAuthMethodsAction({ authenticated: false } as AuthStatus) });

-          expect(authEffects.checkTokenCookie$).toBeObservable(expected);
-        });
-      });
-
-      describe('on SSR', () => {
-        it('should return a RETRIEVE_AUTH_METHODS action in response to a CHECK_AUTHENTICATION_TOKEN_COOKIE action when authenticated is false', () => {
-          spyOn((authEffects as any).authService, 'checkAuthenticationCookie').and.returnValue(
-            observableOf(
-              { authenticated: false })
-          );
-          spyOn((authEffects as any).authService, 'getRetrieveAuthMethodsAction').and.returnValue(
-            new RetrieveAuthMethodsAction({ authenticated: false } as AuthStatus, true)
-          );
-          actions = hot('--a-', { a: { type: AuthActionTypes.CHECK_AUTHENTICATION_TOKEN_COOKIE } });
-
-          const expected = cold('--b-', { b: new RetrieveAuthMethodsAction({ authenticated: false } as AuthStatus, true) });
-
-          expect(authEffects.checkTokenCookie$).toBeObservable(expected);
-        });
+        expect(authEffects.checkTokenCookie$).toBeObservable(expected);
      });
    });

@@ -383,74 +359,27 @@ describe('AuthEffects', () => {

  describe('retrieveMethods$', () => {

-    describe('on CSR', () => {
-      describe('when retrieve authentication methods succeeded', () => {
-        it('should return a RETRIEVE_AUTH_METHODS_SUCCESS action in response to a RETRIEVE_AUTH_METHODS action', () => {
-          actions = hot('--a-', { a:
-            {
-              type: AuthActionTypes.RETRIEVE_AUTH_METHODS,
-              payload: { status: authStatus, blocking: false}
-            }
-          });
+    describe('when retrieve authentication methods succeeded', () => {
+      it('should return a RETRIEVE_AUTH_METHODS_SUCCESS action in response to a RETRIEVE_AUTH_METHODS action', () => {
+        actions = hot('--a-', { a: { type: AuthActionTypes.RETRIEVE_AUTH_METHODS } });

-          const expected = cold('--b-', { b: new RetrieveAuthMethodsSuccessAction(authMethodsMock, false) });
+        const expected = cold('--b-', { b: new RetrieveAuthMethodsSuccessAction(authMethodsMock) });

-          expect(authEffects.retrieveMethods$).toBeObservable(expected);
-        });
-      });
-
-      describe('when retrieve authentication methods failed', () => {
-        it('should return a RETRIEVE_AUTH_METHODS_ERROR action in response to a RETRIEVE_AUTH_METHODS action', () => {
-          spyOn((authEffects as any).authService, 'retrieveAuthMethodsFromAuthStatus').and.returnValue(observableThrow(''));
-
-          actions = hot('--a-', { a:
-            {
-              type: AuthActionTypes.RETRIEVE_AUTH_METHODS,
-              payload: { status: authStatus, blocking: false}
-            }
-          });
-
-          const expected = cold('--b-', { b: new RetrieveAuthMethodsErrorAction(false) });
-
-          expect(authEffects.retrieveMethods$).toBeObservable(expected);
-        });
+        expect(authEffects.retrieveMethods$).toBeObservable(expected);
      });
    });

-    describe('on SSR', () => {
-      describe('when retrieve authentication methods succeeded', () => {
-        it('should return a RETRIEVE_AUTH_METHODS_SUCCESS action in response to a RETRIEVE_AUTH_METHODS action', () => {
-          actions = hot('--a-', { a:
-            {
-              type: AuthActionTypes.RETRIEVE_AUTH_METHODS,
-              payload: { status: authStatus, blocking: true}
-            }
-          });
+    describe('when retrieve authentication methods failed', () => {
+      it('should return a RETRIEVE_AUTH_METHODS_ERROR action in response to a RETRIEVE_AUTH_METHODS action', () => {
+        spyOn((authEffects as any).authService, 'retrieveAuthMethodsFromAuthStatus').and.returnValue(observableThrow(''));

-          const expected = cold('--b-', { b: new RetrieveAuthMethodsSuccessAction(authMethodsMock, true) });
+        actions = hot('--a-', { a: { type: AuthActionTypes.RETRIEVE_AUTH_METHODS } });

-          expect(authEffects.retrieveMethods$).toBeObservable(expected);
-        });
-      });
+        const expected = cold('--b-', { b: new RetrieveAuthMethodsErrorAction() });

-      describe('when retrieve authentication methods failed', () => {
-        it('should return a RETRIEVE_AUTH_METHODS_ERROR action in response to a RETRIEVE_AUTH_METHODS action', () => {
-          spyOn((authEffects as any).authService, 'retrieveAuthMethodsFromAuthStatus').and.returnValue(observableThrow(''));
-
-          actions = hot('--a-', { a:
-            {
-              type: AuthActionTypes.RETRIEVE_AUTH_METHODS,
-              payload: { status: authStatus, blocking: true}
-            }
-          });
-
-          const expected = cold('--b-', { b: new RetrieveAuthMethodsErrorAction(true) });
-
-          expect(authEffects.retrieveMethods$).toBeObservable(expected);
-        });
+        expect(authEffects.retrieveMethods$).toBeObservable(expected);
      });
    });
-
  });

  describe('clearInvalidTokenOnRehydrate$', () => {
--- a/src/app/core/auth/auth.effects.ts
+++ b/src/app/core/auth/auth.effects.ts
@@ -1,13 +1,14 @@
 import { Injectable, NgZone } from '@angular/core';

 import {
+  asyncScheduler,
  combineLatest as observableCombineLatest,
  Observable,
  of as observableOf,
-  timer,
-  asyncScheduler, queueScheduler
+  queueScheduler,
+  timer
 } from 'rxjs';
-import { catchError, filter, map, switchMap, take, tap, observeOn } from 'rxjs/operators';
+import { catchError, filter, map, observeOn, switchMap, take, tap } from 'rxjs/operators';
 // import @ngrx
 import { Actions, Effect, ofType } from '@ngrx/effects';
 import { Action, select, Store } from '@ngrx/store';
@@ -43,7 +44,8 @@ import {
  RetrieveAuthMethodsAction,
  RetrieveAuthMethodsErrorAction,
  RetrieveAuthMethodsSuccessAction,
-  RetrieveTokenAction, SetUserAsIdleAction
+  RetrieveTokenAction,
+  SetUserAsIdleAction
 } from './auth.actions';
 import { hasValue } from '../../shared/empty.util';
 import { environment } from '../../../environments/environment';
@@ -161,7 +163,7 @@ export class AuthEffects {
          if (response.authenticated) {
            return new RetrieveTokenAction();
          } else {
-            return this.authService.getRetrieveAuthMethodsAction(response);
+            return new RetrieveAuthMethodsAction(response);
          }
        }),
        catchError((error) => observableOf(new AuthenticatedErrorAction(error)))
@@ -250,10 +252,10 @@ export class AuthEffects {
    .pipe(
      ofType(AuthActionTypes.RETRIEVE_AUTH_METHODS),
      switchMap((action: RetrieveAuthMethodsAction) => {
-        return this.authService.retrieveAuthMethodsFromAuthStatus(action.payload.status)
+        return this.authService.retrieveAuthMethodsFromAuthStatus(action.payload)
          .pipe(
-            map((authMethodModels: AuthMethod[]) => new RetrieveAuthMethodsSuccessAction(authMethodModels, action.payload.blocking)),
-            catchError((error) => observableOf(new RetrieveAuthMethodsErrorAction(action.payload.blocking)))
+            map((authMethodModels: AuthMethod[]) => new RetrieveAuthMethodsSuccessAction(authMethodModels)),
+            catchError((error) => observableOf(new RetrieveAuthMethodsErrorAction()))
          );
      })
    );
--- a/src/app/core/auth/auth.reducer.spec.ts
+++ b/src/app/core/auth/auth.reducer.spec.ts
@@ -23,7 +23,9 @@ import {
  RetrieveAuthMethodsAction,
  RetrieveAuthMethodsErrorAction,
  RetrieveAuthMethodsSuccessAction,
-  SetRedirectUrlAction, SetUserAsIdleAction, UnsetUserAsIdleAction
+  SetRedirectUrlAction,
+  SetUserAsIdleAction,
+  UnsetUserAsIdleAction
 } from './auth.actions';
 import { AuthTokenInfo } from './models/auth-token-info.model';
 import { EPersonMock } from '../../shared/testing/eperson.mock';
@@ -551,7 +553,7 @@ describe('authReducer', () => {
      authMethods: [],
      idle: false
    };
-    const action = new RetrieveAuthMethodsAction(new AuthStatus(), true);
+    const action = new RetrieveAuthMethodsAction(new AuthStatus());
    const newState = authReducer(initialState, action);
    state = {
      authenticated: false,
@@ -577,7 +579,7 @@ describe('authReducer', () => {
      new AuthMethod(AuthMethodType.Password),
      new AuthMethod(AuthMethodType.Shibboleth, 'location')
    ];
-    const action = new RetrieveAuthMethodsSuccessAction(authMethods, false);
+    const action = new RetrieveAuthMethodsSuccessAction(authMethods);
    const newState = authReducer(initialState, action);
    state = {
      authenticated: false,
@@ -590,33 +592,7 @@ describe('authReducer', () => {
    expect(newState).toEqual(state);
  });

-  it('should properly set the state, in response to a RETRIEVE_AUTH_METHODS_SUCCESS action with blocking as true', () => {
-    initialState = {
-      authenticated: false,
-      loaded: false,
-      blocking: true,
-      loading: true,
-      authMethods: [],
-      idle: false
-    };
-    const authMethods = [
-      new AuthMethod(AuthMethodType.Password),
-      new AuthMethod(AuthMethodType.Shibboleth, 'location')
-    ];
-    const action = new RetrieveAuthMethodsSuccessAction(authMethods, true);
-    const newState = authReducer(initialState, action);
-    state = {
-      authenticated: false,
-      loaded: false,
-      blocking: true,
-      loading: false,
-      authMethods: authMethods,
-      idle: false
-    };
-    expect(newState).toEqual(state);
-  });
-
-  it('should properly set the state, in response to a RETRIEVE_AUTH_METHODS_ERROR action ', () => {
+  it('should properly set the state, in response to a RETRIEVE_AUTH_METHODS_ERROR action', () => {
    initialState = {
      authenticated: false,
      loaded: false,
@@ -626,7 +602,7 @@ describe('authReducer', () => {
      idle: false
    };

-    const action = new RetrieveAuthMethodsErrorAction(false);
+    const action = new RetrieveAuthMethodsErrorAction();
    const newState = authReducer(initialState, action);
    state = {
      authenticated: false,
@@ -680,27 +656,4 @@ describe('authReducer', () => {
    };
    expect(newState).toEqual(state);
  });
-
-  it('should properly set the state, in response to a RETRIEVE_AUTH_METHODS_ERROR action with blocking as true', () => {
-    initialState = {
-      authenticated: false,
-      loaded: false,
-      blocking: true,
-      loading: true,
-      authMethods: [],
-      idle: false
-    };
-
-    const action = new RetrieveAuthMethodsErrorAction(true);
-    const newState = authReducer(initialState, action);
-    state = {
-      authenticated: false,
-      loaded: false,
-      blocking: true,
-      loading: false,
-      authMethods: [new AuthMethod(AuthMethodType.Password)],
-      idle: false
-    };
-    expect(newState).toEqual(state);
-  });
 });
--- a/src/app/core/auth/auth.reducer.ts
+++ b/src/app/core/auth/auth.reducer.ts
@@ -10,7 +10,6 @@ import {
  RedirectWhenTokenExpiredAction,
  RefreshTokenSuccessAction,
  RetrieveAuthenticatedEpersonSuccessAction,
-  RetrieveAuthMethodsErrorAction,
  RetrieveAuthMethodsSuccessAction,
  SetRedirectUrlAction
 } from './auth.actions';
@@ -217,14 +216,14 @@ export function authReducer(state: any = initialState, action: AuthActions): Aut
    case AuthActionTypes.RETRIEVE_AUTH_METHODS_SUCCESS:
      return Object.assign({}, state, {
        loading: false,
-        blocking: (action as RetrieveAuthMethodsSuccessAction).payload.blocking,
-        authMethods: (action as RetrieveAuthMethodsSuccessAction).payload.authMethods
+        blocking: false,
+        authMethods: (action as RetrieveAuthMethodsSuccessAction).payload
      });

    case AuthActionTypes.RETRIEVE_AUTH_METHODS_ERROR:
      return Object.assign({}, state, {
        loading: false,
-        blocking: (action as RetrieveAuthMethodsErrorAction).payload,
+        blocking: false,
        authMethods: [new AuthMethod(AuthMethodType.Password)]
      });

--- a/src/app/core/auth/auth.service.ts
+++ b/src/app/core/auth/auth.service.ts
@@ -34,9 +34,9 @@ import {
 } from './selectors';
 import { AppState } from '../../app.reducer';
 import {
-  CheckAuthenticationTokenAction, RefreshTokenAction,
+  CheckAuthenticationTokenAction,
+  RefreshTokenAction,
  ResetAuthenticationMessagesAction,
-  RetrieveAuthMethodsAction,
  SetRedirectUrlAction,
  SetUserAsIdleAction,
  UnsetUserAsIdleAction
@@ -573,15 +573,6 @@ export class AuthService {
    );
  }

-  /**
-   * Return a new instance of RetrieveAuthMethodsAction
-   *
-   * @param authStatus The auth status
-   */
-  getRetrieveAuthMethodsAction(authStatus: AuthStatus): RetrieveAuthMethodsAction {
-    return new RetrieveAuthMethodsAction(authStatus, false);
-  }
-
  /**
   * Determines if current user is idle
   * @returns {Observable<boolean>}
--- a/src/app/core/auth/server-auth.service.ts
+++ b/src/app/core/auth/server-auth.service.ts
@@ -4,13 +4,12 @@ import { HttpHeaders } from '@angular/common/http';
 import { Observable } from 'rxjs';
 import { map } from 'rxjs/operators';

-import { isNotEmpty, hasValue } from '../../shared/empty.util';
+import { hasValue, isNotEmpty } from '../../shared/empty.util';
 import { HttpOptions } from '../dspace-rest/dspace-rest.service';
 import { AuthService } from './auth.service';
 import { AuthStatus } from './models/auth-status.model';
 import { AuthTokenInfo } from './models/auth-token-info.model';
 import { RemoteData } from '../data/remote-data';
-import { RetrieveAuthMethodsAction } from './auth.actions';

 /**
 * The auth service.
@@ -61,13 +60,4 @@ export class ServerAuthService extends AuthService {
      map((rd: RemoteData<AuthStatus>) => Object.assign(new AuthStatus(), rd.payload))
    );
  }
-
-  /**
-   * Return a new instance of RetrieveAuthMethodsAction
-   *
-   * @param authStatus The auth status
-   */
-  getRetrieveAuthMethodsAction(authStatus: AuthStatus): RetrieveAuthMethodsAction {
-    return new RetrieveAuthMethodsAction(authStatus, true);
-  }
 }
--- a/src/app/shared/testing/auth-service.stub.ts
+++ b/src/app/shared/testing/auth-service.stub.ts
@@ -6,7 +6,6 @@ import { EPerson } from '../../core/eperson/models/eperson.model';
 import { createSuccessfulRemoteDataObject$ } from '../remote-data.utils';
 import { AuthMethod } from '../../core/auth/models/auth.method';
 import { hasValue } from '../empty.util';
-import { RetrieveAuthMethodsAction } from '../../core/auth/auth.actions';

 export const authMethodsMock = [
  new AuthMethod('password'),
@@ -167,12 +166,4 @@ export class AuthServiceStub {
  clearRedirectUrl() {
    return;
  }
-
-  public replaceToken(token: AuthTokenInfo) {
-    return token;
-  }
-
-  getRetrieveAuthMethodsAction(authStatus: AuthStatus): RetrieveAuthMethodsAction {
-    return;
-  }
 }