Skip to content

Commit 46e1d90

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 4035134 commit 46e1d90

14 files changed

Lines changed: 497 additions & 35 deletions

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { SharedModule } from '../shared/shared.module';
55
import { AccessControlRoutingModule } from './access-control-routing.module';
66
import { EPeopleRegistryComponent } from './epeople-registry/epeople-registry.component';
77
import { EPersonFormComponent } from './epeople-registry/eperson-form/eperson-form.component';
8+
import { EPersonDeleteGuardService } from './epeople-registry/eperson-delete-guard.service';
89
import { GroupFormComponent } from './group-registry/group-form/group-form.component';
910
import { MembersListComponent } from './group-registry/group-form/members-list/members-list.component';
1011
import { SubgroupsListComponent } from './group-registry/group-form/subgroup-list/subgroups-list.component';
@@ -45,6 +46,7 @@ export const ValidateEmailErrorStateMatcher: DynamicErrorMessagesMatcher =
4546
provide: DYNAMIC_ERROR_MESSAGES_MATCHER,
4647
useValue: ValidateEmailErrorStateMatcher
4748
},
49+
EPersonDeleteGuardService,
4850
]
4951
})
5052
/**

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

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -77,11 +77,27 @@ <h3 id="search" class="border-bottom pb-2">{{labelPrefix + 'search.head' | trans
7777
title="{{labelPrefix + 'table.edit.buttons.edit' | translate: {name: epersonDto.eperson.name} }}">
7878
<i class="fas fa-edit fa-fw"></i>
7979
</button>
80-
<button [disabled]="!epersonDto.ableToDelete" (click)="deleteEPerson(epersonDto.eperson)"
81-
class="btn btn-outline-danger btn-sm access-control-deleteEPersonButton"
82-
title="{{labelPrefix + 'table.edit.buttons.remove' | translate: {name: epersonDto.eperson.name} }}">
83-
<i class="fas fa-trash-alt fa-fw"></i>
84-
</button>
80+
<ng-container *ngIf="currentAuthenticatedUserId">
81+
<span *ngIf="isCurrentUser(epersonDto.eperson); else enabledDeleteButton"
82+
tabindex="0"
83+
[ngbTooltip]="selfDeleteWarningLabel | translate"
84+
container="body">
85+
<button [disabled]="true"
86+
tabindex="-1"
87+
[attr.aria-label]="selfDeleteWarningLabel | translate"
88+
class="btn btn-outline-danger btn-sm access-control-deleteEPersonButton"
89+
type="button">
90+
<i class="fas fa-trash-alt fa-fw"></i>
91+
</button>
92+
</span>
93+
</ng-container>
94+
<ng-template #enabledDeleteButton>
95+
<button [disabled]="!epersonDto.ableToDelete" (click)="deleteEPerson(epersonDto.eperson)"
96+
class="btn btn-outline-danger btn-sm access-control-deleteEPersonButton"
97+
title="{{labelPrefix + 'table.edit.buttons.remove' | translate: {name: epersonDto.eperson.name} }}">
98+
<i class="fas fa-trash-alt fa-fw"></i>
99+
</button>
100+
</ng-template>
85101
</div>
86102
</td>
87103
</tr>

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
@@ -23,6 +23,8 @@ import { TranslateLoaderMock } from '../../shared/mocks/translate-loader.mock';
2323
import { NotificationsServiceStub } from '../../shared/testing/notifications-service.stub';
2424
import { RouterStub } from '../../shared/testing/router.stub';
2525
import { AuthorizationDataService } from '../../core/data/feature-authorization/authorization-data.service';
26+
import { AuthService } from '../../core/auth/auth.service';
27+
import { EPersonDeleteGuardService } from './eperson-delete-guard.service';
2628
import { RequestService } from '../../core/data/request.service';
2729
import { PaginationService } from '../../core/pagination/pagination.service';
2830
import { PaginationServiceStub } from '../../shared/testing/pagination-service.stub';
@@ -37,6 +39,8 @@ describe('EPeopleRegistryComponent', () => {
3739
let mockEPeople;
3840
let ePersonDataServiceStub: any;
3941
let authorizationService: AuthorizationDataService;
42+
let authService: jasmine.SpyObj<AuthService>;
43+
let deleteGuard: jasmine.SpyObj<EPersonDeleteGuardService>;
4044
let modalService;
4145

4246
let paginationService;
@@ -117,6 +121,12 @@ describe('EPeopleRegistryComponent', () => {
117121
authorizationService = jasmine.createSpyObj('authorizationService', {
118122
isAuthorized: observableOf(true)
119123
});
124+
authService = jasmine.createSpyObj('authService', ['getAuthenticatedUserFromStore']);
125+
authService.getAuthenticatedUserFromStore.and.returnValue(observableOf(Object.assign(new EPerson(), { id: 'different-user-id' })));
126+
deleteGuard = jasmine.createSpyObj('deleteGuard', ['isCurrentUser', 'getDeleteWarningLabel', 'isSelfDeletionError', 'showSelfDeleteNotification']);
127+
deleteGuard.isCurrentUser.and.callFake((ePerson: EPerson, currentId: string) => !!ePerson?.id && ePerson.id === currentId);
128+
deleteGuard.getDeleteWarningLabel.and.returnValue(observableOf(undefined));
129+
deleteGuard.isSelfDeletionError.and.returnValue(false);
120130
builderService = getMockFormBuilderService();
121131
translateService = getMockTranslateService();
122132

@@ -135,6 +145,8 @@ describe('EPeopleRegistryComponent', () => {
135145
{ provide: EPersonDataService, useValue: ePersonDataServiceStub },
136146
{ provide: NotificationsService, useValue: new NotificationsServiceStub() },
137147
{ provide: AuthorizationDataService, useValue: authorizationService },
148+
{ provide: AuthService, useValue: authService },
149+
{ provide: EPersonDeleteGuardService, useValue: deleteGuard },
138150
{ provide: FormBuilderService, useValue: builderService },
139151
{ provide: Router, useValue: new RouterStub() },
140152
{ provide: RequestService, useValue: jasmine.createSpyObj('requestService', ['removeByHrefSubstring']) },
@@ -257,6 +269,25 @@ describe('EPeopleRegistryComponent', () => {
257269
});
258270
});
259271

272+
describe('when an EPerson is the currently authenticated user', () => {
273+
beforeEach(() => {
274+
component.currentAuthenticatedUserId = EPersonMock.id;
275+
fixture.detectChanges();
276+
});
277+
278+
it('renders the delete button for that row as disabled', () => {
279+
const deleteButtons = fixture.debugElement.queryAll(By.css('.access-control-deleteEPersonButton'));
280+
const disabled = deleteButtons.filter((button) => button.nativeElement.disabled);
281+
expect(disabled.length).toBe(1);
282+
});
283+
284+
it('notifies instead of opening the confirmation modal', () => {
285+
component.deleteEPerson(EPersonMock);
286+
expect(deleteGuard.showSelfDeleteNotification).toHaveBeenCalled();
287+
expect(modalService.open).not.toHaveBeenCalled();
288+
});
289+
});
290+
260291
describe('delete EPerson button when the isAuthorized returns false', () => {
261292
let ePeopleDeleteButton;
262293
beforeEach(() => {

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

Lines changed: 38 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ import { RequestService } from '../../core/data/request.service';
2121
import { PageInfo } from '../../core/shared/page-info.model';
2222
import { NoContent } from '../../core/shared/NoContent.model';
2323
import { PaginationService } from '../../core/pagination/pagination.service';
24+
import { AuthService } from '../../core/auth/auth.service';
25+
import { EPersonDeleteGuardService, SELF_DELETE_WARNING_LABEL } from './eperson-delete-guard.service';
2426

2527
@Component({
2628
selector: 'ds-epeople-registry',
@@ -33,6 +35,9 @@ import { PaginationService } from '../../core/pagination/pagination.service';
3335
export class EPeopleRegistryComponent implements OnInit, OnDestroy {
3436

3537
labelPrefix = 'admin.access-control.epeople.';
38+
selfDeleteWarningLabel = SELF_DELETE_WARNING_LABEL;
39+
40+
currentAuthenticatedUserId: string;
3641

3742
/**
3843
* 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 {
8994
private translateService: TranslateService,
9095
private notificationsService: NotificationsService,
9196
private authorizationService: AuthorizationDataService,
97+
private authService: AuthService,
98+
private deleteGuard: EPersonDeleteGuardService,
9299
private formBuilder: FormBuilder,
93100
private router: Router,
94101
private modalService: NgbModal,
@@ -113,6 +120,9 @@ export class EPeopleRegistryComponent implements OnInit, OnDestroy {
113120
this.searching$.next(true);
114121
this.isEPersonFormShown = false;
115122
this.search({scope: this.currentSearchScope, query: this.currentSearchQuery});
123+
this.subs.push(this.authService.getAuthenticatedUserFromStore().subscribe((currentUser: EPerson) => {
124+
this.currentAuthenticatedUserId = currentUser?.id;
125+
}));
116126
this.subs.push(this.epersonService.getActiveEPerson().subscribe((eperson: EPerson) => {
117127
if (eperson != null && eperson.id) {
118128
this.isEPersonFormShown = true;
@@ -224,30 +234,46 @@ export class EPeopleRegistryComponent implements OnInit, OnDestroy {
224234
*/
225235
deleteEPerson(ePerson: EPerson) {
226236
if (hasValue(ePerson.id)) {
227-
const modalRef = this.modalService.open(ConfirmationModalComponent);
228-
modalRef.componentInstance.dso = ePerson;
229-
modalRef.componentInstance.headerLabel = 'confirmation-modal.delete-eperson.header';
230-
modalRef.componentInstance.infoLabel = 'confirmation-modal.delete-eperson.info';
231-
modalRef.componentInstance.cancelLabel = 'confirmation-modal.delete-eperson.cancel';
232-
modalRef.componentInstance.confirmLabel = 'confirmation-modal.delete-eperson.confirm';
233-
modalRef.componentInstance.brandColor = 'danger';
234-
modalRef.componentInstance.confirmIcon = 'fas fa-trash';
235-
modalRef.componentInstance.response.pipe(take(1)).subscribe((confirm: boolean) => {
236-
if (confirm) {
237-
if (hasValue(ePerson.id)) {
237+
if (!hasValue(this.currentAuthenticatedUserId)) {
238+
return;
239+
}
240+
241+
if (this.isCurrentUser(ePerson)) {
242+
this.deleteGuard.showSelfDeleteNotification();
243+
return;
244+
}
245+
246+
this.deleteGuard.getDeleteWarningLabel(ePerson).pipe(take(1)).subscribe((warningLabel: string | undefined) => {
247+
const modalRef = this.modalService.open(ConfirmationModalComponent);
248+
modalRef.componentInstance.dso = ePerson;
249+
modalRef.componentInstance.headerLabel = 'confirmation-modal.delete-eperson.header';
250+
modalRef.componentInstance.infoLabel = 'confirmation-modal.delete-eperson.info';
251+
modalRef.componentInstance.warningLabel = warningLabel;
252+
modalRef.componentInstance.cancelLabel = 'confirmation-modal.delete-eperson.cancel';
253+
modalRef.componentInstance.confirmLabel = 'confirmation-modal.delete-eperson.confirm';
254+
modalRef.componentInstance.brandColor = 'danger';
255+
modalRef.componentInstance.confirmIcon = 'fas fa-trash';
256+
modalRef.componentInstance.response.pipe(take(1)).subscribe((confirm: boolean) => {
257+
if (confirm) {
238258
this.epersonService.deleteEPerson(ePerson).pipe(getFirstCompletedRemoteData()).subscribe((restResponse: RemoteData<NoContent>) => {
239259
if (restResponse.hasSucceeded) {
240260
this.notificationsService.success(this.translateService.get(this.labelPrefix + 'notification.deleted.success', {name: ePerson.name}));
261+
} else if (this.isCurrentUser(ePerson) || this.deleteGuard.isSelfDeletionError(restResponse)) {
262+
this.deleteGuard.showSelfDeleteNotification();
241263
} else {
242264
this.notificationsService.error('Error occured when trying to delete EPerson with id: ' + ePerson.id + ' with code: ' + restResponse.statusCode + ' and message: ' + restResponse.errorMessage);
243265
}
244266
});
245267
}
246-
}
268+
});
247269
});
248270
}
249271
}
250272

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

0 commit comments

Comments
 (0)