Merge pull request #1075 from atmire/Edit-item-page-permissions-fixes

Edit item page permissions fixes

src/app/+item-page/edit-item-page/edit-item-page.component.html

@@ -5,11 +5,18 @@
             <div class="pt-2">
                 <ul class="nav nav-tabs justify-content-start">
                     <li *ngFor="let page of pages" class="nav-item">
-                        <a class="nav-link"
+                        <a *ngIf="(page.enabled | async)"
-                           [ngClass]="{'active' : page === currentPage}"
+                           class="nav-link"
-                           [routerLink]="['./' + page]">
+                           [ngClass]="{'active' : page.page === currentPage}"
-                            {{'item.edit.tabs.' + page + '.head' | translate}}
+                           [routerLink]="['./' + page.page]">
+                            {{'item.edit.tabs.' + page.page + '.head' | translate}}
                         </a>
+                        <span [ngbTooltip]="'item.edit.tabs.disabled.tooltip' | translate">
+                            <button *ngIf="!(page.enabled | async)"
+                                 class="nav-link disabled">
+                                {{'item.edit.tabs.' + page.page + '.head' | translate}}
+                            </button>
+                        </span>
                     </li>
                 </ul>
                 <div class="tab-pane active">

src/app/+item-page/edit-item-page/edit-item-page.component.spec.ts

