Merge pull request #467 from mspalti/login_redirect_url

Redirecting user to same page after login.
DSpace · Sep 19, 2019 · d85cc0a · d85cc0a
2 parents 5917d92 + ca04873
commit d85cc0a
Show file tree

Hide file tree

Showing 10 changed files with 105 additions and 24 deletions.
diff --git a/src/app/+login-page/login-page.component.html b/src/app/+login-page/login-page.component.html
@@ -3,7 +3,8 @@
     <div>
       <img class="mb-4 login-logo" src="assets/images/dspace-logo.png">
       <h1 class="h3 mb-0 font-weight-normal">{{"login.form.header" | translate}}</h1>
-      <ds-log-in></ds-log-in>
+      <ds-log-in
+      [isStandalonePage]="true"></ds-log-in>
     </div>
   </div>
 </div>
diff --git a/src/app/core/auth/auth.service.spec.ts b/src/app/core/auth/auth.service.spec.ts
@@ -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<AuthState>;
   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 },
@@ -138,20 +142,21 @@ describe('AuthService test', () => {
           { provide: AuthRequestService, useValue: authRequest },
           { provide: REQUEST, useValue: {} },
           { provide: Router, useValue: routerStub },
+          { provide: RouteService, useValue: routeServiceStub },
           { provide: RemoteDataBuildService, useValue: rdbService },
           CookieService,
           AuthService
         ]
       }).compileComponents();
     }));
 
-    beforeEach(inject([CookieService, AuthRequestService, Store, Router], (cookieService: CookieService, authReqService: AuthRequestService, store: Store<AppState>, router: Router) => {
+    beforeEach(inject([CookieService, AuthRequestService, Store, Router, RouteService], (cookieService: CookieService, authReqService: AuthRequestService, store: Store<AppState>, 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 +194,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 +203,7 @@ describe('AuthService test', () => {
       }).compileComponents();
     }));
 
-    beforeEach(inject([ClientCookieService, AuthRequestService, Store, Router], (cookieService: ClientCookieService, authReqService: AuthRequestService, store: Store<AppState>, router: Router) => {
+    beforeEach(inject([ClientCookieService, AuthRequestService, Store, Router, RouteService], (cookieService: ClientCookieService, authReqService: AuthRequestService, store: Store<AppState>, router: Router, routeService: RouteService) => {
       const expiredToken: AuthTokenInfo = new AuthTokenInfo('test_token');
       expiredToken.expires = Date.now() - (1000 * 60 * 60);
       authenticatedState = {
@@ -212,11 +218,14 @@ 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;
+      routeServiceMock = TestBed.get(RouteService);
+      routerStub = TestBed.get(Router);
       spyOn(storage, 'get');
       spyOn(storage, 'remove');
       spyOn(storage, 'set');
+
     }));
 
     it('should throw false when token is not valid', () => {
@@ -238,5 +247,32 @@ describe('AuthService test', () => {
       expect(storage.remove).toHaveBeenCalled();
     });
 
+    it ('should set redirect url to previous page', () => {
+      spyOn(routeServiceMock, 'getHistory').and.callThrough();
+      authService.redirectAfterLoginSuccess(true);
+      expect(routeServiceMock.getHistory).toHaveBeenCalled();
+      expect(routerStub.navigate).toHaveBeenCalledWith(['/collection/123']);
+    });
+
+    it ('should set redirect url to current page', () => {
+      spyOn(routeServiceMock, 'getHistory').and.callThrough();
+      authService.redirectAfterLoginSuccess(false);
+      expect(routeServiceMock.getHistory).toHaveBeenCalled();
+      expect(routerStub.navigate).toHaveBeenCalledWith(['/home']);
+    });
+
+    it ('should redirect to / and not to /login', () => {
+      spyOn(routeServiceMock, 'getHistory').and.returnValue(observableOf(['/login', '/login']));
+      authService.redirectAfterLoginSuccess(true);
+      expect(routeServiceMock.getHistory).toHaveBeenCalled();
+      expect(routerStub.navigate).toHaveBeenCalledWith(['/']);
+    });
+
+    it ('should redirect to / when no redirect url is found', () => {
+      spyOn(routeServiceMock, 'getHistory').and.returnValue(observableOf(['']));
+      authService.redirectAfterLoginSuccess(true);
+      expect(routeServiceMock.getHistory).toHaveBeenCalled();
+      expect(routerStub.navigate).toHaveBeenCalledWith(['/']);
+    });
   });
 });
diff --git 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<AppState>,
               protected rdbService: RemoteDataBuildService
@@ -337,7 +339,7 @@ export class AuthService {
   /**
    * Redirect to the route navigated before the login
    */
-  public redirectToPreviousUrl() {
+  public redirectAfterLoginSuccess(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);
+          });
         }
-      })
+      });
 
   }
 
+  protected 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/core/auth/server-auth.service.ts b/src/app/core/auth/server-auth.service.ts
@@ -1,12 +1,12 @@
-import { map, switchMap, take } from 'rxjs/operators';
+import { filter, map, switchMap, take } from 'rxjs/operators';
 import { Injectable } from '@angular/core';
 
 import { Observable } from 'rxjs';
 import { HttpHeaders } from '@angular/common/http';
 import { HttpOptions } from '../dspace-rest-v2/dspace-rest-v2.service';
 import { AuthStatus } from './models/auth-status.model';
