Skip to content

Commit 872f574

Browse files
author
Matus Kasak
committed
Prevent an admin from deleting their own account
Backport of the UFAL self-delete guard from dtq-dev (dspace-angular #1335, #1357, #1373) to this customer branch. The EPeople registry and the EPerson form now hide/disable the delete action for the currently authenticated user (with an explanatory tooltip), show a contextual warning in the confirmation modal when the target is a submitter and/or an administrator, and surface a friendly notification when the backend rejects a self-delete. Shared logic lives in the new EPersonDeleteGuardService so both call sites stay in sync. Refs dataquest-dev/dspace-customers#855
1 parent c09dd5e commit 872f574

13 files changed

Lines changed: 544 additions & 46 deletions

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

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -74,12 +74,24 @@ <h2 id="search" class="border-bottom pb-2">
7474
title="{{labelPrefix + 'table.edit.buttons.edit' | translate: { name: dsoNameService.getName(epersonDto.eperson) } }}">
7575
<i class="fas fa-edit fa-fw"></i>
7676
</button>
77-
@if (epersonDto.ableToDelete) {
78-
<button (click)="deleteEPerson(epersonDto.eperson)"
79-
class="delete-button btn btn-outline-danger btn-sm access-control-deleteEPersonButton"
80-
title="{{labelPrefix + 'table.edit.buttons.remove' | translate: { name: dsoNameService.getName(epersonDto.eperson) } }}">
81-
<i class="fas fa-trash-alt fa-fw"></i>
82-
</button>
77+
@if (epersonDto.ableToDelete && currentAuthenticatedUserId) {
78+
@if (isCurrentUser(epersonDto.eperson)) {
79+
<span tabindex="0" [ngbTooltip]="selfDeleteWarningLabel | translate" container="body">
80+
<button [dsBtnDisabled]="true"
81+
tabindex="-1"
82+
[attr.aria-label]="selfDeleteWarningLabel | translate"
83+
class="delete-button btn btn-outline-danger btn-sm access-control-deleteEPersonButton"
84+
type="button">
85+
<i class="fas fa-trash-alt fa-fw"></i>
86+
</button>
87+
</span>
88+
} @else {
89+
<button (click)="deleteEPerson(epersonDto.eperson)"
90+
class="delete-button btn btn-outline-danger btn-sm access-control-deleteEPersonButton"
91+
title="{{labelPrefix + 'table.edit.buttons.remove' | translate: { name: dsoNameService.getName(epersonDto.eperson) } }}">
92+
<i class="fas fa-trash-alt fa-fw"></i>
93+
</button>
94+
}
8395
}
8496
</div>
8597
</td>

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import {
3030
of,
3131
} from 'rxjs';
3232

33+
import { AuthService } from '../../core/auth/auth.service';
3334
import { AuthorizationDataService } from '../../core/data/feature-authorization/authorization-data.service';
3435
import { FindListOptions } from '../../core/data/find-list-options.model';
3536
import {
@@ -57,6 +58,7 @@ import {
5758
import { NotificationsServiceStub } from '../../shared/testing/notifications-service.stub';
5859
import { PaginationServiceStub } from '../../shared/testing/pagination-service.stub';
5960
import { EPeopleRegistryComponent } from './epeople-registry.component';
61+
import { EPersonDeleteGuardService } from './eperson-delete-guard.service';
6062
import { EPersonFormComponent } from './eperson-form/eperson-form.component';
6163

6264
describe('EPeopleRegistryComponent', () => {
@@ -67,6 +69,8 @@ describe('EPeopleRegistryComponent', () => {
6769
let mockEPeople: EPerson[];
6870
let ePersonDataServiceStub: any;
6971
let authorizationService: AuthorizationDataService;
72+
let authService: jasmine.SpyObj<AuthService>;
73+
let deleteGuard: jasmine.SpyObj<EPersonDeleteGuardService>;
7074
let modalService: NgbModal;
7175
let paginationService: PaginationServiceStub;
7276

@@ -149,6 +153,12 @@ describe('EPeopleRegistryComponent', () => {
149153
});
150154
builderService = getMockFormBuilderService();
151155

156+
authService = jasmine.createSpyObj('authService', ['getAuthenticatedUserFromStore']);
157+
authService.getAuthenticatedUserFromStore.and.returnValue(of(Object.assign(new EPerson(), { id: 'different-user-id' })));
158+
deleteGuard = jasmine.createSpyObj('deleteGuard', ['isCurrentUser', 'getDeleteWarningLabel', 'isSelfDeletionError', 'showSelfDeleteNotification']);
159+
deleteGuard.isCurrentUser.and.callFake((ePerson: EPerson, currentId: string) => !!ePerson?.id && ePerson.id === currentId);
160+
deleteGuard.getDeleteWarningLabel.and.returnValue(of(undefined));
161+
deleteGuard.isSelfDeletionError.and.returnValue(false);
152162
paginationService = new PaginationServiceStub();
153163
TestBed.configureTestingModule({
154164
imports: [CommonModule, NgbModule, FormsModule, ReactiveFormsModule, BrowserModule, RouterTestingModule.withRoutes([]),
@@ -157,6 +167,8 @@ describe('EPeopleRegistryComponent', () => {
157167
{ provide: EPersonDataService, useValue: ePersonDataServiceStub },
158168
{ provide: NotificationsService, useValue: new NotificationsServiceStub() },
159169
{ provide: AuthorizationDataService, useValue: authorizationService },
170+
{ provide: AuthService, useValue: authService },
171+
{ provide: EPersonDeleteGuardService, useValue: deleteGuard },
160172
{ provide: FormBuilderService, useValue: builderService },
161173
{ provide: Router, useValue: new RouterMock() },
162174
{ provide: RequestService, useValue: jasmine.createSpyObj('requestService', ['removeByHrefSubstring']) },
@@ -257,6 +269,25 @@ describe('EPeopleRegistryComponent', () => {
257269
});
258270
});
259271
});
272+
273+
describe('when the ePerson is the currently authenticated user', () => {
274+
beforeEach(() => {
275+
component.currentAuthenticatedUserId = EPersonMock.id;
276+
fixture.detectChanges();
277+
});
278+
279+
it('renders the delete button for that row as disabled', () => {
280+
const deleteButtons = fixture.debugElement.queryAll(By.css('.access-control-deleteEPersonButton'));
281+
const disabled = deleteButtons.filter((button) => button.nativeElement.getAttribute('aria-disabled') === 'true');
282+
expect(disabled.length).toBe(1);
283+
});
284+
285+
it('notifies instead of opening the confirmation modal', () => {
286+
component.deleteEPerson(EPersonMock);
287+
expect(deleteGuard.showSelfDeleteNotification).toHaveBeenCalled();
288+
expect(modalService.open).not.toHaveBeenCalled();
289+
});
290+
});
260291
});
261292

262293

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

Lines changed: 48 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,10 @@ import {
1515
Router,
1616
RouterModule,
1717
} from '@angular/router';
18-
import { NgbModal } from '@ng-bootstrap/ng-bootstrap';
18+
import {
19+
NgbModal,
20+
NgbTooltipModule,
21+
} from '@ng-bootstrap/ng-bootstrap';
1922
import {
2023
TranslateModule,
2124
TranslateService,
@@ -32,6 +35,7 @@ import {
3235
take,
3336
} from 'rxjs/operators';
3437

38+
import { AuthService } from '../../core/auth/auth.service';
3539
import { DSONameService } from '../../core/breadcrumbs/dso-name.service';
3640
import { AuthorizationDataService } from '../../core/data/feature-authorization/authorization-data.service';
3741
import { FeatureID } from '../../core/data/feature-authorization/feature-id';
@@ -51,6 +55,7 @@ import {
5155
getFirstCompletedRemoteData,
5256
} from '../../core/shared/operators';
5357
import { PageInfo } from '../../core/shared/page-info.model';
58+
import { BtnDisabledDirective } from '../../shared/btn-disabled.directive';
5459
import { ConfirmationModalComponent } from '../../shared/confirmation-modal/confirmation-modal.component';
5560
import { hasValue } from '../../shared/empty.util';
5661
import { ThemedLoadingComponent } from '../../shared/loading/themed-loading.component';
@@ -61,14 +66,20 @@ import {
6166
getEPersonEditRoute,
6267
getEPersonsRoute,
6368
} from '../access-control-routing-paths';
69+
import {
70+
EPersonDeleteGuardService,
71+
SELF_DELETE_WARNING_LABEL,
72+
} from './eperson-delete-guard.service';
6473
import { EPersonFormComponent } from './eperson-form/eperson-form.component';
6574

6675
@Component({
6776
selector: 'ds-epeople-registry',
6877
templateUrl: './epeople-registry.component.html',
6978
imports: [
7079
AsyncPipe,
80+
BtnDisabledDirective,
7181
EPersonFormComponent,
82+
NgbTooltipModule,
7283
NgClass,
7384
PaginationComponent,
7485
ReactiveFormsModule,
@@ -85,6 +96,9 @@ import { EPersonFormComponent } from './eperson-form/eperson-form.component';
8596
export class EPeopleRegistryComponent implements OnInit, OnDestroy {
8697

8798
labelPrefix = 'admin.access-control.epeople.';
99+
selfDeleteWarningLabel = SELF_DELETE_WARNING_LABEL;
100+
101+
currentAuthenticatedUserId: string;
88102

89103
/**
90104
* A list of all the current EPeople within the repository or the result of the search
@@ -138,6 +152,8 @@ export class EPeopleRegistryComponent implements OnInit, OnDestroy {
138152
private translateService: TranslateService,
139153
private notificationsService: NotificationsService,
140154
private authorizationService: AuthorizationDataService,
155+
private authService: AuthService,
156+
private deleteGuard: EPersonDeleteGuardService,
141157
private formBuilder: UntypedFormBuilder,
142158
private router: Router,
143159
private modalService: NgbModal,
@@ -164,6 +180,9 @@ export class EPeopleRegistryComponent implements OnInit, OnDestroy {
164180
this.searching$.next(true);
165181
this.search({ scope: this.currentSearchScope, query: this.currentSearchQuery });
166182
this.activeEPerson$ = this.epersonService.getActiveEPerson();
183+
this.subs.push(this.authService.getAuthenticatedUserFromStore().subscribe((currentUser: EPerson) => {
184+
this.currentAuthenticatedUserId = currentUser?.id;
185+
}));
167186
this.subs.push(this.ePeople$.pipe(
168187
switchMap((epeople: PaginatedList<EPerson>) => {
169188
if (epeople.pageInfo.totalElements > 0) {
@@ -236,30 +255,46 @@ export class EPeopleRegistryComponent implements OnInit, OnDestroy {
236255
*/
237256
deleteEPerson(ePerson: EPerson) {
238257
if (hasValue(ePerson.id)) {
239-
const modalRef = this.modalService.open(ConfirmationModalComponent);
240-
modalRef.componentInstance.name = this.dsoNameService.getName(ePerson);
241-
modalRef.componentInstance.headerLabel = 'confirmation-modal.delete-eperson.header';
242-
modalRef.componentInstance.infoLabel = 'confirmation-modal.delete-eperson.info';
243-
modalRef.componentInstance.cancelLabel = 'confirmation-modal.delete-eperson.cancel';
244-
modalRef.componentInstance.confirmLabel = 'confirmation-modal.delete-eperson.confirm';
245-
modalRef.componentInstance.brandColor = 'danger';
246-
modalRef.componentInstance.confirmIcon = 'fas fa-trash';
247-
modalRef.componentInstance.response.pipe(take(1)).subscribe((confirm: boolean) => {
248-
if (confirm) {
249-
if (hasValue(ePerson.id)) {
258+
if (!hasValue(this.currentAuthenticatedUserId)) {
259+
return;
260+
}
261+
262+
if (this.isCurrentUser(ePerson)) {
263+
this.deleteGuard.showSelfDeleteNotification();
264+
return;
265+
}
266+
267+
this.deleteGuard.getDeleteWarningLabel(ePerson).pipe(take(1)).subscribe((warningLabel: string | undefined) => {
268+
const modalRef = this.modalService.open(ConfirmationModalComponent);
269+
modalRef.componentInstance.name = this.dsoNameService.getName(ePerson);
270+
modalRef.componentInstance.headerLabel = 'confirmation-modal.delete-eperson.header';
271+
modalRef.componentInstance.infoLabel = 'confirmation-modal.delete-eperson.info';
272+
modalRef.componentInstance.warningLabel = warningLabel;
273+
modalRef.componentInstance.cancelLabel = 'confirmation-modal.delete-eperson.cancel';
274+
modalRef.componentInstance.confirmLabel = 'confirmation-modal.delete-eperson.confirm';
275+
modalRef.componentInstance.brandColor = 'danger';
276+
modalRef.componentInstance.confirmIcon = 'fas fa-trash';
277+
modalRef.componentInstance.response.pipe(take(1)).subscribe((confirm: boolean) => {
278+
if (confirm) {
250279
this.epersonService.deleteEPerson(ePerson).pipe(getFirstCompletedRemoteData()).subscribe((restResponse: RemoteData<NoContent>) => {
251280
if (restResponse.hasSucceeded) {
252281
this.notificationsService.success(this.translateService.get(this.labelPrefix + 'notification.deleted.success', { name: this.dsoNameService.getName(ePerson) }));
282+
} else if (this.isCurrentUser(ePerson) || this.deleteGuard.isSelfDeletionError(restResponse)) {
283+
this.deleteGuard.showSelfDeleteNotification();
253284
} else {
254285
this.notificationsService.error(this.translateService.get(this.labelPrefix + 'notification.deleted.success', { id: ePerson.id, statusCode: restResponse.statusCode, errorMessage: restResponse.errorMessage }));
255286
}
256287
});
257288
}
258-
}
289+
});
259290
});
260291
}
261292
}
262293

294+
isCurrentUser(ePerson: EPerson): boolean {
295+
return this.deleteGuard.isCurrentUser(ePerson, this.currentAuthenticatedUserId);
296+
}
297+
263298
/**
264299
* Unsub all subscriptions
265300
*/
Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
import {
2+
fakeAsync,
3+
TestBed,
4+
tick,
5+
} from '@angular/core/testing';
6+
import { TranslateService } from '@ngx-translate/core';
7+
import {
8+
of,
9+
throwError as observableThrowError,
10+
} from 'rxjs';
11+
12+
import { AuthorizationDataService } from '../../core/data/feature-authorization/authorization-data.service';
13+
import { FeatureID } from '../../core/data/feature-authorization/feature-id';
14+
import { buildPaginatedList } from '../../core/data/paginated-list.model';
15+
import { DSpaceObject } from '../../core/shared/dspace-object.model';
16+
import { PageInfo } from '../../core/shared/page-info.model';
17+
import { SearchService } from '../../core/shared/search/search.service';
18+
import { WorkflowItemDataService } from '../../core/submission/workflowitem-data.service';
19+
import { WorkspaceitemDataService } from '../../core/submission/workspaceitem-data.service';
20+
import { NotificationsService } from '../../shared/notifications/notifications.service';
21+
import {
22+
createFailedRemoteDataObject$,
23+
createSuccessfulRemoteDataObject$,
24+
} from '../../shared/remote-data.utils';
25+
import { SearchObjects } from '../../shared/search/models/search-objects.model';
26+
import { EPersonMock } from '../../shared/testing/eperson.mock';
27+
import { NotificationsServiceStub } from '../../shared/testing/notifications-service.stub';
28+
import { EPersonDeleteGuardService } from './eperson-delete-guard.service';
29+
30+
describe('EPersonDeleteGuardService', () => {
31+
let service: EPersonDeleteGuardService;
32+
let authorizationService: jasmine.SpyObj<AuthorizationDataService>;
33+
let workspaceItemDataService: jasmine.SpyObj<WorkspaceitemDataService>;
34+
let workflowItemDataService: jasmine.SpyObj<WorkflowItemDataService>;
35+
let searchService: jasmine.SpyObj<SearchService>;
36+
let notificationsService: NotificationsServiceStub;
37+
let translateService: jasmine.SpyObj<TranslateService>;
38+
39+
const remoteList = (totalElements: number) => createSuccessfulRemoteDataObject$(
40+
buildPaginatedList(new PageInfo({ elementsPerPage: 1, totalElements, totalPages: 1, currentPage: 1 }), []),
41+
);
42+
const searchObjects = (totalElements: number) => createSuccessfulRemoteDataObject$(Object.assign(
43+
new SearchObjects<DSpaceObject>(),
44+
buildPaginatedList(new PageInfo({ elementsPerPage: 1, totalElements, totalPages: 1, currentPage: 1 }), []),
45+
));
46+
47+
beforeEach(() => {
48+
authorizationService = jasmine.createSpyObj('authorizationService', ['isAuthorized']);
49+
authorizationService.isAuthorized.and.returnValue(of(false));
50+
workspaceItemDataService = jasmine.createSpyObj('workspaceItemDataService', ['searchBy']);
51+
workspaceItemDataService.searchBy.and.returnValue(remoteList(0));
52+
workflowItemDataService = jasmine.createSpyObj('workflowItemDataService', ['searchBy']);
53+
workflowItemDataService.searchBy.and.returnValue(remoteList(0));
54+
searchService = jasmine.createSpyObj('searchService', ['search']);
55+
searchService.search.and.returnValue(searchObjects(0));
56+
notificationsService = new NotificationsServiceStub();
57+
translateService = jasmine.createSpyObj('translateService', ['get']);
58+
translateService.get.and.callFake((key: string) => of(key));
59+
60+
TestBed.configureTestingModule({
61+
providers: [
62+
EPersonDeleteGuardService,
63+
{ provide: AuthorizationDataService, useValue: authorizationService },
64+
{ provide: WorkspaceitemDataService, useValue: workspaceItemDataService },
65+
{ provide: WorkflowItemDataService, useValue: workflowItemDataService },
66+
{ provide: SearchService, useValue: searchService },
67+
{ provide: NotificationsService, useValue: notificationsService },
68+
{ provide: TranslateService, useValue: translateService },
69+
],
70+
});
71+
service = TestBed.inject(EPersonDeleteGuardService);
72+
});
73+
74+
describe('isCurrentUser', () => {
75+
it('is true only when the ids match', () => {
76+
expect(service.isCurrentUser(EPersonMock, EPersonMock.id)).toBeTrue();
77+
expect(service.isCurrentUser(EPersonMock, 'someone-else')).toBeFalse();
78+
expect(service.isCurrentUser(undefined, EPersonMock.id)).toBeFalsy();
79+
});
80+
});
81+
82+
describe('getDeleteWarningLabel', () => {
83+
it('returns undefined when the user is neither a submitter nor an admin', fakeAsync(() => {
84+
let label: string | undefined = 'unset';
85+
service.getDeleteWarningLabel(EPersonMock).subscribe((value) => label = value);
86+
tick();
87+
expect(label).toBeUndefined();
88+
}));
89+
90+
it('returns the submitter warning when the user has submitted items', fakeAsync(() => {
91+
workspaceItemDataService.searchBy.and.returnValue(remoteList(1));
92+
let label: string;
93+
service.getDeleteWarningLabel(EPersonMock).subscribe((value) => label = value);
94+
tick();
95+
expect(label).toBe('admin.access-control.epeople.delete.warning.submitter');
96+
}));
97+
98+
it('returns the admin warning, querying the AdministratorOf feature for the target user', fakeAsync(() => {
99+
authorizationService.isAuthorized.and.returnValue(of(true));
100+
let label: string;
101+
service.getDeleteWarningLabel(EPersonMock).subscribe((value) => label = value);
102+
tick();
103+
expect(authorizationService.isAuthorized).toHaveBeenCalledWith(FeatureID.AdministratorOf, undefined, EPersonMock.id);
104+
expect(label).toBe('admin.access-control.epeople.delete.warning.admin');
105+
}));
106+
107+
it('returns the combined warning when both apply', fakeAsync(() => {
108+
workspaceItemDataService.searchBy.and.returnValue(remoteList(1));
109+
authorizationService.isAuthorized.and.returnValue(of(true));
110+
let label: string;
111+
service.getDeleteWarningLabel(EPersonMock).subscribe((value) => label = value);
112+
tick();
113+
expect(label).toBe('admin.access-control.epeople.delete.warning.submitterAndAdmin');
114+
}));
115+
116+
it('degrades each probe to false on error so a failed lookup never blocks the delete', fakeAsync(() => {
117+
workspaceItemDataService.searchBy.and.returnValue(observableThrowError(() => new Error('boom')));
118+
searchService.search.and.returnValue(observableThrowError(() => new Error('boom')));
119+
authorizationService.isAuthorized.and.returnValue(observableThrowError(() => new Error('boom')));
120+
let emitted = false;
121+
let label: string | undefined = 'unset';
122+
service.getDeleteWarningLabel(EPersonMock).subscribe((value) => {
123+
emitted = true;
124+
label = value;
125+
});
126+
tick();
127+
expect(emitted).toBeTrue();
128+
expect(label).toBeUndefined();
129+
}));
130+
});
131+
132+
describe('isSelfDeletionError', () => {
133+
it('recognises the backend self-delete rejection', fakeAsync(() => {
134+
let rd;
135+
createFailedRemoteDataObject$('You, as admin user, cannot delete yourself', 400).subscribe((value) => rd = value);
136+
tick();
137+
expect(service.isSelfDeletionError(rd)).toBeTrue();
138+
}));
139+
140+
it('ignores other failures', fakeAsync(() => {
141+
let rd;
142+
createFailedRemoteDataObject$('server error', 500).subscribe((value) => rd = value);
143+
tick();
144+
expect(service.isSelfDeletionError(rd)).toBeFalsy();
145+
expect(service.isSelfDeletionError(null)).toBeFalsy();
146+
}));
147+
});
148+
149+
describe('showSelfDeleteNotification', () => {
150+
it('emits the self-delete error notification', () => {
151+
service.showSelfDeleteNotification();
152+
expect(notificationsService.error).toHaveBeenCalled();
153+
let translatedKey: string;
154+
notificationsService.error.calls.mostRecent().args[0].subscribe((value) => translatedKey = value);
155+
expect(translatedKey).toBe('admin.access-control.epeople.notification.deleted.forbidden.self');
156+
});
157+
});
158+
});

0 commit comments

Comments
 (0)