TUL/Prevent an admin from deleting their own account

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';
@@ -45,6 +46,7 @@ export const ValidateEmailErrorStateMatcher: DynamicErrorMessagesMatcher =
       provide: DYNAMIC_ERROR_MESSAGES_MATCHER,
       useValue: ValidateEmailErrorStateMatcher
     },
+    EPersonDeleteGuardService,
   ]
 })
 /**

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

@@ -77,11 +77,27 @@ <h3 id="search" class="border-bottom pb-2">{{labelPrefix + 'search.head' | trans
                             title="{{labelPrefix + 'table.edit.buttons.edit' | translate: {name: epersonDto.eperson.name} }}">
                       <i class="fas fa-edit fa-fw"></i>
                     </button>
-                    <button [disabled]="!epersonDto.ableToDelete" (click)="deleteEPerson(epersonDto.eperson)"
-                            class="btn btn-outline-danger btn-sm access-control-deleteEPersonButton"
-                            title="{{labelPrefix + 'table.edit.buttons.remove' | translate: {name: epersonDto.eperson.name} }}">
-                      <i class="fas fa-trash-alt fa-fw"></i>
-                    </button>
+                    <ng-container *ngIf="currentAuthenticatedUserId">
+                      <span *ngIf="isCurrentUser(epersonDto.eperson); else enabledDeleteButton"
+                            tabindex="0"
+                            [ngbTooltip]="selfDeleteWarningLabel | translate"
+                            container="body">
+                        <button [disabled]="true"
+                                tabindex="-1"
+                                [attr.aria-label]="selfDeleteWarningLabel | translate"
+                                class="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-template #enabledDeleteButton>
+                      <button [disabled]="!epersonDto.ableToDelete" (click)="deleteEPerson(epersonDto.eperson)"
+                              class="btn btn-outline-danger btn-sm access-control-deleteEPersonButton"
+                              title="{{labelPrefix + 'table.edit.buttons.remove' | translate: {name: epersonDto.eperson.name} }}">
+                        <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

@@ -23,6 +23,8 @@ 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 { AuthService } from '../../core/auth/auth.service';
+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';
@@ -37,6 +39,8 @@ describe('EPeopleRegistryComponent', () => {
   let mockEPeople;
   let ePersonDataServiceStub: any;
   let authorizationService: AuthorizationDataService;
+  let authService: jasmine.SpyObj<AuthService>;
+  let deleteGuard: jasmine.SpyObj<EPersonDeleteGuardService>;
   let modalService;
 
   let paginationService;
@@ -117,6 +121,12 @@ describe('EPeopleRegistryComponent', () => {
     authorizationService = jasmine.createSpyObj('authorizationService', {
       isAuthorized: observableOf(true)
     });
+    authService = jasmine.createSpyObj('authService', ['getAuthenticatedUserFromStore']);
+    authService.getAuthenticatedUserFromStore.and.returnValue(observableOf(Object.assign(new EPerson(), { id: 'different-user-id' })));
+    deleteGuard = jasmine.createSpyObj('deleteGuard', ['isCurrentUser', 'getDeleteWarningLabel', 'isSelfDeletionError', 'showSelfDeleteNotification']);
+    deleteGuard.isCurrentUser.and.callFake((ePerson: EPerson, currentId: string) => !!ePerson?.id && ePerson.id === currentId);
+    deleteGuard.getDeleteWarningLabel.and.returnValue(observableOf(undefined));
+    deleteGuard.isSelfDeletionError.and.returnValue(false);
     builderService = getMockFormBuilderService();
     translateService = getMockTranslateService();
 
@@ -135,6 +145,8 @@ describe('EPeopleRegistryComponent', () => {
         { provide: EPersonDataService, useValue: ePersonDataServiceStub },
         { provide: NotificationsService, useValue: new NotificationsServiceStub() },
         { provide: AuthorizationDataService, useValue: authorizationService },
+        { provide: AuthService, useValue: authService },
+        { provide: EPersonDeleteGuardService, useValue: deleteGuard },
         { provide: FormBuilderService, useValue: builderService },
         { provide: Router, useValue: new RouterStub() },
         { provide: RequestService, useValue: jasmine.createSpyObj('requestService', ['removeByHrefSubstring']) },
@@ -257,6 +269,25 @@ describe('EPeopleRegistryComponent', () => {
     });
   });
 
+  describe('when an EPerson is the currently authenticated user', () => {
+    beforeEach(() => {
+      component.currentAuthenticatedUserId = EPersonMock.id;
+      fixture.detectChanges();
+    });
+
+    it('renders the delete button for that row as disabled', () => {
+      const deleteButtons = fixture.debugElement.queryAll(By.css('.access-control-deleteEPersonButton'));
+      const disabled = deleteButtons.filter((button) => button.nativeElement.disabled);
+      expect(disabled.length).toBe(1);
+    });
+
+    it('notifies instead of opening the confirmation modal', () => {
+      component.deleteEPerson(EPersonMock);
+      expect(deleteGuard.showSelfDeleteNotification).toHaveBeenCalled();
+      expect(modalService.open).not.toHaveBeenCalled();
+    });
+  });
+
   describe('delete EPerson button when the isAuthorized returns false', () => {
     let ePeopleDeleteButton;
     beforeEach(() => {

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

@@ -21,6 +21,8 @@ import { RequestService } from '../../core/data/request.service';
 import { PageInfo } from '../../core/shared/page-info.model';
 import { NoContent } from '../../core/shared/NoContent.model';
 import { PaginationService } from '../../core/pagination/pagination.service';
+import { AuthService } from '../../core/auth/auth.service';
+import { EPersonDeleteGuardService, SELF_DELETE_WARNING_LABEL } from './eperson-delete-guard.service';
 
 @Component({
   selector: 'ds-epeople-registry',
@@ -33,6 +35,9 @@ import { PaginationService } from '../../core/pagination/pagination.service';
 export class EPeopleRegistryComponent implements OnInit, OnDestroy {
 
   labelPrefix = 'admin.access-control.epeople.';
+  selfDeleteWarningLabel = SELF_DELETE_WARNING_LABEL;
+
+  currentAuthenticatedUserId: string;
 
   /**
    * A list of all the current EPeople within the repository or the result of the search
@@ -89,6 +94,8 @@ export class EPeopleRegistryComponent implements OnInit, OnDestroy {
               private translateService: TranslateService,
               private notificationsService: NotificationsService,
               private authorizationService: AuthorizationDataService,
+              private authService: AuthService,
+              private deleteGuard: EPersonDeleteGuardService,
               private formBuilder: FormBuilder,
               private router: Router,
               private modalService: NgbModal,
@@ -113,6 +120,9 @@ export class EPeopleRegistryComponent implements OnInit, OnDestroy {
     this.searching$.next(true);
     this.isEPersonFormShown = false;
     this.search({scope: this.currentSearchScope, query: this.currentSearchQuery});
+    this.subs.push(this.authService.getAuthenticatedUserFromStore().subscribe((currentUser: EPerson) => {
+      this.currentAuthenticatedUserId = currentUser?.id;
+    }));
     this.subs.push(this.epersonService.getActiveEPerson().subscribe((eperson: EPerson) => {
       if (eperson != null && eperson.id) {
         this.isEPersonFormShown = true;
@@ -224,30 +234,46 @@ export class EPeopleRegistryComponent implements OnInit, OnDestroy {
    */
   deleteEPerson(ePerson: EPerson) {
     if (hasValue(ePerson.id)) {
-      const modalRef = this.modalService.open(ConfirmationModalComponent);
-      modalRef.componentInstance.dso = ePerson;
-      modalRef.componentInstance.headerLabel = 'confirmation-modal.delete-eperson.header';
-      modalRef.componentInstance.infoLabel = 'confirmation-modal.delete-eperson.info';
-      modalRef.componentInstance.cancelLabel = 'confirmation-modal.delete-eperson.cancel';
-      modalRef.componentInstance.confirmLabel = 'confirmation-modal.delete-eperson.confirm';
-      modalRef.componentInstance.brandColor = 'danger';
-      modalRef.componentInstance.confirmIcon = 'fas fa-trash';
-      modalRef.componentInstance.response.pipe(take(1)).subscribe((confirm: boolean) => {
-        if (confirm) {
-          if (hasValue(ePerson.id)) {
+      if (!hasValue(this.currentAuthenticatedUserId)) {
+        return;
+      }
+
+      if (this.isCurrentUser(ePerson)) {
+        this.deleteGuard.showSelfDeleteNotification();
+        return;
+      }
+
+      this.deleteGuard.getDeleteWarningLabel(ePerson).pipe(take(1)).subscribe((warningLabel: string | undefined) => {
+        const modalRef = this.modalService.open(ConfirmationModalComponent);
+        modalRef.componentInstance.dso = ePerson;
+        modalRef.componentInstance.headerLabel = 'confirmation-modal.delete-eperson.header';
+        modalRef.componentInstance.infoLabel = 'confirmation-modal.delete-eperson.info';
+        modalRef.componentInstance.warningLabel = warningLabel;
+        modalRef.componentInstance.cancelLabel = 'confirmation-modal.delete-eperson.cancel';
+        modalRef.componentInstance.confirmLabel = 'confirmation-modal.delete-eperson.confirm';
+        modalRef.componentInstance.brandColor = 'danger';
+        modalRef.componentInstance.confirmIcon = 'fas fa-trash';
+        modalRef.componentInstance.response.pipe(take(1)).subscribe((confirm: boolean) => {
+          if (confirm) {
             this.epersonService.deleteEPerson(ePerson).pipe(getFirstCompletedRemoteData()).subscribe((restResponse: RemoteData<NoContent>) => {
               if (restResponse.hasSucceeded) {
                 this.notificationsService.success(this.translateService.get(this.labelPrefix + 'notification.deleted.success', {name: ePerson.name}));
+              } else if (this.isCurrentUser(ePerson) || this.deleteGuard.isSelfDeletionError(restResponse)) {
+                this.deleteGuard.showSelfDeleteNotification();
               } else {
                 this.notificationsService.error('Error occured when trying to delete EPerson with id: ' + ePerson.id + ' with code: ' + restResponse.statusCode + ' and message: ' + restResponse.errorMessage);
               }
             });
           }
-        }
+        });
       });
     }
   }
 
+  isCurrentUser(ePerson: EPerson): boolean {
+    return this.deleteGuard.isCurrentUser(ePerson, this.currentAuthenticatedUserId);
+  }
+
   /**
    * Unsub all subscriptions
    */

src/app/access-control/epeople-registry/eperson-delete-guard.service.spec.ts

@@ -0,0 +1,147 @@
+import { fakeAsync, TestBed, tick } from '@angular/core/testing';
+import { of as observableOf, throwError as observableThrowError } from 'rxjs';
+import { TranslateService } from '@ngx-translate/core';
+import { AuthorizationDataService } from '../../core/data/feature-authorization/authorization-data.service';
+import { FeatureID } from '../../core/data/feature-authorization/feature-id';
+import { buildPaginatedList } from '../../core/data/paginated-list.model';
+import { PageInfo } from '../../core/shared/page-info.model';
+import { DSpaceObject } from '../../core/shared/dspace-object.model';
+import { SearchService } from '../../core/shared/search/search.service';
+import { WorkflowItemDataService } from '../../core/submission/workflowitem-data.service';
+import { WorkspaceitemDataService } from '../../core/submission/workspaceitem-data.service';
+import { NotificationsService } from '../../shared/notifications/notifications.service';
+import { createFailedRemoteDataObject$, createSuccessfulRemoteDataObject$ } from '../../shared/remote-data.utils';
+import { SearchObjects } from '../../shared/search/models/search-objects.model';
+import { NotificationsServiceStub } from '../../shared/testing/notifications-service.stub';
+import { EPersonMock } from '../../shared/testing/eperson.mock';
+import { EPersonDeleteGuardService } from './eperson-delete-guard.service';
+
+describe('EPersonDeleteGuardService', () => {
+  let service: EPersonDeleteGuardService;
+  let authorizationService: jasmine.SpyObj<AuthorizationDataService>;
+  let workspaceItemDataService: jasmine.SpyObj<WorkspaceitemDataService>;
+  let workflowItemDataService: jasmine.SpyObj<WorkflowItemDataService>;
+  let searchService: jasmine.SpyObj<SearchService>;
+  let notificationsService: NotificationsServiceStub;
+  let translateService: jasmine.SpyObj<TranslateService>;
+
+  const remoteList = (totalElements: number) => createSuccessfulRemoteDataObject$(
+    buildPaginatedList(new PageInfo({ elementsPerPage: 1, totalElements, totalPages: 1, currentPage: 1 }), [])
+  );
+  const searchObjects = (totalElements: number) => createSuccessfulRemoteDataObject$(Object.assign(
+    new SearchObjects<DSpaceObject>(),
+    buildPaginatedList(new PageInfo({ elementsPerPage: 1, totalElements, totalPages: 1, currentPage: 1 }), [])
+  ));
+
+  beforeEach(() => {
+    authorizationService = jasmine.createSpyObj('authorizationService', ['isAuthorized']);
+    authorizationService.isAuthorized.and.returnValue(observableOf(false));
+    workspaceItemDataService = jasmine.createSpyObj('workspaceItemDataService', ['searchBy']);
+    workspaceItemDataService.searchBy.and.returnValue(remoteList(0));
+    workflowItemDataService = jasmine.createSpyObj('workflowItemDataService', ['searchBy']);
+    workflowItemDataService.searchBy.and.returnValue(remoteList(0));
+    searchService = jasmine.createSpyObj('searchService', ['search']);
+    searchService.search.and.returnValue(searchObjects(0));
+    notificationsService = new NotificationsServiceStub();
+    translateService = jasmine.createSpyObj('translateService', ['get']);
+    translateService.get.and.callFake((key: string) => observableOf(key));
+
+    TestBed.configureTestingModule({
+      providers: [
+        EPersonDeleteGuardService,
+        { provide: AuthorizationDataService, useValue: authorizationService },
+        { provide: WorkspaceitemDataService, useValue: workspaceItemDataService },
+        { provide: WorkflowItemDataService, useValue: workflowItemDataService },
+        { provide: SearchService, useValue: searchService },
+        { provide: NotificationsService, useValue: notificationsService },
+        { provide: TranslateService, useValue: translateService },
+      ],
+    });
+    service = TestBed.inject(EPersonDeleteGuardService);
+  });
+
+  describe('isCurrentUser', () => {
+    it('is true only when the ids match', () => {
+      expect(service.isCurrentUser(EPersonMock, EPersonMock.id)).toBeTrue();
+      expect(service.isCurrentUser(EPersonMock, 'someone-else')).toBeFalse();
+      expect(service.isCurrentUser(undefined, EPersonMock.id)).toBeFalsy();
+    });
+  });
+
+  describe('getDeleteWarningLabel', () => {
+    it('returns undefined when the user is neither a submitter nor an admin', fakeAsync(() => {
+      let label: string | undefined = 'unset';
+      service.getDeleteWarningLabel(EPersonMock).subscribe((value) => label = value);
+      tick();
+      expect(label).toBeUndefined();
+    }));
+
+    it('returns the submitter warning when the user has submitted items', fakeAsync(() => {
+      workspaceItemDataService.searchBy.and.returnValue(remoteList(1));
+      let label: string;
+      service.getDeleteWarningLabel(EPersonMock).subscribe((value) => label = value);
+      tick();
+      expect(label).toBe('admin.access-control.epeople.delete.warning.submitter');
+    }));
+
+    it('returns the admin warning, querying the AdministratorOf feature for the target user', fakeAsync(() => {
+      authorizationService.isAuthorized.and.returnValue(observableOf(true));
+      let label: string;
+      service.getDeleteWarningLabel(EPersonMock).subscribe((value) => label = value);
+      tick();
+      expect(authorizationService.isAuthorized).toHaveBeenCalledWith(FeatureID.AdministratorOf, undefined, EPersonMock.id);
+      expect(label).toBe('admin.access-control.epeople.delete.warning.admin');
+    }));
+
+    it('returns the combined warning when both apply', fakeAsync(() => {
+      workspaceItemDataService.searchBy.and.returnValue(remoteList(1));
+      authorizationService.isAuthorized.and.returnValue(observableOf(true));
+      let label: string;
+      service.getDeleteWarningLabel(EPersonMock).subscribe((value) => label = value);
+      tick();
+      expect(label).toBe('admin.access-control.epeople.delete.warning.submitterAndAdmin');
+    }));
+
+    it('degrades each probe to false on error so a failed lookup never blocks the delete', fakeAsync(() => {
+      workspaceItemDataService.searchBy.and.returnValue(observableThrowError(() => new Error('boom')));
+      searchService.search.and.returnValue(observableThrowError(() => new Error('boom')));
+      authorizationService.isAuthorized.and.returnValue(observableThrowError(() => new Error('boom')));
+      let emitted = false;
+      let label: string | undefined = 'unset';
+      service.getDeleteWarningLabel(EPersonMock).subscribe((value) => {
+        emitted = true;
+        label = value;
+      });
+      tick();
+      expect(emitted).toBeTrue();
+      expect(label).toBeUndefined();
+    }));
+  });
+
+  describe('isSelfDeletionError', () => {
+    it('recognises the backend self-delete rejection', fakeAsync(() => {
+      let rd;
+      createFailedRemoteDataObject$('You, as admin user, cannot delete yourself', 400).subscribe((value) => rd = value);
+      tick();
+      expect(service.isSelfDeletionError(rd)).toBeTrue();
+    }));
+
+    it('ignores other failures', fakeAsync(() => {
+      let rd;
+      createFailedRemoteDataObject$('server error', 500).subscribe((value) => rd = value);
+      tick();
+      expect(service.isSelfDeletionError(rd)).toBeFalsy();
+      expect(service.isSelfDeletionError(null)).toBeFalsy();
+    }));
+  });
+
+  describe('showSelfDeleteNotification', () => {
+    it('emits the self-delete error notification', () => {
+      service.showSelfDeleteNotification();
+      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');
+    });
+  });
+});