From f131aa8730799d6028a2a4bcd51fc874ac242179 Mon Sep 17 00:00:00 2001 From: Matus Kasak Date: Thu, 6 Aug 2026 13:00:32 +0200 Subject: [PATCH] JCU/perf(api): stop asking the API questions an anonymous user cannot answer An anonymous visitor produced 4 failed REST requests on every page and 6 on an item page. The frontend fires them as feature detection, then discards the error - nothing is broken, but every page view writes 401s into the backend log and errors into the browser console, and each one is a round trip nobody needs. Five call sites asked first and checked the permission afterwards; they now check first: export.menu / import.menu /api/system/scripts/metadata-{export,import} -> only when the user is a site administrator create-report.menu /api/config/properties/contentreport.enable -> only when the user is a site administrator withdrawn-reinstate-item.menu /api/config/correctiontypes/search/findByItem -> only when the user is logged in qa-event-notification /api/integration/qualityassurancesources/search/byTarget -> only when the user may see QA events (canSeeQA) notify-requests-status /api/ldn/notifyrequests/ -> only when COAR Notify is enabled for the user The authorizations these now depend on are already fetched for other reasons, so no request is added. Behaviour for a user who does have the permission is unchanged. The remaining 404s (google.analytics.key, bulkedit.export.max.items, authentication-password.domain.valid) come from properties that are unset or not exposed in the backend and belong in local.cfg, not here. Fixes the frontend part of L3 from dataquest-dev/dspace-customers#853. Co-Authored-By: Claude Opus 5 (1M context) --- .../notify-requests-status.component.spec.ts | 19 +++++++ .../notify-requests-status.component.ts | 37 ++++++++----- .../qa-event-notification.component.spec.ts | 33 ++++++++++++ .../qa-event-notification.component.ts | 38 ++++++++----- .../menu/providers/create-report.menu.spec.ts | 11 ++++ .../menu/providers/create-report.menu.ts | 53 +++++++++++-------- .../shared/menu/providers/export.menu.spec.ts | 15 +++++- src/app/shared/menu/providers/export.menu.ts | 19 ++++--- .../shared/menu/providers/import.menu.spec.ts | 15 +++++- src/app/shared/menu/providers/import.menu.ts | 19 ++++--- .../withdrawn-reinstate-item.menu.spec.ts | 17 ++++++ .../withdrawn-reinstate-item.menu.ts | 27 +++++++--- 12 files changed, 232 insertions(+), 71 deletions(-) diff --git a/src/app/item-page/simple/notify-requests-status/notify-requests-status-component/notify-requests-status.component.spec.ts b/src/app/item-page/simple/notify-requests-status/notify-requests-status-component/notify-requests-status.component.spec.ts index 6c0f6139116..9a5ae56bad2 100644 --- a/src/app/item-page/simple/notify-requests-status/notify-requests-status-component/notify-requests-status.component.spec.ts +++ b/src/app/item-page/simple/notify-requests-status/notify-requests-status-component/notify-requests-status.component.spec.ts @@ -6,8 +6,10 @@ import { waitForAsync, } from '@angular/core/testing'; import { TranslateModule } from '@ngx-translate/core'; +import { of } from 'rxjs'; import { NotifyRequestsStatusDataService } from 'src/app/core/data/notify-services-status-data.service'; +import { NotifyInfoService } from '../../../../core/coar-notify/notify-info/notify-info.service'; import { createSuccessfulRemoteDataObject$ } from '../../../../shared/remote-data.utils'; import { NotifyRequestsStatus } from '../notify-requests-status.model'; import { RequestStatusEnum } from '../notify-status.enum'; @@ -18,6 +20,7 @@ describe('NotifyRequestsStatusComponent', () => { let component: NotifyRequestsStatusComponent; let fixture: ComponentFixture; let notifyInfoServiceSpy; + let notifyInfoSpy; const mock: NotifyRequestsStatus = Object.assign(new NotifyRequestsStatus(), { notifyStatus: [], @@ -28,10 +31,14 @@ describe('NotifyRequestsStatusComponent', () => { notifyInfoServiceSpy = { getNotifyRequestsStatus:() => createSuccessfulRemoteDataObject$(mock), }; + notifyInfoSpy = { + isCoarConfigEnabled: () => of(true), + }; TestBed.configureTestingModule({ imports: [TranslateModule.forRoot(), NotifyRequestsStatusComponent], providers: [ { provide: NotifyRequestsStatusDataService, useValue: notifyInfoServiceSpy }, + { provide: NotifyInfoService, useValue: notifyInfoSpy }, ], }).overrideComponent(NotifyRequestsStatusComponent, { remove: { @@ -66,6 +73,18 @@ describe('NotifyRequestsStatusComponent', () => { }); })); + it('should not ask the backend for the request status when COAR Notify is disabled', fakeAsync(() => { + spyOn(notifyInfoSpy, 'isCoarConfigEnabled').and.returnValue(of(false)); + spyOn(notifyInfoServiceSpy, 'getNotifyRequestsStatus').and.callThrough(); + + component.itemUuid = 'testUuid'; + component.ngOnInit(); + component.requestMap$.subscribe(); + tick(); + + expect(notifyInfoServiceSpy.getNotifyRequestsStatus).not.toHaveBeenCalled(); + })); + it('should group data by status', () => { const mockData: NotifyRequestsStatus = Object.assign(new NotifyRequestsStatus(), { notifyStatus: [ diff --git a/src/app/item-page/simple/notify-requests-status/notify-requests-status-component/notify-requests-status.component.ts b/src/app/item-page/simple/notify-requests-status/notify-requests-status-component/notify-requests-status.component.ts index 3a735798bbb..e51acd644f7 100644 --- a/src/app/item-page/simple/notify-requests-status/notify-requests-status-component/notify-requests-status.component.ts +++ b/src/app/item-page/simple/notify-requests-status/notify-requests-status-component/notify-requests-status.component.ts @@ -9,11 +9,14 @@ import { OnInit, } from '@angular/core'; import { + EMPTY, filter, map, Observable, + switchMap, } from 'rxjs'; +import { NotifyInfoService } from '../../../../core/coar-notify/notify-info/notify-info.service'; import { NotifyRequestsStatusDataService } from '../../../../core/data/notify-services-status-data.service'; import { getFirstCompletedRemoteData, @@ -55,20 +58,30 @@ export class NotifyRequestsStatusComponent implements OnInit { */ requestMap$: Observable>; - constructor(private notifyInfoService: NotifyRequestsStatusDataService) { } + constructor( + private notifyRequestsStatusDataService: NotifyRequestsStatusDataService, + private notifyInfoService: NotifyInfoService, + ) { } ngOnInit(): void { - this.requestMap$ = this.notifyInfoService - .getNotifyRequestsStatus(this.itemUuid) - .pipe( - getFirstCompletedRemoteData(), - filter((data) => hasValue(data)), - getRemoteDataPayload(), - filter((data: NotifyRequestsStatus) => hasValue(data)), - map((data: NotifyRequestsStatus) => { - return this.groupDataByStatus(data); - }), - ); + // /api/ldn/notifyrequests/ answers 401 unless COAR Notify is both enabled and visible to + // the current user, so this was a guaranteed error on every item page for anonymous visitors. + // The same feature flag already gates the COAR links built by ItemPageComponent. + this.requestMap$ = this.notifyInfoService.isCoarConfigEnabled().pipe( + switchMap((coarEnabled: boolean) => coarEnabled + ? this.notifyRequestsStatusDataService + .getNotifyRequestsStatus(this.itemUuid) + .pipe( + getFirstCompletedRemoteData(), + filter((data) => hasValue(data)), + getRemoteDataPayload(), + filter((data: NotifyRequestsStatus) => hasValue(data)), + map((data: NotifyRequestsStatus) => { + return this.groupDataByStatus(data); + }), + ) + : EMPTY), + ); } /** diff --git a/src/app/item-page/simple/qa-event-notification/qa-event-notification.component.spec.ts b/src/app/item-page/simple/qa-event-notification/qa-event-notification.component.spec.ts index 16141b0359b..50d8f9bd56c 100644 --- a/src/app/item-page/simple/qa-event-notification/qa-event-notification.component.spec.ts +++ b/src/app/item-page/simple/qa-event-notification/qa-event-notification.component.spec.ts @@ -13,6 +13,8 @@ import { SplitPipe } from 'src/app/shared/utils/split.pipe'; import { APP_DATA_SERVICES_MAP } from '../../../../config/app-config.interface'; import { RemoteDataBuildService } from '../../../core/cache/builders/remote-data-build.service'; import { ObjectCacheService } from '../../../core/cache/object-cache.service'; +import { AuthorizationDataService } from '../../../core/data/feature-authorization/authorization-data.service'; +import { FeatureID } from '../../../core/data/feature-authorization/feature-id'; import { RequestService } from '../../../core/data/request.service'; import { QualityAssuranceSourceObject } from '../../../core/notifications/qa/models/quality-assurance-source.model'; import { QualityAssuranceSourceDataService } from '../../../core/notifications/qa/source/quality-assurance-source-data.service'; @@ -28,6 +30,7 @@ describe('QaEventNotificationComponent', () => { let component: QaEventNotificationComponent; let fixture: ComponentFixture; let qualityAssuranceSourceDataServiceStub: any; + let authorizationServiceStub: any; const obj = Object.assign(new QualityAssuranceSourceObject(), { id: 'sourceName:target', @@ -43,12 +46,16 @@ describe('QaEventNotificationComponent', () => { qualityAssuranceSourceDataServiceStub = { getSourcesByTarget: () => objPL, }; + authorizationServiceStub = { + isAuthorized: () => of(true), + }; await TestBed.configureTestingModule({ imports: [CommonModule, TranslateModule.forRoot(), QaEventNotificationComponent, SplitPipe], providers: [ { provide: APP_DATA_SERVICES_MAP, useValue: {} }, { provide: ActivatedRoute, useValue: new ActivatedRouteStub() }, { provide: QualityAssuranceSourceDataService, useValue: qualityAssuranceSourceDataServiceStub }, + { provide: AuthorizationDataService, useValue: authorizationServiceStub }, { provide: RequestService, useValue: {} }, { provide: NotificationsService, useValue: {} }, { provide: HALEndpointService, useValue: new HALEndpointServiceStub('test') }, @@ -78,4 +85,30 @@ describe('QaEventNotificationComponent', () => { const route = component.getQualityAssuranceRoute(); expect(route).toBe('/notifications/quality-assurance'); }); + + it('should ask for the sources when the user is allowed to see QA events', (done) => { + // the data service is provided by the component itself, so spy on the instance it actually uses + const sourceService = fixture.debugElement.injector.get(QualityAssuranceSourceDataService); + spyOn(sourceService, 'getSourcesByTarget').and.returnValue(objPL); + spyOn(authorizationServiceStub, 'isAuthorized').and.returnValue(of(true)); + + component.getQualityAssuranceSources$().subscribe((sources) => { + expect(authorizationServiceStub.isAuthorized).toHaveBeenCalledWith(FeatureID.CanSeeQA); + expect(sourceService.getSourcesByTarget).toHaveBeenCalled(); + expect(sources).toEqual([obj]); + done(); + }); + }); + + it('should not ask for the sources when the user cannot see QA events', (done) => { + const sourceService = fixture.debugElement.injector.get(QualityAssuranceSourceDataService); + spyOn(sourceService, 'getSourcesByTarget').and.returnValue(objPL); + spyOn(authorizationServiceStub, 'isAuthorized').and.returnValue(of(false)); + + component.getQualityAssuranceSources$().subscribe((sources) => { + expect(sourceService.getSourcesByTarget).not.toHaveBeenCalled(); + expect(sources).toEqual([]); + done(); + }); + }); }); diff --git a/src/app/item-page/simple/qa-event-notification/qa-event-notification.component.ts b/src/app/item-page/simple/qa-event-notification/qa-event-notification.component.ts index aaa9d05196a..5e699b14ac5 100644 --- a/src/app/item-page/simple/qa-event-notification/qa-event-notification.component.ts +++ b/src/app/item-page/simple/qa-event-notification/qa-event-notification.component.ts @@ -8,14 +8,20 @@ import { } from '@angular/core'; import { RouterLink } from '@angular/router'; import { TranslateModule } from '@ngx-translate/core'; -import { Observable } from 'rxjs'; +import { + Observable, + of, +} from 'rxjs'; import { catchError, map, + switchMap, } from 'rxjs/operators'; import { getNotificatioQualityAssuranceRoute } from '../../../admin/admin-routing-paths'; import { RequestParam } from '../../../core/cache/models/request-param.model'; +import { AuthorizationDataService } from '../../../core/data/feature-authorization/authorization-data.service'; +import { FeatureID } from '../../../core/data/feature-authorization/feature-id'; import { FindListOptions } from '../../../core/data/find-list-options.model'; import { PaginatedList } from '../../../core/data/paginated-list.model'; import { RemoteData } from '../../../core/data/remote-data'; @@ -54,6 +60,7 @@ export class QaEventNotificationComponent implements OnChanges { constructor( private qualityAssuranceSourceDataService: QualityAssuranceSourceDataService, + private authorizationService: AuthorizationDataService, ) {} /** @@ -73,17 +80,24 @@ export class QaEventNotificationComponent implements OnChanges { const findListTopicOptions: FindListOptions = { searchParams: [new RequestParam('target', this.item.uuid)], }; - return this.qualityAssuranceSourceDataService.getSourcesByTarget(findListTopicOptions, false) - .pipe( - getFirstCompletedRemoteData(), - map((data: RemoteData>) => { - if (data.hasSucceeded) { - return data.payload.page; - } - return []; - }), - catchError(() => []), - ); + // /api/integration/qualityassurancesources/search/byTarget answers 401 to anyone who is not + // allowed to see quality assurance events, so asking for it without checking the feature first + // produced a guaranteed error on every item page for every anonymous visitor. The notification + // this component renders is only actionable by a user who has that permission anyway. + return this.authorizationService.isAuthorized(FeatureID.CanSeeQA).pipe( + switchMap((canSeeQA: boolean) => canSeeQA + ? this.qualityAssuranceSourceDataService.getSourcesByTarget(findListTopicOptions, false).pipe( + getFirstCompletedRemoteData(), + map((data: RemoteData>) => { + if (data.hasSucceeded) { + return data.payload.page; + } + return []; + }), + catchError(() => of([])), + ) + : of([])), + ); } /** diff --git a/src/app/shared/menu/providers/create-report.menu.spec.ts b/src/app/shared/menu/providers/create-report.menu.spec.ts index 270f14fcf79..35146e3ea89 100644 --- a/src/app/shared/menu/providers/create-report.menu.spec.ts +++ b/src/app/shared/menu/providers/create-report.menu.spec.ts @@ -97,4 +97,15 @@ describe('CreateReportMenuProvider', () => { done(); }); }); + + it('should not read contentreport.enable when the user is not a site administrator', (done) => { + (authorizationServiceStub.isAuthorized as jasmine.Spy).and.returnValue(of(false)); + (configurationDataService.findByPropertyName as jasmine.Spy).calls.reset(); + + provider.getTopSection().subscribe((section) => { + expect(configurationDataService.findByPropertyName).not.toHaveBeenCalled(); + expect(section.visible).toBeFalse(); + done(); + }); + }); }); diff --git a/src/app/shared/menu/providers/create-report.menu.ts b/src/app/shared/menu/providers/create-report.menu.ts index 1d6ff3f4f8e..66fae2620fc 100644 --- a/src/app/shared/menu/providers/create-report.menu.ts +++ b/src/app/shared/menu/providers/create-report.menu.ts @@ -8,10 +8,13 @@ import { Injectable } from '@angular/core'; import { - combineLatest as observableCombineLatest, Observable, + of, } from 'rxjs'; -import { map } from 'rxjs/operators'; +import { + map, + switchMap, +} from 'rxjs/operators'; import { ConfigurationDataService } from '../../../core/data/configuration-data.service'; import { AuthorizationDataService } from '../../../core/data/feature-authorization/authorization-data.service'; @@ -37,19 +40,33 @@ export class CreateReportMenuProvider extends AbstractExpandableMenuProvider { super(); } + /** + * Whether the Reports menu should be shown at all: the user is a site administrator *and* the + * content report feature is switched on in the backend. + * + * The order matters. The authorization is checked first and the configuration is only fetched for + * an administrator, because /api/config/properties/contentreport.enable answers 404 whenever the + * property is not set — which is the default — and that request used to be fired on every page + * load for every visitor, including anonymous ones who can never see this menu. + */ + private isReportMenuAvailable(): Observable { + return this.authorizationService.isAuthorized(FeatureID.AdministratorOf).pipe( + switchMap((isSiteAdmin: boolean) => isSiteAdmin + ? this.configurationDataService.findByPropertyName('contentreport.enable').pipe( + getFirstCompletedRemoteData(), + map((res: RemoteData) => res.hasSucceeded && res.payload && res.payload.values[0] === 'true'), + ) + : of(false)), + ); + } + getSubSections(): Observable { - return observableCombineLatest([ - this.configurationDataService.findByPropertyName('contentreport.enable').pipe( - getFirstCompletedRemoteData(), - map((res: RemoteData) => res.hasSucceeded && res.payload && res.payload.values[0] === 'true'), - ), - this.authorizationService.isAuthorized(FeatureID.AdministratorOf), - ]).pipe( - map(([reportEnabled, isSiteAdmin]: [boolean, boolean]) => { + return this.isReportMenuAvailable().pipe( + map((available: boolean) => { return [ /* Collections Report */ { - visible: isSiteAdmin && reportEnabled, + visible: available, model: { type: MenuItemType.LINK, text: 'menu.section.reports.collections', @@ -59,7 +76,7 @@ export class CreateReportMenuProvider extends AbstractExpandableMenuProvider { }, /* Queries Report */ { - visible: isSiteAdmin && reportEnabled, + visible: available, model: { type: MenuItemType.LINK, text: 'menu.section.reports.queries', @@ -72,16 +89,10 @@ export class CreateReportMenuProvider extends AbstractExpandableMenuProvider { } getTopSection(): Observable { - return observableCombineLatest([ - this.configurationDataService.findByPropertyName('contentreport.enable').pipe( - getFirstCompletedRemoteData(), - map((res: RemoteData) => res.hasSucceeded && res.payload && res.payload.values[0] === 'true'), - ), - this.authorizationService.isAuthorized(FeatureID.AdministratorOf), - ]).pipe( - map(([reportEnabled, isSiteAdmin]: [boolean, boolean]) => { + return this.isReportMenuAvailable().pipe( + map((available: boolean) => { return { - visible: isSiteAdmin && reportEnabled, + visible: available, model: { type: MenuItemType.TEXT, text: 'menu.section.reports', diff --git a/src/app/shared/menu/providers/export.menu.spec.ts b/src/app/shared/menu/providers/export.menu.spec.ts index df97a042696..6d3691c33cd 100644 --- a/src/app/shared/menu/providers/export.menu.spec.ts +++ b/src/app/shared/menu/providers/export.menu.spec.ts @@ -49,17 +49,19 @@ describe('ExportMenuProvider', () => { let provider: ExportMenuProvider; let authorizationServiceStub = new AuthorizationDataServiceStub(); + let scriptServiceStub: ScriptServiceStub; beforeEach(() => { spyOn(authorizationServiceStub, 'isAuthorized').and.returnValue( of(true), ); + scriptServiceStub = new ScriptServiceStub(); TestBed.configureTestingModule({ providers: [ ExportMenuProvider, { provide: AuthorizationDataService, useValue: authorizationServiceStub }, - { provide: ScriptDataService, useClass: ScriptServiceStub }, + { provide: ScriptDataService, useValue: scriptServiceStub }, ], }); provider = TestBed.inject(ExportMenuProvider); @@ -82,4 +84,15 @@ describe('ExportMenuProvider', () => { done(); }); }); + + it('getSubSections should not query the script endpoint when the user is not an administrator', (done) => { + (authorizationServiceStub.isAuthorized as jasmine.Spy).and.returnValue(of(false)); + spyOn(scriptServiceStub, 'scriptWithNameExistsAndCanExecute').and.callThrough(); + + provider.getSubSections().subscribe((sections) => { + expect(scriptServiceStub.scriptWithNameExistsAndCanExecute).not.toHaveBeenCalled(); + expect(sections.every((section) => section.visible === false)).toBeTrue(); + done(); + }); + }); }); diff --git a/src/app/shared/menu/providers/export.menu.ts b/src/app/shared/menu/providers/export.menu.ts index aeccbb1c5a0..c660e1e710b 100644 --- a/src/app/shared/menu/providers/export.menu.ts +++ b/src/app/shared/menu/providers/export.menu.ts @@ -9,10 +9,10 @@ import { Injectable } from '@angular/core'; import { NgbModal } from '@ng-bootstrap/ng-bootstrap'; import { - combineLatest as observableCombineLatest, map, Observable, of, + switchMap, } from 'rxjs'; import { AuthorizationDataService } from '../../../core/data/feature-authorization/authorization-data.service'; @@ -55,14 +55,17 @@ export class ExportMenuProvider extends AbstractExpandableMenuProvider { } public getSubSections(): Observable { - return observableCombineLatest([ - this.authorizationService.isAuthorized(FeatureID.AdministratorOf), - this.scriptDataService.scriptWithNameExistsAndCanExecute(METADATA_EXPORT_SCRIPT_NAME), - ]).pipe( - map(([authorized, metadataExportScriptExists]: [boolean, boolean]) => { + return this.authorizationService.isAuthorized(FeatureID.AdministratorOf).pipe( + // Ask about the script only once we know the user may run it. /api/system/scripts/ + // answers 401 to anyone who cannot execute it, so for an anonymous visitor this request was + // a guaranteed error on every single page load. + switchMap((authorized: boolean) => authorized + ? this.scriptDataService.scriptWithNameExistsAndCanExecute(METADATA_EXPORT_SCRIPT_NAME) + : of(false)), + map((canExportMetadata: boolean) => { return [ { - visible: authorized && metadataExportScriptExists, + visible: canExportMetadata, model: { type: MenuItemType.ONCLICK, text: 'menu.section.export_metadata', @@ -72,7 +75,7 @@ export class ExportMenuProvider extends AbstractExpandableMenuProvider { }, }, { - visible: authorized && metadataExportScriptExists, + visible: canExportMetadata, model: { type: MenuItemType.ONCLICK, text: 'menu.section.export_batch', diff --git a/src/app/shared/menu/providers/import.menu.spec.ts b/src/app/shared/menu/providers/import.menu.spec.ts index 8292445ade7..450ce3053d6 100644 --- a/src/app/shared/menu/providers/import.menu.spec.ts +++ b/src/app/shared/menu/providers/import.menu.spec.ts @@ -48,17 +48,19 @@ describe('ImportMenuProvider', () => { let provider: ImportMenuProvider; let authorizationServiceStub = new AuthorizationDataServiceStub(); + let scriptServiceStub: ScriptServiceStub; beforeEach(() => { spyOn(authorizationServiceStub, 'isAuthorized').and.returnValue( of(true), ); + scriptServiceStub = new ScriptServiceStub(); TestBed.configureTestingModule({ providers: [ ImportMenuProvider, { provide: AuthorizationDataService, useValue: authorizationServiceStub }, - { provide: ScriptDataService, useClass: ScriptServiceStub }, + { provide: ScriptDataService, useValue: scriptServiceStub }, ], }); provider = TestBed.inject(ImportMenuProvider); @@ -81,4 +83,15 @@ describe('ImportMenuProvider', () => { done(); }); }); + + it('getSubSections should not query the script endpoint when the user is not an administrator', (done) => { + (authorizationServiceStub.isAuthorized as jasmine.Spy).and.returnValue(of(false)); + spyOn(scriptServiceStub, 'scriptWithNameExistsAndCanExecute').and.callThrough(); + + provider.getSubSections().subscribe((sections) => { + expect(scriptServiceStub.scriptWithNameExistsAndCanExecute).not.toHaveBeenCalled(); + expect(sections.every((section) => section.visible === false)).toBeTrue(); + done(); + }); + }); }); diff --git a/src/app/shared/menu/providers/import.menu.ts b/src/app/shared/menu/providers/import.menu.ts index 909fc9329db..e6d2dc72747 100644 --- a/src/app/shared/menu/providers/import.menu.ts +++ b/src/app/shared/menu/providers/import.menu.ts @@ -9,10 +9,10 @@ import { Injectable } from '@angular/core'; import { NgbModal } from '@ng-bootstrap/ng-bootstrap'; import { - combineLatest as observableCombineLatest, map, Observable, of, + switchMap, } from 'rxjs'; import { AuthorizationDataService } from '../../../core/data/feature-authorization/authorization-data.service'; @@ -52,14 +52,17 @@ export class ImportMenuProvider extends AbstractExpandableMenuProvider { } public getSubSections(): Observable { - return observableCombineLatest([ - this.authorizationService.isAuthorized(FeatureID.AdministratorOf), - this.scriptDataService.scriptWithNameExistsAndCanExecute(METADATA_IMPORT_SCRIPT_NAME), - ]).pipe( - map(([authorized, metadataImportScriptExists]) => { + return this.authorizationService.isAuthorized(FeatureID.AdministratorOf).pipe( + // Ask about the script only once we know the user may run it. /api/system/scripts/ + // answers 401 to anyone who cannot execute it, so for an anonymous visitor this request was + // a guaranteed error on every single page load. + switchMap((authorized: boolean) => authorized + ? this.scriptDataService.scriptWithNameExistsAndCanExecute(METADATA_IMPORT_SCRIPT_NAME) + : of(false)), + map((canImportMetadata: boolean) => { return [ { - visible: authorized && metadataImportScriptExists, + visible: canImportMetadata, model: { type: MenuItemType.LINK, text: 'menu.section.import_metadata', @@ -67,7 +70,7 @@ export class ImportMenuProvider extends AbstractExpandableMenuProvider { }, }, { - visible: authorized && metadataImportScriptExists, + visible: canImportMetadata, model: { type: MenuItemType.LINK, text: 'menu.section.import_batch', diff --git a/src/app/shared/menu/providers/withdrawn-reinstate-item.menu.spec.ts b/src/app/shared/menu/providers/withdrawn-reinstate-item.menu.spec.ts index d757165cc6d..ef075d8bb4f 100644 --- a/src/app/shared/menu/providers/withdrawn-reinstate-item.menu.spec.ts +++ b/src/app/shared/menu/providers/withdrawn-reinstate-item.menu.spec.ts @@ -1,6 +1,8 @@ import { TestBed } from '@angular/core/testing'; import { TranslateModule } from '@ngx-translate/core'; +import { of } from 'rxjs'; +import { AuthService } from '../../../core/auth/auth.service'; import { Item } from '../../../core/shared/item.model'; import { ITEM } from '../../../core/shared/item.resource-type'; import { CorrectionTypeDataService } from '../../../core/submission/correctiontype-data.service'; @@ -56,6 +58,7 @@ describe('WithdrawnReinstateItemMenuProvider', () => { let correctionTypeDataService; let dsoWithdrawnReinstateModalService; + let authService; beforeEach(() => { const correctionType = Object.assign(new CorrectionType(), { @@ -69,6 +72,9 @@ describe('WithdrawnReinstateItemMenuProvider', () => { dsoWithdrawnReinstateModalService = jasmine.createSpyObj('dsoWithdrawnReinstateModalService', ['openCreateWithdrawnReinstateModal']); + authService = jasmine.createSpyObj('authService', { + 'isAuthenticated': of(true), + }); TestBed.configureTestingModule({ imports: [TranslateModule.forRoot()], @@ -76,6 +82,7 @@ describe('WithdrawnReinstateItemMenuProvider', () => { WithdrawnReinstateItemMenuProvider, { provide: CorrectionTypeDataService, useValue: correctionTypeDataService }, { provide: DsoWithdrawnReinstateModalService, useValue: dsoWithdrawnReinstateModalService }, + { provide: AuthService, useValue: authService }, ], }); provider = TestBed.inject(WithdrawnReinstateItemMenuProvider); @@ -92,5 +99,15 @@ describe('WithdrawnReinstateItemMenuProvider', () => { done(); }); }); + + it('should not query the correction types for an anonymous user', (done) => { + authService.isAuthenticated.and.returnValue(of(false)); + + provider.getSectionsForContext(item).subscribe((sections) => { + expect(correctionTypeDataService.findByItem).not.toHaveBeenCalled(); + expect(sections.every((section) => !section.visible)).toBeTrue(); + done(); + }); + }); }); }); diff --git a/src/app/shared/menu/providers/withdrawn-reinstate-item.menu.ts b/src/app/shared/menu/providers/withdrawn-reinstate-item.menu.ts index 44c9998c51c..879b32bc23f 100644 --- a/src/app/shared/menu/providers/withdrawn-reinstate-item.menu.ts +++ b/src/app/shared/menu/providers/withdrawn-reinstate-item.menu.ts @@ -7,17 +7,23 @@ */ import { Injectable } from '@angular/core'; import { - combineLatest, Observable, + of, } from 'rxjs'; -import { map } from 'rxjs/operators'; +import { + map, + switchMap, +} from 'rxjs/operators'; +import { AuthService } from '../../../core/auth/auth.service'; +import { PaginatedList } from '../../../core/data/paginated-list.model'; import { Item } from '../../../core/shared/item.model'; import { getFirstCompletedRemoteData, getRemoteDataPayload, } from '../../../core/shared/operators'; import { CorrectionTypeDataService } from '../../../core/submission/correctiontype-data.service'; +import { CorrectionType } from '../../../core/submission/models/correctiontype.model'; import { DsoWithdrawnReinstateModalService, REQUEST_REINSTATE, @@ -36,17 +42,22 @@ export class WithdrawnReinstateItemMenuProvider extends DSpaceObjectPageMenuProv constructor( protected dsoWithdrawnReinstateModalService: DsoWithdrawnReinstateModalService, protected correctionTypeDataService: CorrectionTypeDataService, + protected authService: AuthService, ) { super(); } public getSectionsForContext(item: Item): Observable { - return combineLatest([ - this.correctionTypeDataService.findByItem(item.uuid, true).pipe( - getFirstCompletedRemoteData(), - getRemoteDataPayload()), - ]).pipe( - map(([correction]) => { + // /api/config/correctiontypes/search/findByItem answers 401 to anonymous users, so this was a + // guaranteed error on every item page. Requesting a withdrawal or a reinstatement is only + // possible while logged in, so there is nothing to ask about before then. + return this.authService.isAuthenticated().pipe( + switchMap((authenticated: boolean) => authenticated + ? this.correctionTypeDataService.findByItem(item.uuid, true).pipe( + getFirstCompletedRemoteData(), + getRemoteDataPayload()) + : of(null as PaginatedList)), + map((correction: PaginatedList) => { return [ { visible: item.isArchived && correction?.page.some((c) => c.topic === REQUEST_WITHDRAWN),