From b23522d39fd80666976a71462c5ba751b2c4a253 Mon Sep 17 00:00:00 2001 From: Marie Verdonck Date: Fri, 28 May 2021 10:33:51 +0200 Subject: [PATCH 01/16] 79700: Auto-refreshing the token & Needed config --- src/app/app.component.ts | 2 +- src/app/core/auth/auth.interceptor.ts | 17 +------- src/app/core/auth/auth.service.ts | 54 ++++++++++++++++++++++++-- src/assets/i18n/en.json5 | 2 + src/config/auth-config.interfaces.ts | 15 ++++++- src/environments/environment.common.ts | 24 ++++++++++-- 6 files changed, 90 insertions(+), 24 deletions(-) diff --git a/src/app/app.component.ts b/src/app/app.component.ts index c9996f275a..2c01bf637b 100644 --- a/src/app/app.component.ts +++ b/src/app/app.component.ts @@ -130,7 +130,7 @@ export class AppComponent implements OnInit, AfterViewInit { console.info(environment); } this.storeCSSVariables(); - + this.authService.trackTokenExpiration(); } ngOnInit() { diff --git a/src/app/core/auth/auth.interceptor.ts b/src/app/core/auth/auth.interceptor.ts index 7b9a08de92..d16f46a849 100644 --- a/src/app/core/auth/auth.interceptor.ts +++ b/src/app/core/auth/auth.interceptor.ts @@ -28,7 +28,7 @@ import { AuthMethodType } from './models/auth.method-type'; @Injectable() export class AuthInterceptor implements HttpInterceptor { - // Intercetor is called twice per request, + // Interceptor is called twice per request, // so to prevent RefreshTokenAction is dispatched twice // we're creating a refresh token request list protected refreshTokenRequestUrls = []; @@ -216,23 +216,8 @@ export class AuthInterceptor implements HttpInterceptor { let authorization: string; if (authService.isTokenExpired()) { - authService.setRedirectUrl(this.router.url); - // The access token is expired - // Redirect to the login route - this.store.dispatch(new RedirectWhenTokenExpiredAction('auth.messages.expired')); return observableOf(null); } else if ((!this.isAuthRequest(req) || this.isLogoutResponse(req)) && isNotEmpty(token)) { - // Intercept a request that is not to the authentication endpoint - authService.isTokenExpiring().pipe( - filter((isExpiring) => isExpiring)) - .subscribe(() => { - // If the current request url is already in the refresh token request list, skip it - if (isUndefined(find(this.refreshTokenRequestUrls, req.url))) { - // When a token is about to expire, refresh it - this.store.dispatch(new RefreshTokenAction(token)); - this.refreshTokenRequestUrls.push(req.url); - } - }); // Get the auth header from the service. authorization = authService.buildAuthHeader(token); let newHeaders = req.headers.set('authorization', authorization); diff --git a/src/app/core/auth/auth.service.ts b/src/app/core/auth/auth.service.ts index ed4fca615c..4903c30f15 100644 --- a/src/app/core/auth/auth.service.ts +++ b/src/app/core/auth/auth.service.ts @@ -33,7 +33,7 @@ import { } from './selectors'; import { AppState } from '../../app.reducer'; import { - CheckAuthenticationTokenAction, + CheckAuthenticationTokenAction, RefreshTokenAction, ResetAuthenticationMessagesAction, RetrieveAuthMethodsAction, SetRedirectUrlAction @@ -46,6 +46,9 @@ import { getAllSucceededRemoteDataPayload } from '../shared/operators'; import { AuthMethod } from './models/auth.method'; import { HardRedirectService } from '../services/hard-redirect.service'; import { RemoteData } from '../data/remote-data'; +import { environment } from '../../../environments/environment'; +import { NotificationsService } from '../../shared/notifications/notifications.service'; +import { TranslateService } from '@ngx-translate/core'; export const LOGIN_ROUTE = '/login'; export const LOGOUT_ROUTE = '/logout'; @@ -64,6 +67,11 @@ export class AuthService { */ protected _authenticated: boolean; + /** + * Timer to track time until token refresh + */ + private tokenRefreshTimer; + constructor(@Inject(REQUEST) protected req: any, @Inject(NativeWindowService) protected _window: NativeWindowRef, @Optional() @Inject(RESPONSE) private response: any, @@ -73,7 +81,9 @@ export class AuthService { protected routeService: RouteService, protected storage: CookieService, protected store: Store, - protected hardRedirectService: HardRedirectService + protected hardRedirectService: HardRedirectService, + private notificationService: NotificationsService, + private translateService: TranslateService ) { this.store.pipe( select(isAuthenticated), @@ -298,7 +308,7 @@ export class AuthService { */ public getToken(): AuthTokenInfo { let token: AuthTokenInfo; - this.store.pipe(select(getAuthenticationToken)) + this.store.pipe(take(1), select(getAuthenticationToken)) .subscribe((authTokenInfo: AuthTokenInfo) => { // Retrieve authentication token info and check if is valid token = authTokenInfo || null; @@ -306,6 +316,44 @@ export class AuthService { return token; } + /** + * Method that checks when the session token from store expires and refreshes it when needed + */ + public trackTokenExpiration(): void { + let token: AuthTokenInfo; + let currentlyRefreshingToken = false; + this.store.pipe(select(getAuthenticationToken)).subscribe((authTokenInfo: AuthTokenInfo) => { + // If new token is undefined an it wasn't previously => Refresh failed + if (currentlyRefreshingToken && token != undefined && authTokenInfo == undefined) { + // Token refresh failed => Error notification => 10 second wait => Page reloads & user logged out + this.notificationService.error(this.translateService.get('auth.messages.token-refresh-failed')); + setTimeout(() => this.navigateToRedirectUrl(this.hardRedirectService.getCurrentRoute()), 10000); + currentlyRefreshingToken = false; + } + // If new token.expires is different => Refresh succeeded + if (currentlyRefreshingToken && authTokenInfo != undefined && token.expires != authTokenInfo.expires) { + currentlyRefreshingToken = false; + } + // Check if/when token needs to be refreshed + if (!currentlyRefreshingToken) { + token = authTokenInfo || null; + if (token != undefined && token != null) { + let timeLeftBeforeRefresh = token.expires - new Date().getTime() - environment.auth.rest.timeLeftBeforeTokenRefresh; + if (timeLeftBeforeRefresh < 0) { + timeLeftBeforeRefresh = 0; + } + if (hasValue(this.tokenRefreshTimer)) { + clearTimeout(this.tokenRefreshTimer); + } + this.tokenRefreshTimer = setTimeout(() => { + this.store.dispatch(new RefreshTokenAction(token)); + currentlyRefreshingToken = true; + }, timeLeftBeforeRefresh); + } + } + }); + } + /** * Check if a token is next to be expired * @returns {boolean} diff --git a/src/assets/i18n/en.json5 b/src/assets/i18n/en.json5 index 3e2cc67ecb..cf19be3730 100644 --- a/src/assets/i18n/en.json5 +++ b/src/assets/i18n/en.json5 @@ -526,6 +526,8 @@ "auth.messages.expired": "Your session has expired. Please log in again.", + "auth.messages.token-refresh-failed": "Refreshing your session token failed. Please log in again.", + "bitstream.download.page": "Now downloading {{bitstream}}..." , diff --git a/src/config/auth-config.interfaces.ts b/src/config/auth-config.interfaces.ts index cc3d97c6b8..59ebda12da 100644 --- a/src/config/auth-config.interfaces.ts +++ b/src/config/auth-config.interfaces.ts @@ -6,5 +6,18 @@ export interface AuthTarget { } export interface AuthConfig extends Config { - target: AuthTarget; + target?: AuthTarget; + + ui: { + // The amount of time before the idle warning is shown + timeUntilIdle: number; + // The amount of time the user has to react after the idle warning is shown before they are logged out. + idleGracePeriod: number; + }; + + rest: { + // If the rest token expires in less than this amount of time, it will be refreshed automatically. + // This is independent from the idle warning. + timeLeftBeforeTokenRefresh: number; + }; } diff --git a/src/environments/environment.common.ts b/src/environments/environment.common.ts index 4e246f7243..a7d4ec8a00 100644 --- a/src/environments/environment.common.ts +++ b/src/environments/environment.common.ts @@ -2,7 +2,6 @@ import { GlobalConfig } from '../config/global-config.interface'; import { NotificationAnimationsType } from '../app/shared/notifications/models/notification-animations-type'; import { BrowseByType } from '../app/+browse-by/+browse-by-switcher/browse-by-decorator'; import { RestRequestMethod } from '../app/core/data/rest-request-method'; -import { BASE_THEME_NAME } from '../app/shared/theme-support/theme.constants'; export const environment: GlobalConfig = { production: true, @@ -43,6 +42,25 @@ export const environment: GlobalConfig = { timePerMethod: {[RestRequestMethod.PATCH]: 3} as any // time in seconds } }, + // Authority settings + auth: { + // Authority UI settings + ui: { + // the amount of time before the idle warning is shown + // timeUntilIdle: 15 * 60 * 1000, // 15 minutes + timeUntilIdle: 1 * 60 * 1000, // 1 minutes + // the amount of time the user has to react after the idle warning is shown before they are logged out. + // idleGracePeriod: 5 * 60 * 1000, // 5 minutes + idleGracePeriod: 1 * 60 * 1000, // 1 minutes + }, + // Authority REST settings + rest: { + // If the rest token expires in less than this amount of time, it will be refreshed automatically. + // This is independent from the idle warning. + // timeLeftBeforeTokenRefresh: 2 * 60 * 1000, // 2 minutes + timeLeftBeforeTokenRefresh: 0.25 * 60 * 1000, // 25 seconds + }, + }, // Form settings form: { // NOTE: Map server-side validators to comparative Angular form validators @@ -267,8 +285,8 @@ export const environment: GlobalConfig = { ], // Whether the UI should rewrite file download URLs to match its domain. Only necessary to enable when running UI and REST API on separate domains rewriteDownloadUrls: false, - // Whether to enable media viewer for image and/or video Bitstreams (i.e. Bitstreams whose MIME type starts with "image" or "video"). - // For images, this enables a gallery viewer where you can zoom or page through images. + // Whether to enable media viewer for image and/or video Bitstreams (i.e. Bitstreams whose MIME type starts with "image" or "video"). + // For images, this enables a gallery viewer where you can zoom or page through images. // For videos, this enables embedded video streaming mediaViewer: { image: false, From 38387d1a0f07ef1d1fea0b0dbdb104a91d0ba368 Mon Sep 17 00:00:00 2001 From: Marie Verdonck Date: Fri, 28 May 2021 17:22:26 +0200 Subject: [PATCH 02/16] 79700: Tracking idleness & idle modal --- .../browse-by-metadata-page.component.ts | 1 - src/app/app.module.ts | 2 + src/app/core/auth/auth.actions.ts | 27 ++- src/app/core/auth/auth.effects.ts | 33 +++- src/app/core/auth/auth.reducer.spec.ts | 164 +++++++++++++----- src/app/core/auth/auth.reducer.ts | 22 ++- src/app/core/auth/auth.service.spec.ts | 18 +- src/app/core/auth/auth.service.ts | 39 ++++- src/app/core/auth/selectors.ts | 17 ++ src/app/root/root.component.ts | 26 ++- .../auth-nav-menu.component.spec.ts | 6 +- .../user-menu/user-menu.component.spec.ts | 6 +- .../idle-modal/idle-modal.component.html | 18 ++ .../shared/idle-modal/idle-modal.component.ts | 85 +++++++++ src/app/shared/mocks/auth.service.mock.ts | 7 + src/assets/i18n/en.json5 | 12 +- src/environments/mock-environment.ts | 16 ++ 17 files changed, 433 insertions(+), 66 deletions(-) create mode 100644 src/app/shared/idle-modal/idle-modal.component.html create mode 100644 src/app/shared/idle-modal/idle-modal.component.ts diff --git a/src/app/+browse-by/+browse-by-metadata-page/browse-by-metadata-page.component.ts b/src/app/+browse-by/+browse-by-metadata-page/browse-by-metadata-page.component.ts index f5adefc779..5b84daab6e 100644 --- a/src/app/+browse-by/+browse-by-metadata-page/browse-by-metadata-page.component.ts +++ b/src/app/+browse-by/+browse-by-metadata-page/browse-by-metadata-page.component.ts @@ -167,7 +167,6 @@ export class BrowseByMetadataPageComponent implements OnInit { * @param value The value of the browse-entry to display items for */ updatePageWithItems(searchOptions: BrowseEntrySearchOptions, value: string) { - console.log('updatePAge', searchOptions); this.items$ = this.browseService.getBrowseItemsFor(value, searchOptions); } diff --git a/src/app/app.module.ts b/src/app/app.module.ts index 3d45ffbfc2..18b97c8e9e 100755 --- a/src/app/app.module.ts +++ b/src/app/app.module.ts @@ -47,6 +47,7 @@ import { ThemedHeaderComponent } from './header/themed-header.component'; import { ThemedFooterComponent } from './footer/themed-footer.component'; import { ThemedBreadcrumbsComponent } from './breadcrumbs/themed-breadcrumbs.component'; import { ThemedHeaderNavbarWrapperComponent } from './header-nav-wrapper/themed-header-navbar-wrapper.component'; +import { IdleModalComponent } from './shared/idle-modal/idle-modal.component'; export function getBase() { return environment.ui.nameSpace; @@ -144,6 +145,7 @@ const DECLARATIONS = [ ThemedBreadcrumbsComponent, ForbiddenComponent, ThemedForbiddenComponent, + IdleModalComponent ]; const EXPORTS = [ diff --git a/src/app/core/auth/auth.actions.ts b/src/app/core/auth/auth.actions.ts index e2cef3562f..ad3f9a9711 100644 --- a/src/app/core/auth/auth.actions.ts +++ b/src/app/core/auth/auth.actions.ts @@ -34,7 +34,9 @@ export const AuthActionTypes = { RETRIEVE_AUTHENTICATED_EPERSON: type('dspace/auth/RETRIEVE_AUTHENTICATED_EPERSON'), RETRIEVE_AUTHENTICATED_EPERSON_SUCCESS: type('dspace/auth/RETRIEVE_AUTHENTICATED_EPERSON_SUCCESS'), RETRIEVE_AUTHENTICATED_EPERSON_ERROR: type('dspace/auth/RETRIEVE_AUTHENTICATED_EPERSON_ERROR'), - REDIRECT_AFTER_LOGIN_SUCCESS: type('dspace/auth/REDIRECT_AFTER_LOGIN_SUCCESS') + REDIRECT_AFTER_LOGIN_SUCCESS: type('dspace/auth/REDIRECT_AFTER_LOGIN_SUCCESS'), + SET_USER_AS_IDLE: type('dspace/auth/SET_USER_AS_IDLE'), + UNSET_USER_AS_IDLE: type('dspace/auth/UNSET_USER_AS_IDLE') }; /* tslint:disable:max-classes-per-file */ @@ -404,6 +406,24 @@ export class RetrieveAuthenticatedEpersonErrorAction implements Action { this.payload = payload ; } } + +/** + * Set the current user as being idle. + * @class SetUserAsIdleAction + * @implements {Action} + */ +export class SetUserAsIdleAction implements Action { + public type: string = AuthActionTypes.SET_USER_AS_IDLE; +} + +/** + * Unset the current user as being idle. + * @class UnsetUserAsIdleAction + * @implements {Action} + */ +export class UnsetUserAsIdleAction implements Action { + public type: string = AuthActionTypes.UNSET_USER_AS_IDLE; +} /* tslint:enable:max-classes-per-file */ /** @@ -434,4 +454,7 @@ export type AuthActions | RetrieveAuthenticatedEpersonErrorAction | RetrieveAuthenticatedEpersonSuccessAction | SetRedirectUrlAction - | RedirectAfterLoginSuccessAction; + | RedirectAfterLoginSuccessAction + | SetUserAsIdleAction + | UnsetUserAsIdleAction; + diff --git a/src/app/core/auth/auth.effects.ts b/src/app/core/auth/auth.effects.ts index 2ef90dd76c..c133310471 100644 --- a/src/app/core/auth/auth.effects.ts +++ b/src/app/core/auth/auth.effects.ts @@ -1,6 +1,6 @@ import { Injectable } from '@angular/core'; -import { combineLatest as observableCombineLatest, Observable, of as observableOf } from 'rxjs'; +import { combineLatest as observableCombineLatest, Observable, of as observableOf, timer } from 'rxjs'; import { catchError, filter, map, switchMap, take, tap } from 'rxjs/operators'; // import @ngrx import { Actions, Effect, ofType } from '@ngrx/effects'; @@ -37,9 +37,19 @@ import { RetrieveAuthMethodsAction, RetrieveAuthMethodsErrorAction, RetrieveAuthMethodsSuccessAction, - RetrieveTokenAction + RetrieveTokenAction, SetUserAsIdleAction } from './auth.actions'; import { hasValue } from '../../shared/empty.util'; +import { environment } from '../../../environments/environment'; +import { RequestActionTypes } from '../data/request.actions'; +import { NotificationsActionTypes } from '../../shared/notifications/notifications.actions'; +import { ObjectCacheActionTypes } from '../cache/object-cache.actions'; +import { NO_OP_ACTION_TYPE } from '../../shared/ngrx/no-op.action'; + +// Action Types that do not break/prevent the user from an idle state +const IDLE_TIMER_IGNORE_TYPES: string[] + = [...Object.values(AuthActionTypes).filter((t: string) => t !== AuthActionTypes.UNSET_USER_AS_IDLE), + ...Object.values(RequestActionTypes), ...Object.values(NotificationsActionTypes)]; @Injectable() export class AuthEffects { @@ -242,6 +252,25 @@ export class AuthEffects { }) ); + /** + * For any action that is not in {@link IDLE_TIMER_IGNORE_TYPES} that comes in => Start the idleness timer + * If the idleness timer runs out (so no un-ignored action come through for that amount of time) + * => Return the action to set the user as idle ({@link SetUserAsIdleAction}) + * @method trackIdleness + */ + @Effect() + public trackIdleness$: Observable = this.actions$.pipe( + filter((action: Action) => !IDLE_TIMER_IGNORE_TYPES.includes(action.type)), + // Using switchMap the timer will be interrupted and restarted if a new action comes in, so idleness timer restarts + switchMap(() => { + this.authService.isAuthenticated(); + return timer(environment.auth.ui.timeUntilIdle); + }), + map(() => { + return new SetUserAsIdleAction(); + }) + ); + /** * @constructor * @param {Actions} actions$ diff --git a/src/app/core/auth/auth.reducer.spec.ts b/src/app/core/auth/auth.reducer.spec.ts index 914a1a152d..f721c8c208 100644 --- a/src/app/core/auth/auth.reducer.spec.ts +++ b/src/app/core/auth/auth.reducer.spec.ts @@ -23,7 +23,7 @@ import { RetrieveAuthMethodsAction, RetrieveAuthMethodsErrorAction, RetrieveAuthMethodsSuccessAction, - SetRedirectUrlAction + SetRedirectUrlAction, SetUserAsIdleAction, UnsetUserAsIdleAction } from './auth.actions'; import { AuthTokenInfo } from './models/auth-token-info.model'; import { EPersonMock } from '../../shared/testing/eperson.mock'; @@ -44,6 +44,7 @@ describe('authReducer', () => { loaded: false, blocking: true, loading: false, + idle: false }; const action = new AuthenticateAction('user', 'password'); const newState = authReducer(initialState, action); @@ -53,7 +54,8 @@ describe('authReducer', () => { blocking: true, error: undefined, loading: true, - info: undefined + info: undefined, + idle: false }; expect(newState).toEqual(state); @@ -66,7 +68,8 @@ describe('authReducer', () => { error: undefined, blocking: true, loading: true, - info: undefined + info: undefined, + idle: false }; const action = new AuthenticationSuccessAction(mockTokenInfo); const newState = authReducer(initialState, action); @@ -81,7 +84,8 @@ describe('authReducer', () => { error: undefined, blocking: true, loading: true, - info: undefined + info: undefined, + idle: false }; const action = new AuthenticationErrorAction(mockError); const newState = authReducer(initialState, action); @@ -92,7 +96,8 @@ describe('authReducer', () => { loading: false, info: undefined, authToken: undefined, - error: 'Test error message' + error: 'Test error message', + idle: false }; expect(newState).toEqual(state); @@ -105,7 +110,8 @@ describe('authReducer', () => { loaded: false, error: undefined, loading: true, - info: undefined + info: undefined, + idle: false }; const action = new AuthenticatedAction(mockTokenInfo); const newState = authReducer(initialState, action); @@ -115,7 +121,8 @@ describe('authReducer', () => { loaded: false, error: undefined, loading: true, - info: undefined + info: undefined, + idle: false }; expect(newState).toEqual(state); }); @@ -127,7 +134,8 @@ describe('authReducer', () => { error: undefined, blocking: true, loading: true, - info: undefined + info: undefined, + idle: false }; const action = new AuthenticatedSuccessAction(true, mockTokenInfo, EPersonMock._links.self.href); const newState = authReducer(initialState, action); @@ -138,7 +146,8 @@ describe('authReducer', () => { error: undefined, blocking: true, loading: true, - info: undefined + info: undefined, + idle: false }; expect(newState).toEqual(state); }); @@ -150,7 +159,8 @@ describe('authReducer', () => { error: undefined, blocking: true, loading: true, - info: undefined + info: undefined, + idle: false }; const action = new AuthenticatedErrorAction(mockError); const newState = authReducer(initialState, action); @@ -161,7 +171,8 @@ describe('authReducer', () => { loaded: true, blocking: false, loading: false, - info: undefined + info: undefined, + idle: false }; expect(newState).toEqual(state); }); @@ -172,6 +183,7 @@ describe('authReducer', () => { loaded: false, blocking: false, loading: false, + idle: false }; const action = new CheckAuthenticationTokenAction(); const newState = authReducer(initialState, action); @@ -180,6 +192,7 @@ describe('authReducer', () => { loaded: false, blocking: true, loading: true, + idle: false }; expect(newState).toEqual(state); }); @@ -190,6 +203,7 @@ describe('authReducer', () => { loaded: false, blocking: false, loading: true, + idle: false }; const action = new CheckAuthenticationTokenCookieAction(); const newState = authReducer(initialState, action); @@ -198,6 +212,7 @@ describe('authReducer', () => { loaded: false, blocking: true, loading: true, + idle: false }; expect(newState).toEqual(state); }); @@ -211,7 +226,8 @@ describe('authReducer', () => { blocking: false, loading: false, info: undefined, - userId: EPersonMock.id + userId: EPersonMock.id, + idle: false }; const action = new LogOutAction(); @@ -229,7 +245,8 @@ describe('authReducer', () => { blocking: false, loading: false, info: undefined, - userId: EPersonMock.id + userId: EPersonMock.id, + idle: false }; const action = new LogOutSuccessAction(); @@ -243,7 +260,8 @@ describe('authReducer', () => { loading: true, info: undefined, refreshing: false, - userId: undefined + userId: undefined, + idle: false }; expect(newState).toEqual(state); }); @@ -257,7 +275,8 @@ describe('authReducer', () => { blocking: false, loading: false, info: undefined, - userId: EPersonMock.id + userId: EPersonMock.id, + idle: false }; const action = new LogOutErrorAction(mockError); @@ -270,7 +289,8 @@ describe('authReducer', () => { blocking: false, loading: false, info: undefined, - userId: EPersonMock.id + userId: EPersonMock.id, + idle: false }; expect(newState).toEqual(state); }); @@ -283,7 +303,8 @@ describe('authReducer', () => { error: undefined, blocking: true, loading: true, - info: undefined + info: undefined, + idle: false }; const action = new RetrieveAuthenticatedEpersonSuccessAction(EPersonMock.id); const newState = authReducer(initialState, action); @@ -295,7 +316,8 @@ describe('authReducer', () => { blocking: false, loading: false, info: undefined, - userId: EPersonMock.id + userId: EPersonMock.id, + idle: false }; expect(newState).toEqual(state); }); @@ -307,7 +329,8 @@ describe('authReducer', () => { error: undefined, blocking: true, loading: true, - info: undefined + info: undefined, + idle: false }; const action = new RetrieveAuthenticatedEpersonErrorAction(mockError); const newState = authReducer(initialState, action); @@ -318,7 +341,8 @@ describe('authReducer', () => { loaded: true, blocking: false, loading: false, - info: undefined + info: undefined, + idle: false }; expect(newState).toEqual(state); }); @@ -332,7 +356,8 @@ describe('authReducer', () => { blocking: false, loading: false, info: undefined, - userId: EPersonMock.id + userId: EPersonMock.id, + idle: false }; const newTokenInfo = new AuthTokenInfo('Refreshed token'); const action = new RefreshTokenAction(newTokenInfo); @@ -346,7 +371,8 @@ describe('authReducer', () => { loading: false, info: undefined, userId: EPersonMock.id, - refreshing: true + refreshing: true, + idle: false }; expect(newState).toEqual(state); }); @@ -361,7 +387,8 @@ describe('authReducer', () => { loading: false, info: undefined, userId: EPersonMock.id, - refreshing: true + refreshing: true, + idle: false }; const newTokenInfo = new AuthTokenInfo('Refreshed token'); const action = new RefreshTokenSuccessAction(newTokenInfo); @@ -375,7 +402,8 @@ describe('authReducer', () => { loading: false, info: undefined, userId: EPersonMock.id, - refreshing: false + refreshing: false, + idle: false }; expect(newState).toEqual(state); }); @@ -390,7 +418,8 @@ describe('authReducer', () => { loading: false, info: undefined, userId: EPersonMock.id, - refreshing: true + refreshing: true, + idle: false }; const action = new RefreshTokenErrorAction(); const newState = authReducer(initialState, action); @@ -403,7 +432,8 @@ describe('authReducer', () => { loading: false, info: undefined, refreshing: false, - userId: undefined + userId: undefined, + idle: false }; expect(newState).toEqual(state); }); @@ -417,7 +447,8 @@ describe('authReducer', () => { blocking: false, loading: false, info: undefined, - userId: EPersonMock.id + userId: EPersonMock.id, + idle: false }; state = { @@ -428,7 +459,8 @@ describe('authReducer', () => { loading: false, error: undefined, info: 'Message', - userId: undefined + userId: undefined, + idle: false }; }); @@ -450,6 +482,7 @@ describe('authReducer', () => { loaded: false, blocking: false, loading: false, + idle: false }; const action = new AddAuthenticationMessageAction('Message'); const newState = authReducer(initialState, action); @@ -458,7 +491,8 @@ describe('authReducer', () => { loaded: false, blocking: false, loading: false, - info: 'Message' + info: 'Message', + idle: false }; expect(newState).toEqual(state); }); @@ -470,7 +504,8 @@ describe('authReducer', () => { blocking: false, loading: false, error: 'Error', - info: 'Message' + info: 'Message', + idle: false }; const action = new ResetAuthenticationMessagesAction(); const newState = authReducer(initialState, action); @@ -480,7 +515,8 @@ describe('authReducer', () => { blocking: false, loading: false, error: undefined, - info: undefined + info: undefined, + idle: false }; expect(newState).toEqual(state); }); @@ -490,7 +526,8 @@ describe('authReducer', () => { authenticated: false, loaded: false, blocking: false, - loading: false + loading: false, + idle: false }; const action = new SetRedirectUrlAction('redirect.url'); const newState = authReducer(initialState, action); @@ -499,7 +536,8 @@ describe('authReducer', () => { loaded: false, blocking: false, loading: false, - redirectUrl: 'redirect.url' + redirectUrl: 'redirect.url', + idle: false }; expect(newState).toEqual(state); }); @@ -510,7 +548,8 @@ describe('authReducer', () => { loaded: false, blocking: false, loading: false, - authMethods: [] + authMethods: [], + idle: false }; const action = new RetrieveAuthMethodsAction(new AuthStatus(), true); const newState = authReducer(initialState, action); @@ -519,7 +558,8 @@ describe('authReducer', () => { loaded: false, blocking: true, loading: true, - authMethods: [] + authMethods: [], + idle: false }; expect(newState).toEqual(state); }); @@ -530,7 +570,8 @@ describe('authReducer', () => { loaded: false, blocking: true, loading: true, - authMethods: [] + authMethods: [], + idle: false }; const authMethods = [ new AuthMethod(AuthMethodType.Password), @@ -543,7 +584,8 @@ describe('authReducer', () => { loaded: false, blocking: false, loading: false, - authMethods: authMethods + authMethods: authMethods, + idle: false }; expect(newState).toEqual(state); }); @@ -554,7 +596,8 @@ describe('authReducer', () => { loaded: false, blocking: true, loading: true, - authMethods: [] + authMethods: [], + idle: false }; const authMethods = [ new AuthMethod(AuthMethodType.Password), @@ -588,7 +631,50 @@ describe('authReducer', () => { loaded: false, blocking: false, loading: false, - authMethods: [new AuthMethod(AuthMethodType.Password)] + authMethods: [new AuthMethod(AuthMethodType.Password)], + idle: false + }; + expect(newState).toEqual(state); + }); + + it('should properly set the state, in response to a SET_USER_AS_IDLE action', () => { + initialState = { + authenticated: true, + loaded: true, + blocking: false, + loading: false, + idle: false + }; + + const action = new SetUserAsIdleAction(); + const newState = authReducer(initialState, action); + state = { + authenticated: true, + loaded: true, + blocking: false, + loading: false, + idle: true + }; + expect(newState).toEqual(state); + }); + + it('should properly set the state, in response to a UNSET_USER_AS_IDLE action', () => { + initialState = { + authenticated: true, + loaded: true, + blocking: false, + loading: false, + idle: true + }; + + const action = new UnsetUserAsIdleAction(); + const newState = authReducer(initialState, action); + state = { + authenticated: true, + loaded: true, + blocking: false, + loading: false, + 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 dfe29a3ef2..0424a58898 100644 --- a/src/app/core/auth/auth.reducer.ts +++ b/src/app/core/auth/auth.reducer.ts @@ -59,6 +59,9 @@ export interface AuthState { // all authentication Methods enabled at the backend authMethods?: AuthMethod[]; + // true when the current user is idle + idle: boolean; + } /** @@ -69,7 +72,8 @@ const initialState: AuthState = { loaded: false, blocking: true, loading: false, - authMethods: [] + authMethods: [], + idle: false }; /** @@ -234,6 +238,22 @@ export function authReducer(state: any = initialState, action: AuthActions): Aut blocking: true, }); + case AuthActionTypes.SET_USER_AS_IDLE: + if (state.authenticated) { + return Object.assign({}, state, { + idle: true, + }); + } else { + return Object.assign({}, state, { + idle: false, + }); + } + + case AuthActionTypes.UNSET_USER_AS_IDLE: + return Object.assign({}, state, { + idle: false, + }); + default: return state; } diff --git a/src/app/core/auth/auth.service.spec.ts b/src/app/core/auth/auth.service.spec.ts index 505f925e8e..d54ffdae16 100644 --- a/src/app/core/auth/auth.service.spec.ts +++ b/src/app/core/auth/auth.service.spec.ts @@ -27,6 +27,10 @@ import { createSuccessfulRemoteDataObject$ } from '../../shared/remote-data.util import { authMethodsMock } from '../../shared/testing/auth-service.stub'; import { AuthMethod } from './models/auth.method'; import { HardRedirectService } from '../services/hard-redirect.service'; +import { NotificationsService } from '../../shared/notifications/notifications.service'; +import { TranslateService } from '@ngx-translate/core'; +import { getMockTranslateService } from '../../shared/mocks/translate.service.mock'; +import { NotificationsServiceStub } from '../../shared/testing/notifications-service.stub'; describe('AuthService test', () => { @@ -107,6 +111,8 @@ describe('AuthService test', () => { { provide: Store, useValue: mockStore }, { provide: EPersonDataService, useValue: mockEpersonDataService }, { provide: HardRedirectService, useValue: hardRedirectService }, + { provide: NotificationsService, useValue: NotificationsServiceStub }, + { provide: TranslateService, useValue: getMockTranslateService() }, CookieService, AuthService ], @@ -207,13 +213,13 @@ describe('AuthService test', () => { }).compileComponents(); })); - beforeEach(inject([CookieService, AuthRequestService, Store, Router, RouteService], (cookieService: CookieService, authReqService: AuthRequestService, store: Store, router: Router, routeService: RouteService) => { + beforeEach(inject([CookieService, AuthRequestService, Store, Router, RouteService], (cookieService: CookieService, authReqService: AuthRequestService, store: Store, router: Router, routeService: RouteService, notificationsService: NotificationsService, translateService: TranslateService) => { store .subscribe((state) => { (state as any).core = Object.create({}); (state as any).core.auth = authenticatedState; }); - authService = new AuthService({}, window, undefined, authReqService, mockEpersonDataService, router, routeService, cookieService, store, hardRedirectService); + authService = new AuthService({}, window, undefined, authReqService, mockEpersonDataService, router, routeService, cookieService, store, hardRedirectService, notificationsService, translateService); })); it('should return true when user is logged in', () => { @@ -277,7 +283,7 @@ describe('AuthService test', () => { }).compileComponents(); })); - beforeEach(inject([ClientCookieService, AuthRequestService, Store, Router, RouteService], (cookieService: ClientCookieService, authReqService: AuthRequestService, store: Store, router: Router, routeService: RouteService) => { + beforeEach(inject([ClientCookieService, AuthRequestService, Store, Router, RouteService], (cookieService: ClientCookieService, authReqService: AuthRequestService, store: Store, router: Router, routeService: RouteService, notificationsService: NotificationsService, translateService: TranslateService) => { const expiredToken: AuthTokenInfo = new AuthTokenInfo('test_token'); expiredToken.expires = Date.now() - (1000 * 60 * 60); authenticatedState = { @@ -292,7 +298,7 @@ describe('AuthService test', () => { (state as any).core = Object.create({}); (state as any).core.auth = authenticatedState; }); - authService = new AuthService({}, window, undefined, authReqService, mockEpersonDataService, router, routeService, cookieService, store, hardRedirectService); + authService = new AuthService({}, window, undefined, authReqService, mockEpersonDataService, router, routeService, cookieService, store, hardRedirectService, notificationsService, translateService); storage = (authService as any).storage; routeServiceMock = TestBed.inject(RouteService); routerStub = TestBed.inject(Router); @@ -493,13 +499,13 @@ describe('AuthService test', () => { }).compileComponents(); })); - beforeEach(inject([CookieService, AuthRequestService, Store, Router, RouteService], (cookieService: CookieService, authReqService: AuthRequestService, store: Store, router: Router, routeService: RouteService) => { + beforeEach(inject([CookieService, AuthRequestService, Store, Router, RouteService], (cookieService: CookieService, authReqService: AuthRequestService, store: Store, router: Router, routeService: RouteService, notificationsService: NotificationsService, translateService: TranslateService) => { store .subscribe((state) => { (state as any).core = Object.create({}); (state as any).core.auth = unAuthenticatedState; }); - authService = new AuthService({}, window, undefined, authReqService, mockEpersonDataService, router, routeService, cookieService, store, hardRedirectService); + authService = new AuthService({}, window, undefined, authReqService, mockEpersonDataService, router, routeService, cookieService, store, hardRedirectService, notificationsService, translateService); })); it('should return null for the shortlived token', () => { diff --git a/src/app/core/auth/auth.service.ts b/src/app/core/auth/auth.service.ts index 4903c30f15..7b7c61f741 100644 --- a/src/app/core/auth/auth.service.ts +++ b/src/app/core/auth/auth.service.ts @@ -29,6 +29,7 @@ import { getRedirectUrl, isAuthenticated, isAuthenticatedLoaded, + isIdle, isTokenRefreshing } from './selectors'; import { AppState } from '../../app.reducer'; @@ -36,7 +37,9 @@ import { CheckAuthenticationTokenAction, RefreshTokenAction, ResetAuthenticationMessagesAction, RetrieveAuthMethodsAction, - SetRedirectUrlAction + SetRedirectUrlAction, + SetUserAsIdleAction, + UnsetUserAsIdleAction } from './auth.actions'; import { NativeWindowRef, NativeWindowService } from '../services/window.service'; import { Base64EncodeUrl } from '../../shared/utils/encode-decode.util'; @@ -197,7 +200,7 @@ export class AuthService { return this.store.pipe( select(getAuthenticatedUserId), hasValueOperator(), - switchMap((id: string) => this.epersonService.findById(id) ), + switchMap((id: string) => this.epersonService.findById(id)), getAllSucceededRemoteDataPayload() ); } @@ -279,7 +282,7 @@ export class AuthService { // Send a request that sign end the session let headers = new HttpHeaders(); headers = headers.append('Content-Type', 'application/x-www-form-urlencoded'); - const options: HttpOptions = Object.create({ headers, responseType: 'text' }); + const options: HttpOptions = Object.create({headers, responseType: 'text'}); return this.authRequestService.postToEndpoint('logout', options).pipe( map((rd: RemoteData) => { const status = rd.payload; @@ -324,20 +327,20 @@ export class AuthService { let currentlyRefreshingToken = false; this.store.pipe(select(getAuthenticationToken)).subscribe((authTokenInfo: AuthTokenInfo) => { // If new token is undefined an it wasn't previously => Refresh failed - if (currentlyRefreshingToken && token != undefined && authTokenInfo == undefined) { + if (currentlyRefreshingToken && token !== undefined && authTokenInfo === undefined) { // Token refresh failed => Error notification => 10 second wait => Page reloads & user logged out this.notificationService.error(this.translateService.get('auth.messages.token-refresh-failed')); setTimeout(() => this.navigateToRedirectUrl(this.hardRedirectService.getCurrentRoute()), 10000); currentlyRefreshingToken = false; } // If new token.expires is different => Refresh succeeded - if (currentlyRefreshingToken && authTokenInfo != undefined && token.expires != authTokenInfo.expires) { + if (currentlyRefreshingToken && authTokenInfo !== undefined && token.expires !== authTokenInfo.expires) { currentlyRefreshingToken = false; } // Check if/when token needs to be refreshed if (!currentlyRefreshingToken) { token = authTokenInfo || null; - if (token != undefined && token != null) { + if (token !== undefined && token !== null) { let timeLeftBeforeRefresh = token.expires - new Date().getTime() - environment.auth.rest.timeLeftBeforeTokenRefresh; if (timeLeftBeforeRefresh < 0) { timeLeftBeforeRefresh = 0; @@ -394,7 +397,7 @@ export class AuthService { // Set the cookie expire date const expires = new Date(expireDate); - const options: CookieAttributes = { expires: expires }; + const options: CookieAttributes = {expires: expires}; // Save cookie with the token return this.storage.set(TOKENITEM, token, options); @@ -483,7 +486,7 @@ export class AuthService { // Set the cookie expire date const expires = new Date(expireDate); - const options: CookieAttributes = { expires: expires }; + const options: CookieAttributes = {expires: expires}; this.storage.set(REDIRECT_COOKIE, url, options); this.store.dispatch(new SetRedirectUrlAction(isNotUndefined(url) ? url : '')); } @@ -576,4 +579,24 @@ export class AuthService { return new RetrieveAuthMethodsAction(authStatus, false); } + /** + * Determines if current user is idle + * @returns {Observable} + */ + public isUserIdle(): Observable { + return this.store.pipe(select(isIdle)); + } + + /** + * Set idle of auth state + * @returns {Observable} + */ + public setIdle(idle: boolean): void { + if (idle) { + this.store.dispatch(new SetUserAsIdleAction()); + } else { + this.store.dispatch(new UnsetUserAsIdleAction()); + } + } + } diff --git a/src/app/core/auth/selectors.ts b/src/app/core/auth/selectors.ts index c4e95a0fb3..9ee9f7eb2e 100644 --- a/src/app/core/auth/selectors.ts +++ b/src/app/core/auth/selectors.ts @@ -115,6 +115,14 @@ const _getRedirectUrl = (state: AuthState) => state.redirectUrl; const _getAuthenticationMethods = (state: AuthState) => state.authMethods; +/** + * Returns true if the user is idle. + * @function _isIdle + * @param {State} state + * @returns {boolean} + */ +const _isIdle = (state: AuthState) => state.idle; + /** * Returns the authentication methods enabled at the backend * @function getAuthenticationMethods @@ -231,3 +239,12 @@ export const getRegistrationError = createSelector(getAuthState, _getRegistratio * @return {string} */ export const getRedirectUrl = createSelector(getAuthState, _getRedirectUrl); + +/** + * Returns true if the user is idle + * @function isIdle + * @param {AuthState} state + * @param {any} props + * @return {boolean} + */ +export const isIdle = createSelector(getAuthState, _isIdle); diff --git a/src/app/root/root.component.ts b/src/app/root/root.component.ts index 576a0152be..c2d3c96951 100644 --- a/src/app/root/root.component.ts +++ b/src/app/root/root.component.ts @@ -1,4 +1,4 @@ -import { map } from 'rxjs/operators'; +import { map, take } from 'rxjs/operators'; import { Component, Inject, OnInit, Optional, Input } from '@angular/core'; import { Router } from '@angular/router'; @@ -21,6 +21,8 @@ import { environment } from '../../environments/environment'; import { LocaleService } from '../core/locale/locale.service'; import { KlaroService } from '../shared/cookies/klaro.service'; import { slideSidebarPadding } from '../shared/animations/slide'; +import { NgbModal } from '@ng-bootstrap/ng-bootstrap'; +import { IdleModalComponent } from '../shared/idle-modal/idle-modal.component'; @Component({ selector: 'ds-root', @@ -47,6 +49,11 @@ export class RootComponent implements OnInit { */ @Input() shouldShowRouteLoader: boolean; + /** + * Whether or not the idle modal is is currently open + */ + idleModalOpen: boolean; + constructor( @Inject(NativeWindowService) private _window: NativeWindowRef, private translate: TranslateService, @@ -60,7 +67,8 @@ export class RootComponent implements OnInit { private menuService: MenuService, private windowService: HostWindowService, private localeService: LocaleService, - @Optional() private cookiesService: KlaroService + @Optional() private cookiesService: KlaroService, + private modalService: NgbModal ) { } @@ -75,5 +83,19 @@ export class RootComponent implements OnInit { .pipe( map(([collapsed, mobile]) => collapsed || mobile) ); + + this.authService.isUserIdle().subscribe((userIdle: boolean) => { + if (userIdle) { + if (!this.idleModalOpen) { + const modalRef = this.modalService.open(IdleModalComponent); + this.idleModalOpen = true; + modalRef.componentInstance.response.pipe(take(1)).subscribe((closed: boolean) => { + if (closed) { + this.idleModalOpen = false; + } + }); + } + } + }); } } diff --git a/src/app/shared/auth-nav-menu/auth-nav-menu.component.spec.ts b/src/app/shared/auth-nav-menu/auth-nav-menu.component.spec.ts index 185cf3e92a..6aad3dd636 100644 --- a/src/app/shared/auth-nav-menu/auth-nav-menu.component.spec.ts +++ b/src/app/shared/auth-nav-menu/auth-nav-menu.component.spec.ts @@ -44,7 +44,8 @@ describe('AuthNavMenuComponent', () => { authenticated: false, loaded: false, blocking: false, - loading: false + loading: false, + idle: false }; authState = { authenticated: true, @@ -52,7 +53,8 @@ describe('AuthNavMenuComponent', () => { blocking: false, loading: false, authToken: new AuthTokenInfo('test_token'), - userId: EPersonMock.id + userId: EPersonMock.id, + idle: false }; } diff --git a/src/app/shared/auth-nav-menu/user-menu/user-menu.component.spec.ts b/src/app/shared/auth-nav-menu/user-menu/user-menu.component.spec.ts index c0f5f13e8f..983fe68274 100644 --- a/src/app/shared/auth-nav-menu/user-menu/user-menu.component.spec.ts +++ b/src/app/shared/auth-nav-menu/user-menu/user-menu.component.spec.ts @@ -37,7 +37,8 @@ describe('UserMenuComponent', () => { blocking: false, loading: false, authToken: new AuthTokenInfo('test_token'), - userId: EPersonMock.id + userId: EPersonMock.id, + idle: false }; authStateLoading = { authenticated: true, @@ -45,7 +46,8 @@ describe('UserMenuComponent', () => { blocking: false, loading: true, authToken: null, - userId: EPersonMock.id + userId: EPersonMock.id, + idle: false }; } diff --git a/src/app/shared/idle-modal/idle-modal.component.html b/src/app/shared/idle-modal/idle-modal.component.html new file mode 100644 index 0000000000..665ebb9672 --- /dev/null +++ b/src/app/shared/idle-modal/idle-modal.component.html @@ -0,0 +1,18 @@ +
+
{{ "idle-modal.header" | translate }} + +
+
+

{{ "idle-modal.info" | translate:{timeToExpire: timeToExpire} }}

+
+
+ + +
+
diff --git a/src/app/shared/idle-modal/idle-modal.component.ts b/src/app/shared/idle-modal/idle-modal.component.ts new file mode 100644 index 0000000000..750657c2e4 --- /dev/null +++ b/src/app/shared/idle-modal/idle-modal.component.ts @@ -0,0 +1,85 @@ +import { Component, OnInit, Output } from '@angular/core'; +import { NgbActiveModal } from '@ng-bootstrap/ng-bootstrap'; +import { environment } from '../../../environments/environment'; +import { AuthService } from '../../core/auth/auth.service'; +import { Subject } from 'rxjs'; +import { hasValue } from '../empty.util'; + +@Component({ + selector: 'ds-idle-modal', + templateUrl: 'idle-modal.component.html', +}) +export class IdleModalComponent implements OnInit { + + /** + * Total time of idleness before session expires (in minutes) + * (environment.auth.ui.timeUntilIdle + environment.auth.ui.idleGracePeriod / 1000 / 60) + */ + timeToExpire: number; + + /** + * Timer to track time grace period + */ + private graceTimer; + + /** + * An event fired when the modal is closed + */ + @Output() + response: Subject = new Subject(); + + constructor(private activeModal: NgbActiveModal, + private authService: AuthService) { + this.timeToExpire = (environment.auth.ui.timeUntilIdle + environment.auth.ui.idleGracePeriod) / 1000 / 60; // ms => min + } + + ngOnInit() { + if (hasValue(this.graceTimer)) { + clearTimeout(this.graceTimer); + } + this.graceTimer = setTimeout(() => { + this.logOutPressed(); + }, environment.auth.ui.idleGracePeriod); + } + + /** + * When extend session is pressed + */ + extendSessionPressed() { + this.extendSessionAndCloseModal(); + } + + /** + * Close modal and logout + */ + logOutPressed() { + this.authService.logout(); + this.closeModal(); + } + + /** + * When close is pressed + */ + closePressed() { + this.extendSessionAndCloseModal(); + } + + /** + * Close the modal and extend session + */ + extendSessionAndCloseModal() { + if (hasValue(this.graceTimer)) { + clearTimeout(this.graceTimer); + } + this.authService.setIdle(false); + this.closeModal(); + } + + /** + * Close the modal and set the response to true so RootComponent knows the modal was closed + */ + closeModal() { + this.activeModal.close(); + this.response.next(true); + } +} diff --git a/src/app/shared/mocks/auth.service.mock.ts b/src/app/shared/mocks/auth.service.mock.ts index 98878bd6c1..bb39d08284 100644 --- a/src/app/shared/mocks/auth.service.mock.ts +++ b/src/app/shared/mocks/auth.service.mock.ts @@ -19,4 +19,11 @@ export class AuthServiceMock { public setRedirectUrl(url: string) { } + + public trackTokenExpiration(): void { + } + + public isUserIdle(): Observable { + return observableOf(false); + } } diff --git a/src/assets/i18n/en.json5 b/src/assets/i18n/en.json5 index cf19be3730..6105c79cd9 100644 --- a/src/assets/i18n/en.json5 +++ b/src/assets/i18n/en.json5 @@ -3691,5 +3691,15 @@ "workflow-item.send-back.button.cancel": "Cancel", - "workflow-item.send-back.button.confirm": "Send back" + "workflow-item.send-back.button.confirm": "Send back", + + + + "idle-modal.header": "Session will expire soon", + + "idle-modal.info": "For security reasons, user sessions expire after {{ timeToExpire }} minutes of inactivity. Your session will expire soon. Would you like to extend it or log out?”", + + "idle-modal.log-out": "Log out", + + "idle-modal.extend-session": "Extend session" } diff --git a/src/environments/mock-environment.ts b/src/environments/mock-environment.ts index 8de5b187ad..050f99ea29 100644 --- a/src/environments/mock-environment.ts +++ b/src/environments/mock-environment.ts @@ -35,6 +35,22 @@ export const environment: Partial = { timePerMethod: {[RestRequestMethod.PATCH]: 3} as any // time in seconds } }, + // Authority settings + auth: { + // Authority UI settings + ui: { + // the amount of time before the idle warning is shown + timeUntilIdle: 20000, // 20 sec + // the amount of time the user has to react after the idle warning is shown before they are logged out. + idleGracePeriod: 20000, // 20 sec + }, + // Authority REST settings + rest: { + // If the rest token expires in less than this amount of time, it will be refreshed automatically. + // This is independent from the idle warning. + timeLeftBeforeTokenRefresh: 20000, // 20 sec + }, + }, // Form settings form: { // NOTE: Map server-side validators to comparative Angular form validators From e88baa1995d86f34d70b0fde13e078ce3c67ea37 Mon Sep 17 00:00:00 2001 From: Marie Verdonck Date: Thu, 3 Jun 2021 14:29:50 +0200 Subject: [PATCH 03/16] 79700: specs for modal, auth check for idleness tracking & stop blocking at token success --- src/app/core/auth/auth.effects.ts | 1 - src/app/core/auth/auth.reducer.ts | 1 + src/app/root/root.component.ts | 30 ++-- .../idle-modal/idle-modal.component.spec.ts | 128 ++++++++++++++++++ src/assets/i18n/en.json5 | 2 +- src/environments/environment.common.ts | 7 +- 6 files changed, 151 insertions(+), 18 deletions(-) create mode 100644 src/app/shared/idle-modal/idle-modal.component.spec.ts diff --git a/src/app/core/auth/auth.effects.ts b/src/app/core/auth/auth.effects.ts index c133310471..f7b81dc4ef 100644 --- a/src/app/core/auth/auth.effects.ts +++ b/src/app/core/auth/auth.effects.ts @@ -263,7 +263,6 @@ export class AuthEffects { filter((action: Action) => !IDLE_TIMER_IGNORE_TYPES.includes(action.type)), // Using switchMap the timer will be interrupted and restarted if a new action comes in, so idleness timer restarts switchMap(() => { - this.authService.isAuthenticated(); return timer(environment.auth.ui.timeUntilIdle); }), map(() => { diff --git a/src/app/core/auth/auth.reducer.ts b/src/app/core/auth/auth.reducer.ts index 0424a58898..f26ddb0182 100644 --- a/src/app/core/auth/auth.reducer.ts +++ b/src/app/core/auth/auth.reducer.ts @@ -193,6 +193,7 @@ export function authReducer(state: any = initialState, action: AuthActions): Aut return Object.assign({}, state, { authToken: (action as RefreshTokenSuccessAction).payload, refreshing: false, + blocking: false }); case AuthActionTypes.ADD_MESSAGE: diff --git a/src/app/root/root.component.ts b/src/app/root/root.component.ts index c2d3c96951..81ae1a745c 100644 --- a/src/app/root/root.component.ts +++ b/src/app/root/root.component.ts @@ -2,7 +2,12 @@ import { map, take } from 'rxjs/operators'; import { Component, Inject, OnInit, Optional, Input } from '@angular/core'; import { Router } from '@angular/router'; -import { combineLatest as combineLatestObservable, Observable, of } from 'rxjs'; +import { + combineLatest as observableCombineLatest, + combineLatest as combineLatestObservable, + Observable, + of +} from 'rxjs'; import { Store } from '@ngrx/store'; import { TranslateService } from '@ngx-translate/core'; import { Angulartics2GoogleAnalytics } from 'angulartics2/ga'; @@ -84,18 +89,19 @@ export class RootComponent implements OnInit { map(([collapsed, mobile]) => collapsed || mobile) ); - this.authService.isUserIdle().subscribe((userIdle: boolean) => { - if (userIdle) { - if (!this.idleModalOpen) { - const modalRef = this.modalService.open(IdleModalComponent); - this.idleModalOpen = true; - modalRef.componentInstance.response.pipe(take(1)).subscribe((closed: boolean) => { - if (closed) { - this.idleModalOpen = false; - } - }); + observableCombineLatest([this.authService.isUserIdle(), this.authService.isAuthenticated()]) + .subscribe(([userIdle, authenticated]) => { + if (userIdle && authenticated) { + if (!this.idleModalOpen) { + const modalRef = this.modalService.open(IdleModalComponent); + this.idleModalOpen = true; + modalRef.componentInstance.response.pipe(take(1)).subscribe((closed: boolean) => { + if (closed) { + this.idleModalOpen = false; + } + }); + } } - } }); } } diff --git a/src/app/shared/idle-modal/idle-modal.component.spec.ts b/src/app/shared/idle-modal/idle-modal.component.spec.ts new file mode 100644 index 0000000000..639cbd6ad1 --- /dev/null +++ b/src/app/shared/idle-modal/idle-modal.component.spec.ts @@ -0,0 +1,128 @@ +import { DebugElement, NO_ERRORS_SCHEMA } from '@angular/core'; +import { ComponentFixture, fakeAsync, TestBed, tick, waitForAsync } from '@angular/core/testing'; +import { NgbActiveModal } from '@ng-bootstrap/ng-bootstrap'; +import { TranslateModule } from '@ngx-translate/core'; +import { IdleModalComponent } from './idle-modal.component'; +import { AuthService } from '../../core/auth/auth.service'; +import { By } from '@angular/platform-browser'; + +describe('IdleModalComponent', () => { + let component: IdleModalComponent; + let fixture: ComponentFixture; + let debugElement: DebugElement; + + const modalStub = jasmine.createSpyObj('modalStub', ['close']); + const authServiceStub = jasmine.createSpyObj('authService', ['setIdle', 'logout']); + + beforeEach(waitForAsync(() => { + TestBed.configureTestingModule({ + imports: [TranslateModule.forRoot()], + declarations: [IdleModalComponent], + providers: [ + { provide: NgbActiveModal, useValue: modalStub }, + { provide: AuthService, useValue: authServiceStub } + ], + schemas: [NO_ERRORS_SCHEMA] + }).compileComponents(); + })); + + beforeEach(() => { + fixture = TestBed.createComponent(IdleModalComponent); + component = fixture.componentInstance; + debugElement = fixture.debugElement; + fixture.detectChanges(); + }); + + it('should create', () => { + expect(component).toBeTruthy(); + }); + + describe('extendSessionPressed', () => { + beforeEach(fakeAsync(() => { + spyOn(component.response, 'next'); + component.extendSessionPressed(); + })); + it('should set idle to false', () => { + expect(authServiceStub.setIdle).toHaveBeenCalledWith(false); + }); + it('should close the modal', () => { + expect(modalStub.close).toHaveBeenCalled(); + }); + it('response \'closed\' should have true as next', () => { + expect(component.response.next).toHaveBeenCalledWith(true); + }); + }); + + describe('logOutPressed', () => { + beforeEach(() => { + component.logOutPressed(); + }); + it('should logout', () => { + expect(authServiceStub.logout).toHaveBeenCalled(); + }); + it('should close the modal', () => { + expect(modalStub.close).toHaveBeenCalled(); + }); + }); + + describe('closePressed', () => { + beforeEach(fakeAsync(() => { + spyOn(component.response, 'next'); + component.closePressed(); + })); + it('should set idle to false', () => { + expect(authServiceStub.setIdle).toHaveBeenCalledWith(false); + }); + it('should close the modal', () => { + expect(modalStub.close).toHaveBeenCalled(); + }); + it('response \'closed\' should have true as next', () => { + expect(component.response.next).toHaveBeenCalledWith(true); + }); + }); + + describe('when the click method emits on extend session button', () => { + beforeEach(fakeAsync(() => { + spyOn(component, 'extendSessionPressed'); + debugElement.query(By.css('button.confirm')).triggerEventHandler('click', { + preventDefault: () => {/**/ + } + }); + tick(); + fixture.detectChanges(); + })); + it('should call the extendSessionPressed method on the component', () => { + expect(component.extendSessionPressed).toHaveBeenCalled(); + }); + }); + + describe('when the click method emits on log out button', () => { + beforeEach(fakeAsync(() => { + spyOn(component, 'logOutPressed'); + debugElement.query(By.css('button.cancel')).triggerEventHandler('click', { + preventDefault: () => {/**/ + } + }); + tick(); + fixture.detectChanges(); + })); + it('should call the logOutPressed method on the component', () => { + expect(component.logOutPressed).toHaveBeenCalled(); + }); + }); + + describe('when the click method emits on close button', () => { + beforeEach(fakeAsync(() => { + spyOn(component, 'closePressed'); + debugElement.query(By.css('.close')).triggerEventHandler('click', { + preventDefault: () => {/**/ + } + }); + tick(); + fixture.detectChanges(); + })); + it('should call the closePressed method on the component', () => { + expect(component.closePressed).toHaveBeenCalled(); + }); + }); +}); diff --git a/src/assets/i18n/en.json5 b/src/assets/i18n/en.json5 index 6105c79cd9..5501b92aa7 100644 --- a/src/assets/i18n/en.json5 +++ b/src/assets/i18n/en.json5 @@ -3697,7 +3697,7 @@ "idle-modal.header": "Session will expire soon", - "idle-modal.info": "For security reasons, user sessions expire after {{ timeToExpire }} minutes of inactivity. Your session will expire soon. Would you like to extend it or log out?”", + "idle-modal.info": "For security reasons, user sessions expire after {{ timeToExpire }} minutes of inactivity. Your session will expire soon. Would you like to extend it or log out?", "idle-modal.log-out": "Log out", diff --git a/src/environments/environment.common.ts b/src/environments/environment.common.ts index a7d4ec8a00..24496386e9 100644 --- a/src/environments/environment.common.ts +++ b/src/environments/environment.common.ts @@ -48,17 +48,16 @@ export const environment: GlobalConfig = { ui: { // the amount of time before the idle warning is shown // timeUntilIdle: 15 * 60 * 1000, // 15 minutes - timeUntilIdle: 1 * 60 * 1000, // 1 minutes + timeUntilIdle: 30 * 1000, // 30 seconds // the amount of time the user has to react after the idle warning is shown before they are logged out. // idleGracePeriod: 5 * 60 * 1000, // 5 minutes - idleGracePeriod: 1 * 60 * 1000, // 1 minutes + idleGracePeriod: 1 * 60 * 1000, // 1 minute }, // Authority REST settings rest: { // If the rest token expires in less than this amount of time, it will be refreshed automatically. // This is independent from the idle warning. - // timeLeftBeforeTokenRefresh: 2 * 60 * 1000, // 2 minutes - timeLeftBeforeTokenRefresh: 0.25 * 60 * 1000, // 25 seconds + timeLeftBeforeTokenRefresh: 2 * 60 * 1000, // 2 minutes }, }, // Form settings From 91b4c81986e6b5b00f8a12b6c6054822bd7b4f48 Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Tue, 15 Jun 2021 15:48:10 +0200 Subject: [PATCH 04/16] run idle timer outside of angular zone --- src/app/core/auth/auth.effects.ts | 34 ++++++++++++------- .../core/utilities/enter-zone.scheduler.ts | 19 +++++++++++ .../core/utilities/leave-zone.scheduler.ts | 19 +++++++++++ 3 files changed, 60 insertions(+), 12 deletions(-) create mode 100644 src/app/core/utilities/enter-zone.scheduler.ts create mode 100644 src/app/core/utilities/leave-zone.scheduler.ts diff --git a/src/app/core/auth/auth.effects.ts b/src/app/core/auth/auth.effects.ts index f7b81dc4ef..8ce10c0c6b 100644 --- a/src/app/core/auth/auth.effects.ts +++ b/src/app/core/auth/auth.effects.ts @@ -1,7 +1,13 @@ -import { Injectable } from '@angular/core'; +import { Injectable, NgZone } from '@angular/core'; -import { combineLatest as observableCombineLatest, Observable, of as observableOf, timer } from 'rxjs'; -import { catchError, filter, map, switchMap, take, tap } from 'rxjs/operators'; +import { + combineLatest as observableCombineLatest, + Observable, + of as observableOf, + timer, + asyncScheduler, queueScheduler +} from 'rxjs'; +import { catchError, filter, map, switchMap, take, tap, observeOn } from 'rxjs/operators'; // import @ngrx import { Actions, Effect, ofType } from '@ngrx/effects'; import { Action, select, Store } from '@ngrx/store'; @@ -43,8 +49,8 @@ import { hasValue } from '../../shared/empty.util'; import { environment } from '../../../environments/environment'; import { RequestActionTypes } from '../data/request.actions'; import { NotificationsActionTypes } from '../../shared/notifications/notifications.actions'; -import { ObjectCacheActionTypes } from '../cache/object-cache.actions'; -import { NO_OP_ACTION_TYPE } from '../../shared/ngrx/no-op.action'; +import { LeaveZoneScheduler } from '../utilities/leave-zone.scheduler'; +import { EnterZoneScheduler } from '../utilities/enter-zone.scheduler'; // Action Types that do not break/prevent the user from an idle state const IDLE_TIMER_IGNORE_TYPES: string[] @@ -261,22 +267,26 @@ export class AuthEffects { @Effect() public trackIdleness$: Observable = this.actions$.pipe( filter((action: Action) => !IDLE_TIMER_IGNORE_TYPES.includes(action.type)), - // Using switchMap the timer will be interrupted and restarted if a new action comes in, so idleness timer restarts - switchMap(() => { - return timer(environment.auth.ui.timeUntilIdle); - }), - map(() => { - return new SetUserAsIdleAction(); - }) + // Using switchMap the effect will stop subscribing to the previous timer if a new action comes + // in, and start a new timer + switchMap(() => + // Start a timer outside of Angular's zone + timer(environment.auth.ui.timeUntilIdle, new LeaveZoneScheduler(this.zone, asyncScheduler)) + ), + // Re-enter the zone to dispatch the action + observeOn(new EnterZoneScheduler(this.zone, queueScheduler)), + map(() => new SetUserAsIdleAction()), ); /** * @constructor * @param {Actions} actions$ + * @param {NgZone} zone * @param {AuthService} authService * @param {Store} store */ constructor(private actions$: Actions, + private zone: NgZone, private authService: AuthService, private store: Store) { } diff --git a/src/app/core/utilities/enter-zone.scheduler.ts b/src/app/core/utilities/enter-zone.scheduler.ts new file mode 100644 index 0000000000..96aee7d9a5 --- /dev/null +++ b/src/app/core/utilities/enter-zone.scheduler.ts @@ -0,0 +1,19 @@ +import { SchedulerLike, Subscription } from 'rxjs'; +import { NgZone } from '@angular/core'; + +/** + * An RXJS scheduler that will re-enter the Angular zone to run what's scheduled + */ +export class EnterZoneScheduler implements SchedulerLike { + constructor(private zone: NgZone, private scheduler: SchedulerLike) { } + + schedule(...args: any[]): Subscription { + return this.zone.run(() => + this.scheduler.schedule.apply(this.scheduler, args) + ); + } + + now (): number { + return this.scheduler.now(); + } +} diff --git a/src/app/core/utilities/leave-zone.scheduler.ts b/src/app/core/utilities/leave-zone.scheduler.ts new file mode 100644 index 0000000000..2587563661 --- /dev/null +++ b/src/app/core/utilities/leave-zone.scheduler.ts @@ -0,0 +1,19 @@ +import { SchedulerLike, Subscription } from 'rxjs'; +import { NgZone } from '@angular/core'; + +/** + * An RXJS scheduler that will run what's scheduled outside of the Angular zone + */ +export class LeaveZoneScheduler implements SchedulerLike { + constructor(private zone: NgZone, private scheduler: SchedulerLike) { } + + schedule(...args: any[]): Subscription { + return this.zone.runOutsideAngular(() => + this.scheduler.schedule.apply(this.scheduler, args) + ); + } + + now (): number { + return this.scheduler.now(); + } +} From 4b1f0864696fb06a8a10457184b90dbe0dc15d75 Mon Sep 17 00:00:00 2001 From: Marie Verdonck Date: Fri, 18 Jun 2021 13:18:17 +0200 Subject: [PATCH 05/16] 79700: Feedback 2021-06-15 applied --- src/app/app.component.ts | 37 +++++++++++++++-- src/app/core/auth/auth.reducer.ts | 12 ++---- src/app/core/auth/auth.service.ts | 13 +++--- src/app/root/root.component.ts | 40 ++----------------- .../idle-modal/idle-modal.component.spec.ts | 12 +++++- .../shared/idle-modal/idle-modal.component.ts | 7 +++- 6 files changed, 64 insertions(+), 57 deletions(-) diff --git a/src/app/app.component.ts b/src/app/app.component.ts index 2c01bf637b..48e1e6f3d1 100644 --- a/src/app/app.component.ts +++ b/src/app/app.component.ts @@ -1,4 +1,4 @@ -import { delay, distinctUntilChanged, filter, take } from 'rxjs/operators'; +import { delay, distinctUntilChanged, filter, take, withLatestFrom } from 'rxjs/operators'; import { AfterViewInit, ChangeDetectionStrategy, @@ -11,7 +11,7 @@ import { } from '@angular/core'; import { NavigationCancel, NavigationEnd, NavigationStart, Router } from '@angular/router'; -import { BehaviorSubject, Observable, of } from 'rxjs'; +import { BehaviorSubject, combineLatest as observableCombineLatest, Observable, of } from 'rxjs'; import { select, Store } from '@ngrx/store'; import { TranslateService } from '@ngx-translate/core'; import { Angulartics2GoogleAnalytics } from 'angulartics2/ga'; @@ -38,6 +38,8 @@ import { ThemeService } from './shared/theme-support/theme.service'; import { BASE_THEME_NAME } from './shared/theme-support/theme.constants'; import { DEFAULT_THEME_CONFIG } from './shared/theme-support/theme.effects'; import { BreadcrumbsService } from './breadcrumbs/breadcrumbs.service'; +import { IdleModalComponent } from './shared/idle-modal/idle-modal.component'; +import { NgbModal } from '@ng-bootstrap/ng-bootstrap'; @Component({ selector: 'ds-app', @@ -70,6 +72,11 @@ export class AppComponent implements OnInit, AfterViewInit { isThemeLoading$: BehaviorSubject = new BehaviorSubject(false); + /** + * Whether or not the idle modal is is currently open + */ + idleModalOpen: boolean; + constructor( @Inject(NativeWindowService) private _window: NativeWindowRef, @Inject(DOCUMENT) private document: any, @@ -87,6 +94,7 @@ export class AppComponent implements OnInit, AfterViewInit { private windowService: HostWindowService, private localeService: LocaleService, private breadcrumbsService: BreadcrumbsService, + private modalService: NgbModal, @Optional() private cookiesService: KlaroService, @Optional() private googleAnalyticsService: GoogleAnalyticsService, ) { @@ -108,6 +116,11 @@ export class AppComponent implements OnInit, AfterViewInit { } }); + if (isPlatformBrowser(this.platformId)) { + this.authService.trackTokenExpiration(); + this.trackIdleModal(); + } + // Load all the languages that are defined as active from the config file translate.addLangs(environment.languages.filter((LangConfig) => LangConfig.active === true).map((a) => a.code)); @@ -130,7 +143,6 @@ export class AppComponent implements OnInit, AfterViewInit { console.info(environment); } this.storeCSSVariables(); - this.authService.trackTokenExpiration(); } ngOnInit() { @@ -229,4 +241,23 @@ export class AppComponent implements OnInit, AfterViewInit { }; head.appendChild(link); } + + private trackIdleModal() { + const isIdle$ = this.authService.isUserIdle(); + const isAuthenticated$ = this.authService.isAuthenticated(); + isIdle$.pipe(withLatestFrom(isAuthenticated$)) + .subscribe(([userIdle, authenticated]) => { + if (userIdle && authenticated) { + if (!this.idleModalOpen) { + const modalRef = this.modalService.open(IdleModalComponent); + this.idleModalOpen = true; + modalRef.componentInstance.response.pipe(take(1)).subscribe((closed: boolean) => { + if (closed) { + this.idleModalOpen = false; + } + }); + } + } + }); + } } diff --git a/src/app/core/auth/auth.reducer.ts b/src/app/core/auth/auth.reducer.ts index f26ddb0182..ef803503c8 100644 --- a/src/app/core/auth/auth.reducer.ts +++ b/src/app/core/auth/auth.reducer.ts @@ -240,15 +240,9 @@ export function authReducer(state: any = initialState, action: AuthActions): Aut }); case AuthActionTypes.SET_USER_AS_IDLE: - if (state.authenticated) { - return Object.assign({}, state, { - idle: true, - }); - } else { - return Object.assign({}, state, { - idle: false, - }); - } + return Object.assign({}, state, { + idle: true, + }); case AuthActionTypes.UNSET_USER_AS_IDLE: return Object.assign({}, state, { diff --git a/src/app/core/auth/auth.service.ts b/src/app/core/auth/auth.service.ts index 7b7c61f741..a5b5d70704 100644 --- a/src/app/core/auth/auth.service.ts +++ b/src/app/core/auth/auth.service.ts @@ -282,7 +282,7 @@ export class AuthService { // Send a request that sign end the session let headers = new HttpHeaders(); headers = headers.append('Content-Type', 'application/x-www-form-urlencoded'); - const options: HttpOptions = Object.create({headers, responseType: 'text'}); + const options: HttpOptions = Object.create({ headers, responseType: 'text' }); return this.authRequestService.postToEndpoint('logout', options).pipe( map((rd: RemoteData) => { const status = rd.payload; @@ -447,11 +447,14 @@ export class AuthService { * @param redirectUrl */ public navigateToRedirectUrl(redirectUrl: string) { - let url = `/reload/${new Date().getTime()}`; - if (isNotEmpty(redirectUrl) && !redirectUrl.startsWith(LOGIN_ROUTE)) { - url += `?redirect=${encodeURIComponent(redirectUrl)}`; + // Don't do redirect if already on reload url + if (!hasValue(redirectUrl) || !redirectUrl.includes('/reload/')) { + let url = `/reload/${new Date().getTime()}`; + if (isNotEmpty(redirectUrl) && !redirectUrl.startsWith(LOGIN_ROUTE)) { + url += `?redirect=${encodeURIComponent(redirectUrl)}`; + } + this.hardRedirectService.redirect(url); } - this.hardRedirectService.redirect(url); } /** diff --git a/src/app/root/root.component.ts b/src/app/root/root.component.ts index 81ae1a745c..209f17b520 100644 --- a/src/app/root/root.component.ts +++ b/src/app/root/root.component.ts @@ -1,13 +1,8 @@ -import { map, take } from 'rxjs/operators'; -import { Component, Inject, OnInit, Optional, Input } from '@angular/core'; +import { map } from 'rxjs/operators'; +import { Component, Inject, OnInit, Input } from '@angular/core'; import { Router } from '@angular/router'; -import { - combineLatest as observableCombineLatest, - combineLatest as combineLatestObservable, - Observable, - of -} from 'rxjs'; +import { combineLatest as combineLatestObservable, Observable, of } from 'rxjs'; import { Store } from '@ngrx/store'; import { TranslateService } from '@ngx-translate/core'; import { Angulartics2GoogleAnalytics } from 'angulartics2/ga'; @@ -23,11 +18,7 @@ import { HostWindowService } from '../shared/host-window.service'; import { ThemeConfig } from '../../config/theme.model'; import { Angulartics2DSpace } from '../statistics/angulartics/dspace-provider'; import { environment } from '../../environments/environment'; -import { LocaleService } from '../core/locale/locale.service'; -import { KlaroService } from '../shared/cookies/klaro.service'; import { slideSidebarPadding } from '../shared/animations/slide'; -import { NgbModal } from '@ng-bootstrap/ng-bootstrap'; -import { IdleModalComponent } from '../shared/idle-modal/idle-modal.component'; @Component({ selector: 'ds-root', @@ -54,11 +45,6 @@ export class RootComponent implements OnInit { */ @Input() shouldShowRouteLoader: boolean; - /** - * Whether or not the idle modal is is currently open - */ - idleModalOpen: boolean; - constructor( @Inject(NativeWindowService) private _window: NativeWindowRef, private translate: TranslateService, @@ -70,10 +56,7 @@ export class RootComponent implements OnInit { private router: Router, private cssService: CSSVariableService, private menuService: MenuService, - private windowService: HostWindowService, - private localeService: LocaleService, - @Optional() private cookiesService: KlaroService, - private modalService: NgbModal + private windowService: HostWindowService ) { } @@ -88,20 +71,5 @@ export class RootComponent implements OnInit { .pipe( map(([collapsed, mobile]) => collapsed || mobile) ); - - observableCombineLatest([this.authService.isUserIdle(), this.authService.isAuthenticated()]) - .subscribe(([userIdle, authenticated]) => { - if (userIdle && authenticated) { - if (!this.idleModalOpen) { - const modalRef = this.modalService.open(IdleModalComponent); - this.idleModalOpen = true; - modalRef.componentInstance.response.pipe(take(1)).subscribe((closed: boolean) => { - if (closed) { - this.idleModalOpen = false; - } - }); - } - } - }); } } diff --git a/src/app/shared/idle-modal/idle-modal.component.spec.ts b/src/app/shared/idle-modal/idle-modal.component.spec.ts index 639cbd6ad1..552315d1a4 100644 --- a/src/app/shared/idle-modal/idle-modal.component.spec.ts +++ b/src/app/shared/idle-modal/idle-modal.component.spec.ts @@ -5,6 +5,7 @@ import { TranslateModule } from '@ngx-translate/core'; import { IdleModalComponent } from './idle-modal.component'; import { AuthService } from '../../core/auth/auth.service'; import { By } from '@angular/platform-browser'; +import { HardRedirectService } from '../../core/services/hard-redirect.service'; describe('IdleModalComponent', () => { let component: IdleModalComponent; @@ -12,15 +13,18 @@ describe('IdleModalComponent', () => { let debugElement: DebugElement; const modalStub = jasmine.createSpyObj('modalStub', ['close']); - const authServiceStub = jasmine.createSpyObj('authService', ['setIdle', 'logout']); + const authServiceStub = jasmine.createSpyObj('authService', ['setIdle', 'logout', 'navigateToRedirectUrl']); + let hardRedirectService; beforeEach(waitForAsync(() => { + hardRedirectService = jasmine.createSpyObj('hardRedirectService', ['getCurrentRoute']); TestBed.configureTestingModule({ imports: [TranslateModule.forRoot()], declarations: [IdleModalComponent], providers: [ { provide: NgbActiveModal, useValue: modalStub }, - { provide: AuthService, useValue: authServiceStub } + { provide: AuthService, useValue: authServiceStub }, + { provide: HardRedirectService, useValue: hardRedirectService } ], schemas: [NO_ERRORS_SCHEMA] }).compileComponents(); @@ -63,6 +67,10 @@ describe('IdleModalComponent', () => { it('should close the modal', () => { expect(modalStub.close).toHaveBeenCalled(); }); + it('should reload', () => { + expect(hardRedirectService.getCurrentRoute).toHaveBeenCalled(); + expect(authServiceStub.navigateToRedirectUrl).toHaveBeenCalled(); + }); }); describe('closePressed', () => { diff --git a/src/app/shared/idle-modal/idle-modal.component.ts b/src/app/shared/idle-modal/idle-modal.component.ts index 750657c2e4..d812d3ffc1 100644 --- a/src/app/shared/idle-modal/idle-modal.component.ts +++ b/src/app/shared/idle-modal/idle-modal.component.ts @@ -4,6 +4,7 @@ import { environment } from '../../../environments/environment'; import { AuthService } from '../../core/auth/auth.service'; import { Subject } from 'rxjs'; import { hasValue } from '../empty.util'; +import { HardRedirectService } from '../../core/services/hard-redirect.service'; @Component({ selector: 'ds-idle-modal', @@ -29,7 +30,8 @@ export class IdleModalComponent implements OnInit { response: Subject = new Subject(); constructor(private activeModal: NgbActiveModal, - private authService: AuthService) { + private authService: AuthService, + protected hardRedirectService: HardRedirectService) { this.timeToExpire = (environment.auth.ui.timeUntilIdle + environment.auth.ui.idleGracePeriod) / 1000 / 60; // ms => min } @@ -53,8 +55,9 @@ export class IdleModalComponent implements OnInit { * Close modal and logout */ logOutPressed() { - this.authService.logout(); this.closeModal(); + this.authService.logout(); + this.authService.navigateToRedirectUrl(this.hardRedirectService.getCurrentRoute()); } /** From c696b783931ccd0cbf35fc019a2a33338c9ad6f7 Mon Sep 17 00:00:00 2001 From: Marie Verdonck Date: Fri, 18 Jun 2021 13:24:34 +0200 Subject: [PATCH 06/16] 79700: idle time and grace period testing times removed --- src/environments/environment.common.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/environments/environment.common.ts b/src/environments/environment.common.ts index 24496386e9..87be9b4260 100644 --- a/src/environments/environment.common.ts +++ b/src/environments/environment.common.ts @@ -47,11 +47,9 @@ export const environment: GlobalConfig = { // Authority UI settings ui: { // the amount of time before the idle warning is shown - // timeUntilIdle: 15 * 60 * 1000, // 15 minutes - timeUntilIdle: 30 * 1000, // 30 seconds + timeUntilIdle: 15 * 60 * 1000, // 15 minutes // the amount of time the user has to react after the idle warning is shown before they are logged out. - // idleGracePeriod: 5 * 60 * 1000, // 5 minutes - idleGracePeriod: 1 * 60 * 1000, // 1 minute + idleGracePeriod: 5 * 60 * 1000, // 5 minutes }, // Authority REST settings rest: { From ddcb27da3f9f7144b2f5e8af2d9480da455cdb15 Mon Sep 17 00:00:00 2001 From: Marie Verdonck Date: Fri, 18 Jun 2021 16:01:53 +0200 Subject: [PATCH 07/16] 79700: logout via store, automatic redirect --- .../idle-modal/idle-modal.component.spec.ts | 23 +++++++++---------- .../shared/idle-modal/idle-modal.component.ts | 9 ++++---- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/app/shared/idle-modal/idle-modal.component.spec.ts b/src/app/shared/idle-modal/idle-modal.component.spec.ts index 552315d1a4..847bf6ac4f 100644 --- a/src/app/shared/idle-modal/idle-modal.component.spec.ts +++ b/src/app/shared/idle-modal/idle-modal.component.spec.ts @@ -5,26 +5,29 @@ import { TranslateModule } from '@ngx-translate/core'; import { IdleModalComponent } from './idle-modal.component'; import { AuthService } from '../../core/auth/auth.service'; import { By } from '@angular/platform-browser'; -import { HardRedirectService } from '../../core/services/hard-redirect.service'; +import { Store } from '@ngrx/store'; +import { LogOutAction } from '../../core/auth/auth.actions'; describe('IdleModalComponent', () => { let component: IdleModalComponent; let fixture: ComponentFixture; let debugElement: DebugElement; - const modalStub = jasmine.createSpyObj('modalStub', ['close']); - const authServiceStub = jasmine.createSpyObj('authService', ['setIdle', 'logout', 'navigateToRedirectUrl']); - let hardRedirectService; + let modalStub; + let authServiceStub; + let storeStub; beforeEach(waitForAsync(() => { - hardRedirectService = jasmine.createSpyObj('hardRedirectService', ['getCurrentRoute']); + modalStub = jasmine.createSpyObj('modalStub', ['close']); + authServiceStub = jasmine.createSpyObj('authService', ['setIdle']); + storeStub = jasmine.createSpyObj('store', ['dispatch']); TestBed.configureTestingModule({ imports: [TranslateModule.forRoot()], declarations: [IdleModalComponent], providers: [ { provide: NgbActiveModal, useValue: modalStub }, { provide: AuthService, useValue: authServiceStub }, - { provide: HardRedirectService, useValue: hardRedirectService } + { provide: Store, useValue: storeStub } ], schemas: [NO_ERRORS_SCHEMA] }).compileComponents(); @@ -61,15 +64,11 @@ describe('IdleModalComponent', () => { beforeEach(() => { component.logOutPressed(); }); - it('should logout', () => { - expect(authServiceStub.logout).toHaveBeenCalled(); - }); it('should close the modal', () => { expect(modalStub.close).toHaveBeenCalled(); }); - it('should reload', () => { - expect(hardRedirectService.getCurrentRoute).toHaveBeenCalled(); - expect(authServiceStub.navigateToRedirectUrl).toHaveBeenCalled(); + it('should send logout action', () => { + expect(storeStub.dispatch).toHaveBeenCalledWith(new LogOutAction()); }); }); diff --git a/src/app/shared/idle-modal/idle-modal.component.ts b/src/app/shared/idle-modal/idle-modal.component.ts index d812d3ffc1..35fafcf5cf 100644 --- a/src/app/shared/idle-modal/idle-modal.component.ts +++ b/src/app/shared/idle-modal/idle-modal.component.ts @@ -4,7 +4,9 @@ import { environment } from '../../../environments/environment'; import { AuthService } from '../../core/auth/auth.service'; import { Subject } from 'rxjs'; import { hasValue } from '../empty.util'; -import { HardRedirectService } from '../../core/services/hard-redirect.service'; +import { Store } from '@ngrx/store'; +import { AppState } from '../../app.reducer'; +import { LogOutAction } from '../../core/auth/auth.actions'; @Component({ selector: 'ds-idle-modal', @@ -31,7 +33,7 @@ export class IdleModalComponent implements OnInit { constructor(private activeModal: NgbActiveModal, private authService: AuthService, - protected hardRedirectService: HardRedirectService) { + private store: Store) { this.timeToExpire = (environment.auth.ui.timeUntilIdle + environment.auth.ui.idleGracePeriod) / 1000 / 60; // ms => min } @@ -56,8 +58,7 @@ export class IdleModalComponent implements OnInit { */ logOutPressed() { this.closeModal(); - this.authService.logout(); - this.authService.navigateToRedirectUrl(this.hardRedirectService.getCurrentRoute()); + this.store.dispatch(new LogOutAction()); } /** From 829ce12710d7d1c13f93f00e0895614fb4bdebb5 Mon Sep 17 00:00:00 2001 From: Marie Verdonck Date: Fri, 18 Jun 2021 17:53:34 +0200 Subject: [PATCH 08/16] lgtm alerts --- src/app/app.component.ts | 2 +- src/app/core/auth/auth.interceptor.ts | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/app/app.component.ts b/src/app/app.component.ts index 48e1e6f3d1..4feee0e585 100644 --- a/src/app/app.component.ts +++ b/src/app/app.component.ts @@ -11,7 +11,7 @@ import { } from '@angular/core'; import { NavigationCancel, NavigationEnd, NavigationStart, Router } from '@angular/router'; -import { BehaviorSubject, combineLatest as observableCombineLatest, Observable, of } from 'rxjs'; +import { BehaviorSubject, Observable, of } from 'rxjs'; import { select, Store } from '@ngrx/store'; import { TranslateService } from '@ngx-translate/core'; import { Angulartics2GoogleAnalytics } from 'angulartics2/ga'; diff --git a/src/app/core/auth/auth.interceptor.ts b/src/app/core/auth/auth.interceptor.ts index d16f46a849..a49030110b 100644 --- a/src/app/core/auth/auth.interceptor.ts +++ b/src/app/core/auth/auth.interceptor.ts @@ -1,6 +1,6 @@ import { Observable, of as observableOf, throwError as observableThrowError } from 'rxjs'; -import { catchError, filter, map } from 'rxjs/operators'; +import { catchError, map } from 'rxjs/operators'; import { Injectable, Injector } from '@angular/core'; import { HttpErrorResponse, @@ -12,14 +12,13 @@ import { HttpResponse, HttpResponseBase } from '@angular/common/http'; -import { find } from 'lodash'; import { AppState } from '../../app.reducer'; import { AuthService } from './auth.service'; import { AuthStatus } from './models/auth-status.model'; import { AuthTokenInfo } from './models/auth-token-info.model'; -import { hasValue, isNotEmpty, isNotNull, isUndefined } from '../../shared/empty.util'; -import { RedirectWhenTokenExpiredAction, RefreshTokenAction } from './auth.actions'; +import { hasValue, isNotEmpty, isNotNull } from '../../shared/empty.util'; +import { RedirectWhenTokenExpiredAction } from './auth.actions'; import { Store } from '@ngrx/store'; import { Router } from '@angular/router'; import { AuthMethod } from './models/auth.method'; From 6c219e72d53ece28293c60c22dea5b7ec1012d99 Mon Sep 17 00:00:00 2001 From: Marie Verdonck Date: Thu, 1 Jul 2021 15:50:21 +0200 Subject: [PATCH 09/16] 79700: Doc fixes, Spec tests authService & ariaLabelledBy for idle-modal --- src/app/app.component.ts | 2 +- src/app/core/auth/auth.service.spec.ts | 82 ++++++++++++++++++- .../idle-modal/idle-modal.component.html | 2 +- src/environments/environment.common.ts | 6 +- src/environments/mock-environment.ts | 6 +- 5 files changed, 88 insertions(+), 10 deletions(-) diff --git a/src/app/app.component.ts b/src/app/app.component.ts index 4feee0e585..356025da9e 100644 --- a/src/app/app.component.ts +++ b/src/app/app.component.ts @@ -249,7 +249,7 @@ export class AppComponent implements OnInit, AfterViewInit { .subscribe(([userIdle, authenticated]) => { if (userIdle && authenticated) { if (!this.idleModalOpen) { - const modalRef = this.modalService.open(IdleModalComponent); + const modalRef = this.modalService.open(IdleModalComponent, { ariaLabelledBy: 'idle-modal.header' }); this.idleModalOpen = true; modalRef.componentInstance.response.pipe(take(1)).subscribe((closed: boolean) => { if (closed) { diff --git a/src/app/core/auth/auth.service.spec.ts b/src/app/core/auth/auth.service.spec.ts index d54ffdae16..ced8bb94c8 100644 --- a/src/app/core/auth/auth.service.spec.ts +++ b/src/app/core/auth/auth.service.spec.ts @@ -31,6 +31,7 @@ import { NotificationsService } from '../../shared/notifications/notifications.s import { TranslateService } from '@ngx-translate/core'; import { getMockTranslateService } from '../../shared/mocks/translate.service.mock'; import { NotificationsServiceStub } from '../../shared/testing/notifications-service.stub'; +import { SetUserAsIdleAction, UnsetUserAsIdleAction } from './auth.actions'; describe('AuthService test', () => { @@ -51,6 +52,7 @@ describe('AuthService test', () => { let token: AuthTokenInfo; let authenticatedState; let unAuthenticatedState; + let idleState; let linkService; let hardRedirectService; @@ -68,14 +70,24 @@ describe('AuthService test', () => { loaded: true, loading: false, authToken: token, - user: EPersonMock + user: EPersonMock, + idle: false }; unAuthenticatedState = { authenticated: false, loaded: true, loading: false, authToken: undefined, - user: undefined + user: undefined, + idle: false + }; + idleState = { + authenticated: true, + loaded: true, + loading: false, + authToken: token, + user: EPersonMock, + idle: true }; authRequest = new AuthRequestServiceStub(); routeStub = new ActivatedRouteStub(); @@ -186,6 +198,26 @@ describe('AuthService test', () => { expect(authMethods.length).toBe(2); }); }); + + describe('setIdle true', () => { + beforeEach(() => { + authService.setIdle(true); + }); + + it('store should dispatch SetUserAsIdleAction', () => { + expect(mockStore.dispatch).toHaveBeenCalledWith(new SetUserAsIdleAction()); + }); + }); + + describe('setIdle false', () => { + beforeEach(() => { + authService.setIdle(false); + }); + + it('store should dispatch UnsetUserAsIdleAction', () => { + expect(mockStore.dispatch).toHaveBeenCalledWith(new UnsetUserAsIdleAction()); + }); + }); }); describe('', () => { @@ -256,6 +288,12 @@ describe('AuthService test', () => { }); }); + it('isUserIdle should return false when user is not yet idle', () => { + authService.isUserIdle().subscribe((status: boolean) => { + expect(status).toBe(false); + }); + }); + }); describe('', () => { @@ -514,4 +552,44 @@ describe('AuthService test', () => { }); }); }); + + describe('when user is idle', () => { + beforeEach(waitForAsync(() => { + init(); + TestBed.configureTestingModule({ + imports: [ + StoreModule.forRoot({ authReducer }, { + runtimeChecks: { + strictStateImmutability: false, + strictActionImmutability: false + } + }) + ], + providers: [ + { provide: AuthRequestService, useValue: authRequest }, + { provide: REQUEST, useValue: {} }, + { provide: Router, useValue: routerStub }, + { provide: RouteService, useValue: routeServiceStub }, + { provide: RemoteDataBuildService, useValue: linkService }, + CookieService, + AuthService + ] + }).compileComponents(); + })); + + beforeEach(inject([CookieService, AuthRequestService, Store, Router, RouteService], (cookieService: CookieService, authReqService: AuthRequestService, store: Store, router: Router, routeService: RouteService, notificationsService: NotificationsService, translateService: TranslateService) => { + store + .subscribe((state) => { + (state as any).core = Object.create({}); + (state as any).core.auth = idleState; + }); + authService = new AuthService({}, window, undefined, authReqService, mockEpersonDataService, router, routeService, cookieService, store, hardRedirectService, notificationsService, translateService); + })); + + it('isUserIdle should return true when user is not idle', () => { + authService.isUserIdle().subscribe((status: boolean) => { + expect(status).toBe(true); + }); + }); + }); }); diff --git a/src/app/shared/idle-modal/idle-modal.component.html b/src/app/shared/idle-modal/idle-modal.component.html index 665ebb9672..beea91fe7b 100644 --- a/src/app/shared/idle-modal/idle-modal.component.html +++ b/src/app/shared/idle-modal/idle-modal.component.html @@ -1,5 +1,5 @@
-
{{ "idle-modal.header" | translate }} +
-
From 32003a72fdb785722942c9bc958802e1954ea3dc Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Fri, 18 Jun 2021 11:57:36 +0200 Subject: [PATCH 11/16] Fix hardcoded submission section IDs --- .../submission-form-collection.component.ts | 3 ++- .../form/submission-form.component.html | 1 - .../submission-upload-files.component.ts | 25 ++++++++----------- .../submission/sections/sections.service.ts | 16 ++++++++++++ 4 files changed, 29 insertions(+), 16 deletions(-) diff --git a/src/app/submission/form/collection/submission-form-collection.component.ts b/src/app/submission/form/collection/submission-form-collection.component.ts index ba7eb88c6f..f90814f185 100644 --- a/src/app/submission/form/collection/submission-form-collection.component.ts +++ b/src/app/submission/form/collection/submission-form-collection.component.ts @@ -28,6 +28,7 @@ import { CollectionDataService } from '../../../core/data/collection-data.servic import { CollectionDropdownComponent } from '../../../shared/collection-dropdown/collection-dropdown.component'; import { SectionsService } from '../../sections/sections.service'; import { getFirstSucceededRemoteDataPayload } from '../../../core/shared/operators'; +import { SectionsType } from '../../sections/sections-type'; /** * This component allows to show the current collection the submission belonging to and to change it. @@ -142,7 +143,7 @@ export class SubmissionFormCollectionComponent implements OnChanges, OnInit { */ ngOnInit() { this.pathCombiner = new JsonPatchOperationPathCombiner('sections', 'collection'); - this.available$ = this.sectionsService.isSectionAvailable(this.submissionId, 'collection'); + this.available$ = this.sectionsService.isSectionTypeAvailable(this.submissionId, SectionsType.collection); } /** diff --git a/src/app/submission/form/submission-form.component.html b/src/app/submission/form/submission-form.component.html index 33b5d4be12..c86d4e0195 100644 --- a/src/app/submission/form/submission-form.component.html +++ b/src/app/submission/form/submission-form.component.html @@ -3,7 +3,6 @@
diff --git a/src/app/submission/form/submission-upload-files/submission-upload-files.component.ts b/src/app/submission/form/submission-upload-files/submission-upload-files.component.ts index 2c348d2c87..e49127e80f 100644 --- a/src/app/submission/form/submission-upload-files/submission-upload-files.component.ts +++ b/src/app/submission/form/submission-upload-files/submission-upload-files.component.ts @@ -13,6 +13,7 @@ import { UploaderOptions } from '../../../shared/uploader/uploader-options.model import parseSectionErrors from '../../utils/parseSectionErrors'; import { SubmissionJsonPatchOperationsService } from '../../../core/submission/submission-json-patch-operations.service'; import { WorkspaceItem } from '../../../core/submission/models/workspaceitem.model'; +import { SectionsType } from '../../sections/sections-type'; /** * This component represents the drop zone that provides to add files to the submission. @@ -35,12 +36,6 @@ export class SubmissionUploadFilesComponent implements OnChanges { */ @Input() submissionId: string; - /** - * The upload section id - * @type {string} - */ - @Input() sectionId: string; - /** * The uploader configuration options * @type {UploaderOptions} @@ -110,7 +105,7 @@ export class SubmissionUploadFilesComponent implements OnChanges { * Check if upload functionality is enabled */ ngOnChanges() { - this.uploadEnabled = this.sectionService.isSectionAvailable(this.submissionId, this.sectionId); + this.uploadEnabled = this.sectionService.isSectionTypeAvailable(this.submissionId, SectionsType.Upload); } /** @@ -136,14 +131,16 @@ export class SubmissionUploadFilesComponent implements OnChanges { .forEach((sectionId) => { const sectionData = normalizeSectionData(sections[sectionId]); const sectionErrors = errorsList[sectionId]; - if (sectionId === 'upload') { - // Look for errors on upload - if ((isEmpty(sectionErrors))) { - this.notificationsService.success(null, this.translate.get('submission.sections.upload.upload-successful')); - } else { - this.notificationsService.error(null, this.translate.get('submission.sections.upload.upload-failed')); + this.sectionService.isSectionType(this.submissionId, sectionId, SectionsType.Upload).subscribe((isUpload) => { + if (isUpload) { + // Look for errors on upload + if ((isEmpty(sectionErrors))) { + this.notificationsService.success(null, this.translate.get('submission.sections.upload.upload-successful')); + } else { + this.notificationsService.error(null, this.translate.get('submission.sections.upload.upload-failed')); + } } - } + }); this.sectionService.updateSectionData(this.submissionId, sectionId, sectionData, sectionErrors); }); } diff --git a/src/app/submission/sections/sections.service.ts b/src/app/submission/sections/sections.service.ts index d8d1491cb7..afe5dde570 100644 --- a/src/app/submission/sections/sections.service.ts +++ b/src/app/submission/sections/sections.service.ts @@ -328,6 +328,22 @@ export class SectionsService { distinctUntilChanged()); } + /** + * Check if given section id is of a given section type + * @param submissionId + * @param sectionId + * @param sectionType + */ + public isSectionType(submissionId: string, sectionId: string, sectionType: SectionsType): Observable { + return this.store.select(submissionObjectFromIdSelector(submissionId)).pipe( + filter((submissionState: SubmissionObjectEntry) => isNotUndefined(submissionState)), + map((submissionState: SubmissionObjectEntry) => { + return isNotUndefined(submissionState.sections) && isNotUndefined(submissionState.sections[sectionId] + && submissionState.sections[sectionId].sectionType === sectionType ); + }), + distinctUntilChanged()); + } + /** * Dispatch a new [EnableSectionAction] to add a new section and move page target to it * From 72e97ca6b44ec19dc49e9b422169999f0d1d06d5 Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Fri, 18 Jun 2021 12:33:01 +0200 Subject: [PATCH 12/16] Update unit tests --- .../shared/testing/sections-service.stub.ts | 2 + ...bmission-form-collection.component.spec.ts | 2 +- .../submission-upload-files.component.spec.ts | 83 ++++++++++--------- 3 files changed, 45 insertions(+), 42 deletions(-) diff --git a/src/app/shared/testing/sections-service.stub.ts b/src/app/shared/testing/sections-service.stub.ts index 3b311c5e19..c372cceadd 100644 --- a/src/app/shared/testing/sections-service.stub.ts +++ b/src/app/shared/testing/sections-service.stub.ts @@ -10,6 +10,8 @@ export class SectionsServiceStub { isSectionEnabled = jasmine.createSpy('isSectionEnabled'); isSectionReadOnly = jasmine.createSpy('isSectionReadOnly'); isSectionAvailable = jasmine.createSpy('isSectionAvailable'); + isSectionTypeAvailable = jasmine.createSpy('isSectionTypeAvailable'); + isSectionType = jasmine.createSpy('isSectionType'); addSection = jasmine.createSpy('addSection'); removeSection = jasmine.createSpy('removeSection'); updateSectionData = jasmine.createSpy('updateSectionData'); diff --git a/src/app/submission/form/collection/submission-form-collection.component.spec.ts b/src/app/submission/form/collection/submission-form-collection.component.spec.ts index 14dfcef864..5b9946e1a4 100644 --- a/src/app/submission/form/collection/submission-form-collection.component.spec.ts +++ b/src/app/submission/form/collection/submission-form-collection.component.spec.ts @@ -120,7 +120,7 @@ describe('SubmissionFormCollectionComponent Component', () => { }); const sectionsService: any = jasmine.createSpyObj('sectionsService', { - isSectionAvailable: of(true) + isSectionTypeAvailable: of(true) }); beforeEach(waitForAsync(() => { diff --git a/src/app/submission/form/submission-upload-files/submission-upload-files.component.spec.ts b/src/app/submission/form/submission-upload-files/submission-upload-files.component.spec.ts index 4a943276e5..cc7d0e8484 100644 --- a/src/app/submission/form/submission-upload-files/submission-upload-files.component.spec.ts +++ b/src/app/submission/form/submission-upload-files/submission-upload-files.component.spec.ts @@ -83,7 +83,6 @@ describe('SubmissionUploadFilesComponent Component', () => { const html = ` `; testFixture = createTestComponent(html, TestComponent) as ComponentFixture; @@ -108,11 +107,11 @@ describe('SubmissionUploadFilesComponent Component', () => { compAsAny = comp; submissionServiceStub = TestBed.inject(SubmissionService as any); sectionsServiceStub = TestBed.inject(SectionsService as any); + sectionsServiceStub.isSectionTypeAvailable.and.returnValue(observableOf(true)); notificationsServiceStub = TestBed.inject(NotificationsService as any); translateService = TestBed.inject(TranslateService); comp.submissionId = submissionId; comp.collectionId = collectionId; - comp.sectionId = 'upload'; comp.uploadFilesOptions = Object.assign(new UploaderOptions(),{ url: '', authToken: null, @@ -133,7 +132,7 @@ describe('SubmissionUploadFilesComponent Component', () => { }); it('should init uploadEnabled properly', () => { - sectionsServiceStub.isSectionAvailable.and.returnValue(hot('-a-b', { + sectionsServiceStub.isSectionTypeAvailable.and.returnValue(hot('-a-b', { a: false, b: true })); @@ -149,53 +148,56 @@ describe('SubmissionUploadFilesComponent Component', () => { expect(compAsAny.uploadEnabled).toBeObservable(expected); }); - it('should show a success notification and call updateSectionData on upload complete', () => { - - const expectedErrors: any = mockUploadResponse1ParsedErrors; - compAsAny.uploadEnabled = observableOf(true); - fixture.detectChanges(); - - comp.onCompleteItem(Object.assign({}, uploadRestResponse, { sections: mockSectionsData })); - - Object.keys(mockSectionsData).forEach((sectionId) => { - expect(sectionsServiceStub.updateSectionData).toHaveBeenCalledWith( - submissionId, - sectionId, - mockSectionsData[sectionId], - expectedErrors[sectionId] - ); + describe('on upload complete', () => { + beforeEach(() => { + sectionsServiceStub.isSectionType.and.callFake((submissionId, sectionId, sectionType) => { + return observableOf(sectionId === 'upload') + }); + compAsAny.uploadEnabled = observableOf(true); }); - expect(notificationsServiceStub.success).toHaveBeenCalled(); + it('should show a success notification and call updateSectionData if successful', () => { + const expectedErrors: any = mockUploadResponse1ParsedErrors; + fixture.detectChanges(); - }); + comp.onCompleteItem(Object.assign({}, uploadRestResponse, { sections: mockSectionsData })); - it('should show an error notification and call updateSectionData on upload complete', () => { + Object.keys(mockSectionsData).forEach((sectionId) => { + expect(sectionsServiceStub.updateSectionData).toHaveBeenCalledWith( + submissionId, + sectionId, + mockSectionsData[sectionId], + expectedErrors[sectionId] + ); + }); - const responseErrors = mockUploadResponse2Errors; + expect(notificationsServiceStub.success).toHaveBeenCalled(); - const expectedErrors: any = mockUploadResponse2ParsedErrors; - compAsAny.uploadEnabled = observableOf(true); - fixture.detectChanges(); - - comp.onCompleteItem(Object.assign({}, uploadRestResponse, { - sections: mockSectionsData, - errors: responseErrors.errors - })); - - Object.keys(mockSectionsData).forEach((sectionId) => { - expect(sectionsServiceStub.updateSectionData).toHaveBeenCalledWith( - submissionId, - sectionId, - mockSectionsData[sectionId], - expectedErrors[sectionId] - ); }); - expect(notificationsServiceStub.success).not.toHaveBeenCalled(); + it('should show an error notification and call updateSectionData if unsuccessful', () => { + const responseErrors = mockUploadResponse2Errors; + const expectedErrors: any = mockUploadResponse2ParsedErrors; + fixture.detectChanges(); + comp.onCompleteItem(Object.assign({}, uploadRestResponse, { + sections: mockSectionsData, + errors: responseErrors.errors + })); + + Object.keys(mockSectionsData).forEach((sectionId) => { + expect(sectionsServiceStub.updateSectionData).toHaveBeenCalledWith( + submissionId, + sectionId, + mockSectionsData[sectionId], + expectedErrors[sectionId] + ); + }); + + expect(notificationsServiceStub.success).not.toHaveBeenCalled(); + + }); }); - }); }); @@ -208,7 +210,6 @@ class TestComponent { submissionId = mockSubmissionId; collectionId = mockSubmissionCollectionId; - sectionId = 'upload'; uploadFilesOptions = Object.assign(new UploaderOptions(), { url: '', authToken: null, From a3ba4e59b3954393e1ac77c3453a5a1d602480c6 Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Fri, 18 Jun 2021 13:01:07 +0200 Subject: [PATCH 13/16] Fix lint issues --- .../submission-upload-files.component.spec.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/app/submission/form/submission-upload-files/submission-upload-files.component.spec.ts b/src/app/submission/form/submission-upload-files/submission-upload-files.component.spec.ts index cc7d0e8484..823cbb5d82 100644 --- a/src/app/submission/form/submission-upload-files/submission-upload-files.component.spec.ts +++ b/src/app/submission/form/submission-upload-files/submission-upload-files.component.spec.ts @@ -150,9 +150,7 @@ describe('SubmissionUploadFilesComponent Component', () => { describe('on upload complete', () => { beforeEach(() => { - sectionsServiceStub.isSectionType.and.callFake((submissionId, sectionId, sectionType) => { - return observableOf(sectionId === 'upload') - }); + sectionsServiceStub.isSectionType.and.callFake((_, sectionId, __) => observableOf(sectionId === 'upload')); compAsAny.uploadEnabled = observableOf(true); }); From 7670ba8a43abf474aeb1b531b32336a4c87b0f04 Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Fri, 18 Jun 2021 13:59:39 +0200 Subject: [PATCH 14/16] Fix duplicate notifications --- .../submission-upload-files.component.ts | 24 ++++++++++--------- .../submission/sections/sections.service.ts | 4 ++-- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/app/submission/form/submission-upload-files/submission-upload-files.component.ts b/src/app/submission/form/submission-upload-files/submission-upload-files.component.ts index e49127e80f..b1b5051458 100644 --- a/src/app/submission/form/submission-upload-files/submission-upload-files.component.ts +++ b/src/app/submission/form/submission-upload-files/submission-upload-files.component.ts @@ -2,7 +2,7 @@ import { Component, Input, OnChanges } from '@angular/core'; import { TranslateService } from '@ngx-translate/core'; import { Observable, of as observableOf, Subscription } from 'rxjs'; -import { first } from 'rxjs/operators'; +import { first, take } from 'rxjs/operators'; import { SectionsService } from '../../sections/sections.service'; import { hasValue, isEmpty, isNotEmpty } from '../../../shared/empty.util'; @@ -131,16 +131,18 @@ export class SubmissionUploadFilesComponent implements OnChanges { .forEach((sectionId) => { const sectionData = normalizeSectionData(sections[sectionId]); const sectionErrors = errorsList[sectionId]; - this.sectionService.isSectionType(this.submissionId, sectionId, SectionsType.Upload).subscribe((isUpload) => { - if (isUpload) { - // Look for errors on upload - if ((isEmpty(sectionErrors))) { - this.notificationsService.success(null, this.translate.get('submission.sections.upload.upload-successful')); - } else { - this.notificationsService.error(null, this.translate.get('submission.sections.upload.upload-failed')); - } - } - }); + this.sectionService.isSectionType(this.submissionId, sectionId, SectionsType.Upload) + .pipe(take(1)) + .subscribe((isUpload) => { + if (isUpload) { + // Look for errors on upload + if ((isEmpty(sectionErrors))) { + this.notificationsService.success(null, this.translate.get('submission.sections.upload.upload-successful')); + } else { + this.notificationsService.error(null, this.translate.get('submission.sections.upload.upload-failed')); + } + } + }); this.sectionService.updateSectionData(this.submissionId, sectionId, sectionData, sectionErrors); }); } diff --git a/src/app/submission/sections/sections.service.ts b/src/app/submission/sections/sections.service.ts index afe5dde570..05e9a96267 100644 --- a/src/app/submission/sections/sections.service.ts +++ b/src/app/submission/sections/sections.service.ts @@ -338,8 +338,8 @@ export class SectionsService { return this.store.select(submissionObjectFromIdSelector(submissionId)).pipe( filter((submissionState: SubmissionObjectEntry) => isNotUndefined(submissionState)), map((submissionState: SubmissionObjectEntry) => { - return isNotUndefined(submissionState.sections) && isNotUndefined(submissionState.sections[sectionId] - && submissionState.sections[sectionId].sectionType === sectionType ); + return isNotUndefined(submissionState.sections) && isNotUndefined(submissionState.sections[sectionId]) + && submissionState.sections[sectionId].sectionType === sectionType; }), distinctUntilChanged()); } From 8736c0b572064f6b19c1229b672506d9d2d2cbab Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Fri, 2 Jul 2021 18:12:18 +0200 Subject: [PATCH 15/16] Add unit tests for SectionsService#isSectionType --- .../sections/sections.service.spec.ts | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/app/submission/sections/sections.service.spec.ts b/src/app/submission/sections/sections.service.spec.ts index 0d63beeaea..d199f1beae 100644 --- a/src/app/submission/sections/sections.service.spec.ts +++ b/src/app/submission/sections/sections.service.spec.ts @@ -334,6 +334,38 @@ describe('SectionsService test suite', () => { }); }); + describe('isSectionType', () => { + it('should return true if the section matches the provided type', () => { + store.select.and.returnValue(observableOf(submissionState)); + + const expected = cold('(b|)', { + b: true + }); + + expect(service.isSectionType(submissionId, 'upload', SectionsType.Upload)).toBeObservable(expected); + }); + + it('should return false if the section doesn\'t match the provided type', () => { + store.select.and.returnValue(observableOf(submissionState)); + + const expected = cold('(b|)', { + b: false + }); + + expect(service.isSectionType(submissionId, sectionId, SectionsType.Upload)).toBeObservable(expected); + }); + + it('should return false if the provided sectionId doesn\'t exist', () => { + store.select.and.returnValue(observableOf(submissionState)); + + const expected = cold('(b|)', { + b: false + }); + + expect(service.isSectionType(submissionId, 'no-such-id', SectionsType.Upload)).toBeObservable(expected); + }); + }); + describe('addSection', () => { it('should dispatch a new EnableSectionAction a move target to new section', () => { From bfcff12499b687417e58a6dc81c7d6c3842cbc83 Mon Sep 17 00:00:00 2001 From: Yura Bondarenko Date: Wed, 7 Jul 2021 11:35:12 +0200 Subject: [PATCH 16/16] Fix hardcoded reference to 'upload' section --- .../upload/file/section-upload-file.component.ts | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/app/submission/sections/upload/file/section-upload-file.component.ts b/src/app/submission/sections/upload/file/section-upload-file.component.ts index d4c901b290..c18796d7f3 100644 --- a/src/app/submission/sections/upload/file/section-upload-file.component.ts +++ b/src/app/submission/sections/upload/file/section-upload-file.component.ts @@ -291,14 +291,13 @@ export class SubmissionSectionUploadFileComponent implements OnChanges, OnInit { this.pathCombiner.subRootElement); }) ).subscribe((result: SubmissionObject[]) => { - if (result[0].sections.upload) { - Object.keys((result[0].sections.upload as WorkspaceitemSectionUploadObject).files) - .filter((key) => (result[0].sections.upload as WorkspaceitemSectionUploadObject).files[key].uuid === this.fileId) + if (result[0].sections[this.sectionId]) { + const uploadSection = (result[0].sections[this.sectionId] as WorkspaceitemSectionUploadObject); + Object.keys(uploadSection.files) + .filter((key) => uploadSection.files[key].uuid === this.fileId) .forEach((key) => this.uploadService.updateFileData( - this.submissionId, - this.sectionId, - this.fileId, - (result[0].sections.upload as WorkspaceitemSectionUploadObject).files[key])); + this.submissionId, this.sectionId, this.fileId, uploadSection.files[key]) + ); } this.switchMode(); }));