-import { isNotEmpty } from '../../shared/empty.util';
-import { AuthService } from './auth.service';
+import { isEmpty, isNotEmpty } from '../../shared/empty.util';
+import { AuthService, LOGIN_ROUTE } from './auth.service';
 import { AuthTokenInfo } from './models/auth-token-info.model';
 import { CheckAuthenticationTokenAction } from './auth.actions';
 import { EPerson } from '../eperson/models/eperson.model';
@@ -54,7 +54,7 @@ export class ServerAuthService extends AuthService {
   /**
    * Redirect to the route navigated before the login
    */
-  public redirectToPreviousUrl() {
+  public redirectAfterLoginSuccess(isStandalonePage: boolean) {
     this.getRedirectUrl().pipe(
       take(1))
       .subscribe((redirectUrl) => {
@@ -67,10 +67,15 @@ export class ServerAuthService extends AuthService {
           const url = decodeURIComponent(redirectUrl);
           this.router.navigateByUrl(url);
         } else {
-          this.router.navigate(['/']);
+          // 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/shared/auth-nav-menu/auth-nav-menu.component.html b/src/app/shared/auth-nav-menu/auth-nav-menu.component.html
@@ -3,7 +3,8 @@
     <div ngbDropdown placement="bottom-right" class="d-inline-block" @fadeInOut>
       <a href="#" id="dropdownLogin" (click)="$event.preventDefault()" ngbDropdownToggle class="px-1">{{ 'nav.login' | translate }}</a>
       <div id="loginDropdownMenu" [ngClass]="{'pl-3 pr-3': (loading | async)}" ngbDropdownMenu aria-labelledby="dropdownLogin">
-        <ds-log-in></ds-log-in>
+        <ds-log-in
+        [isStandalonePage]="false"></ds-log-in>
       </div>
     </div>
   </li>

diff --git a/src/app/shared/log-in/log-in.component.spec.ts b/src/app/shared/log-in/log-in.component.spec.ts
@@ -95,6 +95,7 @@ describe('LogInComponent', () => {
     // verify Store.dispatch() is invoked
     expect(page.navigateSpy.calls.any()).toBe(true, 'Store.dispatch not invoked');
   });
+
 });
 
 /**

diff --git a/src/app/shared/log-in/log-in.component.ts b/src/app/shared/log-in/log-in.component.ts
@@ -1,5 +1,5 @@
 import { filter, map, takeWhile } from 'rxjs/operators';
-import { Component, OnDestroy, OnInit } from '@angular/core';
+import { Component, Input, OnDestroy, OnInit } from '@angular/core';
 import { FormBuilder, FormGroup, Validators } from '@angular/forms';
 
 import { select, Store } from '@ngrx/store';
@@ -20,6 +20,7 @@ import { CoreState } from '../../core/core.reducers';
 import { isNotEmpty } from '../empty.util';
 import { fadeOut } from '../animations/fade';
 import { AuthService } from '../../core/auth/auth.service';
+import { Router } from '@angular/router';
 
 /**
  * /users/sign-in
@@ -81,10 +82,13 @@ export class LogInComponent implements OnDestroy, OnInit {
    */
   private alive = true;
 
+  @Input() isStandalonePage: boolean;
+
   /**
    * @constructor
    * @param {AuthService} authService
    * @param {FormBuilder} formBuilder
+   * @param {Router} router
    * @param {Store<State>} store
    */
   constructor(
@@ -135,7 +139,7 @@ export class LogInComponent implements OnDestroy, OnInit {
       takeWhile(() => this.alive),
       filter((authenticated) => authenticated))
       .subscribe(() => {
-          this.authService.redirectToPreviousUrl();
+          this.authService.redirectAfterLoginSuccess(this.isStandalonePage);
         }
       );
   }
@@ -188,4 +192,5 @@ export class LogInComponent implements OnDestroy, OnInit {
     // clear form
     this.form.reset();
   }
+
 }
diff --git a/src/app/shared/testing/auth-service-stub.ts b/src/app/shared/testing/auth-service-stub.ts
@@ -10,6 +10,7 @@ export class AuthServiceStub {
 
   token: AuthTokenInfo = new AuthTokenInfo('token_test');
   private _tokenExpired = false;
+  private redirectUrl;
 
   constructor() {
     this.token.expires = Date.now() + (1000 * 60 * 60);
@@ -88,7 +89,11 @@ export class AuthServiceStub {
   }
 
   setRedirectUrl(url: string) {
-    return;
+    this.redirectUrl = url;
+  }
+
+  getRedirectUrl() {
+    return observableOf(this.redirectUrl);
   }
 
   public storeToken(token: AuthTokenInfo) {

diff --git a/src/app/shared/testing/route-service-stub.ts b/src/app/shared/testing/route-service-stub.ts
@@ -27,6 +27,9 @@ export const routeServiceStub: any = {
   },
   getRouteDataValue: (param) => {
     return observableOf({})
+  },
+  getHistory: () => {
+    return observableOf(['/home','/collection/123','/home'])
   }
   /* tslint:enable:no-empty */
 };
diff --git a/src/modules/app/server-app.module.ts b/src/modules/app/server-app.module.ts
@@ -64,7 +64,7 @@ export function createTranslateLoader() {
     {
       provide: SubmissionService,
       useClass: ServerSubmissionService
-    },
+    }
   ]
 })
 export class ServerAppModule {