From 4488a450c0dbaa7bfb048132a4f1d81c7405c57a Mon Sep 17 00:00:00 2001 From: Giuseppe Digilio Date: Tue, 13 Feb 2018 12:56:07 +0100 Subject: [PATCH] Fixed authentication module --- src/app/+login-page/login-page.component.ts | 15 ++++++- src/app/core/auth/auth.actions.ts | 28 ++++++++++--- src/app/core/auth/auth.effects.ts | 5 ++- src/app/core/auth/auth.interceptor.ts | 11 +++-- src/app/core/auth/auth.reducers.ts | 46 +++++++++++---------- src/app/core/auth/auth.service.ts | 26 ++++++++---- src/app/core/auth/authenticated.guard.ts | 18 ++++---- src/app/core/auth/selectors.ts | 6 +-- src/app/shared/log-in/log-in.component.ts | 8 ++-- 9 files changed, 104 insertions(+), 59 deletions(-) diff --git a/src/app/+login-page/login-page.component.ts b/src/app/+login-page/login-page.component.ts index 14bb443a4e..2752973130 100644 --- a/src/app/+login-page/login-page.component.ts +++ b/src/app/+login-page/login-page.component.ts @@ -1,10 +1,21 @@ -import { Component } from '@angular/core'; +import { Component, OnDestroy } from '@angular/core'; + +import { Store } from '@ngrx/store'; + +import { AppState } from '../app.reducer'; +import { ResetAuthenticationMessagesAction } from '../core/auth/auth.actions'; @Component({ selector: 'ds-login-page', styleUrls: ['./login-page.component.scss'], templateUrl: './login-page.component.html' }) -export class LoginPageComponent { +export class LoginPageComponent implements OnDestroy { + constructor(private store: Store) {} + + ngOnDestroy() { + // Clear all authentication messages when leaving login page + this.store.dispatch(new ResetAuthenticationMessagesAction()); + } } diff --git a/src/app/core/auth/auth.actions.ts b/src/app/core/auth/auth.actions.ts index b9a71b71a1..239de2fb24 100644 --- a/src/app/core/auth/auth.actions.ts +++ b/src/app/core/auth/auth.actions.ts @@ -17,8 +17,9 @@ export const AuthActionTypes = { AUTHENTICATED_SUCCESS: type('dspace/auth/AUTHENTICATED_SUCCESS'), CHECK_AUTHENTICATION_TOKEN: type('dspace/auth/CHECK_AUTHENTICATION_TOKEN'), CHECK_AUTHENTICATION_TOKEN_ERROR: type('dspace/auth/CHECK_AUTHENTICATION_TOKEN_ERROR'), - REDIRECT: type('dspace/auth/REDIRECT'), - RESET_ERROR: type('dspace/auth/RESET_ERROR'), + REDIRECT_TOKEN_EXPIRED: type('dspace/auth/REDIRECT_TOKEN_EXPIRED'), + REDIRECT_AUTHENTICATION_REQUIRED: type('dspace/auth/REDIRECT_AUTHENTICATION_REQUIRED'), + RESET_MESSAGES: type('dspace/auth/RESET_MESSAGES'), LOG_OUT: type('dspace/auth/LOG_OUT'), LOG_OUT_ERROR: type('dspace/auth/LOG_OUT_ERROR'), LOG_OUT_SUCCESS: type('dspace/auth/LOG_OUT_SUCCESS'), @@ -171,13 +172,27 @@ export class LogOutSuccessAction implements Action { constructor(public payload?: any) {} } +/** + * Redirect to login page when authentication is required. + * @class RedirectWhenAuthenticationIsRequiredAction + * @implements {Action} + */ +export class RedirectWhenAuthenticationIsRequiredAction implements Action { + public type: string = AuthActionTypes.REDIRECT_AUTHENTICATION_REQUIRED; + payload: string; + + constructor(message: string) { + this.payload = message ; + } +} + /** * Redirect to login page when token is expired. * @class RedirectWhenTokenExpiredAction * @implements {Action} */ export class RedirectWhenTokenExpiredAction implements Action { - public type: string = AuthActionTypes.REDIRECT; + public type: string = AuthActionTypes.REDIRECT_TOKEN_EXPIRED; payload: string; constructor(message: string) { @@ -229,11 +244,11 @@ export class RegistrationSuccessAction implements Action { /** * Reset error. - * @class ResetAuthenticationErrorAction + * @class ResetAuthenticationMessagesAction * @implements {Action} */ -export class ResetAuthenticationErrorAction implements Action { - public type: string = AuthActionTypes.RESET_ERROR; +export class ResetAuthenticationMessagesAction implements Action { + public type: string = AuthActionTypes.RESET_MESSAGES; } /* tslint:enable:max-classes-per-file */ @@ -252,6 +267,7 @@ export type AuthActions | AuthenticationSuccessAction | CheckAuthenticationTokenAction | CheckAuthenticationTokenErrorAction + | RedirectWhenAuthenticationIsRequiredAction | RedirectWhenTokenExpiredAction | RegistrationAction | RegistrationErrorAction diff --git a/src/app/core/auth/auth.effects.ts b/src/app/core/auth/auth.effects.ts index 2343862cbb..b066d5ca92 100644 --- a/src/app/core/auth/auth.effects.ts +++ b/src/app/core/auth/auth.effects.ts @@ -112,11 +112,12 @@ export class AuthEffects { @Effect({dispatch: false}) public logOutSuccess: Observable = this.actions$ .ofType(AuthActionTypes.LOG_OUT_SUCCESS) - .do((action: LogOutSuccessAction) => this.authService.removeToken()); + .do(() => this.authService.removeToken()) + .do(() => this.authService.refreshPage()); @Effect({dispatch: false}) public redirectToLogin: Observable = this.actions$ - .ofType(AuthActionTypes.REDIRECT) + .ofType(AuthActionTypes.REDIRECT_TOKEN_EXPIRED, AuthActionTypes.REDIRECT_AUTHENTICATION_REQUIRED) .do(() => this.authService.redirectToLogin()); /** diff --git a/src/app/core/auth/auth.interceptor.ts b/src/app/core/auth/auth.interceptor.ts index 49a1213636..a45e5ef694 100644 --- a/src/app/core/auth/auth.interceptor.ts +++ b/src/app/core/auth/auth.interceptor.ts @@ -1,20 +1,19 @@ import { Injectable, Injector } from '@angular/core'; -import { Router } from '@angular/router'; import { - HttpClient, HttpEvent, HttpInterceptor, HttpHandler, HttpRequest, HttpResponse, + HttpEvent, HttpInterceptor, HttpHandler, HttpRequest, HttpResponse, HttpErrorResponse } from '@angular/common/http'; + import { Observable } from 'rxjs/Rx'; import 'rxjs/add/observable/throw' import 'rxjs/add/operator/catch'; +import { AppState } from '../../app.reducer'; +import { AuthError } from './models/auth-error.model'; import { AuthService } from './auth.service'; import { AuthStatus } from './models/auth-status.model'; -import { AuthType } from './auth-type'; -import { ResourceType } from '../shared/resource-type'; import { AuthTokenInfo } from './models/auth-token-info.model'; import { isNotEmpty } from '../../shared/empty.util'; -import { AppState } from '../../app.reducer'; import { RedirectWhenTokenExpiredAction } from './auth.actions'; import { Store } from '@ngrx/store'; @@ -47,7 +46,7 @@ export class AuthInterceptor implements HttpInterceptor { authStatus.token = new AuthTokenInfo(accessToken); } else { authStatus.authenticated = false; - authStatus.error = isNotEmpty(error) ? JSON.parse(error) : null; + authStatus.error = isNotEmpty(error) ? ((typeof error === 'string') ? JSON.parse(error) : error) : null; } return authStatus; } diff --git a/src/app/core/auth/auth.reducers.ts b/src/app/core/auth/auth.reducers.ts index 1aa800daed..07a1fe3e7e 100644 --- a/src/app/core/auth/auth.reducers.ts +++ b/src/app/core/auth/auth.reducers.ts @@ -1,7 +1,8 @@ // import actions import { AuthActions, AuthActionTypes, AuthenticatedSuccessAction, AuthenticationErrorAction, - AuthenticationSuccessAction, LogOutErrorAction, RedirectWhenTokenExpiredAction + AuthenticationSuccessAction, LogOutErrorAction, RedirectWhenAuthenticationIsRequiredAction, + RedirectWhenTokenExpiredAction } from './auth.actions'; // import models @@ -26,7 +27,7 @@ export interface AuthState { loading: boolean; // info message - message?: string; + info?: string; // the authenticated user user?: Eperson; @@ -54,7 +55,7 @@ export function authReducer(state: any = initialState, action: AuthActions): Aut return Object.assign({}, state, { error: undefined, loading: true, - message: undefined + info: undefined }); case AuthActionTypes.AUTHENTICATED_ERROR: @@ -71,7 +72,7 @@ export function authReducer(state: any = initialState, action: AuthActions): Aut loaded: true, error: undefined, loading: false, - message: undefined, + info: undefined, user: (action as AuthenticatedSuccessAction).payload.user }); @@ -96,18 +97,6 @@ export function authReducer(state: any = initialState, action: AuthActions): Aut loading: false }); - case AuthActionTypes.REGISTRATION_SUCCESS: - return state; - - case AuthActionTypes.RESET_ERROR: - return Object.assign({}, state, { - authenticated: false, - error: undefined, - loaded: false, - loading: false, - message: undefined, - }); - case AuthActionTypes.LOG_OUT_ERROR: return Object.assign({}, state, { authenticated: true, @@ -121,16 +110,17 @@ export function authReducer(state: any = initialState, action: AuthActions): Aut error: undefined, loaded: false, loading: false, - message: undefined, + info: undefined, user: undefined }); - case AuthActionTypes.REDIRECT: + case AuthActionTypes.REDIRECT_AUTHENTICATION_REQUIRED: + case AuthActionTypes.REDIRECT_TOKEN_EXPIRED: return Object.assign({}, state, { authenticated: false, loaded: false, loading: false, - message: (action as RedirectWhenTokenExpiredAction).payload, + info: (action as RedirectWhenTokenExpiredAction as RedirectWhenAuthenticationIsRequiredAction).payload, user: undefined }); @@ -139,7 +129,19 @@ export function authReducer(state: any = initialState, action: AuthActions): Aut authenticated: false, error: undefined, loading: true, - message: undefined + info: undefined + }); + + case AuthActionTypes.REGISTRATION_SUCCESS: + return state; + + case AuthActionTypes.RESET_MESSAGES: + return Object.assign({}, state, { + authenticated: false, + error: undefined, + loaded: false, + loading: false, + info: undefined, }); default: @@ -181,11 +183,11 @@ export const getAuthenticationError = (state: AuthState) => state.error; /** * Returns the authentication info message. - * @function getAuthenticationError + * @function getAuthenticationInfo * @param {State} state * @returns {String} */ -export const getAuthenticationMessage = (state: AuthState) => state.message; +export const getAuthenticationInfo = (state: AuthState) => state.info; /** * Returns true if request is in progress. diff --git a/src/app/core/auth/auth.service.ts b/src/app/core/auth/auth.service.ts index 3b76c0c1a6..69732e9ee0 100644 --- a/src/app/core/auth/auth.service.ts +++ b/src/app/core/auth/auth.service.ts @@ -10,12 +10,12 @@ import { AuthStatus } from './models/auth-status.model'; import { AuthTokenInfo, TOKENITEM } from './models/auth-token-info.model'; import { isNotEmpty, isNotNull, isNotUndefined } from '../../shared/empty.util'; import { CookieService } from '../../shared/services/cookie.service'; -import { ActivatedRoute, Router } from '@angular/router'; import { isAuthenticated } from './selectors'; import { AppState, routerStateSelector } from '../../app.reducer'; import { Store } from '@ngrx/store'; -import { ResetAuthenticationErrorAction } from './auth.actions'; +import { ResetAuthenticationMessagesAction } from './auth.actions'; import { RouterReducerState } from '@ngrx/router-store'; +import { Router } from '@angular/router'; export const LOGIN_ROUTE = '/login'; /** @@ -36,8 +36,7 @@ export class AuthService { */ private _redirectUrl: string; - constructor(private route: ActivatedRoute, - private authRequestService: AuthRequestService, + constructor(private authRequestService: AuthRequestService, private router: Router, private storage: CookieService, private store: Store) { @@ -46,7 +45,7 @@ export class AuthService { .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 it + // and is not the login route, clear redirect url and messages this.store.select(routerStateSelector) .filter((routerState: RouterReducerState) => isNotUndefined(routerState)) .filter((routerState: RouterReducerState) => @@ -54,7 +53,7 @@ export class AuthService { && isNotEmpty(this._redirectUrl) && (routerState.state.url !== this._redirectUrl)) .distinctUntilChanged() - .subscribe((routerState: RouterReducerState) => { + .subscribe(() => { this._redirectUrl = ''; }) } @@ -125,7 +124,7 @@ export class AuthService { * Clear authentication errors */ public resetAuthenticationError(): void { - this.store.dispatch(new ResetAuthenticationErrorAction()); + this.store.dispatch(new ResetAuthenticationMessagesAction()); } /** @@ -221,6 +220,19 @@ export class AuthService { } } + /** + * Refresh route navigated + */ + public refreshPage() { + this.store.select(routerStateSelector) + .take(1) + .subscribe((router) => { + // TODO Chack a way to hard refresh the same route + // this.router.navigate([router.state.url], { replaceUrl: true }); + this.router.navigate(['/']); + }) + } + /** * Get redirect url */ diff --git a/src/app/core/auth/authenticated.guard.ts b/src/app/core/auth/authenticated.guard.ts index 29db8bc46d..1f74feb0f7 100644 --- a/src/app/core/auth/authenticated.guard.ts +++ b/src/app/core/auth/authenticated.guard.ts @@ -8,6 +8,8 @@ import { Store } from '@ngrx/store'; import { CoreState } from '../core.reducers'; import { isAuthenticated, isAuthenticationLoading } from './selectors'; import { AuthService } from './auth.service'; +import { RedirectWhenAuthenticationIsRequiredAction } from './auth.actions'; +import { isEmpty } from '../../shared/empty.util'; /** * Prevent unauthorized activating and loading of routes @@ -27,7 +29,6 @@ export class AuthenticatedGuard implements CanActivate, CanLoad { */ canActivate(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable { const url = state.url; - return this.handleAuth(url); } @@ -50,16 +51,19 @@ export class AuthenticatedGuard implements CanActivate, CanLoad { } private handleAuth(url: string): Observable { - console.log('handleAuth', url) // get observable const observable = this.store.select(isAuthenticated); // redirect to sign in page if user is not authenticated - observable.subscribe((authenticated) => { - if (!authenticated) { - this.authService.redirectUrl = url; - this.authService.redirectToLogin(); - } + observable + // .filter(() => isEmpty(this.router.routerState.snapshot.url) || this.router.routerState.snapshot.url === url) + .take(1) + .subscribe((authenticated) => { + console.log('handleAuth', url, this.router.routerState.snapshot.url); + if (!authenticated) { + this.authService.redirectUrl = url; + this.store.dispatch(new RedirectWhenAuthenticationIsRequiredAction('Login required')); + } }); return observable; diff --git a/src/app/core/auth/selectors.ts b/src/app/core/auth/selectors.ts index 5cf4da58dc..1a918fa41a 100644 --- a/src/app/core/auth/selectors.ts +++ b/src/app/core/auth/selectors.ts @@ -40,12 +40,12 @@ export const getAuthenticationError = createSelector(getAuthState, auth.getAuthe /** * Returns the authentication info message. - * @function getAuthenticationError + * @function getAuthenticationInfo * @param {AuthState} state * @param {any} props - * @return {Error} + * @return {string} */ -export const getAuthenticationMessage = createSelector(getAuthState, auth.getAuthenticationMessage); +export const getAuthenticationInfo = createSelector(getAuthState, auth.getAuthenticationInfo); /** * Returns true if the user is authenticated diff --git a/src/app/shared/log-in/log-in.component.ts b/src/app/shared/log-in/log-in.component.ts index b0521fe7d1..ee988a3a14 100644 --- a/src/app/shared/log-in/log-in.component.ts +++ b/src/app/shared/log-in/log-in.component.ts @@ -10,11 +10,11 @@ import 'rxjs/add/operator/filter'; import 'rxjs/add/operator/takeWhile'; // actions -import { AuthenticateAction, ResetAuthenticationErrorAction } from '../../core/auth/auth.actions'; +import { AuthenticateAction, ResetAuthenticationMessagesAction } from '../../core/auth/auth.actions'; // reducers import { - getAuthenticationError, getAuthenticationMessage, + getAuthenticationError, getAuthenticationInfo, isAuthenticated, isAuthenticationLoading, } from '../../core/auth/selectors'; @@ -109,7 +109,7 @@ export class LogInComponent implements OnDestroy, OnInit { }); // set error - this.message = this.store.select(getAuthenticationMessage) + this.message = this.store.select(getAuthenticationInfo) .map((message) => { this.hasMessage = (isNotEmpty(message)); return message; @@ -140,7 +140,7 @@ export class LogInComponent implements OnDestroy, OnInit { */ public resetErrorOrMessage() { if (this.hasError || this.hasMessage) { - this.store.dispatch(new ResetAuthenticationErrorAction()); + this.store.dispatch(new ResetAuthenticationMessagesAction()); this.hasError = false; this.hasMessage = false; }