diff --git a/src/app/core/auth/auth.service.spec.ts b/src/app/core/auth/auth.service.spec.ts index ab2e6fd86b..6044e9ecb6 100644 --- a/src/app/core/auth/auth.service.spec.ts +++ b/src/app/core/auth/auth.service.spec.ts @@ -4,7 +4,7 @@ import { ActivatedRoute, Router } from '@angular/router'; import { Store, StoreModule } from '@ngrx/store'; import { REQUEST } from '@nguniversal/express-engine/tokens'; -import { of as observableOf } from 'rxjs'; +import {of, of as observableOf} from 'rxjs'; import { authReducer, AuthState } from './auth.reducer'; import { NativeWindowRef, NativeWindowService } from '../services/window.service'; @@ -23,11 +23,14 @@ import { AppState } from '../../app.reducer'; import { ClientCookieService } from '../services/client-cookie.service'; import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; import { getMockRemoteDataBuildService } from '../../shared/mocks/mock-remote-data-build.service'; +import {routeServiceStub} from '../../shared/testing/route-service-stub'; +import {RouteService} from '../services/route.service'; describe('AuthService test', () => { let mockStore: Store; let authService: AuthService; + let routeServiceMock: RouteService; let authRequest; let window; let routerStub; @@ -74,6 +77,7 @@ describe('AuthService test', () => { { provide: NativeWindowService, useValue: window }, { provide: REQUEST, useValue: {} }, { provide: Router, useValue: routerStub }, + { provide: RouteService, useValue: routeServiceStub }, { provide: ActivatedRoute, useValue: routeStub }, { provide: Store, useValue: mockStore }, { provide: RemoteDataBuildService, useValue: rdbService }, @@ -82,6 +86,8 @@ describe('AuthService test', () => { ], }); authService = TestBed.get(AuthService); + routeServiceMock = TestBed.get(RouteService); + spyOn(authService, 'setRedirectUrl'); }); it('should return the authentication status object when user credentials are correct', () => { @@ -124,6 +130,31 @@ describe('AuthService test', () => { expect(authService.logout.bind(null)).toThrow(); }); + it ('should dispatch authentication', () => { + authService.doAuthentication(true, '', ''); + expect(mockStore.dispatch).toHaveBeenCalled(); + }); + + it ('should set redirect url to previous page', () => { + spyOn(routeServiceMock, 'getHistory').and.callThrough(); + authService.doAuthentication(true, '', ''); + expect(routeServiceMock.getHistory).toHaveBeenCalled(); + expect(authService.setRedirectUrl).toHaveBeenCalledWith('/collection/123'); + }); + + it ('should set redirect url to current page', () => { + spyOn(routeServiceMock, 'getHistory').and.callThrough(); + authService.doAuthentication(false, '', ''); + expect(routeServiceMock.getHistory).toHaveBeenCalled(); + expect(authService.setRedirectUrl).toHaveBeenCalledWith('/home'); + }); + + it ('should not set redirect url to /login', () => { + spyOn(routeServiceMock, 'getHistory').and.returnValue(of(['/login', '/login'])); + authService.doAuthentication(true, '', ''); + expect(routeServiceMock.getHistory).toHaveBeenCalled(); + expect(authService.setRedirectUrl).not.toHaveBeenCalled(); + }); }); describe('', () => { @@ -138,6 +169,7 @@ describe('AuthService test', () => { { provide: AuthRequestService, useValue: authRequest }, { provide: REQUEST, useValue: {} }, { provide: Router, useValue: routerStub }, + { provide: RouteService, useValue: routeServiceStub }, { provide: RemoteDataBuildService, useValue: rdbService }, CookieService, AuthService @@ -145,13 +177,13 @@ describe('AuthService test', () => { }).compileComponents(); })); - beforeEach(inject([CookieService, AuthRequestService, Store, Router], (cookieService: CookieService, authReqService: AuthRequestService, store: Store, router: Router) => { + beforeEach(inject([CookieService, AuthRequestService, Store, Router, RouteService], (cookieService: CookieService, authReqService: AuthRequestService, store: Store, router: Router, routeService: RouteService) => { store .subscribe((state) => { (state as any).core = Object.create({}); (state as any).core.auth = authenticatedState; }); - authService = new AuthService({}, window, undefined, authReqService, router, cookieService, store, rdbService); + authService = new AuthService({}, window, undefined, authReqService, router, routeService, cookieService, store, rdbService); })); it('should return true when user is logged in', () => { @@ -189,6 +221,7 @@ describe('AuthService test', () => { { provide: AuthRequestService, useValue: authRequest }, { provide: REQUEST, useValue: {} }, { provide: Router, useValue: routerStub }, + { provide: RouteService, useValue: routeServiceStub }, { provide: RemoteDataBuildService, useValue: rdbService }, ClientCookieService, CookieService, @@ -197,7 +230,7 @@ describe('AuthService test', () => { }).compileComponents(); })); - beforeEach(inject([ClientCookieService, AuthRequestService, Store, Router], (cookieService: ClientCookieService, authReqService: AuthRequestService, store: Store, router: Router) => { + beforeEach(inject([ClientCookieService, AuthRequestService, Store, Router, RouteService], (cookieService: ClientCookieService, authReqService: AuthRequestService, store: Store, router: Router, routeService: RouteService) => { const expiredToken: AuthTokenInfo = new AuthTokenInfo('test_token'); expiredToken.expires = Date.now() - (1000 * 60 * 60); authenticatedState = { @@ -212,7 +245,7 @@ describe('AuthService test', () => { (state as any).core = Object.create({}); (state as any).core.auth = authenticatedState; }); - authService = new AuthService({}, window, undefined, authReqService, router, cookieService, store, rdbService); + authService = new AuthService({}, window, undefined, authReqService, router, routeService, cookieService, store, rdbService); storage = (authService as any).storage; spyOn(storage, 'get'); spyOn(storage, 'remove'); diff --git a/src/app/core/auth/auth.service.ts b/src/app/core/auth/auth.service.ts index 08c94b02f2..615fee5d56 100644 --- a/src/app/core/auth/auth.service.ts +++ b/src/app/core/auth/auth.service.ts @@ -22,6 +22,7 @@ import { ResetAuthenticationMessagesAction, SetRedirectUrlAction } from './auth. import { NativeWindowRef, NativeWindowService } from '../services/window.service'; import { Base64EncodeUrl } from '../../shared/utils/encode-decode.util'; import { RemoteDataBuildService } from '../cache/builders/remote-data-build.service'; +import {RouteService} from '../services/route.service'; export const LOGIN_ROUTE = '/login'; export const LOGOUT_ROUTE = '/logout'; @@ -45,6 +46,7 @@ export class AuthService { protected authRequestService: AuthRequestService, @Optional() @Inject(RESPONSE) private response: any, protected router: Router, + protected routeService: RouteService, protected storage: CookieService, protected store: Store, protected rdbService: RemoteDataBuildService @@ -337,7 +339,7 @@ export class AuthService { /** * Redirect to the route navigated before the login */ - public redirectToPreviousUrl() { + public redirectToPreviousUrl(isStandalonePage: boolean) { this.getRedirectUrl().pipe( take(1)) .subscribe((redirectUrl) => { @@ -346,18 +348,39 @@ export class AuthService { this.clearRedirectUrl(); this.router.onSameUrlNavigation = 'reload'; const url = decodeURIComponent(redirectUrl); - this.router.navigateByUrl(url); - /* 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.navigateToRedirectUrl(url); } else { - this.router.navigate(['/']); - /* 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 = '/'; + // 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); + }); } - }) + }); } + private navigateToRedirectUrl(url: string) { + // 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.navigate(['/']); + /* 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.navigate([url]); + } + } + /** * Refresh route navigated */ @@ -400,4 +423,5 @@ export class AuthService { this.store.dispatch(new SetRedirectUrlAction('')); this.storage.remove(REDIRECT_COOKIE); } + } 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 8ad423629d..6df992fff7 100644 --- a/src/app/shared/log-in/log-in.component.spec.ts +++ b/src/app/shared/log-in/log-in.component.spec.ts @@ -16,8 +16,6 @@ import { AppState } from '../../app.reducer'; import {AppRoutingModule} from '../../app-routing.module'; import {PageNotFoundComponent} from '../../pagenotfound/pagenotfound.component'; import {APP_BASE_HREF} from '@angular/common'; -import {HostWindowService} from '../host-window.service'; -import {HostWindowServiceStub} from '../testing/host-window-service-stub'; import {RouterStub} from '../testing/router-stub'; import {Router} from '@angular/router'; import {RouteService} from '../services/route.service'; @@ -110,135 +108,6 @@ describe('LogInComponent', () => { expect(page.navigateSpy.calls.any()).toBe(true, 'Store.dispatch not invoked'); }); - it('should set the redirect url', () => { - fixture.detectChanges(); - - // set FormControl values - component.form.controls.email.setValue('user'); - component.form.controls.password.setValue('password'); - - const authService: AuthService = TestBed.get(AuthService); - spyOn(authService, 'setRedirectUrl'); - - component.submit(); - - // the redirect url should be set upon submit - expect(authService.setRedirectUrl).toHaveBeenCalled(); - }); - - it('should not set the redirect url to /login', () => { - fixture.detectChanges(); - - const router: Router = TestBed.get(Router); - router.navigateByUrl('/login') - - const authService: AuthService = TestBed.get(AuthService); - - // set FormControl values - component.form.controls.email.setValue('user'); - component.form.controls.password.setValue('password'); - - spyOn(authService, 'setRedirectUrl'); - - component.submit(); - - expect(authService.setRedirectUrl).not.toHaveBeenCalled(); - }); - - it('should not set the redirect url on init', () => { - - const authService: AuthService = TestBed.get(AuthService); - spyOn(authService, 'setRedirectUrl'); - - component.isStandalonePage = false; - - fixture.detectChanges(); - expect(authService.setRedirectUrl).not.toHaveBeenCalledWith(); - - }); - -}); - -describe('LogInComponent on small screen', () => { - - let component: LogInComponent; - let fixture: ComponentFixture; - let page: Page; - let user: EPerson; - - const authState = { - authenticated: false, - loaded: false, - loading: false, - }; - - beforeEach(() => { - user = EPersonMock; - }); - - beforeEach(async(() => { - // refine the test module by declaring the test component - TestBed.configureTestingModule({ - imports: [ - FormsModule, - ReactiveFormsModule, - StoreModule.forRoot(authReducer), - AppRoutingModule, - TranslateModule.forRoot() - ], - declarations: [ - LogInComponent, - PageNotFoundComponent - ], - providers: [ - {provide: AuthService, useClass: AuthServiceStub}, - {provide: APP_BASE_HREF, useValue: '/'}, - {provide: Router, useClass: RouterStub}, - {provide: RouteService, useValue: routeServiceStub } - ], - schemas: [ - CUSTOM_ELEMENTS_SCHEMA - ] - }) - .compileComponents(); - - })); - - beforeEach(inject([Store], (store: Store) => { - store - .subscribe((state) => { - (state as any).core = Object.create({}); - (state as any).core.auth = authState; - }); - - // create component and test fixture - fixture = TestBed.createComponent(LogInComponent); - - // get test component from the fixture - component = fixture.componentInstance; - - // create page - page = new Page(component, fixture); - - // verify the fixture is stable (no pending tasks) - fixture.whenStable().then(() => { - page.addPageElements(); - }); - - })); - - it('should set the redirect url on init', () => { - const authService: AuthService = TestBed.get(AuthService); - spyOn(authService, 'setRedirectUrl'); - component.isStandalonePage = true; - fixture.detectChanges(); - // set FormControl values - component.form.controls.email.setValue('user'); - component.form.controls.password.setValue('password'); - expect(authService.setRedirectUrl).toHaveBeenCalledWith('collection/123'); - - }); - }); /** diff --git a/src/app/shared/log-in/log-in.component.ts b/src/app/shared/log-in/log-in.component.ts index 3a4ff151bf..aa798548f4 100644 --- a/src/app/shared/log-in/log-in.component.ts +++ b/src/app/shared/log-in/log-in.component.ts @@ -1,9 +1,9 @@ -import {filter, map, pairwise, take, takeUntil, takeWhile, tap} from 'rxjs/operators'; +import {filter, map, takeWhile } from 'rxjs/operators'; import {Component, Input, OnDestroy, OnInit} from '@angular/core'; import { FormBuilder, FormGroup, Validators } from '@angular/forms'; import { select, Store } from '@ngrx/store'; -import {Observable, Subject} from 'rxjs'; +import {Observable} from 'rxjs'; import { AuthenticateAction, ResetAuthenticationMessagesAction @@ -19,10 +19,8 @@ import { CoreState } from '../../core/core.reducers'; import {isNotEmpty} from '../empty.util'; import { fadeOut } from '../animations/fade'; -import {AuthService, LOGIN_ROUTE} from '../../core/auth/auth.service'; +import {AuthService} from '../../core/auth/auth.service'; import {Router} from '@angular/router'; -import {HostWindowService} from '../host-window.service'; -import {RouteService} from '../services/route.service'; /** * /users/sign-in @@ -84,8 +82,6 @@ export class LogInComponent implements OnDestroy, OnInit { */ private alive = true; - private redirectUrl = LOGIN_ROUTE; - @Input() isStandalonePage: boolean; /** @@ -99,7 +95,6 @@ export class LogInComponent implements OnDestroy, OnInit { constructor( private authService: AuthService, private formBuilder: FormBuilder, - private routeService: RouteService, private router: Router, private store: Store ) { @@ -113,16 +108,6 @@ export class LogInComponent implements OnDestroy, OnInit { // set isAuthenticated this.isAuthenticated = this.store.pipe(select(isAuthenticated)); - // for mobile login, set the redirect url to the previous route - if (this.isStandalonePage) { - this.routeService.getHistory().pipe( - take(1) - ).subscribe((history) => { - const previousIndex = history.length - 2; - this.setRedirectUrl(history[previousIndex]); - }); - } - // set formGroup this.form = this.formBuilder.group({ email: ['', Validators.required], @@ -156,7 +141,7 @@ export class LogInComponent implements OnDestroy, OnInit { takeWhile(() => this.alive), filter((authenticated) => authenticated)) .subscribe(() => { - this.authService.redirectToPreviousUrl(); + this.authService.redirectToPreviousUrl(this.isStandalonePage); } ); } @@ -203,21 +188,11 @@ export class LogInComponent implements OnDestroy, OnInit { email.trim(); password.trim(); - this.setRedirectUrl(this.router.url); // dispatch AuthenticationAction this.store.dispatch(new AuthenticateAction(email, password)); + // clear form this.form.reset(); - } - /** - * Sets the redirect url if not equal to LOGIN_ROUTE - * @param url - */ - private setRedirectUrl(url: string) { - if (url !== this.redirectUrl) { - this.authService.setRedirectUrl(url) - } - } } diff --git a/src/app/shared/testing/auth-service-stub.ts b/src/app/shared/testing/auth-service-stub.ts index a6d24d5c8b..c5f9d92a53 100644 --- a/src/app/shared/testing/auth-service-stub.ts +++ b/src/app/shared/testing/auth-service-stub.ts @@ -96,6 +96,9 @@ export class AuthServiceStub { return observableOf(this.redirectUrl); } + public doAuthentication(isStandalonePage, email, password) { + return; + } public storeToken(token: AuthTokenInfo) { return; } diff --git a/src/app/shared/testing/route-service-stub.ts b/src/app/shared/testing/route-service-stub.ts index 22e039a815..a493f10a13 100644 --- a/src/app/shared/testing/route-service-stub.ts +++ b/src/app/shared/testing/route-service-stub.ts @@ -29,7 +29,7 @@ export const routeServiceStub: any = { return observableOf({}) }, getHistory: () => { - return observableOf(['/home','collection/123','/login']) + return observableOf(['/home','/collection/123','/home']) } /* tslint:enable:no-empty */ };