@@ -0,0 +1,107 @@
+import { ComponentFixture, fakeAsync, TestBed, waitForAsync } from '@angular/core/testing';
+import { TranslateLoader, TranslateModule } from '@ngx-translate/core';
+import { TranslateLoaderMock } from '../../shared/mocks/translate-loader.mock';
+import { ChangeDetectionStrategy, NO_ERRORS_SCHEMA } from '@angular/core';
+import { ActivatedRoute, ActivatedRouteSnapshot, CanActivate, Router, RouterStateSnapshot, UrlTree } from '@angular/router';
+import { EditItemPageComponent } from './edit-item-page.component';
+import { Observable, of as observableOf } from 'rxjs';
+import { By } from '@angular/platform-browser';
+import { createSuccessfulRemoteDataObject } from '../../shared/remote-data.utils';
+import { Item } from '../../core/shared/item.model';
+
+describe('ItemPageComponent', () => {
+  let comp: EditItemPageComponent;
+  let fixture: ComponentFixture<EditItemPageComponent>;
+
+  class AcceptAllGuard implements CanActivate {
+    canActivate(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable<boolean | UrlTree> | Promise<boolean | UrlTree> | boolean | UrlTree {
+      return observableOf(true);
+    }
+  }
+
+  // tslint:disable-next-line:max-classes-per-file
+  class AcceptNoneGuard implements CanActivate {
+    canActivate(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable<boolean | UrlTree> | Promise<boolean | UrlTree> | boolean | UrlTree {
+      console.log('BLA');
+      return observableOf(false);
+    }
+  }
+
+  const accesiblePages = ['accessible'];
+  const inaccesiblePages = ['inaccessible', 'inaccessibleDoubleGuard'];
+  const mockRoute = {
+    snapshot: {
+      firstChild: {
+        routeConfig: {
+          path: accesiblePages[0]
+        }
+      },
+      routerState: {
+        snapshot: undefined
+      }
+    },
+    routeConfig: {
+      children: [
+        {
+          path: accesiblePages[0],
+          canActivate: [AcceptAllGuard]
+        }, {
+          path: inaccesiblePages[0],
+          canActivate: [AcceptNoneGuard]
+        }, {
+          path: inaccesiblePages[1],
+          canActivate: [AcceptAllGuard, AcceptNoneGuard]
+        },
+      ]
+    },
+    data: observableOf({dso: createSuccessfulRemoteDataObject(new Item())})
+  };
+
+  const mockRouter = {
+    routerState: {
+      snapshot: undefined
+    },
+    events: observableOf(undefined)
+  };
+
+  beforeEach(waitForAsync(() => {
+    TestBed.configureTestingModule({
+      imports: [TranslateModule.forRoot({
+        loader: {
+          provide: TranslateLoader,
+          useClass: TranslateLoaderMock
+        }
+      })],
+      declarations: [EditItemPageComponent],
+      providers: [
+        { provide: ActivatedRoute, useValue: mockRoute },
+        { provide: Router, useValue: mockRouter },
+        AcceptAllGuard,
+        AcceptNoneGuard,
+      ],
+
+      schemas: [NO_ERRORS_SCHEMA]
+    }).overrideComponent(EditItemPageComponent, {
+      set: { changeDetection: ChangeDetectionStrategy.Default }
+    }).compileComponents();
+  }));
+
+  beforeEach(waitForAsync(() => {
+    fixture = TestBed.createComponent(EditItemPageComponent);
+    comp = fixture.componentInstance;
+    spyOn((comp as any).injector, 'get').and.callFake((a) => new a());
+    fixture.detectChanges();
+  }));
+
+  describe('ngOnInit', () => {
+    it('should enable tabs that the user can activate', fakeAsync(() => {
+      const enabledItems = fixture.debugElement.queryAll(By.css('a.nav-link'));
+      expect(enabledItems.length).toBe(accesiblePages.length);
+    }));
+
+    it('should disable tabs that the user can not activate', () => {
+      const disabledItems = fixture.debugElement.queryAll(By.css('button.nav-link.disabled'));
+      expect(disabledItems.length).toBe(inaccesiblePages.length);
+    });
+  });
+});

src/app/+item-page/edit-item-page/edit-item-page.component.ts

@@ -1,12 +1,13 @@
 import { fadeIn, fadeInOut } from '../../shared/animations/fade';
-import { ChangeDetectionStrategy, Component, OnInit } from '@angular/core';
+import { ChangeDetectionStrategy, Component, Injector, OnInit } from '@angular/core';
-import { ActivatedRoute, Router } from '@angular/router';
+import { ActivatedRoute, CanActivate, Route, Router } from '@angular/router';
 import { RemoteData } from '../../core/data/remote-data';
 import { Item } from '../../core/shared/item.model';
-import { Observable } from 'rxjs';
+import { combineLatest as observableCombineLatest, Observable, of as observableOf } from 'rxjs';
 import { map } from 'rxjs/operators';
 import { isNotEmpty } from '../../shared/empty.util';
 import { getItemPageRoute } from '../item-page-routing-paths';
+import { GenericConstructor } from '../../core/shared/generic-constructor';
 
 @Component({
   selector: 'ds-edit-item-page',
@@ -35,9 +36,9 @@ export class EditItemPageComponent implements OnInit {
   /**
    * All possible page outlet strings
    */
-  pages: string[];
+  pages: { page: string, enabled: Observable<boolean> }[];
 
-  constructor(private route: ActivatedRoute, private router: Router) {
+  constructor(private route: ActivatedRoute, private router: Router, private injector: Injector) {
     this.router.events.subscribe(() => {
       this.currentPage = this.route.snapshot.firstChild.routeConfig.path;
     });
@@ -45,8 +46,20 @@ export class EditItemPageComponent implements OnInit {
 
   ngOnInit(): void {
     this.pages = this.route.routeConfig.children
-      .map((child: any) => child.path)
+      .filter((child: Route) => isNotEmpty(child.path))
-      .filter((path: string) => isNotEmpty(path)); // ignore reroutes
+      .map((child: Route) => {
+        let enabled = observableOf(true);
+        if (isNotEmpty(child.canActivate)) {
+          enabled = observableCombineLatest(child.canActivate.map((guardConstructor: GenericConstructor<CanActivate>) => {
+              const guard: CanActivate = this.injector.get<CanActivate>(guardConstructor);
+              return guard.canActivate(this.route.snapshot, this.router.routerState.snapshot);
+            })
+          ).pipe(
+            map((canActivateOutcomes: any[]) => canActivateOutcomes.every((e) => e === true))
+          );
+        }
+        return { page: child.path, enabled: enabled };
+      }); // ignore reroutes
     this.itemRD$ = this.route.data.pipe(map((data) => data.dso));
   }
 

src/app/+item-page/edit-item-page/edit-item-page.routing.module.ts

@@ -22,15 +22,17 @@ import { ResourcePolicyEditComponent } from '../../shared/resource-policies/edit
 import { I18nBreadcrumbsService } from '../../core/breadcrumbs/i18n-breadcrumbs.service';
 import {
   ITEM_EDIT_AUTHORIZATIONS_PATH,
-  ITEM_EDIT_MOVE_PATH,
   ITEM_EDIT_DELETE_PATH,
-  ITEM_EDIT_PUBLIC_PATH,
+  ITEM_EDIT_MOVE_PATH,
   ITEM_EDIT_PRIVATE_PATH,
+  ITEM_EDIT_PUBLIC_PATH,
   ITEM_EDIT_REINSTATE_PATH,
   ITEM_EDIT_WITHDRAW_PATH
 } from './edit-item-page.routing-paths';
 import { ItemPageReinstateGuard } from './item-page-reinstate.guard';
 import { ItemPageWithdrawGuard } from './item-page-withdraw.guard';
+import { ItemPageEditMetadataGuard } from '../item-page-edit-metadata.guard';
+import { ItemPageAdministratorGuard } from '../item-page-administrator.guard';
 
 /**
  * Routing module that handles the routing for the Edit Item page administrator functionality
@@ -57,22 +59,26 @@ import { ItemPageWithdrawGuard } from './item-page-withdraw.guard';
               {
                 path: 'status',
                 component: ItemStatusComponent,
-                data: { title: 'item.edit.tabs.status.title', showBreadcrumbs: true }
+                data: { title: 'item.edit.tabs.status.title', showBreadcrumbs: true },
+                canActivate: [ItemPageAdministratorGuard]
               },
               {
                 path: 'bitstreams',
                 component: ItemBitstreamsComponent,
-                data: { title: 'item.edit.tabs.bitstreams.title', showBreadcrumbs: true }
+                data: { title: 'item.edit.tabs.bitstreams.title', showBreadcrumbs: true },
+                canActivate: [ItemPageAdministratorGuard]
               },
               {
                 path: 'metadata',
                 component: ItemMetadataComponent,
-                data: { title: 'item.edit.tabs.metadata.title', showBreadcrumbs: true }
+                data: { title: 'item.edit.tabs.metadata.title', showBreadcrumbs: true },
+                canActivate: [ItemPageEditMetadataGuard]
               },
               {
                 path: 'relationships',
                 component: ItemRelationshipsComponent,
-                data: { title: 'item.edit.tabs.relationships.title', showBreadcrumbs: true }
+                data: { title: 'item.edit.tabs.relationships.title', showBreadcrumbs: true },
+                canActivate: [ItemPageEditMetadataGuard]
               },
               /* TODO - uncomment & fix when view page exists
               {
@@ -89,12 +95,14 @@ import { ItemPageWithdrawGuard } from './item-page-withdraw.guard';
               {
                 path: 'versionhistory',
                 component: ItemVersionHistoryComponent,
-                data: { title: 'item.edit.tabs.versionhistory.title', showBreadcrumbs: true }
+                data: { title: 'item.edit.tabs.versionhistory.title', showBreadcrumbs: true },
+                canActivate: [ItemPageAdministratorGuard]
               },
               {
                 path: 'mapper',
                 component: ItemCollectionMapperComponent,
-                data: { title: 'item.edit.tabs.item-mapper.title', showBreadcrumbs: true }
+                data: { title: 'item.edit.tabs.item-mapper.title', showBreadcrumbs: true },
+                canActivate: [ItemPageAdministratorGuard]
               }
             ]
           },
@@ -165,7 +173,9 @@ import { ItemPageWithdrawGuard } from './item-page-withdraw.guard';
     ResourcePolicyResolver,
     ResourcePolicyTargetResolver,
     ItemPageReinstateGuard,
-    ItemPageWithdrawGuard
+    ItemPageWithdrawGuard,
+    ItemPageAdministratorGuard,
+    ItemPageEditMetadataGuard,
   ]
 })
 export class EditItemPageRoutingModule {

src/app/+item-page/item-page-edit-metadata.guard.ts

@@ -0,0 +1,31 @@
+import { Injectable } from '@angular/core';
+import { ActivatedRouteSnapshot, Router, RouterStateSnapshot } from '@angular/router';
+import { AuthorizationDataService } from '../core/data/feature-authorization/authorization-data.service';
+import { ItemPageResolver } from './item-page.resolver';
+import { Item } from '../core/shared/item.model';
+import { DsoPageFeatureGuard } from '../core/data/feature-authorization/feature-authorization-guard/dso-page-feature.guard';
+import { Observable, of as observableOf } from 'rxjs';
+import { FeatureID } from '../core/data/feature-authorization/feature-id';
+import { AuthService } from '../core/auth/auth.service';
+
+@Injectable({
+  providedIn: 'root'
+})
+/**
+ * Guard for preventing unauthorized access to certain {@link Item} pages requiring edit metadata rights
+ */
+export class ItemPageEditMetadataGuard extends DsoPageFeatureGuard<Item> {
+  constructor(protected resolver: ItemPageResolver,
+              protected authorizationService: AuthorizationDataService,
+              protected router: Router,
+              protected authService: AuthService) {
+    super(resolver, authorizationService, router, authService);
+  }
+
+  /**
+   * Check edit metadata authorization rights
+   */
+  getFeatureID(next: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable<FeatureID> {
+    return observableOf(FeatureID.CanEditMetadata);
+  }
+}

src/app/+item-page/item-page-routing.module.ts

@@ -37,7 +37,6 @@ import { ThemedFullItemPageComponent } from './full/themed-full-item-page.compon
             path: ITEM_EDIT_PATH,
             loadChildren: () => import('./edit-item-page/edit-item-page.module')
               .then((m) => m.EditItemPageModule),
-            canActivate: [ItemPageAdministratorGuard]
           },
           {
             path: UPLOAD_BITSTREAM_PATH,
@@ -67,7 +66,7 @@ import { ThemedFullItemPageComponent } from './full/themed-full-item-page.compon
     ItemBreadcrumbResolver,
     DSOBreadcrumbsService,
     LinkService,
-    ItemPageAdministratorGuard
+    ItemPageAdministratorGuard,
   ]
 
 })

src/app/core/data/feature-authorization/feature-authorization-guard/dso-page-feature.guard.spec.ts

@@ -32,6 +32,8 @@ describe('DsoPageAdministratorGuard', () => {
   let authService: AuthService;
   let resolver: Resolve<RemoteData<any>>;
   let object: DSpaceObject;
+  let route;
+  let parentRoute;
 
   function init() {
     object = {
@@ -50,6 +52,16 @@ describe('DsoPageAdministratorGuard', () => {
     authService = jasmine.createSpyObj('authService', {
       isAuthenticated: observableOf(true)
     });
+    parentRoute = {
+      params: {
+        id: '3e1a5327-dabb-41ff-af93-e6cab9d032f0'
+      }
+    };
+    route = {
+      params: {
+      },
+      parent: parentRoute
+    };
     guard = new DsoPageFeatureGuardImpl(resolver, authorizationService, router, authService, undefined);
   }
 
@@ -59,10 +71,17 @@ describe('DsoPageAdministratorGuard', () => {
 
   describe('getObjectUrl', () => {
     it('should return the resolved object\'s selflink', (done) => {
-      guard.getObjectUrl(undefined, undefined).subscribe((selflink) => {
+      guard.getObjectUrl(route, undefined).subscribe((selflink) => {
         expect(selflink).toEqual(object.self);
         done();
       });
     });
   });
+
+  describe('getRouteWithDSOId', () => {
+    it('should return the route that has the UUID of the DSO', () => {
+      const foundRoute = (guard as any).getRouteWithDSOId(route);
+      expect(foundRoute).toBe(parentRoute);
+    });
+  });
 });

src/app/core/data/feature-authorization/feature-authorization-guard/dso-page-feature.guard.ts

@@ -7,6 +7,7 @@ import { map } from 'rxjs/operators';
 import { DSpaceObject } from '../../../shared/dspace-object.model';
 import { FeatureAuthorizationGuard } from './feature-authorization.guard';
 import { AuthService } from '../../../auth/auth.service';
+import { hasNoValue, hasValue } from '../../../../shared/empty.util';
 
 /**
  * Abstract Guard for preventing unauthorized access to {@link DSpaceObject} pages that require rights for a specific feature
@@ -24,9 +25,22 @@ export abstract class DsoPageFeatureGuard<T extends DSpaceObject> extends Featur
    * Check authorization rights for the object resolved using the provided resolver
    */
   getObjectUrl(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable<string> {
-    return (this.resolver.resolve(route, state) as Observable<RemoteData<T>>).pipe(
+    const routeWithObjectID = this.getRouteWithDSOId(route);
+    return (this.resolver.resolve(routeWithObjectID, state) as Observable<RemoteData<T>>).pipe(
       getAllSucceededRemoteDataPayload(),
       map((dso) => dso.self)
     );
   }
+
+  /**
+   * Method to resolve resolve (parent) route that contains the UUID of the DSO
+   * @param route The current route
+   */
+  protected getRouteWithDSOId(route: ActivatedRouteSnapshot): ActivatedRouteSnapshot {
+    let routeWithDSOId = route;
+    while (hasNoValue(routeWithDSOId.params.id) && hasValue(routeWithDSOId.parent)) {
+      routeWithDSOId = routeWithDSOId.parent;
+    }
+    return routeWithDSOId;
+  }
 }

src/app/shared/dso-page/dso-page-edit-button/dso-page-edit-button.component.html

@@ -1,5 +1,5 @@
 <a *ngIf="isAuthorized$ | async"
-   [routerLink]="[pageRoute, 'edit']"
+   [routerLink]="[pageRoute, 'edit', 'metadata']"
    class="edit-button btn btn-dark text-light btn-sm"
    [ngbTooltip]="tooltipMsg | translate">
   <i class="fas fa-pencil-alt fa-fw"></i>

src/assets/i18n/en.json5

@@ -1475,6 +1475,8 @@
 
   "item.edit.breadcrumbs": "Edit Item",
 
+  "item.edit.tabs.disabled.tooltip": "You don't have permission to access this tab",
+
 
   "item.edit.tabs.mapper.head": "Collection Mapper",