From 7367f9117666d23e149aeef8af9b0011458a8286 Mon Sep 17 00:00:00 2001 From: Kristof De Langhe Date: Wed, 26 Aug 2020 14:20:47 +0200 Subject: [PATCH 01/15] 72699: Hard redirect after log in --- src/app/app-routing.module.ts | 3 +- src/app/core/auth/auth.effects.ts | 7 --- src/app/core/auth/auth.service.ts | 22 +++----- src/app/core/auth/authenticated.guard.ts | 52 +++++++++---------- src/app/core/core.module.ts | 2 + src/app/core/reload/reload.guard.ts | 17 ++++++ .../services/browser-hard-redirect.service.ts | 19 +++++++ .../core/services/hard-redirect.service.ts | 21 ++++++++ .../services/server-hard-redirect.service.ts | 52 +++++++++++++++++++ src/modules/app/browser-app.module.ts | 21 +++++++- src/modules/app/server-app.module.ts | 6 +++ 11 files changed, 171 insertions(+), 51 deletions(-) create mode 100644 src/app/core/reload/reload.guard.ts create mode 100644 src/app/core/services/browser-hard-redirect.service.ts create mode 100644 src/app/core/services/hard-redirect.service.ts create mode 100644 src/app/core/services/server-hard-redirect.service.ts diff --git a/src/app/app-routing.module.ts b/src/app/app-routing.module.ts index c8ee6ecd8b8..a317cf93340 100644 --- a/src/app/app-routing.module.ts +++ b/src/app/app-routing.module.ts @@ -12,6 +12,7 @@ import { getItemPageRoute } from './+item-page/item-page-routing.module'; import { getCollectionPageRoute } from './+collection-page/collection-page-routing.module'; import { SiteAdministratorGuard } from './core/data/feature-authorization/feature-authorization-guard/site-administrator.guard'; import { UnauthorizedComponent } from './unauthorized/unauthorized.component'; +import { ReloadGuard } from './core/reload/reload.guard'; const ITEM_MODULE_PATH = 'items'; @@ -88,7 +89,7 @@ export function getUnauthorizedPath() { imports: [ RouterModule.forRoot([ { path: '', redirectTo: '/home', pathMatch: 'full' }, - { path: 'reload/:rnd', 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' }, diff --git a/src/app/core/auth/auth.effects.ts b/src/app/core/auth/auth.effects.ts index 37ef3b79bc5..7d1750c04ec 100644 --- a/src/app/core/auth/auth.effects.ts +++ b/src/app/core/auth/auth.effects.ts @@ -201,13 +201,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.service.ts b/src/app/core/auth/auth.service.ts index 7d854d9d4de..b8e6c6609a5 100644 --- a/src/app/core/auth/auth.service.ts +++ b/src/app/core/auth/auth.service.ts @@ -36,6 +36,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,7 +63,8 @@ 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), @@ -440,26 +442,18 @@ export class AuthService { } 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); + 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); } /** diff --git a/src/app/core/auth/authenticated.guard.ts b/src/app/core/auth/authenticated.guard.ts index 7a2f39854c5..2e70a70310f 100644 --- a/src/app/core/auth/authenticated.guard.ts +++ b/src/app/core/auth/authenticated.guard.ts @@ -1,21 +1,27 @@ import { Injectable } from '@angular/core'; -import { ActivatedRouteSnapshot, CanActivate, CanLoad, Route, Router, RouterStateSnapshot } from '@angular/router'; +import { + ActivatedRouteSnapshot, + CanActivate, + Route, + Router, + RouterStateSnapshot, + UrlTree +} from '@angular/router'; import { Observable } from 'rxjs'; -import { take } from 'rxjs/operators'; +import { map } 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 { 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 +30,38 @@ 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 { + private handleAuth(url: string): Observable { // get observable const observable = this.store.pipe(select(isAuthenticated)); // 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 observable.pipe( + 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/core.module.ts b/src/app/core/core.module.ts index 5aa462d5e0e..bc2f80830ce 100644 --- a/src/app/core/core.module.ts +++ b/src/app/core/core.module.ts @@ -162,6 +162,7 @@ import { SubmissionCcLicenceUrl } from './submission/models/submission-cc-licens import { SubmissionCcLicenseUrlDataService } from './submission/submission-cc-license-url-data.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 @@ -289,6 +290,7 @@ const PROVIDERS = [ MetadataSchemaDataService, MetadataFieldDataService, TokenResponseParsingService, + ReloadGuard, // register AuthInterceptor as HttpInterceptor { provide: HTTP_INTERCEPTORS, diff --git a/src/app/core/reload/reload.guard.ts b/src/app/core/reload/reload.guard.ts new file mode 100644 index 00000000000..9880fabb695 --- /dev/null +++ b/src/app/core/reload/reload.guard.ts @@ -0,0 +1,17 @@ +import { ActivatedRouteSnapshot, CanActivate, Router, RouterStateSnapshot, UrlTree } from '@angular/router'; +import { Injectable } from '@angular/core'; +import { isNotEmpty } from '../../shared/empty.util'; + +@Injectable() +export class ReloadGuard implements CanActivate { + constructor(private router: Router) { + } + + 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.ts b/src/app/core/services/browser-hard-redirect.service.ts new file mode 100644 index 00000000000..56c90542972 --- /dev/null +++ b/src/app/core/services/browser-hard-redirect.service.ts @@ -0,0 +1,19 @@ +import {Inject, Injectable} from '@angular/core'; +import {LocationToken} from '../../../modules/app/browser-app.module'; + +@Injectable() +export class BrowserHardRedirectService { + + constructor( + @Inject(LocationToken) protected location: Location, + ) { + } + + redirect(url: string) { + this.location.href = url; + } + + getOriginFromUrl() { + return this.location.origin; + } +} 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..e2c18b61386 --- /dev/null +++ b/src/app/core/services/hard-redirect.service.ts @@ -0,0 +1,21 @@ +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 origin of a request + */ + abstract getOriginFromUrl(); +} 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..4d58443cf48 --- /dev/null +++ b/src/app/core/services/server-hard-redirect.service.ts @@ -0,0 +1,52 @@ +import { Inject, Injectable } from '@angular/core'; +import { Request, Response } from 'express'; +import { REQUEST, RESPONSE } from '@nguniversal/express-engine/tokens'; + +@Injectable() +export class ServerHardRedirectService { + + constructor( + @Inject(REQUEST) protected req: Request, + @Inject(RESPONSE) protected res: Response, + ) { + } + + 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. + } + } + + getOriginFromUrl() { + + return new URL(`${this.req.protocol}://${this.req.get('hostname')}`).toString(); + } +} diff --git a/src/modules/app/browser-app.module.ts b/src/modules/app/browser-app.module.ts index 73a49b02117..44f00fbae47 100644 --- a/src/modules/app/browser-app.module.ts +++ b/src/modules/app/browser-app.module.ts @@ -1,5 +1,5 @@ import { HttpClient, HttpClientModule } from '@angular/common/http'; -import { NgModule } from '@angular/core'; +import { InjectionToken, NgModule } from '@angular/core'; import { BrowserModule, makeStateKey, TransferState } from '@angular/platform-browser'; import { BrowserAnimationsModule } from '@angular/platform-browser/animations'; import { RouterModule } from '@angular/router'; @@ -21,6 +21,8 @@ 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 } from '../../app/core/services/browser-hard-redirect.service'; export const REQ_KEY = makeStateKey('req'); @@ -32,6 +34,13 @@ export function getRequest(transferState: TransferState): any { return transferState.get(REQ_KEY, {}); } +export const LocationToken = new InjectionToken('Location'); + +export function locationProvider(): Location { + return window.location; +} + + @NgModule({ bootstrap: [AppComponent], imports: [ @@ -78,7 +87,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 { From 7fbae8997d76c91f93c7d629d9394acab55aa165 Mon Sep 17 00:00:00 2001 From: Kristof De Langhe Date: Wed, 26 Aug 2020 16:32:03 +0200 Subject: [PATCH 02/15] 72699: JSDocs and Test cases --- src/app/core/auth/auth.service.spec.ts | 34 ++++++++----- src/app/core/reload/reload.guard.spec.ts | 47 ++++++++++++++++++ src/app/core/reload/reload.guard.ts | 9 ++++ .../browser-hard-redirect.service.spec.ts | 40 ++++++++++++++++ .../services/browser-hard-redirect.service.ts | 10 ++++ .../server-hard-redirect.service.spec.ts | 48 +++++++++++++++++++ .../services/server-hard-redirect.service.ts | 10 ++++ 7 files changed, 187 insertions(+), 11 deletions(-) create mode 100644 src/app/core/reload/reload.guard.spec.ts create mode 100644 src/app/core/services/browser-hard-redirect.service.spec.ts create mode 100644 src/app/core/services/server-hard-redirect.service.spec.ts diff --git a/src/app/core/auth/auth.service.spec.ts b/src/app/core/auth/auth.service.spec.ts index 7f2c1e29ccd..ccc77bd4138 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); @@ -320,34 +324,34 @@ describe('AuthService test', () => { 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'); + // 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'); + // 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('/'); + // 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('/'); + // Reload without a redirect URL + expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('/reload/[0-9]*(?!\\?)$'))); }); describe('impersonate', () => { @@ -464,6 +468,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 +508,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/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 index 9880fabb695..78f9dcf642b 100644 --- a/src/app/core/reload/reload.guard.ts +++ b/src/app/core/reload/reload.guard.ts @@ -2,11 +2,20 @@ import { ActivatedRouteSnapshot, CanActivate, Router, RouterStateSnapshot, UrlTr 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); 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..b94b52d46ea --- /dev/null +++ b/src/app/core/services/browser-hard-redirect.service.spec.ts @@ -0,0 +1,40 @@ +import {async, TestBed} from '@angular/core/testing'; +import {BrowserHardRedirectService} from './browser-hard-redirect.service'; + +describe('BrowserHardRedirectService', () => { + + const mockLocation = { + href: undefined, + origin: 'test origin', + } 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 origin', () => { + + it('should return the location origin', () => { + expect(service.getOriginFromUrl()).toEqual('test origin'); + }); + }); +}); diff --git a/src/app/core/services/browser-hard-redirect.service.ts b/src/app/core/services/browser-hard-redirect.service.ts index 56c90542972..71ce7577d2c 100644 --- a/src/app/core/services/browser-hard-redirect.service.ts +++ b/src/app/core/services/browser-hard-redirect.service.ts @@ -1,6 +1,9 @@ import {Inject, Injectable} from '@angular/core'; import {LocationToken} from '../../../modules/app/browser-app.module'; +/** + * Service for performing hard redirects within the browser app module + */ @Injectable() export class BrowserHardRedirectService { @@ -9,10 +12,17 @@ export class BrowserHardRedirectService { ) { } + /** + * Perform a hard redirect to URL + * @param url + */ redirect(url: string) { this.location.href = url; } + /** + * Get the origin of a request + */ getOriginFromUrl() { return this.location.origin; } 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..9704c64c745 --- /dev/null +++ b/src/app/core/services/server-hard-redirect.service.spec.ts @@ -0,0 +1,48 @@ +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 origin', () => { + + beforeEach(() => { + mockRequest.protocol = 'test-protocol'; + mockRequest.get.and.callFake((name) => { + if (name === 'hostname') { + return 'test-host'; + } + }); + }); + + it('should return the location origin', () => { + expect(service.getOriginFromUrl()).toEqual('test-protocol://test-host'); + }); + }); +}); diff --git a/src/app/core/services/server-hard-redirect.service.ts b/src/app/core/services/server-hard-redirect.service.ts index 4d58443cf48..69b67394457 100644 --- a/src/app/core/services/server-hard-redirect.service.ts +++ b/src/app/core/services/server-hard-redirect.service.ts @@ -2,6 +2,9 @@ import { Inject, Injectable } from '@angular/core'; import { Request, Response } from 'express'; import { REQUEST, RESPONSE } from '@nguniversal/express-engine/tokens'; +/** + * Service for performing hard redirects within the server app module + */ @Injectable() export class ServerHardRedirectService { @@ -11,6 +14,10 @@ export class ServerHardRedirectService { ) { } + /** + * Perform a hard redirect to URL + * @param url + */ redirect(url: string) { if (url === this.req.url) { @@ -45,6 +52,9 @@ export class ServerHardRedirectService { } } + /** + * Get the origin of a request + */ getOriginFromUrl() { return new URL(`${this.req.protocol}://${this.req.get('hostname')}`).toString(); From 55c45f5f6c97a9795ba5d61d755e0c8ed372796b Mon Sep 17 00:00:00 2001 From: Kristof De Langhe Date: Thu, 27 Aug 2020 14:50:13 +0200 Subject: [PATCH 03/15] 72699: Hard redirect after log in - loading fixes --- src/app/app.component.html | 5 +- src/app/app.component.scss | 4 ++ src/app/app.component.spec.ts | 28 +++++---- src/app/app.component.ts | 13 ++++- src/app/core/auth/auth.actions.ts | 19 +++++- src/app/core/auth/auth.effects.ts | 22 ++++++- src/app/core/auth/auth.reducer.ts | 7 ++- src/app/core/auth/auth.service.spec.ts | 25 ++++---- src/app/core/auth/auth.service.ts | 58 ++++++++++--------- src/app/core/auth/authenticated.guard.ts | 11 ++-- src/app/core/auth/server-auth.service.ts | 28 --------- .../browser-hard-redirect.service.spec.ts | 7 ++- .../services/browser-hard-redirect.service.ts | 11 ++-- .../core/services/hard-redirect.service.ts | 5 +- .../server-hard-redirect.service.spec.ts | 11 +--- .../services/server-hard-redirect.service.ts | 8 +-- .../log-in-container.component.spec.ts | 8 +++ .../shared/log-in/log-in.component.spec.ts | 7 +++ src/app/shared/log-in/log-in.component.ts | 32 +--------- .../log-in-password.component.spec.ts | 10 +++- .../password/log-in-password.component.ts | 8 +++ .../log-in-shibboleth.component.spec.ts | 7 +++ .../shibboleth/log-in-shibboleth.component.ts | 7 +++ src/app/shared/testing/auth-service.stub.ts | 8 +++ src/modules/app/browser-app.module.ts | 1 - 25 files changed, 204 insertions(+), 146 deletions(-) diff --git a/src/app/app.component.html b/src/app/app.component.html index 8656970f319..6d6f89ea35e 100644 --- a/src/app/app.component.html +++ b/src/app/app.component.html @@ -1,4 +1,7 @@ -
+
+ +
+
{ }); it('should set redirect url to previous page', () => { - spyOn(routeServiceMock, 'getHistory').and.callThrough(); - authService.redirectAfterLoginSuccess(true); - expect(routeServiceMock.getHistory).toHaveBeenCalled(); + (storage.get as jasmine.Spy).and.returnValue('/collection/123'); + authService.redirectAfterLoginSuccess(); // 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(); - authService.redirectAfterLoginSuccess(false); - expect(routeServiceMock.getHistory).toHaveBeenCalled(); + (storage.get as jasmine.Spy).and.returnValue('/home'); + authService.redirectAfterLoginSuccess(); // 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'])); - authService.redirectAfterLoginSuccess(true); - expect(routeServiceMock.getHistory).toHaveBeenCalled(); + it('should redirect to regular reload and not to /login', () => { + (storage.get as jasmine.Spy).and.returnValue('/login'); + authService.redirectAfterLoginSuccess(); // 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([''])); - authService.redirectAfterLoginSuccess(true); - expect(routeServiceMock.getHistory).toHaveBeenCalled(); + it('should not redirect when no redirect url is found', () => { + authService.redirectAfterLoginSuccess(); // Reload without a redirect URL - expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('/reload/[0-9]*(?!\\?)$'))); + expect(hardRedirectService.redirect).not.toHaveBeenCalled(); }); describe('impersonate', () => { diff --git a/src/app/core/auth/auth.service.ts b/src/app/core/auth/auth.service.ts index b8e6c6609a5..f79ae5d0fdb 100644 --- a/src/app/core/auth/auth.service.ts +++ b/src/app/core/auth/auth.service.ts @@ -14,7 +14,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, @@ -413,35 +421,19 @@ export class AuthService { /** * Redirect to the route navigated before the login */ - public redirectAfterLoginSuccess(isStandalonePage: boolean) { + public redirectAfterLoginSuccess() { this.getRedirectUrl().pipe( take(1)) .subscribe((redirectUrl) => { - - if (isNotEmpty(redirectUrl)) { + if (hasValue(redirectUrl)) { this.clearRedirectUrl(); - this.router.onSameUrlNavigation = 'reload'; this.navigateToRedirectUrl(redirectUrl); - } else { - // If redirectUrl is empty use history. - this.routeService.getHistory().pipe( - 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) { + public navigateToRedirectUrl(redirectUrl: string) { let url = `/reload/${new Date().getTime()}`; if (isNotEmpty(redirectUrl) && !redirectUrl.startsWith(LOGIN_ROUTE)) { url += `?redirect=${encodeURIComponent(redirectUrl)}`; @@ -460,12 +452,16 @@ export class AuthService { * 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); + } + }) + ); } /** @@ -482,6 +478,16 @@ export class AuthService { this.store.dispatch(new SetRedirectUrlAction(isNotUndefined(url) ? url : '')); } + 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 2e70a70310f..4feb6ebce03 100644 --- a/src/app/core/auth/authenticated.guard.ts +++ b/src/app/core/auth/authenticated.guard.ts @@ -9,11 +9,11 @@ import { } from '@angular/router'; import { Observable } from 'rxjs'; -import { map } 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 { isAuthenticated, isAuthenticationLoading } from './selectors'; import { AuthService, LOGIN_ROUTE } from './auth.service'; /** @@ -48,11 +48,10 @@ export class AuthenticatedGuard implements CanActivate { } private handleAuth(url: string): Observable { - // get observable - const observable = this.store.pipe(select(isAuthenticated)); - // redirect to sign in page if user is not authenticated - return observable.pipe( + return this.store.pipe(select(isAuthenticationLoading)).pipe( + find((isLoading: boolean) => isLoading === false), + switchMap(() => this.store.pipe(select(isAuthenticated))), map((authenticated) => { if (authenticated) { return authenticated; diff --git a/src/app/core/auth/server-auth.service.ts b/src/app/core/auth/server-auth.service.ts index 7b78255001f..84a74548ce4 100644 --- a/src/app/core/auth/server-auth.service.ts +++ b/src/app/core/auth/server-auth.service.ts @@ -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/services/browser-hard-redirect.service.spec.ts b/src/app/core/services/browser-hard-redirect.service.spec.ts index b94b52d46ea..7eb2c6f494e 100644 --- a/src/app/core/services/browser-hard-redirect.service.spec.ts +++ b/src/app/core/services/browser-hard-redirect.service.spec.ts @@ -5,7 +5,8 @@ describe('BrowserHardRedirectService', () => { const mockLocation = { href: undefined, - origin: 'test origin', + pathname: '/pathname', + search: '/search', } as Location; const service: BrowserHardRedirectService = new BrowserHardRedirectService(mockLocation); @@ -31,10 +32,10 @@ describe('BrowserHardRedirectService', () => { }) }); - describe('when requesting the origin', () => { + describe('when requesting the current route', () => { it('should return the location origin', () => { - expect(service.getOriginFromUrl()).toEqual('test 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 index 71ce7577d2c..725848212f0 100644 --- a/src/app/core/services/browser-hard-redirect.service.ts +++ b/src/app/core/services/browser-hard-redirect.service.ts @@ -1,11 +1,12 @@ -import {Inject, Injectable} from '@angular/core'; -import {LocationToken} from '../../../modules/app/browser-app.module'; +import { Inject, Injectable } from '@angular/core'; +import { LocationToken } from '../../../modules/app/browser-app.module'; +import { HardRedirectService } from './hard-redirect.service'; /** * Service for performing hard redirects within the browser app module */ @Injectable() -export class BrowserHardRedirectService { +export class BrowserHardRedirectService implements HardRedirectService { constructor( @Inject(LocationToken) protected location: Location, @@ -23,7 +24,7 @@ export class BrowserHardRedirectService { /** * Get the origin of a request */ - getOriginFromUrl() { - return this.location.origin; + 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 index e2c18b61386..09757a12507 100644 --- a/src/app/core/services/hard-redirect.service.ts +++ b/src/app/core/services/hard-redirect.service.ts @@ -15,7 +15,8 @@ export abstract class HardRedirectService { abstract redirect(url: string); /** - * Get the origin of a request + * 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 getOriginFromUrl(); + 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 index 9704c64c745..2d09c21eb93 100644 --- a/src/app/core/services/server-hard-redirect.service.spec.ts +++ b/src/app/core/services/server-hard-redirect.service.spec.ts @@ -30,19 +30,14 @@ describe('ServerHardRedirectService', () => { }) }); - describe('when requesting the origin', () => { + describe('when requesting the current route', () => { beforeEach(() => { - mockRequest.protocol = 'test-protocol'; - mockRequest.get.and.callFake((name) => { - if (name === 'hostname') { - return 'test-host'; - } - }); + mockRequest.originalUrl = 'original/url'; }); it('should return the location origin', () => { - expect(service.getOriginFromUrl()).toEqual('test-protocol://test-host'); + 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 index 69b67394457..79755d2dc93 100644 --- a/src/app/core/services/server-hard-redirect.service.ts +++ b/src/app/core/services/server-hard-redirect.service.ts @@ -1,12 +1,13 @@ 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 { +export class ServerHardRedirectService implements HardRedirectService { constructor( @Inject(REQUEST) protected req: Request, @@ -55,8 +56,7 @@ export class ServerHardRedirectService { /** * Get the origin of a request */ - getOriginFromUrl() { - - return new URL(`${this.req.protocol}://${this.req.get('hostname')}`).toString(); + getCurrentRoute() { + return this.req.originalUrl; } } 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/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 32e10fef458..301eb1736b5 100644 --- a/src/app/shared/log-in/log-in.component.ts +++ b/src/app/shared/log-in/log-in.component.ts @@ -1,13 +1,9 @@ -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 { CoreState } from '../../core/core.reducers'; -import { AuthService } from '../../core/auth/auth.service'; import { getForgotPasswordPath, getRegisterPath } from '../../app-routing.module'; /** @@ -19,7 +15,7 @@ import { getForgotPasswordPath, getRegisterPath } from '../../app-routing.module 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 +41,7 @@ export class LogInComponent implements OnInit, OnDestroy { */ public loading: Observable; - /** - * Component state. - * @type {boolean} - */ - private alive = true; - - constructor(private store: Store, - private authService: AuthService,) { + constructor(private store: Store) { } ngOnInit(): void { @@ -66,21 +55,6 @@ 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; } getRegisterPath() { 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..cac10522387 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,8 @@ describe('LogInPasswordComponent', () => { ], providers: [ { provide: AuthService, useClass: AuthServiceStub }, - { provide: 'authMethodProvider', useValue: new AuthMethod(AuthMethodType.Password) } + { provide: 'authMethodProvider', useValue: new AuthMethod(AuthMethodType.Password) }, + { 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..1d144a280e2 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,15 @@ export class LogInPasswordComponent implements OnInit { /** * @constructor * @param {AuthMethod} injectedAuthMethodModel + * @param {AuthService} authService + * @param {HardRedirectService} hardRedirectService * @param {FormBuilder} formBuilder * @param {Store} store */ constructor( @Inject('authMethodProvider') public injectedAuthMethodModel: AuthMethod, + private authService: AuthService, + private hardRedirectService: HardRedirectService, private formBuilder: FormBuilder, private store: Store ) { @@ -134,6 +140,8 @@ export class LogInPasswordComponent implements OnInit { email.trim(); password.trim(); + this.authService.setRedirectUrlIfNotSet(this.hardRedirectService.getCurrentRoute()); + // 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..b029ec63f24 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,10 @@ describe('LogInShibbolethComponent', () => { loaded: false, loading: false, }; + + hardRedirectService = jasmine.createSpyObj('hardRedirectService', { + getCurrentRoute: {} + }); }); beforeEach(async(() => { @@ -59,6 +65,7 @@ describe('LogInShibbolethComponent', () => { { provide: NativeWindowService, useFactory: NativeWindowMockFactory }, { provide: Router, useValue: new RouterStub() }, { provide: ActivatedRoute, useValue: new ActivatedRouteStub() }, + { provide: HardRedirectService, useValue: hardRedirectService }, ], schemas: [ CUSTOM_ELEMENTS_SCHEMA 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..bb5791bd60b 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 @@ -12,6 +12,8 @@ import { isAuthenticated, isAuthenticationLoading } from '../../../../core/auth/ import { RouteService } from '../../../../core/services/route.service'; import { NativeWindowRef, NativeWindowService } from '../../../../core/services/window.service'; import { isNotNull } from '../../../empty.util'; +import { AuthService } from '../../../../core/auth/auth.service'; +import { HardRedirectService } from '../../../../core/services/hard-redirect.service'; @Component({ selector: 'ds-log-in-shibboleth', @@ -51,12 +53,16 @@ export class LogInShibbolethComponent implements OnInit { * @param {AuthMethod} injectedAuthMethodModel * @param {NativeWindowRef} _window * @param {RouteService} route + * @param {AuthService} authService + * @param {HardRedirectService} hardRedirectService * @param {Store} store */ constructor( @Inject('authMethodProvider') public injectedAuthMethodModel: AuthMethod, @Inject(NativeWindowService) protected _window: NativeWindowRef, private route: RouteService, + private authService: AuthService, + private hardRedirectService: HardRedirectService, private store: Store ) { this.authMethod = injectedAuthMethodModel; @@ -75,6 +81,7 @@ export class LogInShibbolethComponent implements OnInit { } redirectToShibboleth() { + this.authService.setRedirectUrlIfNotSet(this.hardRedirectService.getCurrentRoute()) let newLocationUrl = this.location; const currentUrl = this._window.nativeWindow.location.href; const myRegexp = /\?redirectUrl=(.*)/g; diff --git a/src/app/shared/testing/auth-service.stub.ts b/src/app/shared/testing/auth-service.stub.ts index 7e7e70a754c..2f1e9e3bac0 100644 --- a/src/app/shared/testing/auth-service.stub.ts +++ b/src/app/shared/testing/auth-service.stub.ts @@ -154,4 +154,12 @@ export class AuthServiceStub { resetAuthenticationError() { return; } + + setRedirectUrlIfNotSet(url: string) { + return; + } + + redirectAfterLoginSuccess() { + return; + } } diff --git a/src/modules/app/browser-app.module.ts b/src/modules/app/browser-app.module.ts index 44f00fbae47..7306e7db6c5 100644 --- a/src/modules/app/browser-app.module.ts +++ b/src/modules/app/browser-app.module.ts @@ -40,7 +40,6 @@ export function locationProvider(): Location { return window.location; } - @NgModule({ bootstrap: [AppComponent], imports: [ From a3df6ce47469506688c656a42fd9d3c7998e464c Mon Sep 17 00:00:00 2001 From: Kristof De Langhe Date: Thu, 27 Aug 2020 14:56:20 +0200 Subject: [PATCH 04/15] 72699: Remove unnecessary method --- src/app/core/auth/auth.service.ts | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/src/app/core/auth/auth.service.ts b/src/app/core/auth/auth.service.ts index f79ae5d0fdb..1a3dfbbdc04 100644 --- a/src/app/core/auth/auth.service.ts +++ b/src/app/core/auth/auth.service.ts @@ -419,20 +419,9 @@ export class AuthService { } /** - * Redirect to the route navigated before the login + * Perform a hard redirect to the URL + * @param redirectUrl */ - public redirectAfterLoginSuccess() { - this.getRedirectUrl().pipe( - take(1)) - .subscribe((redirectUrl) => { - if (hasValue(redirectUrl)) { - this.clearRedirectUrl(); - this.navigateToRedirectUrl(redirectUrl); - } - }); - - } - public navigateToRedirectUrl(redirectUrl: string) { let url = `/reload/${new Date().getTime()}`; if (isNotEmpty(redirectUrl) && !redirectUrl.startsWith(LOGIN_ROUTE)) { From 61e0b9efb09288021e5fd9a34cbb560fdfa6748c Mon Sep 17 00:00:00 2001 From: Kristof De Langhe Date: Fri, 28 Aug 2020 11:40:22 +0200 Subject: [PATCH 05/15] 72699: Remove redundant subscribe + fix tests --- src/app/core/auth/auth.service.spec.ts | 19 +++++++--------- src/app/core/auth/auth.service.ts | 31 -------------------------- 2 files changed, 8 insertions(+), 42 deletions(-) diff --git a/src/app/core/auth/auth.service.spec.ts b/src/app/core/auth/auth.service.spec.ts index be309b15fb7..d3c2b6c44d5 100644 --- a/src/app/core/auth/auth.service.spec.ts +++ b/src/app/core/auth/auth.service.spec.ts @@ -322,31 +322,28 @@ describe('AuthService test', () => { expect(storage.remove).toHaveBeenCalled(); }); - it('should set redirect url to previous page', () => { - (storage.get as jasmine.Spy).and.returnValue('/collection/123'); - authService.redirectAfterLoginSuccess(); + 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', () => { - (storage.get as jasmine.Spy).and.returnValue('/home'); - authService.redirectAfterLoginSuccess(); + 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 regular reload and not to /login', () => { - (storage.get as jasmine.Spy).and.returnValue('/login'); - authService.redirectAfterLoginSuccess(); + authService.navigateToRedirectUrl('/login'); // Reload without a redirect URL expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('/reload/[0-9]*(?!\\?)$'))); }); - it('should not redirect when no redirect url is found', () => { - authService.redirectAfterLoginSuccess(); + it('should redirect to regular reload when no redirect url is found', () => { + authService.navigateToRedirectUrl(undefined); // Reload without a redirect URL - expect(hardRedirectService.redirect).not.toHaveBeenCalled(); + expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('/reload/[0-9]*(?!\\?)$'))); }); describe('impersonate', () => { diff --git a/src/app/core/auth/auth.service.ts b/src/app/core/auth/auth.service.ts index 1a3dfbbdc04..150e44296eb 100644 --- a/src/app/core/auth/auth.service.ts +++ b/src/app/core/auth/auth.service.ts @@ -78,37 +78,6 @@ export class AuthService { 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; } /** From 724e5d1f12574a5454d5828d425abd3b2c283634 Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Tue, 1 Sep 2020 10:10:39 +0200 Subject: [PATCH 06/15] add blocking state to make dealing with log in errors more user friendly --- src/app/app-routing.module.ts | 82 ++++++++++--------- src/app/app.component.html | 10 ++- src/app/app.component.ts | 15 ++-- src/app/app.module.ts | 14 +++- src/app/core/auth/auth-blocking.guard.spec.ts | 62 ++++++++++++++ src/app/core/auth/auth-blocking.guard.ts | 31 +++++++ src/app/core/auth/auth.reducer.spec.ts | 54 +++++++++++- src/app/core/auth/auth.reducer.ts | 21 ++++- src/app/core/auth/selectors.ts | 18 ++++ .../auth-nav-menu.component.html | 2 +- .../auth-nav-menu.component.spec.ts | 2 + .../user-menu/user-menu.component.spec.ts | 2 + .../impersonate-navbar.component.spec.ts | 1 + 13 files changed, 253 insertions(+), 61 deletions(-) create mode 100644 src/app/core/auth/auth-blocking.guard.spec.ts create mode 100644 src/app/core/auth/auth-blocking.guard.ts diff --git a/src/app/app-routing.module.ts b/src/app/app-routing.module.ts index a317cf93340..51a84d0a1eb 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'; @@ -88,45 +89,48 @@ export function getUnauthorizedPath() { @NgModule({ imports: [ RouterModule.forRoot([ - { 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: '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: '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 6d6f89ea35e..b628424cd46 100644 --- a/src/app/app.component.html +++ b/src/app/app.component.html @@ -1,7 +1,4 @@ -
- -
-
+
+ +
+ diff --git a/src/app/app.component.ts b/src/app/app.component.ts index d5488be6106..3f03cd8dbe9 100644 --- a/src/app/app.component.ts +++ b/src/app/app.component.ts @@ -1,4 +1,4 @@ -import { delay, filter, map, take, distinctUntilChanged } 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, isAuthenticationLoading } from './core/auth/selectors'; +import { isAuthenticationBlocking, isAuthenticationLoading } 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'; @@ -55,7 +55,7 @@ export class AppComponent implements OnInit, AfterViewInit { /** * Whether or not the authenticated has finished loading */ - hasAuthFinishedLoading$: Observable; + isAuthBlocking$: Observable; constructor( @Inject(NativeWindowService) private _window: NativeWindowRef, @@ -94,8 +94,8 @@ export class AppComponent implements OnInit, AfterViewInit { } ngOnInit() { - this.hasAuthFinishedLoading$ = this.store.pipe(select(isAuthenticationLoading)).pipe( - map((isLoading: boolean) => isLoading === false), + this.isAuthBlocking$ = this.store.pipe(select(isAuthenticationBlocking)).pipe( + map((isBlocking: boolean) => isBlocking === false), distinctUntilChanged() ); const env: string = environment.production ? 'Production' : 'Development'; @@ -103,11 +103,6 @@ export class AppComponent implements OnInit, AfterViewInit { 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..7488c0c508d --- /dev/null +++ b/src/app/core/auth/auth-blocking.guard.ts @@ -0,0 +1,31 @@ +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, tap } 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) { + } + + 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.reducer.spec.ts b/src/app/core/auth/auth.reducer.spec.ts index cf934a7f47b..649002903c3 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,6 +239,7 @@ describe('authReducer', () => { authToken: undefined, error: undefined, loaded: false, + blocking: false, loading: false, info: undefined, refreshing: false, @@ -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 0ffd7d05199..9435dd1b1dc 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,7 +66,8 @@ export interface AuthState { const initialState: AuthState = { authenticated: false, loaded: false, - loading: undefined, + 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 }); @@ -139,6 +148,7 @@ export function authReducer(state: any = initialState, action: AuthActions): Aut authToken: undefined, error: undefined, loaded: false, + blocking: false, loading: false, info: undefined, refreshing: false, @@ -151,6 +161,7 @@ export function authReducer(state: any = initialState, action: AuthActions): Aut authenticated: false, authToken: undefined, loaded: false, + blocking: false, loading: false, info: (action as RedirectWhenTokenExpiredAction as RedirectWhenAuthenticationIsRequiredAction).payload, userId: undefined @@ -181,18 +192,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)] }); @@ -204,6 +218,7 @@ export function authReducer(state: any = initialState, action: AuthActions): Aut case AuthActionTypes.REDIRECT_AFTER_LOGIN_SUCCESS: return Object.assign({}, state, { loading: true, + blocking: true, }); default: 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/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 From 17249e954181217f9ecb78ae43e18c6e734fd631 Mon Sep 17 00:00:00 2001 From: Kristof De Langhe Date: Tue, 1 Sep 2020 16:22:16 +0200 Subject: [PATCH 07/15] 72699: Additional fixes for maintaining and clearing the redirect url on login components --- src/app/core/auth/auth.service.ts | 4 ++++ .../container/log-in-container.component.ts | 7 +++++++ src/app/shared/log-in/log-in.component.html | 2 +- src/app/shared/log-in/log-in.component.ts | 19 +++++++++++++++++-- .../log-in-password.component.spec.ts | 1 + .../password/log-in-password.component.ts | 8 +++++++- .../log-in-shibboleth.component.spec.ts | 1 + .../shibboleth/log-in-shibboleth.component.ts | 8 +++++++- src/app/shared/testing/auth-service.stub.ts | 4 ++++ 9 files changed, 49 insertions(+), 5 deletions(-) diff --git a/src/app/core/auth/auth.service.ts b/src/app/core/auth/auth.service.ts index 150e44296eb..3671507cb05 100644 --- a/src/app/core/auth/auth.service.ts +++ b/src/app/core/auth/auth.service.ts @@ -436,6 +436,10 @@ 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)) 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 5285bc65e4c..af76c74a9e7 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.ts b/src/app/shared/log-in/log-in.component.ts index 301eb1736b5..9e289af603a 100644 --- a/src/app/shared/log-in/log-in.component.ts +++ b/src/app/shared/log-in/log-in.component.ts @@ -2,9 +2,16 @@ import { Component, Input, OnInit } from '@angular/core'; import { Observable } from 'rxjs'; 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 { getForgotPasswordPath, getRegisterPath } from '../../app-routing.module'; +import { hasValue } from '../empty.util'; +import { AuthService } from '../../core/auth/auth.service'; /** * /users/sign-in @@ -41,7 +48,8 @@ export class LogInComponent implements OnInit { */ public loading: Observable; - constructor(private store: Store) { + constructor(private store: Store, + private authService: AuthService) { } ngOnInit(): void { @@ -55,6 +63,13 @@ export class LogInComponent implements OnInit { // set isAuthenticated this.isAuthenticated = this.store.pipe(select(isAuthenticated)); + + // 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(); + } + }); } getRegisterPath() { 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 cac10522387..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 @@ -55,6 +55,7 @@ describe('LogInPasswordComponent', () => { providers: [ { provide: AuthService, useClass: AuthServiceStub }, { provide: 'authMethodProvider', useValue: new AuthMethod(AuthMethodType.Password) }, + { provide: 'isStandalonePage', useValue: true }, { provide: HardRedirectService, useValue: hardRedirectService }, ], schemas: [ 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 1d144a280e2..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 @@ -68,6 +68,7 @@ export class LogInPasswordComponent implements OnInit { /** * @constructor * @param {AuthMethod} injectedAuthMethodModel + * @param {boolean} isStandalonePage * @param {AuthService} authService * @param {HardRedirectService} hardRedirectService * @param {FormBuilder} formBuilder @@ -75,6 +76,7 @@ export class LogInPasswordComponent implements OnInit { */ constructor( @Inject('authMethodProvider') public injectedAuthMethodModel: AuthMethod, + @Inject('isStandalonePage') public isStandalonePage: boolean, private authService: AuthService, private hardRedirectService: HardRedirectService, private formBuilder: FormBuilder, @@ -140,7 +142,11 @@ export class LogInPasswordComponent implements OnInit { email.trim(); password.trim(); - this.authService.setRedirectUrlIfNotSet(this.hardRedirectService.getCurrentRoute()); + 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 b029ec63f24..1c5698031de 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 @@ -62,6 +62,7 @@ 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() }, 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 bb5791bd60b..5cc50e46c6f 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 @@ -51,6 +51,7 @@ export class LogInShibbolethComponent implements OnInit { /** * @constructor * @param {AuthMethod} injectedAuthMethodModel + * @param {boolean} isStandalonePage * @param {NativeWindowRef} _window * @param {RouteService} route * @param {AuthService} authService @@ -59,6 +60,7 @@ export class LogInShibbolethComponent implements OnInit { */ constructor( @Inject('authMethodProvider') public injectedAuthMethodModel: AuthMethod, + @Inject('isStandalonePage') public isStandalonePage: boolean, @Inject(NativeWindowService) protected _window: NativeWindowRef, private route: RouteService, private authService: AuthService, @@ -81,7 +83,11 @@ export class LogInShibbolethComponent implements OnInit { } redirectToShibboleth() { - this.authService.setRedirectUrlIfNotSet(this.hardRedirectService.getCurrentRoute()) + if (!this.isStandalonePage) { + this.authService.setRedirectUrl(this.hardRedirectService.getCurrentRoute()); + } else { + this.authService.setRedirectUrlIfNotSet('/'); + } let newLocationUrl = this.location; const currentUrl = this._window.nativeWindow.location.href; const myRegexp = /\?redirectUrl=(.*)/g; diff --git a/src/app/shared/testing/auth-service.stub.ts b/src/app/shared/testing/auth-service.stub.ts index 2f1e9e3bac0..f522ce83063 100644 --- a/src/app/shared/testing/auth-service.stub.ts +++ b/src/app/shared/testing/auth-service.stub.ts @@ -162,4 +162,8 @@ export class AuthServiceStub { redirectAfterLoginSuccess() { return; } + + clearRedirectUrl() { + return; + } } From e9a2b4b368e3def0dc26217cdb02e357276ab601 Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Thu, 3 Sep 2020 12:58:40 +0200 Subject: [PATCH 08/15] made the redirect after logging out blocking as well --- src/app/core/auth/auth.reducer.spec.ts | 4 ++-- src/app/core/auth/auth.reducer.ts | 14 +++++++++++++- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/app/core/auth/auth.reducer.spec.ts b/src/app/core/auth/auth.reducer.spec.ts index 649002903c3..4c6f1e2a256 100644 --- a/src/app/core/auth/auth.reducer.spec.ts +++ b/src/app/core/auth/auth.reducer.spec.ts @@ -239,8 +239,8 @@ describe('authReducer', () => { authToken: undefined, error: undefined, loaded: false, - blocking: false, - loading: false, + blocking: true, + loading: true, info: undefined, refreshing: false, userId: undefined diff --git a/src/app/core/auth/auth.reducer.ts b/src/app/core/auth/auth.reducer.ts index 9435dd1b1dc..6d5635f263b 100644 --- a/src/app/core/auth/auth.reducer.ts +++ b/src/app/core/auth/auth.reducer.ts @@ -141,7 +141,6 @@ 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, @@ -155,6 +154,19 @@ export function authReducer(state: any = initialState, action: AuthActions): Aut 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, { From 5e970bcff23d53c04af29223a40cf4ae387dc888 Mon Sep 17 00:00:00 2001 From: Kristof De Langhe Date: Thu, 3 Sep 2020 13:32:13 +0200 Subject: [PATCH 09/15] 72699: isAuthBlocking$ rename to isNotAuthBlocking$ --- src/app/app.component.html | 2 +- src/app/app.component.ts | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/app/app.component.html b/src/app/app.component.html index b628424cd46..fa534855e79 100644 --- a/src/app/app.component.html +++ b/src/app/app.component.html @@ -1,4 +1,4 @@ -
    +