From 080ddf8a1fabdffb392895e27436cca6a6b0c216 Mon Sep 17 00:00:00 2001 From: Kristof De Langhe Date: Wed, 24 Jun 2020 15:05:24 +0200 Subject: [PATCH] 71429: Unauthorized page fixes --- .../eperson-form/eperson-form.component.ts | 2 +- .../admin-sidebar/admin-sidebar.component.ts | 2 +- src/app/app-routing.module.ts | 8 +++++++- .../authorization-data.service.spec.ts | 20 +++++++++---------- .../authorization-data.service.ts | 2 +- .../feature-authorization.guard.spec.ts | 19 +++++++++--------- .../feature-authorization.guard.ts | 20 +++++-------------- src/app/core/shared/operators.ts | 15 +++++++------- 8 files changed, 41 insertions(+), 47 deletions(-) diff --git a/src/app/+admin/admin-access-control/epeople-registry/eperson-form/eperson-form.component.ts b/src/app/+admin/admin-access-control/epeople-registry/eperson-form/eperson-form.component.ts index 41f360d0a7..6838b8d803 100644 --- a/src/app/+admin/admin-access-control/epeople-registry/eperson-form/eperson-form.component.ts +++ b/src/app/+admin/admin-access-control/epeople-registry/eperson-form/eperson-form.component.ts @@ -245,7 +245,7 @@ export class EPersonFormComponent implements OnInit, OnDestroy { }); })); this.canImpersonate$ = this.epersonService.getActiveEPerson().pipe( - switchMap((eperson) => this.authorizationService.isAuthenticated(FeatureID.LoginOnBehalfOf, hasValue(eperson) ? eperson.self : undefined)) + switchMap((eperson) => this.authorizationService.isAuthorized(FeatureID.LoginOnBehalfOf, hasValue(eperson) ? eperson.self : undefined)) ); }); } diff --git a/src/app/+admin/admin-sidebar/admin-sidebar.component.ts b/src/app/+admin/admin-sidebar/admin-sidebar.component.ts index 30cbf89aaf..114a3e903d 100644 --- a/src/app/+admin/admin-sidebar/admin-sidebar.component.ts +++ b/src/app/+admin/admin-sidebar/admin-sidebar.component.ts @@ -360,7 +360,7 @@ export class AdminSidebarComponent extends MenuComponent implements OnInit { * Create menu sections dependent on whether or not the current user is a site administrator */ createSiteAdministratorMenuSections() { - this.authorizationService.isAuthenticated(FeatureID.AdministratorOf).subscribe((authorized) => { + this.authorizationService.isAuthorized(FeatureID.AdministratorOf).subscribe((authorized) => { const menuList = [ /* Access Control */ { diff --git a/src/app/app-routing.module.ts b/src/app/app-routing.module.ts index 9694ed42b0..0ae3d356a3 100644 --- a/src/app/app-routing.module.ts +++ b/src/app/app-routing.module.ts @@ -64,6 +64,12 @@ export function getDSOPath(dso: DSpaceObject): string { } } +const UNAUTHORIZED_PATH = 'unauthorized'; + +export function getUnauthorizedPath() { + return `/${UNAUTHORIZED_PATH}`; +} + @NgModule({ imports: [ RouterModule.forRoot([ @@ -100,7 +106,7 @@ export function getDSOPath(dso: DSpaceObject): string { path: PROFILE_MODULE_PATH, loadChildren: './profile-page/profile-page.module#ProfilePageModule', canActivate: [AuthenticatedGuard] }, - { path: '401', component: UnauthorizedComponent }, + { path: UNAUTHORIZED_PATH, component: UnauthorizedComponent }, { path: '**', pathMatch: 'full', component: PageNotFoundComponent }, ], { diff --git a/src/app/core/data/feature-authorization/authorization-data.service.spec.ts b/src/app/core/data/feature-authorization/authorization-data.service.spec.ts index 9bb6a5b17e..a7dd8414f1 100644 --- a/src/app/core/data/feature-authorization/authorization-data.service.spec.ts +++ b/src/app/core/data/feature-authorization/authorization-data.service.spec.ts @@ -51,13 +51,13 @@ describe('AuthorizationDataService', () => { const objectUrl = 'fake-object-url'; const ePersonUuid = 'fake-eperson-uuid'; - function createExpected(objectUrl: string, ePersonUuid?: string, featureId?: FeatureID): FindListOptions { - const searchParams = [new RequestParam('uri', objectUrl)]; - if (hasValue(featureId)) { - searchParams.push(new RequestParam('feature', featureId)); + function createExpected(providedObjectUrl: string, providedEPersonUuid?: string, providedFeatureId?: FeatureID): FindListOptions { + const searchParams = [new RequestParam('uri', providedObjectUrl)]; + if (hasValue(providedFeatureId)) { + searchParams.push(new RequestParam('feature', providedFeatureId)); } - if (hasValue(ePersonUuid)) { - searchParams.push(new RequestParam('eperson', ePersonUuid)); + if (hasValue(providedEPersonUuid)) { + searchParams.push(new RequestParam('eperson', providedEPersonUuid)); } return Object.assign(new FindListOptions(), { searchParams }); } @@ -114,7 +114,7 @@ describe('AuthorizationDataService', () => { }); }); - describe('isAuthenticated', () => { + describe('isAuthorized', () => { const validPayload = [ Object.assign(new Authorization()) ] @@ -126,7 +126,7 @@ describe('AuthorizationDataService', () => { }); it('should return false', (done) => { - service.isAuthenticated().subscribe((result) => { + service.isAuthorized().subscribe((result) => { expect(result).toEqual(false); done(); }); @@ -139,7 +139,7 @@ describe('AuthorizationDataService', () => { }); it('should return false', (done) => { - service.isAuthenticated().subscribe((result) => { + service.isAuthorized().subscribe((result) => { expect(result).toEqual(false); done(); }); @@ -152,7 +152,7 @@ describe('AuthorizationDataService', () => { }); it('should return true', (done) => { - service.isAuthenticated().subscribe((result) => { + service.isAuthorized().subscribe((result) => { expect(result).toEqual(true); done(); }); diff --git a/src/app/core/data/feature-authorization/authorization-data.service.ts b/src/app/core/data/feature-authorization/authorization-data.service.ts index 92a56e7bcf..fa7eee0681 100644 --- a/src/app/core/data/feature-authorization/authorization-data.service.ts +++ b/src/app/core/data/feature-authorization/authorization-data.service.ts @@ -60,7 +60,7 @@ export class AuthorizationDataService extends DataService { * If not provided, the UUID of the currently authenticated {@link EPerson} will be used. * @param featureId ID of the {@link Feature} to check {@link Authorization} for */ - isAuthenticated(featureId?: FeatureID, objectUrl?: string, ePersonUuid?: string): Observable { + isAuthorized(featureId?: FeatureID, objectUrl?: string, ePersonUuid?: string): Observable { return this.searchByObject(featureId, objectUrl, ePersonUuid).pipe( map((authorizationRD) => (authorizationRD.statusCode !== 401 && hasValue(authorizationRD.payload) && isNotEmpty(authorizationRD.payload.page))) ); diff --git a/src/app/core/data/feature-authorization/feature-authorization-guard/feature-authorization.guard.spec.ts b/src/app/core/data/feature-authorization/feature-authorization-guard/feature-authorization.guard.spec.ts index fca665862f..f69bd9363a 100644 --- a/src/app/core/data/feature-authorization/feature-authorization-guard/feature-authorization.guard.spec.ts +++ b/src/app/core/data/feature-authorization/feature-authorization-guard/feature-authorization.guard.spec.ts @@ -2,6 +2,7 @@ import { FeatureAuthorizationGuard } from './feature-authorization.guard'; import { AuthorizationDataService } from '../authorization-data.service'; import { FeatureID } from '../feature-id'; import { of as observableOf } from 'rxjs'; +import { Router } from '@angular/router'; /** * Test implementation of abstract class FeatureAuthorizationGuard @@ -9,10 +10,11 @@ import { of as observableOf } from 'rxjs'; */ class FeatureAuthorizationGuardImpl extends FeatureAuthorizationGuard { constructor(protected authorizationService: AuthorizationDataService, + protected router: Router, protected featureId: FeatureID, protected objectUrl: string, protected ePersonUuid: string) { - super(authorizationService); + super(authorizationService, router); } getFeatureID(): FeatureID { @@ -31,6 +33,7 @@ class FeatureAuthorizationGuardImpl extends FeatureAuthorizationGuard { describe('FeatureAuthorizationGuard', () => { let guard: FeatureAuthorizationGuard; let authorizationService: AuthorizationDataService; + let router: Router; let featureId: FeatureID; let objectUrl: string; @@ -44,7 +47,10 @@ describe('FeatureAuthorizationGuard', () => { authorizationService = jasmine.createSpyObj('authorizationService', { isAuthenticated: observableOf(true) }); - guard = new FeatureAuthorizationGuardImpl(authorizationService, featureId, objectUrl, ePersonUuid); + router = jasmine.createSpyObj('router', { + navigateByUrl: {} + }); + guard = new FeatureAuthorizationGuardImpl(authorizationService, router, featureId, objectUrl, ePersonUuid); } beforeEach(() => { @@ -54,14 +60,7 @@ describe('FeatureAuthorizationGuard', () => { describe('canActivate', () => { it('should call authorizationService.isAuthenticated with the appropriate arguments', () => { guard.canActivate(undefined, undefined).subscribe(); - expect(authorizationService.isAuthenticated).toHaveBeenCalledWith(featureId, objectUrl, ePersonUuid); - }); - }); - - describe('canLoad', () => { - it('should call authorizationService.isAuthenticated with the appropriate arguments', () => { - guard.canLoad(undefined, undefined).subscribe(); - expect(authorizationService.isAuthenticated).toHaveBeenCalledWith(featureId, objectUrl, ePersonUuid); + expect(authorizationService.isAuthorized).toHaveBeenCalledWith(featureId, objectUrl, ePersonUuid); }); }); }); diff --git a/src/app/core/data/feature-authorization/feature-authorization-guard/feature-authorization.guard.ts b/src/app/core/data/feature-authorization/feature-authorization-guard/feature-authorization.guard.ts index 708b197441..7806d87b0c 100644 --- a/src/app/core/data/feature-authorization/feature-authorization-guard/feature-authorization.guard.ts +++ b/src/app/core/data/feature-authorization/feature-authorization-guard/feature-authorization.guard.ts @@ -1,23 +1,21 @@ import { ActivatedRouteSnapshot, CanActivate, - CanLoad, - Route, Router, RouterStateSnapshot, - UrlSegment + UrlTree } from '@angular/router'; import { AuthorizationDataService } from '../authorization-data.service'; import { FeatureID } from '../feature-id'; import { Observable } from 'rxjs/internal/Observable'; -import { redirectToUnauthorizedOnFalse } from '../../../shared/operators'; +import { returnUnauthorizedUrlTreeOnFalse } from '../../../shared/operators'; /** * Abstract Guard for preventing unauthorized activating and loading of routes when a user * doesn't have authorized rights on a specific feature and/or object. * Override the desired getters in the parent class for checking specific authorization on a feature and/or object. */ -export abstract class FeatureAuthorizationGuard implements CanActivate, CanLoad { +export abstract class FeatureAuthorizationGuard implements CanActivate { constructor(protected authorizationService: AuthorizationDataService, protected router: Router) { } @@ -26,16 +24,8 @@ export abstract class FeatureAuthorizationGuard implements CanActivate, CanLoad * True when user has authorization rights for the feature and object provided * Redirect the user to the unauthorized page when he/she's not authorized for the given feature */ - canActivate(next: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable { - return this.authorizationService.isAuthenticated(this.getFeatureID(), this.getObjectUrl(), this.getEPersonUuid()).pipe(redirectToUnauthorizedOnFalse(this.router)); - } - - /** - * True when user has authorization rights for the feature and object provided - * Redirect the user to the unauthorized page when he/she's not authorized for the given feature - */ - canLoad(route: Route, segments: UrlSegment[]): Observable { - return this.authorizationService.isAuthenticated(this.getFeatureID(), this.getObjectUrl(), this.getEPersonUuid()).pipe(redirectToUnauthorizedOnFalse(this.router)); + canActivate(next: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable { + return this.authorizationService.isAuthorized(this.getFeatureID(), this.getObjectUrl(), this.getEPersonUuid()).pipe(returnUnauthorizedUrlTreeOnFalse(this.router)); } /** diff --git a/src/app/core/shared/operators.ts b/src/app/core/shared/operators.ts index ca1394005e..4abb71350b 100644 --- a/src/app/core/shared/operators.ts +++ b/src/app/core/shared/operators.ts @@ -1,4 +1,4 @@ -import { Router } from '@angular/router'; +import { Router, UrlTree } from '@angular/router'; import { Observable } from 'rxjs'; import { filter, find, flatMap, map, take, tap } from 'rxjs/operators'; import { hasValue, hasValueOperator, isNotEmpty } from '../../shared/empty.util'; @@ -11,6 +11,7 @@ import { RequestEntry } from '../data/request.reducer'; import { RequestService } from '../data/request.service'; import { BrowseDefinition } from './browse-definition.model'; import { DSpaceObject } from './dspace-object.model'; +import { getUnauthorizedPath } from '../../app-routing.module'; /** * This file contains custom RxJS operators that can be used in multiple places @@ -181,16 +182,14 @@ export const redirectToPageNotFoundOn404 = (router: Router) => })); /** - * Operator that redirects the user to the unauthorized page when the boolean received is false + * Operator that returns a UrlTree to the unauthorized page when the boolean received is false * @param router */ -export const redirectToUnauthorizedOnFalse = (router: Router) => - (source: Observable): Observable => +export const returnUnauthorizedUrlTreeOnFalse = (router: Router) => + (source: Observable): Observable => source.pipe( - tap((authorized: boolean) => { - if (!authorized) { - router.navigateByUrl('/401', { skipLocationChange: true }); - } + map((authorized: boolean) => { + return authorized ? authorized : router.parseUrl(getUnauthorizedPath()) })); export const getFinishedRemoteData = () =>