diff --git a/src/app/core/auth/auth.actions.ts b/src/app/core/auth/auth.actions.ts index ad3f9a9711..15e42c8576 100644 --- 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; - } } /** diff --git a/src/app/core/auth/auth.effects.spec.ts b/src/app/core/auth/auth.effects.spec.ts index cd4f456b44..5d530f39a9 100644 --- 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; - 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$', () => { diff --git a/src/app/core/auth/auth.effects.ts b/src/app/core/auth/auth.effects.ts index 8ce10c0c6b..c142e8873f 100644 --- 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())) ); }) ); diff --git a/src/app/core/auth/auth.reducer.spec.ts b/src/app/core/auth/auth.reducer.spec.ts index ba271fafb5..8cd587b61a 100644 --- 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); - }); }); diff --git a/src/app/core/auth/auth.reducer.ts b/src/app/core/auth/auth.reducer.ts index ef803503c8..2fc79a8861 100644 --- 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)] }); diff --git a/src/app/core/auth/auth.service.ts b/src/app/core/auth/auth.service.ts index a5b5d70704..09848d9044 100644 --- 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} diff --git a/src/app/core/auth/server-auth.service.ts b/src/app/core/auth/server-auth.service.ts index cccc1490f8..ea5a3b41f2 100644 --- 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) => 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); - } } diff --git a/src/app/shared/testing/auth-service.stub.ts b/src/app/shared/testing/auth-service.stub.ts index 9a7622b7b8..3fa09b2b7e 100644 --- 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; - } }