[Port to dtq-dev] 117 better control of user deletion in user administration page

src/app/access-control/access-control.module.ts

@@ -5,6 +5,7 @@ import { SharedModule } from '../shared/shared.module';
 import { AccessControlRoutingModule } from './access-control-routing.module';
 import { EPeopleRegistryComponent } from './epeople-registry/epeople-registry.component';
 import { EPersonFormComponent } from './epeople-registry/eperson-form/eperson-form.component';
+import { EPersonDeleteGuardService } from './epeople-registry/eperson-delete-guard.service';
 import { GroupFormComponent } from './group-registry/group-form/group-form.component';
 import { MembersListComponent } from './group-registry/group-form/members-list/members-list.component';
 import { SubgroupsListComponent } from './group-registry/group-form/subgroup-list/subgroups-list.component';
@@ -57,6 +58,7 @@ export const ValidateEmailErrorStateMatcher: DynamicErrorMessagesMatcher =
       provide: DYNAMIC_ERROR_MESSAGES_MATCHER,
       useValue: ValidateEmailErrorStateMatcher
     },
+    EPersonDeleteGuardService,
   ]
 })
 /**

src/app/access-control/epeople-registry/epeople-registry.component.html

@@ -76,11 +76,26 @@ <h2 id="search" class="border-bottom pb-2">
                           title="{{labelPrefix + 'table.edit.buttons.edit' | translate: { name: dsoNameService.getName(epersonDto.eperson) } }}">
                     <i class="fas fa-edit fa-fw"></i>
                   </button>
-                  <button *ngIf="epersonDto.ableToDelete" (click)="deleteEPerson(epersonDto.eperson)"
-                          class="delete-button btn btn-outline-danger btn-sm access-control-deleteEPersonButton"
-                          title="{{labelPrefix + 'table.edit.buttons.remove' | translate: { name: dsoNameService.getName(epersonDto.eperson) } }}">
-                    <i class="fas fa-trash-alt fa-fw"></i>
-                  </button>
+                  <ng-container *ngIf="epersonDto.ableToDelete && currentAuthenticatedUserId">
+                    <ng-container *ngIf="isCurrentUser(epersonDto.eperson); else enabledDeleteButton">
+                      <span tabindex="0" [ngbTooltip]="selfDeleteWarningLabel | translate">
+                        <button [dsBtnDisabled]="true"
+                                tabindex="-1"
+                                [attr.aria-label]="selfDeleteWarningLabel | translate"
+                                class="delete-button btn btn-outline-danger btn-sm access-control-deleteEPersonButton"
+                                type="button">
+                          <i class="fas fa-trash-alt fa-fw"></i>
+                        </button>
+                      </span>
+                    </ng-container>
+                  </ng-container>
+                  <ng-template #enabledDeleteButton>
+                    <button (click)="deleteEPerson(epersonDto.eperson)"
+                            class="delete-button btn btn-outline-danger btn-sm access-control-deleteEPersonButton"
+                            title="{{labelPrefix + 'table.edit.buttons.remove' | translate: { name: dsoNameService.getName(epersonDto.eperson) } }}">
+                      <i class="fas fa-trash-alt fa-fw"></i>
+                    </button>
+                  </ng-template>
                 </div>
               </td>
             </tr>

src/app/access-control/epeople-registry/epeople-registry.component.spec.ts

@@ -1,33 +1,42 @@
 import { Router } from '@angular/router';
-import { Observable, of as observableOf } from 'rxjs';
+import { Observable, of as observableOf, throwError as observableThrowError } from 'rxjs';
 import { CommonModule } from '@angular/common';
-import { DebugElement, NO_ERRORS_SCHEMA } from '@angular/core';
+import { NO_ERRORS_SCHEMA } from '@angular/core';
 import { ComponentFixture, fakeAsync, TestBed, tick, waitForAsync } from '@angular/core/testing';
 import { FormsModule, ReactiveFormsModule } from '@angular/forms';
 import { BrowserModule, By } from '@angular/platform-browser';
 import { NgbModule } from '@ng-bootstrap/ng-bootstrap';
 import { TranslateLoader, TranslateModule, TranslateService } from '@ngx-translate/core';
 import { buildPaginatedList, PaginatedList } from '../../core/data/paginated-list.model';
 import { RemoteData } from '../../core/data/remote-data';
+import { AuthService } from '../../core/auth/auth.service';
 import { EPersonDataService } from '../../core/eperson/eperson-data.service';
 import { EPerson } from '../../core/eperson/models/eperson.model';
 import { PageInfo } from '../../core/shared/page-info.model';
+import { DSONameService } from '../../core/breadcrumbs/dso-name.service';
 import { FormBuilderService } from '../../shared/form/builder/form-builder.service';
 import { NotificationsService } from '../../shared/notifications/notifications.service';
 import { EPeopleRegistryComponent } from './epeople-registry.component';
 import { EPersonMock, EPersonMock2 } from '../../shared/testing/eperson.mock';
-import { createSuccessfulRemoteDataObject$ } from '../../shared/remote-data.utils';
+import { createFailedRemoteDataObject$, createSuccessfulRemoteDataObject$ } from '../../shared/remote-data.utils';
 import { getMockFormBuilderService } from '../../shared/mocks/form-builder-service.mock';
 import { getMockTranslateService } from '../../shared/mocks/translate.service.mock';
 import { TranslateLoaderMock } from '../../shared/mocks/translate-loader.mock';
 import { NotificationsServiceStub } from '../../shared/testing/notifications-service.stub';
 import { RouterStub } from '../../shared/testing/router.stub';
 import { AuthorizationDataService } from '../../core/data/feature-authorization/authorization-data.service';
+import { FeatureID } from '../../core/data/feature-authorization/feature-id';
+import { EPersonDeleteGuardService } from './eperson-delete-guard.service';
 import { RequestService } from '../../core/data/request.service';
 import { PaginationService } from '../../core/pagination/pagination.service';
 import { PaginationServiceStub } from '../../shared/testing/pagination-service.stub';
+import { WorkspaceitemDataService } from '../../core/submission/workspaceitem-data.service';
+import { WorkflowItemDataService } from '../../core/submission/workflowitem-data.service';
 import { FindListOptions } from '../../core/data/find-list-options.model';
-import {BtnDisabledDirective} from '../../shared/btn-disabled.directive';
+import { SearchService } from '../../core/shared/search/search.service';
+import { DSpaceObject } from '../../core/shared/dspace-object.model';
+import { SearchObjects } from '../../shared/search/models/search-objects.model';
+import { BtnDisabledDirective } from '../../shared/btn-disabled.directive';
 
 describe('EPeopleRegistryComponent', () => {
   let component: EPeopleRegistryComponent;
@@ -38,10 +47,35 @@ describe('EPeopleRegistryComponent', () => {
   let mockEPeople;
   let ePersonDataServiceStub: any;
   let authorizationService: AuthorizationDataService;
+  let authService: jasmine.SpyObj<AuthService>;
+  let workspaceItemDataService: jasmine.SpyObj<WorkspaceitemDataService>;
+  let workflowItemDataService: jasmine.SpyObj<WorkflowItemDataService>;
+  let searchService: jasmine.SpyObj<SearchService>;
+  let notificationsService: NotificationsServiceStub;
   let modalService;
+  let modalRef;
 
   let paginationService;
 
+  const buildRemoteList = <T>(items: T[], totalElements = items.length) => createSuccessfulRemoteDataObject$(
+    buildPaginatedList(new PageInfo({
+      elementsPerPage: items.length || 1,
+      totalElements,
+      totalPages: 1,
+      currentPage: 1
+    }), items)
+  );
+
+  const buildSearchObjects = (totalElements: number) => Object.assign(
+    new SearchObjects<DSpaceObject>(),
+    buildPaginatedList(new PageInfo({
+      elementsPerPage: 1,
+      totalElements,
+      totalPages: 1,
+      currentPage: 1
+    }), [])
+  );
+
   beforeEach(waitForAsync(() => {
     jasmine.getEnv().allowRespy(true);
     mockEPeople = [EPersonMock, EPersonMock2];
@@ -119,8 +153,17 @@ describe('EPeopleRegistryComponent', () => {
     authorizationService = jasmine.createSpyObj('authorizationService', {
       isAuthorized: observableOf(true)
     });
+    authService = jasmine.createSpyObj('authService', ['getAuthenticatedUserFromStore']);
+    authService.getAuthenticatedUserFromStore.and.returnValue(observableOf(EPersonMock2));
+    workspaceItemDataService = jasmine.createSpyObj('workspaceItemDataService', ['searchBy']);
+    workspaceItemDataService.searchBy.and.returnValue(buildRemoteList([], 0));
+    workflowItemDataService = jasmine.createSpyObj('workflowItemDataService', ['searchBy']);
+    workflowItemDataService.searchBy.and.returnValue(buildRemoteList([], 0));
+    searchService = jasmine.createSpyObj('searchService', ['search']);
+    searchService.search.and.returnValue(createSuccessfulRemoteDataObject$(buildSearchObjects(0)));
     builderService = getMockFormBuilderService();
     translateService = getMockTranslateService();
+    notificationsService = new NotificationsServiceStub();
 
     paginationService = new PaginationServiceStub();
     TestBed.configureTestingModule({
@@ -135,12 +178,20 @@ describe('EPeopleRegistryComponent', () => {
       declarations: [EPeopleRegistryComponent, BtnDisabledDirective],
       providers: [
         { provide: EPersonDataService, useValue: ePersonDataServiceStub },
-        { provide: NotificationsService, useValue: new NotificationsServiceStub() },
+        { provide: NotificationsService, useValue: notificationsService },
         { provide: AuthorizationDataService, useValue: authorizationService },
+        { provide: AuthService, useValue: authService },
         { provide: FormBuilderService, useValue: builderService },
+        { provide: WorkspaceitemDataService, useValue: workspaceItemDataService },
+        { provide: WorkflowItemDataService, useValue: workflowItemDataService },
+        { provide: SearchService, useValue: searchService },
+        EPersonDeleteGuardService,
         { provide: Router, useValue: new RouterStub() },
         { provide: RequestService, useValue: jasmine.createSpyObj('requestService', ['removeByHrefSubstring']) },
-        { provide: PaginationService, useValue: paginationService }
+        { provide: PaginationService, useValue: paginationService },
+        { provide: DSONameService, useValue: jasmine.createSpyObj('dsoNameService', {
+          getName: (dso: any) => dso?.name ?? dso?.email ?? dso?.id,
+        }) },
       ],
       schemas: [NO_ERRORS_SCHEMA]
     }).compileComponents();
@@ -150,7 +201,8 @@ describe('EPeopleRegistryComponent', () => {
     fixture = TestBed.createComponent(EPeopleRegistryComponent);
     component = fixture.componentInstance;
     modalService = (component as any).modalService;
-    spyOn(modalService, 'open').and.returnValue(Object.assign({ componentInstance: Object.assign({ response: observableOf(true) }) }));
+    modalRef = Object.assign({ componentInstance: Object.assign({ response: observableOf(true) }) });
+    spyOn(modalService, 'open').and.returnValue(modalRef);
     fixture.detectChanges();
   });
 
@@ -227,6 +279,109 @@ describe('EPeopleRegistryComponent', () => {
         });
       });
     });
+
+    it('should render the self delete button as disabled', () => {
+      const deleteButtons = fixture.debugElement.queryAll(By.css('.access-control-deleteEPersonButton'));
+
+      expect(deleteButtons.length).toBe(2);
+      expect(deleteButtons[0].nativeElement.getAttribute('aria-disabled')).toBeNull();
+      expect(deleteButtons[1].nativeElement.getAttribute('aria-disabled')).toBe('true');
+      expect(deleteButtons[1].nativeElement.classList.contains('disabled')).toBeTrue();
+    });
+
+    it('should call submitter checks and compose the combined warning label', fakeAsync(() => {
+      workspaceItemDataService.searchBy.and.returnValue(buildRemoteList([{} as any], 1));
+      workflowItemDataService.searchBy.and.returnValue(buildRemoteList([], 0));
+      searchService.search.and.returnValue(createSuccessfulRemoteDataObject$(buildSearchObjects(0)));
+      // isAuthorized returns true by default -> the target is treated as an administrator
+      modalRef.componentInstance.response = observableOf(false);
+
+      const deleteButtons = fixture.debugElement.queryAll(By.css('.access-control-deleteEPersonButton'));
+      deleteButtons[0].triggerEventHandler('click', null);
+      tick();
+
+      expect(workspaceItemDataService.searchBy).toHaveBeenCalledWith('findBySubmitter', jasmine.any(FindListOptions));
+      expect(workflowItemDataService.searchBy).toHaveBeenCalledWith('findBySubmitter', jasmine.any(FindListOptions));
+      expect(searchService.search).toHaveBeenCalled();
+      expect(authorizationService.isAuthorized).toHaveBeenCalledWith(FeatureID.AdministratorOf, undefined, EPersonMock.id);
+      expect(modalRef.componentInstance.warningLabel).toBe('admin.access-control.epeople.delete.warning.submitterAndAdmin');
+    }));
+
+    it('should detect administrator via the authorization feature', fakeAsync(() => {
+      workspaceItemDataService.searchBy.and.returnValue(buildRemoteList([], 0));
+      workflowItemDataService.searchBy.and.returnValue(buildRemoteList([], 0));
+      searchService.search.and.returnValue(createSuccessfulRemoteDataObject$(buildSearchObjects(0)));
+      // admin -> true, all submitter probes empty -> admin-only warning
+      (authorizationService.isAuthorized as jasmine.Spy).and.callFake((featureId: FeatureID) => observableOf(featureId === FeatureID.AdministratorOf));
+      modalRef.componentInstance.response = observableOf(false);
+
+      const deleteButtons = fixture.debugElement.queryAll(By.css('.access-control-deleteEPersonButton'));
+      deleteButtons[0].triggerEventHandler('click', null);
+      tick();
+
+      expect(authorizationService.isAuthorized).toHaveBeenCalledWith(FeatureID.AdministratorOf, undefined, EPersonMock.id);
+      expect(modalRef.componentInstance.warningLabel).toBe('admin.access-control.epeople.delete.warning.admin');
+    }));
+
+    it('should still open the delete modal when a submitter probe errors (centralised catchError)', fakeAsync(() => {
+      workspaceItemDataService.searchBy.and.returnValue(observableThrowError(() => new Error('boom')));
+      workflowItemDataService.searchBy.and.returnValue(buildRemoteList([], 0));
+      searchService.search.and.returnValue(createSuccessfulRemoteDataObject$(buildSearchObjects(0)));
+      // CanDelete stays true so the button renders; AdministratorOf false so the only warning could come from submitter probes
+      (authorizationService.isAuthorized as jasmine.Spy).and.callFake((featureId: FeatureID) => observableOf(featureId !== FeatureID.AdministratorOf));
+      modalRef.componentInstance.response = observableOf(false);
+
+      const deleteButtons = fixture.debugElement.queryAll(By.css('.access-control-deleteEPersonButton'));
+      deleteButtons[0].triggerEventHandler('click', null);
+      tick();
+
+      expect(modalService.open).toHaveBeenCalled();
+      expect(modalRef.componentInstance.warningLabel).toBeUndefined();
+    }));
+
+    it('should show a friendly self-delete notification on backend 400 self-delete errors', fakeAsync(() => {
+      modalRef.componentInstance.response = observableOf(true);
+      ePersonDataServiceStub.deleteEPerson = jasmine.createSpy('deleteEPerson').and.returnValue(
+        createFailedRemoteDataObject$('You, as admin user, cannot delete yourself', 400)
+      );
+
+      const deleteButtons = fixture.debugElement.queryAll(By.css('.access-control-deleteEPersonButton'));
+      deleteButtons[0].triggerEventHandler('click', null);
+      tick();
+
+      expect(notificationsService.error).toHaveBeenCalled();
+      let translatedKey: string;
+      notificationsService.error.calls.mostRecent().args[0].subscribe((value) => translatedKey = value);
+      expect(translatedKey).toBe('admin.access-control.epeople.notification.deleted.forbidden.self');
+    }));
+
+    it('should use the deleted.failure key for generic delete failures', fakeAsync(() => {
+      modalRef.componentInstance.response = observableOf(true);
+      ePersonDataServiceStub.deleteEPerson = jasmine.createSpy('deleteEPerson').and.returnValue(
+        createFailedRemoteDataObject$('server error', 500)
+      );
+
+      const deleteButtons = fixture.debugElement.queryAll(By.css('.access-control-deleteEPersonButton'));
+      deleteButtons[0].triggerEventHandler('click', null);
+      tick();
+
+      expect(notificationsService.error).toHaveBeenCalled();
+      let translatedKey: string;
+      notificationsService.error.calls.mostRecent().args[0].subscribe((value) => translatedKey = value);
+      expect(translatedKey).toBe('admin.access-control.epeople.notification.deleted.failure');
+    }));
+
+    it('should not open delete modal before authenticated user id is resolved', fakeAsync(() => {
+      component.currentAuthenticatedUserId = undefined;
+      const deleteSpy = spyOn(ePersonDataServiceStub, 'deleteEPerson').and.callThrough();
+
+      const deleteButtons = fixture.debugElement.queryAll(By.css('.access-control-deleteEPersonButton'));
+      deleteButtons[0].triggerEventHandler('click', null);
+      tick();
+
+      expect(modalService.open).not.toHaveBeenCalled();
+      expect(deleteSpy).not.toHaveBeenCalled();
+    }));
   });
 
   describe('delete EPerson button when the isAuthorized returns false', () => {
@@ -237,12 +392,9 @@ describe('EPeopleRegistryComponent', () => {
       fixture.detectChanges();
     });
 
-    it('should be disabled', () => {
+    it('should be hidden', () => {
       ePeopleDeleteButton = fixture.debugElement.queryAll(By.css('#epeople tr td div button.delete-button'));
-      ePeopleDeleteButton.forEach((deleteButton: DebugElement) => {
-        expect(deleteButton.nativeElement.getAttribute('aria-disabled')).toBe('true');
-        expect(deleteButton.nativeElement.classList.contains('disabled')).toBeTrue();
-      });
+      expect(ePeopleDeleteButton.length).toBe(0);
     });
   });
 });