diff --git a/src/app/app-routing.module.ts b/src/app/app-routing.module.ts index b0317f68eab..d1f87903ac4 100644 --- a/src/app/app-routing.module.ts +++ b/src/app/app-routing.module.ts @@ -1,5 +1,6 @@ import { NgModule } from '@angular/core'; import { RouterModule } from '@angular/router'; +import { AuthBlockingGuard } from './core/auth/auth-blocking.guard'; import { PageNotFoundComponent } from './pagenotfound/pagenotfound.component'; import { AuthenticatedGuard } from './core/auth/authenticated.guard'; @@ -17,50 +18,54 @@ import { import { COLLECTION_MODULE_PATH } from './+collection-page/collection-page-routing-paths'; import { COMMUNITY_MODULE_PATH } from './+community-page/community-page-routing-paths'; import { ITEM_MODULE_PATH } from './+item-page/item-page-routing-paths'; +import { ReloadGuard } from './core/reload/reload.guard'; @NgModule({ imports: [ RouterModule.forRoot([ - { path: '', redirectTo: '/home', pathMatch: 'full' }, - { path: 'reload/:rnd', redirectTo: '/home', pathMatch: 'full' }, - { path: 'home', loadChildren: './+home-page/home-page.module#HomePageModule', data: { showBreadcrumbs: false } }, - { path: 'community-list', loadChildren: './community-list-page/community-list-page.module#CommunityListPageModule' }, - { path: 'id', loadChildren: './+lookup-by-id/lookup-by-id.module#LookupIdModule' }, - { path: 'handle', loadChildren: './+lookup-by-id/lookup-by-id.module#LookupIdModule' }, - { path: REGISTER_PATH, loadChildren: './register-page/register-page.module#RegisterPageModule' }, - { path: FORGOT_PASSWORD_PATH, loadChildren: './forgot-password/forgot-password.module#ForgotPasswordModule' }, - { path: COMMUNITY_MODULE_PATH, loadChildren: './+community-page/community-page.module#CommunityPageModule' }, - { path: COLLECTION_MODULE_PATH, loadChildren: './+collection-page/collection-page.module#CollectionPageModule' }, - { path: ITEM_MODULE_PATH, loadChildren: './+item-page/item-page.module#ItemPageModule' }, - { path: BITSTREAM_MODULE_PATH, loadChildren: './+bitstream-page/bitstream-page.module#BitstreamPageModule' }, - { - path: 'mydspace', - loadChildren: './+my-dspace-page/my-dspace-page.module#MyDSpacePageModule', - canActivate: [AuthenticatedGuard] - }, - { path: 'search', loadChildren: './+search-page/search-page-routing.module#SearchPageRoutingModule' }, - { path: 'browse', loadChildren: './+browse-by/browse-by.module#BrowseByModule'}, - { path: ADMIN_MODULE_PATH, loadChildren: './+admin/admin.module#AdminModule', canActivate: [SiteAdministratorGuard] }, - { path: 'login', loadChildren: './+login-page/login-page.module#LoginPageModule' }, - { path: 'logout', loadChildren: './+logout-page/logout-page.module#LogoutPageModule' }, - { path: 'submit', loadChildren: './+submit-page/submit-page.module#SubmitPageModule' }, - { path: 'import-external', loadChildren: './+import-external-page/import-external-page.module#ImportExternalPageModule' }, - { - path: 'workspaceitems', - loadChildren: './+workspaceitems-edit-page/workspaceitems-edit-page.module#WorkspaceitemsEditPageModule' - }, - { - path: WORKFLOW_ITEM_MODULE_PATH, - loadChildren: './+workflowitems-edit-page/workflowitems-edit-page.module#WorkflowItemsEditPageModule' - }, - { - path: PROFILE_MODULE_PATH, - loadChildren: './profile-page/profile-page.module#ProfilePageModule', canActivate: [AuthenticatedGuard] - }, - { path: 'processes', loadChildren: './process-page/process-page.module#ProcessPageModule', canActivate: [AuthenticatedGuard] }, - { path: UNAUTHORIZED_PATH, component: UnauthorizedComponent }, - { path: '**', pathMatch: 'full', component: PageNotFoundComponent }, - ], + { path: '', canActivate: [AuthBlockingGuard], + children: [ + { path: '', redirectTo: '/home', pathMatch: 'full' }, + { path: 'reload/:rnd', component: PageNotFoundComponent, pathMatch: 'full', canActivate: [ReloadGuard] }, + { path: 'home', loadChildren: './+home-page/home-page.module#HomePageModule', data: { showBreadcrumbs: false } }, + { path: 'community-list', loadChildren: './community-list-page/community-list-page.module#CommunityListPageModule' }, + { path: 'id', loadChildren: './+lookup-by-id/lookup-by-id.module#LookupIdModule' }, + { path: 'handle', loadChildren: './+lookup-by-id/lookup-by-id.module#LookupIdModule' }, + { path: REGISTER_PATH, loadChildren: './register-page/register-page.module#RegisterPageModule' }, + { path: FORGOT_PASSWORD_PATH, loadChildren: './forgot-password/forgot-password.module#ForgotPasswordModule' }, + { path: COMMUNITY_MODULE_PATH, loadChildren: './+community-page/community-page.module#CommunityPageModule' }, + { path: COLLECTION_MODULE_PATH, loadChildren: './+collection-page/collection-page.module#CollectionPageModule' }, + { path: ITEM_MODULE_PATH, loadChildren: './+item-page/item-page.module#ItemPageModule' }, + { path: BITSTREAM_MODULE_PATH, loadChildren: './+bitstream-page/bitstream-page.module#BitstreamPageModule' }, + { + path: 'mydspace', + loadChildren: './+my-dspace-page/my-dspace-page.module#MyDSpacePageModule', + canActivate: [AuthenticatedGuard] + }, + { path: 'search', loadChildren: './+search-page/search-page-routing.module#SearchPageRoutingModule' }, + { path: 'browse', loadChildren: './+browse-by/browse-by.module#BrowseByModule'}, + { path: ADMIN_MODULE_PATH, loadChildren: './+admin/admin.module#AdminModule', canActivate: [SiteAdministratorGuard] }, + { path: 'login', loadChildren: './+login-page/login-page.module#LoginPageModule' }, + { path: 'logout', loadChildren: './+logout-page/logout-page.module#LogoutPageModule' }, + { path: 'submit', loadChildren: './+submit-page/submit-page.module#SubmitPageModule' }, + { path: 'import-external', loadChildren: './+import-external-page/import-external-page.module#ImportExternalPageModule' }, + { + path: 'workspaceitems', + loadChildren: './+workspaceitems-edit-page/workspaceitems-edit-page.module#WorkspaceitemsEditPageModule' + }, + { + path: WORKFLOW_ITEM_MODULE_PATH, + loadChildren: './+workflowitems-edit-page/workflowitems-edit-page.module#WorkflowItemsEditPageModule' + }, + { + path: PROFILE_MODULE_PATH, + loadChildren: './profile-page/profile-page.module#ProfilePageModule', canActivate: [AuthenticatedGuard] + }, + { path: 'processes', loadChildren: './process-page/process-page.module#ProcessPageModule', canActivate: [AuthenticatedGuard] }, + { path: UNAUTHORIZED_PATH, component: UnauthorizedComponent }, + { path: '**', pathMatch: 'full', component: PageNotFoundComponent }, + ]} + ], { onSameUrlNavigation: 'reload', }) diff --git a/src/app/app.component.html b/src/app/app.component.html index 8656970f319..fa534855e79 100644 --- a/src/app/app.component.html +++ b/src/app/app.component.html @@ -1,4 +1,4 @@ -
+
+ +
+ diff --git a/src/app/app.component.scss b/src/app/app.component.scss index 7793b7529c7..b18e7e1402c 100644 --- a/src/app/app.component.scss +++ b/src/app/app.component.scss @@ -47,3 +47,7 @@ ds-admin-sidebar { position: fixed; z-index: $sidebar-z-index; } + +.ds-full-screen-loader { + height: 100vh; +} diff --git a/src/app/app.component.spec.ts b/src/app/app.component.spec.ts index da3cf9537b1..31507831bea 100644 --- a/src/app/app.component.spec.ts +++ b/src/app/app.component.spec.ts @@ -1,9 +1,8 @@ +import * as ngrx from '@ngrx/store'; import { async, ComponentFixture, inject, TestBed } from '@angular/core/testing'; -import { CUSTOM_ELEMENTS_SCHEMA, DebugElement } from '@angular/core'; +import { CUSTOM_ELEMENTS_SCHEMA } from '@angular/core'; import { CommonModule } from '@angular/common'; -import { By } from '@angular/platform-browser'; import { ActivatedRoute, Router } from '@angular/router'; - import { TranslateLoader, TranslateModule } from '@ngx-translate/core'; import { Store, StoreModule } from '@ngrx/store'; import { Angulartics2GoogleAnalytics } from 'angulartics2/ga'; @@ -32,11 +31,11 @@ import { RouterMock } from './shared/mocks/router.mock'; import { Angulartics2DSpace } from './statistics/angulartics/dspace-provider'; import { storeModuleConfig } from './app.reducer'; import { LocaleService } from './core/locale/locale.service'; +import { authReducer } from './core/auth/auth.reducer'; +import { cold } from 'jasmine-marbles'; let comp: AppComponent; let fixture: ComponentFixture; -let de: DebugElement; -let el: HTMLElement; const menuService = new MenuServiceStub(); describe('App component', () => { @@ -52,7 +51,7 @@ describe('App component', () => { return TestBed.configureTestingModule({ imports: [ CommonModule, - StoreModule.forRoot({}, storeModuleConfig), + StoreModule.forRoot(authReducer, storeModuleConfig), TranslateModule.forRoot({ loader: { provide: TranslateLoader, @@ -82,12 +81,19 @@ describe('App component', () => { // synchronous beforeEach beforeEach(() => { - fixture = TestBed.createComponent(AppComponent); + spyOnProperty(ngrx, 'select').and.callFake(() => { + return () => { + return () => cold('a', { + a: { + core: { auth: { loading: false } } + } + }) + }; + }); + fixture = TestBed.createComponent(AppComponent); comp = fixture.componentInstance; // component test instance - // query for the
by CSS element selector - de = fixture.debugElement.query(By.css('div.outer-wrapper')); - el = de.nativeElement; + fixture.detectChanges(); }); it('should create component', inject([AppComponent], (app: AppComponent) => { diff --git a/src/app/app.component.ts b/src/app/app.component.ts index 10f81a9adce..fae2df32206 100644 --- a/src/app/app.component.ts +++ b/src/app/app.component.ts @@ -1,4 +1,4 @@ -import { delay, filter, map, take } from 'rxjs/operators'; +import { delay, map, distinctUntilChanged } from 'rxjs/operators'; import { AfterViewInit, ChangeDetectionStrategy, @@ -19,7 +19,7 @@ import { MetadataService } from './core/metadata/metadata.service'; import { HostWindowResizeAction } from './shared/host-window.actions'; import { HostWindowState } from './shared/search/host-window.reducer'; import { NativeWindowRef, NativeWindowService } from './core/services/window.service'; -import { isAuthenticated } from './core/auth/selectors'; +import { isAuthenticationBlocking } from './core/auth/selectors'; import { AuthService } from './core/auth/auth.service'; import { CSSVariableService } from './shared/sass-helper/sass-helper.service'; import { MenuService } from './shared/menu/menu.service'; @@ -52,6 +52,11 @@ export class AppComponent implements OnInit, AfterViewInit { notificationOptions = environment.notifications; models; + /** + * Whether or not the authentication is currently blocking the UI + */ + isNotAuthBlocking$: Observable; + constructor( @Inject(NativeWindowService) private _window: NativeWindowRef, private translate: TranslateService, @@ -89,16 +94,15 @@ export class AppComponent implements OnInit, AfterViewInit { } ngOnInit() { + this.isNotAuthBlocking$ = this.store.pipe(select(isAuthenticationBlocking)).pipe( + map((isBlocking: boolean) => isBlocking === false), + distinctUntilChanged() + ); const env: string = environment.production ? 'Production' : 'Development'; const color: string = environment.production ? 'red' : 'green'; console.info(`Environment: %c${env}`, `color: ${color}; font-weight: bold;`); this.dispatchWindowSize(this._window.nativeWindow.innerWidth, this._window.nativeWindow.innerHeight); - // Whether is not authenticathed try to retrieve a possible stored auth token - this.store.pipe(select(isAuthenticated), - take(1), - filter((authenticated) => !authenticated) - ).subscribe((authenticated) => this.authService.checkAuthenticationToken()); this.sidebarVisible = this.menuService.isMenuVisible(MenuID.ADMIN); this.collapsedSidebarWidth = this.cssService.getVariable('collapsedSidebarWidth'); diff --git a/src/app/app.module.ts b/src/app/app.module.ts index 33454ed6c59..f1cdd5f2e5d 100755 --- a/src/app/app.module.ts +++ b/src/app/app.module.ts @@ -1,11 +1,11 @@ import { APP_BASE_HREF, CommonModule } from '@angular/common'; import { HttpClientModule } from '@angular/common/http'; -import { NgModule } from '@angular/core'; +import { APP_INITIALIZER, NgModule } from '@angular/core'; import { NgbModule } from '@ng-bootstrap/ng-bootstrap'; import { EffectsModule } from '@ngrx/effects'; import { RouterStateSerializer, StoreRouterConnectingModule } from '@ngrx/router-store'; -import { MetaReducer, StoreModule, USER_PROVIDED_META_REDUCERS } from '@ngrx/store'; +import { MetaReducer, Store, StoreModule, USER_PROVIDED_META_REDUCERS } from '@ngrx/store'; import { StoreDevtoolsModule } from '@ngrx/store-devtools'; import { DYNAMIC_MATCHER_PROVIDERS } from '@ng-dynamic-forms/core'; import { TranslateModule } from '@ngx-translate/core'; @@ -21,6 +21,7 @@ import { AppComponent } from './app.component'; import { appEffects } from './app.effects'; import { appMetaReducers, debugMetaReducers } from './app.metareducers'; import { appReducers, AppState, storeModuleConfig } from './app.reducer'; +import { CheckAuthenticationTokenAction } from './core/auth/auth.actions'; import { CoreModule } from './core/core.module'; import { ClientCookieService } from './core/services/client-cookie.service'; @@ -91,6 +92,15 @@ const PROVIDERS = [ useClass: DSpaceRouterStateSerializer }, ClientCookieService, + // Check the authentication token when the app initializes + { + provide: APP_INITIALIZER, + useFactory: (store: Store,) => { + return () => store.dispatch(new CheckAuthenticationTokenAction()); + }, + deps: [ Store ], + multi: true + }, ...DYNAMIC_MATCHER_PROVIDERS, ]; diff --git a/src/app/core/auth/auth-blocking.guard.spec.ts b/src/app/core/auth/auth-blocking.guard.spec.ts new file mode 100644 index 00000000000..2a89b01a855 --- /dev/null +++ b/src/app/core/auth/auth-blocking.guard.spec.ts @@ -0,0 +1,62 @@ +import { Store } from '@ngrx/store'; +import * as ngrx from '@ngrx/store'; +import { cold, getTestScheduler, initTestScheduler, resetTestScheduler } from 'jasmine-marbles/es6'; +import { of as observableOf } from 'rxjs'; +import { AppState } from '../../app.reducer'; +import { AuthBlockingGuard } from './auth-blocking.guard'; + +describe('AuthBlockingGuard', () => { + let guard: AuthBlockingGuard; + beforeEach(() => { + guard = new AuthBlockingGuard(new Store(undefined, undefined, undefined)); + initTestScheduler(); + }); + + afterEach(() => { + getTestScheduler().flush(); + resetTestScheduler(); + }); + + describe(`canActivate`, () => { + + describe(`when authState.loading is undefined`, () => { + beforeEach(() => { + spyOnProperty(ngrx, 'select').and.callFake(() => { + return () => { + return () => observableOf(undefined); + }; + }) + }); + it(`should not emit anything`, () => { + expect(guard.canActivate()).toBeObservable(cold('|')); + }); + }); + + describe(`when authState.loading is true`, () => { + beforeEach(() => { + spyOnProperty(ngrx, 'select').and.callFake(() => { + return () => { + return () => observableOf(true); + }; + }) + }); + it(`should not emit anything`, () => { + expect(guard.canActivate()).toBeObservable(cold('|')); + }); + }); + + describe(`when authState.loading is false`, () => { + beforeEach(() => { + spyOnProperty(ngrx, 'select').and.callFake(() => { + return () => { + return () => observableOf(false); + }; + }) + }); + it(`should succeed`, () => { + expect(guard.canActivate()).toBeObservable(cold('(a|)', { a: true })); + }); + }); + }); + +}); diff --git a/src/app/core/auth/auth-blocking.guard.ts b/src/app/core/auth/auth-blocking.guard.ts new file mode 100644 index 00000000000..9054f66f8b1 --- /dev/null +++ b/src/app/core/auth/auth-blocking.guard.ts @@ -0,0 +1,34 @@ +import { Injectable } from '@angular/core'; +import { CanActivate } from '@angular/router'; +import { select, Store } from '@ngrx/store'; +import { Observable } from 'rxjs'; +import { distinctUntilChanged, filter, map, take } from 'rxjs/operators'; +import { AppState } from '../../app.reducer'; +import { isAuthenticationBlocking } from './selectors'; + +/** + * A guard that blocks the loading of any + * route until the authentication status has loaded. + * To ensure all rest requests get the correct auth header. + */ +@Injectable({ + providedIn: 'root' +}) +export class AuthBlockingGuard implements CanActivate { + + constructor(private store: Store) { + } + + /** + * True when the authentication isn't blocking everything + */ + canActivate(): Observable { + return this.store.pipe(select(isAuthenticationBlocking)).pipe( + map((isBlocking: boolean) => isBlocking === false), + distinctUntilChanged(), + filter((finished: boolean) => finished === true), + take(1), + ); + } + +} diff --git a/src/app/core/auth/auth.actions.ts b/src/app/core/auth/auth.actions.ts index be4bdf2a265..f80be890349 100644 --- a/src/app/core/auth/auth.actions.ts +++ b/src/app/core/auth/auth.actions.ts @@ -34,6 +34,7 @@ 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') }; /* tslint:disable:max-classes-per-file */ @@ -335,6 +336,20 @@ export class SetRedirectUrlAction implements Action { } } +/** + * Start loading for a hard redirect + * @class StartHardRedirectLoadingAction + * @implements {Action} + */ +export class RedirectAfterLoginSuccessAction implements Action { + public type: string = AuthActionTypes.REDIRECT_AFTER_LOGIN_SUCCESS; + payload: string; + + constructor(url: string) { + this.payload = url; + } +} + /** * Retrieve the authenticated eperson. * @class RetrieveAuthenticatedEpersonAction @@ -402,8 +417,8 @@ export type AuthActions | RetrieveAuthMethodsSuccessAction | RetrieveAuthMethodsErrorAction | RetrieveTokenAction - | ResetAuthenticationMessagesAction | RetrieveAuthenticatedEpersonAction | RetrieveAuthenticatedEpersonErrorAction | RetrieveAuthenticatedEpersonSuccessAction - | SetRedirectUrlAction; + | SetRedirectUrlAction + | RedirectAfterLoginSuccessAction; diff --git a/src/app/core/auth/auth.effects.ts b/src/app/core/auth/auth.effects.ts index 37ef3b79bc5..ab18dcb508d 100644 --- a/src/app/core/auth/auth.effects.ts +++ b/src/app/core/auth/auth.effects.ts @@ -27,6 +27,7 @@ import { CheckAuthenticationTokenCookieAction, LogOutErrorAction, LogOutSuccessAction, + RedirectAfterLoginSuccessAction, RefreshTokenAction, RefreshTokenErrorAction, RefreshTokenSuccessAction, @@ -79,7 +80,26 @@ export class AuthEffects { public authenticatedSuccess$: Observable = this.actions$.pipe( ofType(AuthActionTypes.AUTHENTICATED_SUCCESS), tap((action: AuthenticatedSuccessAction) => this.authService.storeToken(action.payload.authToken)), - map((action: AuthenticatedSuccessAction) => new RetrieveAuthenticatedEpersonAction(action.payload.userHref)) + switchMap((action: AuthenticatedSuccessAction) => this.authService.getRedirectUrl().pipe( + take(1), + map((redirectUrl: string) => [action, redirectUrl]) + )), + map(([action, redirectUrl]: [AuthenticatedSuccessAction, string]) => { + if (hasValue(redirectUrl)) { + return new RedirectAfterLoginSuccessAction(redirectUrl); + } else { + return new RetrieveAuthenticatedEpersonAction(action.payload.userHref); + } + }) + ); + + @Effect({ dispatch: false }) + public redirectAfterLoginSuccess$: Observable = this.actions$.pipe( + ofType(AuthActionTypes.REDIRECT_AFTER_LOGIN_SUCCESS), + tap((action: RedirectAfterLoginSuccessAction) => { + this.authService.clearRedirectUrl(); + this.authService.navigateToRedirectUrl(action.payload); + }) ); // It means "reacts to this action but don't send another" @@ -201,13 +221,6 @@ export class AuthEffects { tap(() => this.authService.refreshAfterLogout()) ); - @Effect({ dispatch: false }) - public redirectToLogin$: Observable = this.actions$ - .pipe(ofType(AuthActionTypes.REDIRECT_AUTHENTICATION_REQUIRED), - tap(() => this.authService.removeToken()), - tap(() => this.authService.redirectToLogin()) - ); - @Effect({ dispatch: false }) public redirectToLoginTokenExpired$: Observable = this.actions$ .pipe( diff --git a/src/app/core/auth/auth.reducer.spec.ts b/src/app/core/auth/auth.reducer.spec.ts index cf934a7f47b..4c6f1e2a256 100644 --- a/src/app/core/auth/auth.reducer.spec.ts +++ b/src/app/core/auth/auth.reducer.spec.ts @@ -42,6 +42,7 @@ describe('authReducer', () => { initialState = { authenticated: false, loaded: false, + blocking: true, loading: false, }; const action = new AuthenticateAction('user', 'password'); @@ -49,6 +50,7 @@ describe('authReducer', () => { state = { authenticated: false, loaded: false, + blocking: true, error: undefined, loading: true, info: undefined @@ -62,6 +64,7 @@ describe('authReducer', () => { authenticated: false, loaded: false, error: undefined, + blocking: true, loading: true, info: undefined }; @@ -76,6 +79,7 @@ describe('authReducer', () => { authenticated: false, loaded: false, error: undefined, + blocking: true, loading: true, info: undefined }; @@ -84,6 +88,7 @@ describe('authReducer', () => { state = { authenticated: false, loaded: false, + blocking: false, loading: false, info: undefined, authToken: undefined, @@ -96,6 +101,7 @@ describe('authReducer', () => { it('should properly set the state, in response to a AUTHENTICATED action', () => { initialState = { authenticated: false, + blocking: false, loaded: false, error: undefined, loading: true, @@ -103,8 +109,15 @@ describe('authReducer', () => { }; const action = new AuthenticatedAction(mockTokenInfo); const newState = authReducer(initialState, action); - - expect(newState).toEqual(initialState); + state = { + authenticated: false, + blocking: true, + loaded: false, + error: undefined, + loading: true, + info: undefined + }; + expect(newState).toEqual(state); }); it('should properly set the state, in response to a AUTHENTICATED_SUCCESS action', () => { @@ -112,6 +125,7 @@ describe('authReducer', () => { authenticated: false, loaded: false, error: undefined, + blocking: true, loading: true, info: undefined }; @@ -122,6 +136,7 @@ describe('authReducer', () => { authToken: mockTokenInfo, loaded: false, error: undefined, + blocking: true, loading: true, info: undefined }; @@ -133,6 +148,7 @@ describe('authReducer', () => { authenticated: false, loaded: false, error: undefined, + blocking: true, loading: true, info: undefined }; @@ -143,6 +159,7 @@ describe('authReducer', () => { authToken: undefined, error: 'Test error message', loaded: true, + blocking: false, loading: false, info: undefined }; @@ -153,6 +170,7 @@ describe('authReducer', () => { initialState = { authenticated: false, loaded: false, + blocking: false, loading: false, }; const action = new CheckAuthenticationTokenAction(); @@ -160,6 +178,7 @@ describe('authReducer', () => { state = { authenticated: false, loaded: false, + blocking: true, loading: true, }; expect(newState).toEqual(state); @@ -169,6 +188,7 @@ describe('authReducer', () => { initialState = { authenticated: false, loaded: false, + blocking: false, loading: true, }; const action = new CheckAuthenticationTokenCookieAction(); @@ -176,6 +196,7 @@ describe('authReducer', () => { state = { authenticated: false, loaded: false, + blocking: true, loading: true, }; expect(newState).toEqual(state); @@ -187,6 +208,7 @@ describe('authReducer', () => { authToken: mockTokenInfo, loaded: true, error: undefined, + blocking: false, loading: false, info: undefined, userId: EPersonMock.id @@ -204,6 +226,7 @@ describe('authReducer', () => { authToken: mockTokenInfo, loaded: true, error: undefined, + blocking: false, loading: false, info: undefined, userId: EPersonMock.id @@ -216,7 +239,8 @@ describe('authReducer', () => { authToken: undefined, error: undefined, loaded: false, - loading: false, + blocking: true, + loading: true, info: undefined, refreshing: false, userId: undefined @@ -230,6 +254,7 @@ describe('authReducer', () => { authToken: mockTokenInfo, loaded: true, error: undefined, + blocking: false, loading: false, info: undefined, userId: EPersonMock.id @@ -242,6 +267,7 @@ describe('authReducer', () => { authToken: mockTokenInfo, loaded: true, error: 'Test error message', + blocking: false, loading: false, info: undefined, userId: EPersonMock.id @@ -255,6 +281,7 @@ describe('authReducer', () => { authToken: mockTokenInfo, loaded: false, error: undefined, + blocking: true, loading: true, info: undefined }; @@ -265,6 +292,7 @@ describe('authReducer', () => { authToken: mockTokenInfo, loaded: true, error: undefined, + blocking: false, loading: false, info: undefined, userId: EPersonMock.id @@ -277,6 +305,7 @@ describe('authReducer', () => { authenticated: false, loaded: false, error: undefined, + blocking: true, loading: true, info: undefined }; @@ -287,6 +316,7 @@ describe('authReducer', () => { authToken: undefined, error: 'Test error message', loaded: true, + blocking: false, loading: false, info: undefined }; @@ -299,6 +329,7 @@ describe('authReducer', () => { authToken: mockTokenInfo, loaded: true, error: undefined, + blocking: false, loading: false, info: undefined, userId: EPersonMock.id @@ -311,6 +342,7 @@ describe('authReducer', () => { authToken: mockTokenInfo, loaded: true, error: undefined, + blocking: false, loading: false, info: undefined, userId: EPersonMock.id, @@ -325,6 +357,7 @@ describe('authReducer', () => { authToken: mockTokenInfo, loaded: true, error: undefined, + blocking: false, loading: false, info: undefined, userId: EPersonMock.id, @@ -338,6 +371,7 @@ describe('authReducer', () => { authToken: newTokenInfo, loaded: true, error: undefined, + blocking: false, loading: false, info: undefined, userId: EPersonMock.id, @@ -352,6 +386,7 @@ describe('authReducer', () => { authToken: mockTokenInfo, loaded: true, error: undefined, + blocking: false, loading: false, info: undefined, userId: EPersonMock.id, @@ -364,6 +399,7 @@ describe('authReducer', () => { authToken: undefined, error: undefined, loaded: false, + blocking: false, loading: false, info: undefined, refreshing: false, @@ -378,6 +414,7 @@ describe('authReducer', () => { authToken: mockTokenInfo, loaded: true, error: undefined, + blocking: false, loading: false, info: undefined, userId: EPersonMock.id @@ -387,6 +424,7 @@ describe('authReducer', () => { authenticated: false, authToken: undefined, loaded: false, + blocking: false, loading: false, error: undefined, info: 'Message', @@ -410,6 +448,7 @@ describe('authReducer', () => { initialState = { authenticated: false, loaded: false, + blocking: false, loading: false, }; const action = new AddAuthenticationMessageAction('Message'); @@ -417,6 +456,7 @@ describe('authReducer', () => { state = { authenticated: false, loaded: false, + blocking: false, loading: false, info: 'Message' }; @@ -427,6 +467,7 @@ describe('authReducer', () => { initialState = { authenticated: false, loaded: false, + blocking: false, loading: false, error: 'Error', info: 'Message' @@ -436,6 +477,7 @@ describe('authReducer', () => { state = { authenticated: false, loaded: false, + blocking: false, loading: false, error: undefined, info: undefined @@ -447,6 +489,7 @@ describe('authReducer', () => { initialState = { authenticated: false, loaded: false, + blocking: false, loading: false }; const action = new SetRedirectUrlAction('redirect.url'); @@ -454,6 +497,7 @@ describe('authReducer', () => { state = { authenticated: false, loaded: false, + blocking: false, loading: false, redirectUrl: 'redirect.url' }; @@ -464,6 +508,7 @@ describe('authReducer', () => { initialState = { authenticated: false, loaded: false, + blocking: false, loading: false, authMethods: [] }; @@ -472,6 +517,7 @@ describe('authReducer', () => { state = { authenticated: false, loaded: false, + blocking: true, loading: true, authMethods: [] }; @@ -482,6 +528,7 @@ describe('authReducer', () => { initialState = { authenticated: false, loaded: false, + blocking: true, loading: true, authMethods: [] }; @@ -494,6 +541,7 @@ describe('authReducer', () => { state = { authenticated: false, loaded: false, + blocking: false, loading: false, authMethods: authMethods }; @@ -504,6 +552,7 @@ describe('authReducer', () => { initialState = { authenticated: false, loaded: false, + blocking: true, loading: true, authMethods: [] }; @@ -513,6 +562,7 @@ describe('authReducer', () => { state = { authenticated: false, loaded: false, + blocking: false, loading: false, authMethods: [new AuthMethod(AuthMethodType.Password)] }; diff --git a/src/app/core/auth/auth.reducer.ts b/src/app/core/auth/auth.reducer.ts index 34c8fe2b417..6d5635f263b 100644 --- a/src/app/core/auth/auth.reducer.ts +++ b/src/app/core/auth/auth.reducer.ts @@ -39,6 +39,10 @@ export interface AuthState { // true when loading loading: boolean; + // true when everything else should wait for authorization + // to complete + blocking: boolean; + // info message info?: string; @@ -62,6 +66,7 @@ export interface AuthState { const initialState: AuthState = { authenticated: false, loaded: false, + blocking: true, loading: false, authMethods: [] }; @@ -86,7 +91,8 @@ export function authReducer(state: any = initialState, action: AuthActions): Aut case AuthActionTypes.CHECK_AUTHENTICATION_TOKEN: case AuthActionTypes.CHECK_AUTHENTICATION_TOKEN_COOKIE: return Object.assign({}, state, { - loading: true + loading: true, + blocking: true }); case AuthActionTypes.AUTHENTICATED_ERROR: @@ -96,6 +102,7 @@ export function authReducer(state: any = initialState, action: AuthActions): Aut authToken: undefined, error: (action as AuthenticationErrorAction).payload.message, loaded: true, + blocking: false, loading: false }); @@ -110,6 +117,7 @@ export function authReducer(state: any = initialState, action: AuthActions): Aut loaded: true, error: undefined, loading: false, + blocking: false, info: undefined, userId: (action as RetrieveAuthenticatedEpersonSuccessAction).payload }); @@ -119,6 +127,7 @@ export function authReducer(state: any = initialState, action: AuthActions): Aut authenticated: false, authToken: undefined, error: (action as AuthenticationErrorAction).payload.message, + blocking: false, loading: false }); @@ -132,25 +141,39 @@ export function authReducer(state: any = initialState, action: AuthActions): Aut error: (action as LogOutErrorAction).payload.message }); - case AuthActionTypes.LOG_OUT_SUCCESS: case AuthActionTypes.REFRESH_TOKEN_ERROR: return Object.assign({}, state, { authenticated: false, authToken: undefined, error: undefined, loaded: false, + blocking: false, loading: false, info: undefined, refreshing: false, userId: undefined }); + case AuthActionTypes.LOG_OUT_SUCCESS: + return Object.assign({}, state, { + authenticated: false, + authToken: undefined, + error: undefined, + loaded: false, + blocking: true, + loading: true, + info: undefined, + refreshing: false, + userId: undefined + }); + case AuthActionTypes.REDIRECT_AUTHENTICATION_REQUIRED: case AuthActionTypes.REDIRECT_TOKEN_EXPIRED: return Object.assign({}, state, { authenticated: false, authToken: undefined, loaded: false, + blocking: false, loading: false, info: (action as RedirectWhenTokenExpiredAction as RedirectWhenAuthenticationIsRequiredAction).payload, userId: undefined @@ -181,18 +204,21 @@ export function authReducer(state: any = initialState, action: AuthActions): Aut // next three cases are used by dynamic rendering of login methods case AuthActionTypes.RETRIEVE_AUTH_METHODS: return Object.assign({}, state, { - loading: true + loading: true, + blocking: true }); case AuthActionTypes.RETRIEVE_AUTH_METHODS_SUCCESS: return Object.assign({}, state, { loading: false, + blocking: false, authMethods: (action as RetrieveAuthMethodsSuccessAction).payload }); case AuthActionTypes.RETRIEVE_AUTH_METHODS_ERROR: return Object.assign({}, state, { loading: false, + blocking: false, authMethods: [new AuthMethod(AuthMethodType.Password)] }); @@ -201,6 +227,12 @@ export function authReducer(state: any = initialState, action: AuthActions): Aut redirectUrl: (action as SetRedirectUrlAction).payload, }); + case AuthActionTypes.REDIRECT_AFTER_LOGIN_SUCCESS: + return Object.assign({}, state, { + loading: true, + blocking: true, + }); + default: return state; } diff --git a/src/app/core/auth/auth.service.spec.ts b/src/app/core/auth/auth.service.spec.ts index 7f2c1e29ccd..d3c2b6c44d5 100644 --- a/src/app/core/auth/auth.service.spec.ts +++ b/src/app/core/auth/auth.service.spec.ts @@ -27,6 +27,7 @@ import { EPersonDataService } from '../eperson/eperson-data.service'; import { createSuccessfulRemoteDataObject$ } from '../../shared/remote-data.utils'; import { authMethodsMock } from '../../shared/testing/auth-service.stub'; import { AuthMethod } from './models/auth.method'; +import { HardRedirectService } from '../services/hard-redirect.service'; describe('AuthService test', () => { @@ -48,6 +49,7 @@ describe('AuthService test', () => { let authenticatedState; let unAuthenticatedState; let linkService; + let hardRedirectService; function init() { mockStore = jasmine.createSpyObj('store', { @@ -77,6 +79,7 @@ describe('AuthService test', () => { linkService = { resolveLinks: {} }; + hardRedirectService = jasmine.createSpyObj('hardRedirectService', ['redirect']); spyOn(linkService, 'resolveLinks').and.returnValue({ authenticated: true, eperson: observableOf({ payload: {} }) }); } @@ -104,6 +107,7 @@ describe('AuthService test', () => { { provide: ActivatedRoute, useValue: routeStub }, { provide: Store, useValue: mockStore }, { provide: EPersonDataService, useValue: mockEpersonDataService }, + { provide: HardRedirectService, useValue: hardRedirectService }, CookieService, AuthService ], @@ -210,7 +214,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); + authService = new AuthService({}, window, undefined, authReqService, mockEpersonDataService, router, routeService, cookieService, store, hardRedirectService); })); it('should return true when user is logged in', () => { @@ -289,7 +293,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); + authService = new AuthService({}, window, undefined, authReqService, mockEpersonDataService, router, routeService, cookieService, store, hardRedirectService); storage = (authService as any).storage; routeServiceMock = TestBed.get(RouteService); routerStub = TestBed.get(Router); @@ -318,36 +322,28 @@ describe('AuthService test', () => { expect(storage.remove).toHaveBeenCalled(); }); - it('should set redirect url to previous page', () => { - spyOn(routeServiceMock, 'getHistory').and.callThrough(); - spyOn(routerStub, 'navigateByUrl'); - authService.redirectAfterLoginSuccess(true); - expect(routeServiceMock.getHistory).toHaveBeenCalled(); - expect(routerStub.navigateByUrl).toHaveBeenCalledWith('/collection/123'); + it('should redirect to reload with redirect url', () => { + authService.navigateToRedirectUrl('/collection/123'); + // Reload with redirect URL set to /collection/123 + expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('/reload/[0-9]*\\?redirect=' + encodeURIComponent('/collection/123')))); }); - it('should set redirect url to current page', () => { - spyOn(routeServiceMock, 'getHistory').and.callThrough(); - spyOn(routerStub, 'navigateByUrl'); - authService.redirectAfterLoginSuccess(false); - expect(routeServiceMock.getHistory).toHaveBeenCalled(); - expect(routerStub.navigateByUrl).toHaveBeenCalledWith('/home'); + it('should redirect to reload with /home', () => { + authService.navigateToRedirectUrl('/home'); + // Reload with redirect URL set to /home + expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('/reload/[0-9]*\\?redirect=' + encodeURIComponent('/home')))); }); - it('should redirect to / and not to /login', () => { - spyOn(routeServiceMock, 'getHistory').and.returnValue(observableOf(['/login', '/login'])); - spyOn(routerStub, 'navigateByUrl'); - authService.redirectAfterLoginSuccess(true); - expect(routeServiceMock.getHistory).toHaveBeenCalled(); - expect(routerStub.navigateByUrl).toHaveBeenCalledWith('/'); + it('should redirect to regular reload and not to /login', () => { + authService.navigateToRedirectUrl('/login'); + // Reload without a redirect URL + expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('/reload/[0-9]*(?!\\?)$'))); }); - it('should redirect to / when no redirect url is found', () => { - spyOn(routeServiceMock, 'getHistory').and.returnValue(observableOf([''])); - spyOn(routerStub, 'navigateByUrl'); - authService.redirectAfterLoginSuccess(true); - expect(routeServiceMock.getHistory).toHaveBeenCalled(); - expect(routerStub.navigateByUrl).toHaveBeenCalledWith('/'); + it('should redirect to regular reload when no redirect url is found', () => { + authService.navigateToRedirectUrl(undefined); + // Reload without a redirect URL + expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('/reload/[0-9]*(?!\\?)$'))); }); describe('impersonate', () => { @@ -464,6 +460,14 @@ describe('AuthService test', () => { }); }); }); + + describe('refreshAfterLogout', () => { + it('should call navigateToRedirectUrl with no url', () => { + spyOn(authService as any, 'navigateToRedirectUrl').and.stub(); + authService.refreshAfterLogout(); + expect((authService as any).navigateToRedirectUrl).toHaveBeenCalled(); + }); + }); }); describe('when user is not logged in', () => { @@ -496,7 +500,7 @@ describe('AuthService test', () => { (state as any).core = Object.create({}); (state as any).core.auth = unAuthenticatedState; }); - authService = new AuthService({}, window, undefined, authReqService, mockEpersonDataService, router, routeService, cookieService, store); + authService = new AuthService({}, window, undefined, authReqService, mockEpersonDataService, router, routeService, cookieService, store, hardRedirectService); })); 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 a25c8ffa5b3..06906346edc 100644 --- a/src/app/core/auth/auth.service.ts +++ b/src/app/core/auth/auth.service.ts @@ -1,11 +1,10 @@ import { Inject, Injectable, Optional } from '@angular/core'; -import { PRIMARY_OUTLET, Router, UrlSegmentGroup, UrlTree } from '@angular/router'; +import { Router } from '@angular/router'; import { HttpHeaders } from '@angular/common/http'; import { REQUEST, RESPONSE } from '@nguniversal/express-engine/tokens'; import { Observable, of as observableOf } from 'rxjs'; -import { distinctUntilChanged, filter, map, startWith, switchMap, take, withLatestFrom } from 'rxjs/operators'; -import { RouterReducerState } from '@ngrx/router-store'; +import { map, startWith, switchMap, take } from 'rxjs/operators'; import { select, Store } from '@ngrx/store'; import { CookieAttributes } from 'js-cookie'; @@ -14,7 +13,15 @@ import { AuthRequestService } from './auth-request.service'; import { HttpOptions } from '../dspace-rest-v2/dspace-rest-v2.service'; import { AuthStatus } from './models/auth-status.model'; import { AuthTokenInfo, TOKENITEM } from './models/auth-token-info.model'; -import { hasValue, hasValueOperator, isEmpty, isNotEmpty, isNotNull, isNotUndefined } from '../../shared/empty.util'; +import { + hasValue, + hasValueOperator, + isEmpty, + isNotEmpty, + isNotNull, + isNotUndefined, + hasNoValue +} from '../../shared/empty.util'; import { CookieService } from '../services/cookie.service'; import { getAuthenticatedUserId, @@ -24,7 +31,7 @@ import { isTokenRefreshing, isAuthenticatedLoaded } from './selectors'; -import { AppState, routerStateSelector } from '../../app.reducer'; +import { AppState } from '../../app.reducer'; import { CheckAuthenticationTokenAction, ResetAuthenticationMessagesAction, @@ -36,6 +43,7 @@ import { RouteService } from '../services/route.service'; import { EPersonDataService } from '../eperson/eperson-data.service'; import { getAllSucceededRemoteDataPayload } from '../shared/operators'; import { AuthMethod } from './models/auth.method'; +import { HardRedirectService } from '../services/hard-redirect.service'; export const LOGIN_ROUTE = '/login'; export const LOGOUT_ROUTE = '/logout'; @@ -62,43 +70,13 @@ export class AuthService { protected router: Router, protected routeService: RouteService, protected storage: CookieService, - protected store: Store + protected store: Store, + protected hardRedirectService: HardRedirectService ) { this.store.pipe( select(isAuthenticated), startWith(false) ).subscribe((authenticated: boolean) => this._authenticated = authenticated); - - // If current route is different from the one setted in authentication guard - // and is not the login route, clear redirect url and messages - const routeUrl$ = this.store.pipe( - select(routerStateSelector), - filter((routerState: RouterReducerState) => isNotUndefined(routerState) - && isNotUndefined(routerState.state) && isNotEmpty(routerState.state.url)), - filter((routerState: RouterReducerState) => !this.isLoginRoute(routerState.state.url)), - map((routerState: RouterReducerState) => routerState.state.url) - ); - const redirectUrl$ = this.store.pipe(select(getRedirectUrl), distinctUntilChanged()); - routeUrl$.pipe( - withLatestFrom(redirectUrl$), - map(([routeUrl, redirectUrl]) => [routeUrl, redirectUrl]) - ).pipe(filter(([routeUrl, redirectUrl]) => isNotEmpty(redirectUrl) && (routeUrl !== redirectUrl))) - .subscribe(() => { - this.clearRedirectUrl(); - }); - } - - /** - * Check if is a login page route - * - * @param {string} url - * @returns {Boolean}. - */ - protected isLoginRoute(url: string) { - const urlTree: UrlTree = this.router.parseUrl(url); - const g: UrlSegmentGroup = urlTree.root.children[PRIMARY_OUTLET]; - const segment = '/' + g.toString(); - return segment === LOGIN_ROUTE; } /** @@ -409,70 +387,38 @@ export class AuthService { } /** - * Redirect to the route navigated before the login + * Perform a hard redirect to the URL + * @param redirectUrl */ - public redirectAfterLoginSuccess(isStandalonePage: boolean) { - this.getRedirectUrl().pipe( - take(1)) - .subscribe((redirectUrl) => { - - if (isNotEmpty(redirectUrl)) { - this.clearRedirectUrl(); - this.router.onSameUrlNavigation = 'reload'; - this.navigateToRedirectUrl(redirectUrl); - } else { - // If redirectUrl is empty use history. - this.routeService.getHistory().pipe( - filter((history) => history.length > 0), - take(1) - ).subscribe((history) => { - let redirUrl; - if (isStandalonePage) { - // For standalone login pages, use the previous route. - redirUrl = history[history.length - 2] || ''; - } else { - redirUrl = history[history.length - 1] || ''; - } - this.navigateToRedirectUrl(redirUrl); - }); - } - }); - - } - - protected navigateToRedirectUrl(redirectUrl: string) { - const url = decodeURIComponent(redirectUrl); - // in case the user navigates directly to /login (via bookmark, etc), or the route history is not found. - if (isEmpty(url) || url.startsWith(LOGIN_ROUTE)) { - this.router.navigateByUrl('/'); - /* TODO Reenable hard redirect when REST API can handle x-forwarded-for, see https://github.com/DSpace/DSpace/pull/2207 */ - // this._window.nativeWindow.location.href = '/'; - } else { - /* TODO Reenable hard redirect when REST API can handle x-forwarded-for, see https://github.com/DSpace/DSpace/pull/2207 */ - // this._window.nativeWindow.location.href = url; - this.router.navigateByUrl(url); + public navigateToRedirectUrl(redirectUrl: string) { + let url = `/reload/${new Date().getTime()}`; + if (isNotEmpty(redirectUrl) && !redirectUrl.startsWith(LOGIN_ROUTE)) { + url += `?redirect=${encodeURIComponent(redirectUrl)}`; } + this.hardRedirectService.redirect(url); } /** * Refresh route navigated */ public refreshAfterLogout() { - // Hard redirect to the reload page with a unique number behind it - // so that all state is definitely lost - this._window.nativeWindow.location.href = `/reload/${new Date().getTime()}`; + this.navigateToRedirectUrl(undefined); } /** * Get redirect url */ getRedirectUrl(): Observable { - const redirectUrl = this.storage.get(REDIRECT_COOKIE); - if (isNotEmpty(redirectUrl)) { - return observableOf(redirectUrl); - } else { - return this.store.pipe(select(getRedirectUrl)); - } + return this.store.pipe( + select(getRedirectUrl), + map((urlFromStore: string) => { + if (hasValue(urlFromStore)) { + return urlFromStore; + } else { + return this.storage.get(REDIRECT_COOKIE); + } + }) + ); } /** @@ -489,6 +435,20 @@ export class AuthService { this.store.dispatch(new SetRedirectUrlAction(isNotUndefined(url) ? url : '')); } + /** + * Set the redirect url if the current one has not been set yet + * @param newRedirectUrl + */ + setRedirectUrlIfNotSet(newRedirectUrl: string) { + this.getRedirectUrl().pipe( + take(1)) + .subscribe((currentRedirectUrl) => { + if (hasNoValue(currentRedirectUrl)) { + this.setRedirectUrl(newRedirectUrl); + } + }) + } + /** * Clear redirect url */ diff --git a/src/app/core/auth/authenticated.guard.ts b/src/app/core/auth/authenticated.guard.ts index 7a2f39854c5..0b9eeec5095 100644 --- a/src/app/core/auth/authenticated.guard.ts +++ b/src/app/core/auth/authenticated.guard.ts @@ -1,21 +1,26 @@ import { Injectable } from '@angular/core'; -import { ActivatedRouteSnapshot, CanActivate, CanLoad, Route, Router, RouterStateSnapshot } from '@angular/router'; +import { + ActivatedRouteSnapshot, + CanActivate, + Router, + RouterStateSnapshot, + UrlTree +} from '@angular/router'; import { Observable } from 'rxjs'; -import { take } from 'rxjs/operators'; +import { map, find, switchMap } from 'rxjs/operators'; import { select, Store } from '@ngrx/store'; import { CoreState } from '../core.reducers'; -import { isAuthenticated } from './selectors'; -import { AuthService } from './auth.service'; -import { RedirectWhenAuthenticationIsRequiredAction } from './auth.actions'; +import { isAuthenticated, isAuthenticationLoading } from './selectors'; +import { AuthService, LOGIN_ROUTE } from './auth.service'; /** * Prevent unauthorized activating and loading of routes * @class AuthenticatedGuard */ @Injectable() -export class AuthenticatedGuard implements CanActivate, CanLoad { +export class AuthenticatedGuard implements CanActivate { /** * @constructor @@ -24,46 +29,37 @@ export class AuthenticatedGuard implements CanActivate, CanLoad { /** * True when user is authenticated + * UrlTree with redirect to login page when user isn't authenticated * @method canActivate */ - canActivate(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable { + canActivate(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable { const url = state.url; return this.handleAuth(url); } /** * True when user is authenticated + * UrlTree with redirect to login page when user isn't authenticated * @method canActivateChild */ - canActivateChild(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable { + canActivateChild(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable { return this.canActivate(route, state); } - /** - * True when user is authenticated - * @method canLoad - */ - canLoad(route: Route): Observable { - const url = `/${route.path}`; - - return this.handleAuth(url); - } - - private handleAuth(url: string): Observable { - // get observable - const observable = this.store.pipe(select(isAuthenticated)); - + private handleAuth(url: string): Observable { // redirect to sign in page if user is not authenticated - observable.pipe( - // .filter(() => isEmpty(this.router.routerState.snapshot.url) || this.router.routerState.snapshot.url === url) - take(1)) - .subscribe((authenticated) => { - if (!authenticated) { + return this.store.pipe(select(isAuthenticationLoading)).pipe( + find((isLoading: boolean) => isLoading === false), + switchMap(() => this.store.pipe(select(isAuthenticated))), + map((authenticated) => { + if (authenticated) { + return authenticated; + } else { this.authService.setRedirectUrl(url); - this.store.dispatch(new RedirectWhenAuthenticationIsRequiredAction('Login required')); + this.authService.removeToken(); + return this.router.createUrlTree([LOGIN_ROUTE]); } - }); - - return observable; + }) + ); } } diff --git a/src/app/core/auth/selectors.ts b/src/app/core/auth/selectors.ts index 173f82e810a..c4e95a0fb32 100644 --- a/src/app/core/auth/selectors.ts +++ b/src/app/core/auth/selectors.ts @@ -65,6 +65,14 @@ const _getAuthenticationInfo = (state: AuthState) => state.info; */ const _isLoading = (state: AuthState) => state.loading; +/** + * Returns true if everything else should wait for authentication. + * @function _isBlocking + * @param {State} state + * @returns {boolean} + */ +const _isBlocking = (state: AuthState) => state.blocking; + /** * Returns true if a refresh token request is in progress. * @function _isRefreshing @@ -170,6 +178,16 @@ export const isAuthenticatedLoaded = createSelector(getAuthState, _isAuthenticat */ export const isAuthenticationLoading = createSelector(getAuthState, _isLoading); +/** + * Returns true if the authentication should block everything else + * + * @function isAuthenticationBlocking + * @param {AuthState} state + * @param {any} props + * @return {boolean} + */ +export const isAuthenticationBlocking = createSelector(getAuthState, _isBlocking); + /** * Returns true if the refresh token request is loading. * @function isTokenRefreshing diff --git a/src/app/core/auth/server-auth.service.ts b/src/app/core/auth/server-auth.service.ts index 7b78255001f..88a4ac406e9 100644 --- a/src/app/core/auth/server-auth.service.ts +++ b/src/app/core/auth/server-auth.service.ts @@ -2,7 +2,7 @@ import { Injectable } from '@angular/core'; import { HttpHeaders } from '@angular/common/http'; import { Observable } from 'rxjs'; -import { filter, map, take } from 'rxjs/operators'; +import { map } from 'rxjs/operators'; import { isNotEmpty } from '../../shared/empty.util'; import { HttpOptions } from '../dspace-rest-v2/dspace-rest-v2.service'; @@ -58,32 +58,4 @@ export class ServerAuthService extends AuthService { map((status: AuthStatus) => Object.assign(new AuthStatus(), status)) ); } - - /** - * Redirect to the route navigated before the login - */ - public redirectAfterLoginSuccess(isStandalonePage: boolean) { - this.getRedirectUrl().pipe( - take(1)) - .subscribe((redirectUrl) => { - if (isNotEmpty(redirectUrl)) { - // override the route reuse strategy - this.router.routeReuseStrategy.shouldReuseRoute = () => { - return false; - }; - this.router.navigated = false; - const url = decodeURIComponent(redirectUrl); - this.router.navigateByUrl(url); - } else { - // If redirectUrl is empty use history. For ssr the history array should contain the requested url. - this.routeService.getHistory().pipe( - filter((history) => history.length > 0), - take(1) - ).subscribe((history) => { - this.navigateToRedirectUrl(history[history.length - 1] || ''); - }); - } - }) - } - } diff --git a/src/app/core/core.module.ts b/src/app/core/core.module.ts index fc78b31d139..2b6328d38a9 100644 --- a/src/app/core/core.module.ts +++ b/src/app/core/core.module.ts @@ -166,6 +166,7 @@ import { VocabularyService } from './submission/vocabularies/vocabulary.service' import { VocabularyTreeviewService } from '../shared/vocabulary-treeview/vocabulary-treeview.service'; import { ConfigurationDataService } from './data/configuration-data.service'; import { ConfigurationProperty } from './shared/configuration-property.model'; +import { ReloadGuard } from './reload/reload.guard'; /** * When not in production, endpoint responses can be mocked for testing purposes @@ -291,6 +292,7 @@ const PROVIDERS = [ MetadataSchemaDataService, MetadataFieldDataService, TokenResponseParsingService, + ReloadGuard, // register AuthInterceptor as HttpInterceptor { provide: HTTP_INTERCEPTORS, diff --git a/src/app/core/reload/reload.guard.spec.ts b/src/app/core/reload/reload.guard.spec.ts new file mode 100644 index 00000000000..317245bafa9 --- /dev/null +++ b/src/app/core/reload/reload.guard.spec.ts @@ -0,0 +1,47 @@ +import { ReloadGuard } from './reload.guard'; +import { Router } from '@angular/router'; + +describe('ReloadGuard', () => { + let guard: ReloadGuard; + let router: Router; + + beforeEach(() => { + router = jasmine.createSpyObj('router', ['parseUrl', 'createUrlTree']); + guard = new ReloadGuard(router); + }); + + describe('canActivate', () => { + let route; + + describe('when the route\'s query params contain a redirect url', () => { + let redirectUrl; + + beforeEach(() => { + redirectUrl = '/redirect/url?param=extra'; + route = { + queryParams: { + redirect: redirectUrl + } + }; + }); + + it('should create a UrlTree with the redirect URL', () => { + guard.canActivate(route, undefined); + expect(router.parseUrl).toHaveBeenCalledWith(redirectUrl); + }); + }); + + describe('when the route\'s query params doesn\'t contain a redirect url', () => { + beforeEach(() => { + route = { + queryParams: {} + }; + }); + + it('should create a UrlTree to home', () => { + guard.canActivate(route, undefined); + expect(router.createUrlTree).toHaveBeenCalledWith(['home']); + }); + }); + }); +}); diff --git a/src/app/core/reload/reload.guard.ts b/src/app/core/reload/reload.guard.ts new file mode 100644 index 00000000000..78f9dcf642b --- /dev/null +++ b/src/app/core/reload/reload.guard.ts @@ -0,0 +1,26 @@ +import { ActivatedRouteSnapshot, CanActivate, Router, RouterStateSnapshot, UrlTree } from '@angular/router'; +import { Injectable } from '@angular/core'; +import { isNotEmpty } from '../../shared/empty.util'; + +/** + * A guard redirecting the user to the URL provided in the route's query params + * When no redirect url is found, the user is redirected to the homepage + */ +@Injectable() +export class ReloadGuard implements CanActivate { + constructor(private router: Router) { + } + + /** + * Get the UrlTree of the URL to redirect to + * @param route + * @param state + */ + canActivate(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): UrlTree { + if (isNotEmpty(route.queryParams.redirect)) { + return this.router.parseUrl(route.queryParams.redirect); + } else { + return this.router.createUrlTree(['home']); + } + } +} diff --git a/src/app/core/services/browser-hard-redirect.service.spec.ts b/src/app/core/services/browser-hard-redirect.service.spec.ts new file mode 100644 index 00000000000..9d4c5df9a2e --- /dev/null +++ b/src/app/core/services/browser-hard-redirect.service.spec.ts @@ -0,0 +1,41 @@ +import {TestBed} from '@angular/core/testing'; +import {BrowserHardRedirectService} from './browser-hard-redirect.service'; + +describe('BrowserHardRedirectService', () => { + + const mockLocation = { + href: undefined, + pathname: '/pathname', + search: '/search', + } as Location; + + const service: BrowserHardRedirectService = new BrowserHardRedirectService(mockLocation); + + beforeEach(() => { + TestBed.configureTestingModule({}); + }); + + it('should be created', () => { + expect(service).toBeTruthy(); + }); + + describe('when performing a redirect', () => { + + const redirect = 'test redirect'; + + beforeEach(() => { + service.redirect(redirect); + }); + + it('should update the location', () => { + expect(mockLocation.href).toEqual(redirect); + }) + }); + + describe('when requesting the current route', () => { + + it('should return the location origin', () => { + expect(service.getCurrentRoute()).toEqual(mockLocation.pathname + mockLocation.search); + }); + }); +}); diff --git a/src/app/core/services/browser-hard-redirect.service.ts b/src/app/core/services/browser-hard-redirect.service.ts new file mode 100644 index 00000000000..0d14b6b8344 --- /dev/null +++ b/src/app/core/services/browser-hard-redirect.service.ts @@ -0,0 +1,35 @@ +import { Inject, Injectable, InjectionToken } from '@angular/core'; +import { HardRedirectService } from './hard-redirect.service'; + +export const LocationToken = new InjectionToken('Location'); + +export function locationProvider(): Location { + return window.location; +} + +/** + * Service for performing hard redirects within the browser app module + */ +@Injectable() +export class BrowserHardRedirectService implements HardRedirectService { + + constructor( + @Inject(LocationToken) protected location: Location, + ) { + } + + /** + * Perform a hard redirect to URL + * @param url + */ + redirect(url: string) { + this.location.href = url; + } + + /** + * Get the origin of a request + */ + getCurrentRoute() { + return this.location.pathname + this.location.search; + } +} diff --git a/src/app/core/services/hard-redirect.service.ts b/src/app/core/services/hard-redirect.service.ts new file mode 100644 index 00000000000..09757a12507 --- /dev/null +++ b/src/app/core/services/hard-redirect.service.ts @@ -0,0 +1,22 @@ +import { Injectable } from '@angular/core'; + +/** + * Service to take care of hard redirects + */ +@Injectable() +export abstract class HardRedirectService { + + /** + * Perform a hard redirect to a given location. + * + * @param url + * the page to redirect to + */ + abstract redirect(url: string); + + /** + * Get the current route, with query params included + * e.g. /search?page=1&query=open%20access&f.dateIssued.min=1980&f.dateIssued.max=2020 + */ + abstract getCurrentRoute(); +} diff --git a/src/app/core/services/server-hard-redirect.service.spec.ts b/src/app/core/services/server-hard-redirect.service.spec.ts new file mode 100644 index 00000000000..2d09c21eb93 --- /dev/null +++ b/src/app/core/services/server-hard-redirect.service.spec.ts @@ -0,0 +1,43 @@ +import { TestBed } from '@angular/core/testing'; +import { ServerHardRedirectService } from './server-hard-redirect.service'; + +describe('ServerHardRedirectService', () => { + + const mockRequest = jasmine.createSpyObj(['get']); + const mockResponse = jasmine.createSpyObj(['redirect', 'end']); + + const service: ServerHardRedirectService = new ServerHardRedirectService(mockRequest, mockResponse); + + beforeEach(() => { + TestBed.configureTestingModule({}); + }); + + it('should be created', () => { + expect(service).toBeTruthy(); + }); + + describe('when performing a redirect', () => { + + const redirect = 'test redirect'; + + beforeEach(() => { + service.redirect(redirect); + }); + + it('should update the response object', () => { + expect(mockResponse.redirect).toHaveBeenCalledWith(302, redirect); + expect(mockResponse.end).toHaveBeenCalled(); + }) + }); + + describe('when requesting the current route', () => { + + beforeEach(() => { + mockRequest.originalUrl = 'original/url'; + }); + + it('should return the location origin', () => { + expect(service.getCurrentRoute()).toEqual(mockRequest.originalUrl); + }); + }); +}); diff --git a/src/app/core/services/server-hard-redirect.service.ts b/src/app/core/services/server-hard-redirect.service.ts new file mode 100644 index 00000000000..79755d2dc93 --- /dev/null +++ b/src/app/core/services/server-hard-redirect.service.ts @@ -0,0 +1,62 @@ +import { Inject, Injectable } from '@angular/core'; +import { Request, Response } from 'express'; +import { REQUEST, RESPONSE } from '@nguniversal/express-engine/tokens'; +import { HardRedirectService } from './hard-redirect.service'; + +/** + * Service for performing hard redirects within the server app module + */ +@Injectable() +export class ServerHardRedirectService implements HardRedirectService { + + constructor( + @Inject(REQUEST) protected req: Request, + @Inject(RESPONSE) protected res: Response, + ) { + } + + /** + * Perform a hard redirect to URL + * @param url + */ + redirect(url: string) { + + if (url === this.req.url) { + return; + } + + if (this.res.finished) { + const req: any = this.req; + req._r_count = (req._r_count || 0) + 1; + + console.warn('Attempted to redirect on a finished response. From', + this.req.url, 'to', url); + + if (req._r_count > 10) { + console.error('Detected a redirection loop. killing the nodejs process'); + process.exit(1); + } + } else { + // attempt to use the already set status + let status = this.res.statusCode || 0; + if (status < 300 || status >= 400) { + // temporary redirect + status = 302; + } + + console.log(`Redirecting from ${this.req.url} to ${url} with ${status}`); + this.res.redirect(status, url); + this.res.end(); + // I haven't found a way to correctly stop Angular rendering. + // So we just let it end its work, though we have already closed + // the response. + } + } + + /** + * Get the origin of a request + */ + getCurrentRoute() { + return this.req.originalUrl; + } +} diff --git a/src/app/shared/auth-nav-menu/auth-nav-menu.component.html b/src/app/shared/auth-nav-menu/auth-nav-menu.component.html index a05381fee8c..fa92939e0fe 100644 --- a/src/app/shared/auth-nav-menu/auth-nav-menu.component.html +++ b/src/app/shared/auth-nav-menu/auth-nav-menu.component.html @@ -1,7 +1,7 @@
  • -
    +
    {{ 'nav.login' | translate }}
    { notAuthState = { authenticated: false, loaded: false, + blocking: false, loading: false }; authState = { authenticated: true, loaded: true, + blocking: false, loading: false, authToken: new AuthTokenInfo('test_token'), userId: EPersonMock.id 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 909b04c7be2..b15330e84a7 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 @@ -34,6 +34,7 @@ describe('UserMenuComponent', () => { authState = { authenticated: true, loaded: true, + blocking: false, loading: false, authToken: new AuthTokenInfo('test_token'), userId: EPersonMock.id @@ -41,6 +42,7 @@ describe('UserMenuComponent', () => { authStateLoading = { authenticated: true, loaded: true, + blocking: false, loading: true, authToken: null, userId: EPersonMock.id diff --git a/src/app/shared/impersonate-navbar/impersonate-navbar.component.spec.ts b/src/app/shared/impersonate-navbar/impersonate-navbar.component.spec.ts index e6ddc3075d8..8e36b2ccb73 100644 --- a/src/app/shared/impersonate-navbar/impersonate-navbar.component.spec.ts +++ b/src/app/shared/impersonate-navbar/impersonate-navbar.component.spec.ts @@ -26,6 +26,7 @@ describe('ImpersonateNavbarComponent', () => { authState = { authenticated: true, loaded: true, + blocking: false, loading: false, authToken: new AuthTokenInfo('test_token'), userId: EPersonMock.id diff --git a/src/app/shared/log-in/container/log-in-container.component.spec.ts b/src/app/shared/log-in/container/log-in-container.component.spec.ts index 41673cc773a..aa4888104f7 100644 --- a/src/app/shared/log-in/container/log-in-container.component.spec.ts +++ b/src/app/shared/log-in/container/log-in-container.component.spec.ts @@ -12,6 +12,7 @@ import { AuthService } from '../../../core/auth/auth.service'; import { AuthMethod } from '../../../core/auth/models/auth.method'; import { AuthServiceStub } from '../../testing/auth-service.stub'; import { createTestComponent } from '../../testing/utils.test'; +import { HardRedirectService } from '../../../core/services/hard-redirect.service'; describe('LogInContainerComponent', () => { @@ -20,7 +21,13 @@ describe('LogInContainerComponent', () => { const authMethod = new AuthMethod('password'); + let hardRedirectService: HardRedirectService; + beforeEach(async(() => { + hardRedirectService = jasmine.createSpyObj('hardRedirectService', { + redirect: {}, + getCurrentRoute: {} + }); // refine the test module by declaring the test component TestBed.configureTestingModule({ imports: [ @@ -35,6 +42,7 @@ describe('LogInContainerComponent', () => { ], providers: [ {provide: AuthService, useClass: AuthServiceStub}, + { provide: HardRedirectService, useValue: hardRedirectService }, LogInContainerComponent ], schemas: [ diff --git a/src/app/shared/log-in/container/log-in-container.component.ts b/src/app/shared/log-in/container/log-in-container.component.ts index 660e616b9dc..46459362bef 100644 --- a/src/app/shared/log-in/container/log-in-container.component.ts +++ b/src/app/shared/log-in/container/log-in-container.component.ts @@ -15,6 +15,12 @@ export class LogInContainerComponent implements OnInit { @Input() authMethod: AuthMethod; + /** + * A boolean representing if LogInContainerComponent is in a standalone page + * @type {boolean} + */ + @Input() isStandalonePage: boolean; + /** * Injector to inject a section component with the @Input parameters * @type {Injector} @@ -36,6 +42,7 @@ export class LogInContainerComponent implements OnInit { this.objectInjector = Injector.create({ providers: [ { provide: 'authMethodProvider', useFactory: () => (this.authMethod), deps: [] }, + { provide: 'isStandalonePage', useFactory: () => (this.isStandalonePage), deps: [] }, ], parent: this.injector }); diff --git a/src/app/shared/log-in/log-in.component.html b/src/app/shared/log-in/log-in.component.html index aaf73e0e1b6..a2af460b059 100644 --- a/src/app/shared/log-in/log-in.component.html +++ b/src/app/shared/log-in/log-in.component.html @@ -4,7 +4,7 @@
    {{"login.form.or-divider" | translate}}
    - +
    diff --git a/src/app/shared/log-in/log-in.component.spec.ts b/src/app/shared/log-in/log-in.component.spec.ts index a9a42bf3dd9..25369498496 100644 --- a/src/app/shared/log-in/log-in.component.spec.ts +++ b/src/app/shared/log-in/log-in.component.spec.ts @@ -18,6 +18,7 @@ import { NativeWindowService } from '../../core/services/window.service'; import { provideMockStore } from '@ngrx/store/testing'; import { createTestComponent } from '../testing/utils.test'; import { RouterTestingModule } from '@angular/router/testing'; +import { HardRedirectService } from '../../core/services/hard-redirect.service'; describe('LogInComponent', () => { @@ -33,8 +34,13 @@ describe('LogInComponent', () => { } } }; + let hardRedirectService: HardRedirectService; beforeEach(async(() => { + hardRedirectService = jasmine.createSpyObj('hardRedirectService', { + redirect: {}, + getCurrentRoute: {} + }); // refine the test module by declaring the test component TestBed.configureTestingModule({ imports: [ @@ -58,6 +64,7 @@ describe('LogInComponent', () => { { provide: NativeWindowService, useFactory: NativeWindowMockFactory }, // { provide: Router, useValue: new RouterStub() }, { provide: ActivatedRoute, useValue: new ActivatedRouteStub() }, + { provide: HardRedirectService, useValue: hardRedirectService }, provideMockStore({ initialState }), LogInComponent ], diff --git a/src/app/shared/log-in/log-in.component.ts b/src/app/shared/log-in/log-in.component.ts index 3586e9c6058..7700d7be823 100644 --- a/src/app/shared/log-in/log-in.component.ts +++ b/src/app/shared/log-in/log-in.component.ts @@ -1,14 +1,17 @@ -import { Component, Input, OnDestroy, OnInit } from '@angular/core'; - +import { Component, Input, OnInit } from '@angular/core'; import { Observable } from 'rxjs'; -import { filter, takeWhile, } from 'rxjs/operators'; import { select, Store } from '@ngrx/store'; - import { AuthMethod } from '../../core/auth/models/auth.method'; -import { getAuthenticationMethods, isAuthenticated, isAuthenticationLoading } from '../../core/auth/selectors'; +import { + getAuthenticationError, + getAuthenticationMethods, + isAuthenticated, + isAuthenticationLoading +} from '../../core/auth/selectors'; import { CoreState } from '../../core/core.reducers'; -import { AuthService } from '../../core/auth/auth.service'; import { getForgotPasswordRoute, getRegisterRoute } from '../../app-routing-paths'; +import { hasValue } from '../empty.util'; +import { AuthService } from '../../core/auth/auth.service'; /** * /users/sign-in @@ -19,7 +22,7 @@ import { getForgotPasswordRoute, getRegisterRoute } from '../../app-routing-path templateUrl: './log-in.component.html', styleUrls: ['./log-in.component.scss'] }) -export class LogInComponent implements OnInit, OnDestroy { +export class LogInComponent implements OnInit { /** * A boolean representing if LogInComponent is in a standalone page @@ -45,14 +48,8 @@ export class LogInComponent implements OnInit, OnDestroy { */ public loading: Observable; - /** - * Component state. - * @type {boolean} - */ - private alive = true; - constructor(private store: Store, - private authService: AuthService,) { + private authService: AuthService) { } ngOnInit(): void { @@ -67,20 +64,12 @@ export class LogInComponent implements OnInit, OnDestroy { // set isAuthenticated this.isAuthenticated = this.store.pipe(select(isAuthenticated)); - // subscribe to success - this.store.pipe( - select(isAuthenticated), - takeWhile(() => this.alive), - filter((authenticated) => authenticated)) - .subscribe(() => { - this.authService.redirectAfterLoginSuccess(this.isStandalonePage); - } - ); - - } - - ngOnDestroy(): void { - this.alive = false; + // Clear the redirect URL if an authentication error occurs and this is not a standalone page + this.store.pipe(select(getAuthenticationError)).subscribe((error) => { + if (hasValue(error) && !this.isStandalonePage) { + this.authService.clearRedirectUrl(); + } + }); } getRegisterRoute() { diff --git a/src/app/shared/log-in/methods/password/log-in-password.component.spec.ts b/src/app/shared/log-in/methods/password/log-in-password.component.spec.ts index 68c939d1bc9..5291c161501 100644 --- a/src/app/shared/log-in/methods/password/log-in-password.component.spec.ts +++ b/src/app/shared/log-in/methods/password/log-in-password.component.spec.ts @@ -15,6 +15,7 @@ import { AuthServiceStub } from '../../../testing/auth-service.stub'; import { AppState } from '../../../../app.reducer'; import { AuthMethod } from '../../../../core/auth/models/auth.method'; import { AuthMethodType } from '../../../../core/auth/models/auth.method-type'; +import { HardRedirectService } from '../../../../core/services/hard-redirect.service'; describe('LogInPasswordComponent', () => { @@ -29,8 +30,14 @@ describe('LogInPasswordComponent', () => { loading: false, }; + let hardRedirectService: HardRedirectService; + beforeEach(() => { user = EPersonMock; + + hardRedirectService = jasmine.createSpyObj('hardRedirectService', { + getCurrentRoute: {} + }); }); beforeEach(async(() => { @@ -47,7 +54,9 @@ describe('LogInPasswordComponent', () => { ], providers: [ { provide: AuthService, useClass: AuthServiceStub }, - { provide: 'authMethodProvider', useValue: new AuthMethod(AuthMethodType.Password) } + { provide: 'authMethodProvider', useValue: new AuthMethod(AuthMethodType.Password) }, + { provide: 'isStandalonePage', useValue: true }, + { provide: HardRedirectService, useValue: hardRedirectService }, ], schemas: [ CUSTOM_ELEMENTS_SCHEMA diff --git a/src/app/shared/log-in/methods/password/log-in-password.component.ts b/src/app/shared/log-in/methods/password/log-in-password.component.ts index 8b0dd8cc040..c72881d3bfc 100644 --- a/src/app/shared/log-in/methods/password/log-in-password.component.ts +++ b/src/app/shared/log-in/methods/password/log-in-password.component.ts @@ -13,6 +13,8 @@ import { fadeOut } from '../../../animations/fade'; import { AuthMethodType } from '../../../../core/auth/models/auth.method-type'; import { renderAuthMethodFor } from '../log-in.methods-decorator'; import { AuthMethod } from '../../../../core/auth/models/auth.method'; +import { AuthService } from '../../../../core/auth/auth.service'; +import { HardRedirectService } from '../../../../core/services/hard-redirect.service'; /** * /users/sign-in @@ -66,11 +68,17 @@ export class LogInPasswordComponent implements OnInit { /** * @constructor * @param {AuthMethod} injectedAuthMethodModel + * @param {boolean} isStandalonePage + * @param {AuthService} authService + * @param {HardRedirectService} hardRedirectService * @param {FormBuilder} formBuilder * @param {Store} store */ constructor( @Inject('authMethodProvider') public injectedAuthMethodModel: AuthMethod, + @Inject('isStandalonePage') public isStandalonePage: boolean, + private authService: AuthService, + private hardRedirectService: HardRedirectService, private formBuilder: FormBuilder, private store: Store ) { @@ -134,6 +142,12 @@ export class LogInPasswordComponent implements OnInit { email.trim(); password.trim(); + if (!this.isStandalonePage) { + this.authService.setRedirectUrl(this.hardRedirectService.getCurrentRoute()); + } else { + this.authService.setRedirectUrlIfNotSet('/'); + } + // dispatch AuthenticationAction this.store.dispatch(new AuthenticateAction(email, password)); diff --git a/src/app/shared/log-in/methods/shibboleth/log-in-shibboleth.component.spec.ts b/src/app/shared/log-in/methods/shibboleth/log-in-shibboleth.component.spec.ts index 7c1e782ee0d..e8b18fa409b 100644 --- a/src/app/shared/log-in/methods/shibboleth/log-in-shibboleth.component.spec.ts +++ b/src/app/shared/log-in/methods/shibboleth/log-in-shibboleth.component.spec.ts @@ -17,6 +17,7 @@ import { NativeWindowService } from '../../../../core/services/window.service'; import { RouterStub } from '../../../testing/router.stub'; import { ActivatedRouteStub } from '../../../testing/active-router.stub'; import { NativeWindowMockFactory } from '../../../mocks/mock-native-window-ref'; +import { HardRedirectService } from '../../../../core/services/hard-redirect.service'; describe('LogInShibbolethComponent', () => { @@ -30,6 +31,7 @@ describe('LogInShibbolethComponent', () => { let location; let authState; + let hardRedirectService: HardRedirectService; beforeEach(() => { user = EPersonMock; @@ -41,6 +43,11 @@ describe('LogInShibbolethComponent', () => { loaded: false, loading: false, }; + + hardRedirectService = jasmine.createSpyObj('hardRedirectService', { + getCurrentRoute: {}, + redirect: {} + }); }); beforeEach(async(() => { @@ -56,9 +63,11 @@ describe('LogInShibbolethComponent', () => { providers: [ { provide: AuthService, useClass: AuthServiceStub }, { provide: 'authMethodProvider', useValue: new AuthMethod(AuthMethodType.Shibboleth, location) }, + { provide: 'isStandalonePage', useValue: true }, { provide: NativeWindowService, useFactory: NativeWindowMockFactory }, { provide: Router, useValue: new RouterStub() }, { provide: ActivatedRoute, useValue: new ActivatedRouteStub() }, + { provide: HardRedirectService, useValue: hardRedirectService }, ], schemas: [ CUSTOM_ELEMENTS_SCHEMA @@ -99,7 +108,7 @@ describe('LogInShibbolethComponent', () => { component.redirectToShibboleth(); - expect(setHrefSpy).toHaveBeenCalledWith(shibbolethBaseUrl + currentUrl) + expect(setHrefSpy).toHaveBeenCalledWith(currentUrl) }); @@ -114,7 +123,7 @@ describe('LogInShibbolethComponent', () => { component.redirectToShibboleth(); - expect(setHrefSpy).toHaveBeenCalledWith(shibbolethBaseUrl + currentUrl) + expect(setHrefSpy).toHaveBeenCalledWith(currentUrl) }); diff --git a/src/app/shared/log-in/methods/shibboleth/log-in-shibboleth.component.ts b/src/app/shared/log-in/methods/shibboleth/log-in-shibboleth.component.ts index 6321e6119ff..b0704d5cb15 100644 --- a/src/app/shared/log-in/methods/shibboleth/log-in-shibboleth.component.ts +++ b/src/app/shared/log-in/methods/shibboleth/log-in-shibboleth.component.ts @@ -11,7 +11,11 @@ import { CoreState } from '../../../../core/core.reducers'; import { isAuthenticated, isAuthenticationLoading } from '../../../../core/auth/selectors'; import { RouteService } from '../../../../core/services/route.service'; import { NativeWindowRef, NativeWindowService } from '../../../../core/services/window.service'; -import { isNotNull } from '../../../empty.util'; +import { isNotNull, isEmpty } from '../../../empty.util'; +import { AuthService } from '../../../../core/auth/auth.service'; +import { HardRedirectService } from '../../../../core/services/hard-redirect.service'; +import { take } from 'rxjs/operators'; +import { URLCombiner } from '../../../../core/url-combiner/url-combiner'; @Component({ selector: 'ds-log-in-shibboleth', @@ -49,14 +53,20 @@ export class LogInShibbolethComponent implements OnInit { /** * @constructor * @param {AuthMethod} injectedAuthMethodModel + * @param {boolean} isStandalonePage * @param {NativeWindowRef} _window * @param {RouteService} route + * @param {AuthService} authService + * @param {HardRedirectService} hardRedirectService * @param {Store} store */ constructor( @Inject('authMethodProvider') public injectedAuthMethodModel: AuthMethod, + @Inject('isStandalonePage') public isStandalonePage: boolean, @Inject(NativeWindowService) protected _window: NativeWindowRef, private route: RouteService, + private authService: AuthService, + private hardRedirectService: HardRedirectService, private store: Store ) { this.authMethod = injectedAuthMethodModel; @@ -75,21 +85,31 @@ export class LogInShibbolethComponent implements OnInit { } redirectToShibboleth() { - let newLocationUrl = this.location; - const currentUrl = this._window.nativeWindow.location.href; - const myRegexp = /\?redirectUrl=(.*)/g; - const match = myRegexp.exec(this.location); - const redirectUrl = (match && match[1]) ? match[1] : null; - - // Check whether the current page is different from the redirect url received from rest - if (isNotNull(redirectUrl) && redirectUrl !== currentUrl) { - // change the redirect url with the current page url - const newRedirectUrl = `?redirectUrl=${currentUrl}`; - newLocationUrl = this.location.replace(/\?redirectUrl=(.*)/g, newRedirectUrl); - } - - // redirect to shibboleth authentication url - this._window.nativeWindow.location.href = newLocationUrl; + + this.authService.getRedirectUrl().pipe(take(1)).subscribe((redirectRoute) => { + if (!this.isStandalonePage) { + redirectRoute = this.hardRedirectService.getCurrentRoute(); + } else if (isEmpty(redirectRoute)) { + redirectRoute = '/'; + } + const correctRedirectUrl = new URLCombiner(this._window.nativeWindow.origin, redirectRoute).toString(); + + let shibbolethServerUrl = this.location; + const myRegexp = /\?redirectUrl=(.*)/g; + const match = myRegexp.exec(this.location); + const redirectUrlFromServer = (match && match[1]) ? match[1] : null; + + // Check whether the current page is different from the redirect url received from rest + if (isNotNull(redirectUrlFromServer) && redirectUrlFromServer !== correctRedirectUrl) { + // change the redirect url with the current page url + const newRedirectUrl = `?redirectUrl=${correctRedirectUrl}`; + shibbolethServerUrl = this.location.replace(/\?redirectUrl=(.*)/g, newRedirectUrl); + } + + // redirect to shibboleth authentication url + this.hardRedirectService.redirect(shibbolethServerUrl); + }); + } } diff --git a/src/app/shared/testing/auth-service.stub.ts b/src/app/shared/testing/auth-service.stub.ts index 7e7e70a754c..f522ce83063 100644 --- a/src/app/shared/testing/auth-service.stub.ts +++ b/src/app/shared/testing/auth-service.stub.ts @@ -154,4 +154,16 @@ export class AuthServiceStub { resetAuthenticationError() { return; } + + setRedirectUrlIfNotSet(url: string) { + return; + } + + redirectAfterLoginSuccess() { + return; + } + + clearRedirectUrl() { + return; + } } diff --git a/src/modules/app/browser-app.module.ts b/src/modules/app/browser-app.module.ts index 73a49b02117..08ce2737c67 100644 --- a/src/modules/app/browser-app.module.ts +++ b/src/modules/app/browser-app.module.ts @@ -21,6 +21,11 @@ import { AuthService } from '../../app/core/auth/auth.service'; import { Angulartics2RouterlessModule } from 'angulartics2/routerlessmodule'; import { SubmissionService } from '../../app/submission/submission.service'; import { StatisticsModule } from '../../app/statistics/statistics.module'; +import { HardRedirectService } from '../../app/core/services/hard-redirect.service'; +import { + BrowserHardRedirectService, + LocationToken, locationProvider +} from '../../app/core/services/browser-hard-redirect.service'; export const REQ_KEY = makeStateKey('req'); @@ -78,7 +83,15 @@ export function getRequest(transferState: TransferState): any { { provide: SubmissionService, useClass: SubmissionService - } + }, + { + provide: HardRedirectService, + useClass: BrowserHardRedirectService, + }, + { + provide: LocationToken, + useFactory: locationProvider, + }, ] }) export class BrowserAppModule { diff --git a/src/modules/app/server-app.module.ts b/src/modules/app/server-app.module.ts index 0ba09182cc2..c8ea81bdec2 100644 --- a/src/modules/app/server-app.module.ts +++ b/src/modules/app/server-app.module.ts @@ -29,6 +29,8 @@ import { ServerLocaleService } from 'src/app/core/locale/server-locale.service'; import { LocaleService } from 'src/app/core/locale/locale.service'; import { HTTP_INTERCEPTORS } from '@angular/common/http'; import { ForwardClientIpInterceptor } from '../../app/core/forward-client-ip/forward-client-ip.interceptor'; +import { HardRedirectService } from '../../app/core/services/hard-redirect.service'; +import { ServerHardRedirectService } from '../../app/core/services/server-hard-redirect.service'; export function createTranslateLoader() { return new TranslateJson5UniversalLoader('dist/server/assets/i18n/', '.json5'); @@ -88,6 +90,10 @@ export function createTranslateLoader() { useClass: ForwardClientIpInterceptor, multi: true }, + { + provide: HardRedirectService, + useClass: ServerHardRedirectService, + }, ] }) export class ServerAppModule {