Proposed authentication service method that sets redirect url and dispatches auth request.

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<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 },
@@ -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<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 +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<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,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');

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 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);
   }
+
 }

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<LogInComponent>;
-  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<AppState>) => {
-    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');
-
-  });
-
 });
 
 /**

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<CoreState>
   ) {
@@ -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)
-    }
-  }
 }

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;
   }

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 */
 };