From eb7d110683ec70d3f0992533b2ce314f89833a21 Mon Sep 17 00:00:00 2001 From: Tim Donohue Date: Wed, 17 Feb 2021 15:53:32 -0600 Subject: [PATCH 1/5] Export all XSRF constants so they can be used elsewhere --- src/app/core/xsrf/xsrf.interceptor.ts | 28 ++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/app/core/xsrf/xsrf.interceptor.ts b/src/app/core/xsrf/xsrf.interceptor.ts index 7b5a66f27a8..0301a709947 100644 --- a/src/app/core/xsrf/xsrf.interceptor.ts +++ b/src/app/core/xsrf/xsrf.interceptor.ts @@ -6,6 +6,13 @@ import { RESTURLCombiner } from '../url-combiner/rest-url-combiner'; import { CookieService } from '../services/cookie.service'; import { throwError } from 'rxjs'; +// Name of XSRF header we may send in requests to backend (this is a standard name defined by Angular) +export const XSRF_REQUEST_HEADER = 'X-XSRF-TOKEN'; +// Name of XSRF header we may receive in responses from backend +export const XSRF_RESPONSE_HEADER = 'DSPACE-XSRF-TOKEN'; +// Name of cookie where we store the XSRF token +export const XSRF_COOKIE = 'XSRF-TOKEN'; + /** * Custom Http Interceptor intercepting Http Requests & Responses to * exchange XSRF/CSRF tokens with the backend. @@ -43,11 +50,6 @@ export class XsrfInterceptor implements HttpInterceptor { * @param next */ intercept(req: HttpRequest, next: HttpHandler): Observable> { - // Name of XSRF header we may send in requests to backend (this is a standard name defined by Angular) - const requestCsrfHeader = 'X-XSRF-TOKEN'; - // Name of XSRF header we may receive in responses from backend - const responseCsrfHeader = 'DSPACE-XSRF-TOKEN'; - // Ensure EVERY request from Angular includes "withCredentials: true". // This allows Angular to receive & send cookies via a CORS request (to // the backend). ONLY requests with credentials will: @@ -71,8 +73,8 @@ export class XsrfInterceptor implements HttpInterceptor { const token = this.tokenExtractor.getToken() as string; // send token in request's X-XSRF-TOKEN header (anti-CSRF security) to backend - if (token !== null && !req.headers.has(requestCsrfHeader)) { - req = req.clone({ headers: req.headers.set(requestCsrfHeader, token) }); + if (token !== null && !req.headers.has(XSRF_REQUEST_HEADER)) { + req = req.clone({ headers: req.headers.set(XSRF_REQUEST_HEADER, token) }); } } // Pass to next interceptor, but intercept EVERY response event as well @@ -82,9 +84,9 @@ export class XsrfInterceptor implements HttpInterceptor { if (response instanceof HttpResponse) { // For every response that comes back, check for the custom // DSPACE-XSRF-TOKEN header sent from the backend. - if (response.headers.has(responseCsrfHeader)) { + if (response.headers.has(XSRF_RESPONSE_HEADER)) { // value of header is a new XSRF token - this.saveXsrfToken(response.headers.get(responseCsrfHeader)); + this.saveXsrfToken(response.headers.get(XSRF_RESPONSE_HEADER)); } } }), @@ -92,9 +94,9 @@ export class XsrfInterceptor implements HttpInterceptor { if (error instanceof HttpErrorResponse) { // For every error that comes back, also check for the custom // DSPACE-XSRF-TOKEN header sent from the backend. - if (error.headers.has(responseCsrfHeader)) { + if (error.headers.has(XSRF_RESPONSE_HEADER)) { // value of header is a new XSRF token - this.saveXsrfToken(error.headers.get(responseCsrfHeader)); + this.saveXsrfToken(error.headers.get(XSRF_RESPONSE_HEADER)); } } // Return error response as is. @@ -111,7 +113,7 @@ export class XsrfInterceptor implements HttpInterceptor { // Save token value as a *new* value of our client-side XSRF-TOKEN cookie. // This is the cookie that is parsed by Angular's tokenExtractor(), // which we will send back in the X-XSRF-TOKEN header per Angular best practices. - this.cookieService.remove('XSRF-TOKEN'); - this.cookieService.set('XSRF-TOKEN', token); + this.cookieService.remove(XSRF_COOKIE); + this.cookieService.set(XSRF_COOKIE, token); } } From 5589016a4e8008f5fd62a71a2a6c71d4a3bff7c1 Mon Sep 17 00:00:00 2001 From: Tim Donohue Date: Wed, 17 Feb 2021 15:54:08 -0600 Subject: [PATCH 2/5] Add XSRF support to uploader component --- src/app/shared/uploader/uploader.component.ts | 43 ++++++++++++++++--- 1 file changed, 37 insertions(+), 6 deletions(-) diff --git a/src/app/shared/uploader/uploader.component.ts b/src/app/shared/uploader/uploader.component.ts index ac5c5c65a20..1264dd44e48 100644 --- a/src/app/shared/uploader/uploader.component.ts +++ b/src/app/shared/uploader/uploader.component.ts @@ -18,6 +18,9 @@ import { UploaderOptions } from './uploader-options.model'; import { hasValue, isNotEmpty, isUndefined } from '../empty.util'; import { UploaderService } from './uploader.service'; import { UploaderProperties } from './uploader-properties.model'; +import { HttpXsrfTokenExtractor } from '@angular/common/http'; +import { XSRF_REQUEST_HEADER, XSRF_RESPONSE_HEADER, XSRF_COOKIE } from '../../core/xsrf/xsrf.interceptor'; +import { CookieService } from '../../core/services/cookie.service'; @Component({ selector: 'ds-uploader', @@ -91,7 +94,9 @@ export class UploaderComponent { } } - constructor(private cdr: ChangeDetectorRef, private scrollToService: ScrollToService, private uploaderService: UploaderService) { + constructor(private cdr: ChangeDetectorRef, private scrollToService: ScrollToService, + private uploaderService: UploaderService, private tokenExtractor: HttpXsrfTokenExtractor, + private cookieService: CookieService) { } /** @@ -108,7 +113,9 @@ export class UploaderComponent { removeAfterUpload: true, autoUpload: this.uploadFilesOptions.autoUpload, method: this.uploadFilesOptions.method, - queueLimit: this.uploadFilesOptions.maxFileNumber + queueLimit: this.uploadFilesOptions.maxFileNumber, + // Ensure the current XSRF token is included in every upload request + headers: [{ name: XSRF_REQUEST_HEADER, value: this.tokenExtractor.getToken() }] }); if (isUndefined(this.enableDragOverDocument)) { @@ -123,10 +130,6 @@ export class UploaderComponent { } ngAfterViewInit() { - // Maybe to remove: needed to avoid CORS issue with our temp upload server - this.uploader.onAfterAddingFile = ((item) => { - item.withCredentials = false; - }); this.uploader.onAfterAddingAll = ((items) => { this.onFileSelected.emit(items); }); @@ -153,11 +156,25 @@ export class UploaderComponent { } this.uploader.onCompleteItem = (item: any, response: any, status: any, headers: any) => { if (isNotEmpty(response)) { + // Check for a changed XSRF token in response & save new token if found + // NOTE: this is only necessary because ng2-file-upload doesn't use an Http service and therefore never + // triggers our xsrf.interceptor.ts. See this bug: https://github.com/valor-software/ng2-file-upload/issues/950 + const token = headers[XSRF_RESPONSE_HEADER.toLowerCase()]; + if (isNotEmpty(token)) { + this.saveXsrfToken(token); + } const responsePath = JSON.parse(response); this.onCompleteItem.emit(responsePath); } }; this.uploader.onErrorItem = (item: any, response: any, status: any, headers: any) => { + // Check for a changed XSRF token in response & save new token if found + // NOTE: this is only necessary because ng2-file-upload doesn't use an Http service and therefore never + // triggers our xsrf.interceptor.ts. See this bug: https://github.com/valor-software/ng2-file-upload/issues/950 + const token = headers[XSRF_RESPONSE_HEADER.toLowerCase()]; + if (isNotEmpty(token)) { + this.saveXsrfToken(token); + } this.onUploadError.emit({ item: item, response: response, status: status, headers: headers }); this.uploader.cancelAll(); }; @@ -201,4 +218,18 @@ export class UploaderComponent { } } + /** + * Save XSRF token found in response. This is a temporary copy of the method in xsrf.interceptor.ts + * It can be removed once ng2-file-upload supports interceptors (see https://github.com/valor-software/ng2-file-upload/issues/950), + * or we switch to a new upload library (see https://github.com/DSpace/dspace-angular/issues/820) + * @param token token found + */ + private saveXsrfToken(token: string) { + // Save token value as a *new* value of our client-side XSRF-TOKEN cookie. + // This is the cookie that is parsed by Angular's tokenExtractor(), + // which we will send back in the X-XSRF-TOKEN header per Angular best practices. + this.cookieService.remove(XSRF_COOKIE); + this.cookieService.set(XSRF_COOKIE, token); + } + } From 59676b7c52e80bb8eec14be14444fc559fe8985a Mon Sep 17 00:00:00 2001 From: Tim Donohue Date: Wed, 17 Feb 2021 17:40:31 -0600 Subject: [PATCH 3/5] Fix broken tests via a new HttpXsrfTokenExtractorMock class. --- ...my-dspace-new-submission.component.spec.ts | 6 ++++++ src/app/core/xsrf/xsrf.interceptor.spec.ts | 20 ++++--------------- .../mocks/http-xsrf-token-extractor.mock.ts | 12 +++++++++++ .../uploader/uploader.component.spec.ts | 8 +++++++- 4 files changed, 29 insertions(+), 17 deletions(-) create mode 100644 src/app/shared/mocks/http-xsrf-token-extractor.mock.ts diff --git a/src/app/+my-dspace-page/my-dspace-new-submission/my-dspace-new-submission.component.spec.ts b/src/app/+my-dspace-page/my-dspace-new-submission/my-dspace-new-submission.component.spec.ts index 4ff31d67cf7..7c6d8918cbb 100644 --- a/src/app/+my-dspace-page/my-dspace-new-submission/my-dspace-new-submission.component.spec.ts +++ b/src/app/+my-dspace-page/my-dspace-new-submission/my-dspace-new-submission.component.spec.ts @@ -21,6 +21,10 @@ import { UploaderService } from '../../shared/uploader/uploader.service'; import { HostWindowService } from '../../shared/host-window.service'; import { HostWindowServiceStub } from '../../shared/testing/host-window-service.stub'; import { UploaderComponent } from '../../shared/uploader/uploader.component'; +import { HttpXsrfTokenExtractor } from '@angular/common/http'; +import { CookieService } from '../../core/services/cookie.service'; +import { CookieServiceMock } from '../../shared/mocks/cookie.service.mock'; +import { HttpXsrfTokenExtractorMock } from '../../shared/mocks/http-xsrf-token-extractor.mock'; describe('MyDSpaceNewSubmissionComponent test', () => { @@ -55,6 +59,8 @@ describe('MyDSpaceNewSubmissionComponent test', () => { ChangeDetectorRef, MyDSpaceNewSubmissionComponent, UploaderService, + { provide: HttpXsrfTokenExtractor, useValue: new HttpXsrfTokenExtractorMock('mock-token') }, + { provide: CookieService, useValue: new CookieServiceMock() }, { provide: HostWindowService, useValue: new HostWindowServiceStub(800) }, ], schemas: [NO_ERRORS_SCHEMA] diff --git a/src/app/core/xsrf/xsrf.interceptor.spec.ts b/src/app/core/xsrf/xsrf.interceptor.spec.ts index 84f10b9e13c..742c4a4a459 100644 --- a/src/app/core/xsrf/xsrf.interceptor.spec.ts +++ b/src/app/core/xsrf/xsrf.interceptor.spec.ts @@ -1,32 +1,20 @@ import { TestBed } from '@angular/core/testing'; import { HttpClientTestingModule, HttpTestingController } from '@angular/common/http/testing'; -import { HttpHeaders, HTTP_INTERCEPTORS, HttpResponse, HttpXsrfTokenExtractor, HttpErrorResponse } from '@angular/common/http'; +import { HttpHeaders, HTTP_INTERCEPTORS, HttpXsrfTokenExtractor } from '@angular/common/http'; import { DspaceRestService } from '../dspace-rest/dspace-rest.service'; import { RestRequestMethod } from '../data/rest-request-method'; import { CookieService } from '../services/cookie.service'; import { CookieServiceMock } from '../../shared/mocks/cookie.service.mock'; import { XsrfInterceptor } from './xsrf.interceptor'; - -/** - * A Mock TokenExtractor which just returns whatever token it is initialized with. - * This mock object is injected into our XsrfInterceptor, so that it always finds - * the same fake XSRF token. - */ -class MockTokenExtractor extends HttpXsrfTokenExtractor { - constructor(private token: string | null) { super(); } - - getToken(): string | null { return this.token; } -} +import { HttpXsrfTokenExtractorMock } from '../../shared/mocks/http-xsrf-token-extractor.mock'; describe(`XsrfInterceptor`, () => { let service: DspaceRestService; let httpMock: HttpTestingController; let cookieService: CookieService; - // Create a MockTokenExtractor which always returns "test-token". This will - // be used as the test HttpXsrfTokenExtractor, see below. + // mock XSRF token const testToken = 'test-token'; - const mockTokenExtractor = new MockTokenExtractor(testToken); // Mock payload/statuses are dummy content as we are not testing the results // of any below requests. We are only testing for X-XSRF-TOKEN header. @@ -46,7 +34,7 @@ describe(`XsrfInterceptor`, () => { useClass: XsrfInterceptor, multi: true, }, - { provide: HttpXsrfTokenExtractor, useValue: mockTokenExtractor }, + { provide: HttpXsrfTokenExtractor, useValue: new HttpXsrfTokenExtractorMock(testToken) }, { provide: CookieService, useValue: new CookieServiceMock() } ], }); diff --git a/src/app/shared/mocks/http-xsrf-token-extractor.mock.ts b/src/app/shared/mocks/http-xsrf-token-extractor.mock.ts new file mode 100644 index 00000000000..78766a0b31b --- /dev/null +++ b/src/app/shared/mocks/http-xsrf-token-extractor.mock.ts @@ -0,0 +1,12 @@ +import { HttpXsrfTokenExtractor } from '@angular/common/http'; + +/** + * A Mock TokenExtractor which just returns whatever token it is initialized with. + * This mock object is injected into our XsrfInterceptor, so that it always finds + * the same fake XSRF token. + */ +export class HttpXsrfTokenExtractorMock extends HttpXsrfTokenExtractor { + constructor(private token: string | null) { super(); } + + getToken(): string | null { return this.token; } +} diff --git a/src/app/shared/uploader/uploader.component.spec.ts b/src/app/shared/uploader/uploader.component.spec.ts index d33c27b8973..6ff54578b5d 100644 --- a/src/app/shared/uploader/uploader.component.spec.ts +++ b/src/app/shared/uploader/uploader.component.spec.ts @@ -10,6 +10,10 @@ import { UploaderComponent } from './uploader.component'; import { FileUploadModule } from 'ng2-file-upload'; import { TranslateModule } from '@ngx-translate/core'; import { createTestComponent } from '../testing/utils.test'; +import { HttpXsrfTokenExtractor } from '@angular/common/http'; +import { CookieService } from '../../core/services/cookie.service'; +import { CookieServiceMock } from '../mocks/cookie.service.mock'; +import { HttpXsrfTokenExtractorMock } from '../mocks/http-xsrf-token-extractor.mock'; describe('Chips component', () => { @@ -33,7 +37,9 @@ describe('Chips component', () => { ChangeDetectorRef, ScrollToService, UploaderComponent, - UploaderService + UploaderService, + { provide: HttpXsrfTokenExtractor, useValue: new HttpXsrfTokenExtractorMock('mock-token') }, + { provide: CookieService, useValue: new CookieServiceMock() }, ], schemas: [CUSTOM_ELEMENTS_SCHEMA] }); From 4a72cb033bcead0f83a6599b9b2dd0b9b92eba5a Mon Sep 17 00:00:00 2001 From: Tim Donohue Date: Fri, 19 Feb 2021 10:58:47 -0600 Subject: [PATCH 4/5] Bug fix. Ensure we check for changed token in empty responses. Also ensure we send token before each upload (as it might change between uploads) --- src/app/shared/uploader/uploader.component.ts | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/app/shared/uploader/uploader.component.ts b/src/app/shared/uploader/uploader.component.ts index 1264dd44e48..05baa735bd9 100644 --- a/src/app/shared/uploader/uploader.component.ts +++ b/src/app/shared/uploader/uploader.component.ts @@ -114,8 +114,6 @@ export class UploaderComponent { autoUpload: this.uploadFilesOptions.autoUpload, method: this.uploadFilesOptions.method, queueLimit: this.uploadFilesOptions.maxFileNumber, - // Ensure the current XSRF token is included in every upload request - headers: [{ name: XSRF_REQUEST_HEADER, value: this.tokenExtractor.getToken() }] }); if (isUndefined(this.enableDragOverDocument)) { @@ -140,6 +138,8 @@ export class UploaderComponent { if (item.url !== this.uploader.options.url) { item.url = this.uploader.options.url; } + // Ensure the current XSRF token is included in every upload request (token may change between items uploaded) + this.uploader.options.headers = [{ name: XSRF_REQUEST_HEADER, value: this.tokenExtractor.getToken() }]; this.onBeforeUpload(); this.isOverDocumentDropZone = observableOf(false); @@ -155,14 +155,15 @@ export class UploaderComponent { }; } this.uploader.onCompleteItem = (item: any, response: any, status: any, headers: any) => { + // Check for a changed XSRF token in response & save new token if found + // NOTE: this is only necessary because ng2-file-upload doesn't use an Http service and therefore never + // triggers our xsrf.interceptor.ts. See this bug: https://github.com/valor-software/ng2-file-upload/issues/950 + const token = headers[XSRF_RESPONSE_HEADER.toLowerCase()]; + if (isNotEmpty(token)) { + this.saveXsrfToken(token); + } + if (isNotEmpty(response)) { - // Check for a changed XSRF token in response & save new token if found - // NOTE: this is only necessary because ng2-file-upload doesn't use an Http service and therefore never - // triggers our xsrf.interceptor.ts. See this bug: https://github.com/valor-software/ng2-file-upload/issues/950 - const token = headers[XSRF_RESPONSE_HEADER.toLowerCase()]; - if (isNotEmpty(token)) { - this.saveXsrfToken(token); - } const responsePath = JSON.parse(response); this.onCompleteItem.emit(responsePath); } From 82035516a75ff14165aaed84292dc35da72b61aa Mon Sep 17 00:00:00 2001 From: Tim Donohue Date: Mon, 1 Mar 2021 16:51:56 -0600 Subject: [PATCH 5/5] Bug fix. Ensure onCompleteItem and onErrorItem both reset the CSRF Header (for next item in queue), as onBeforeUploadItem does not seem to be called for every item in the queue. --- src/app/shared/uploader/uploader.component.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/app/shared/uploader/uploader.component.ts b/src/app/shared/uploader/uploader.component.ts index 05baa735bd9..4ee17ac87b3 100644 --- a/src/app/shared/uploader/uploader.component.ts +++ b/src/app/shared/uploader/uploader.component.ts @@ -155,12 +155,13 @@ export class UploaderComponent { }; } this.uploader.onCompleteItem = (item: any, response: any, status: any, headers: any) => { - // Check for a changed XSRF token in response & save new token if found + // Check for a changed XSRF token in response & save new token if found (to both cookie & header for next request) // NOTE: this is only necessary because ng2-file-upload doesn't use an Http service and therefore never // triggers our xsrf.interceptor.ts. See this bug: https://github.com/valor-software/ng2-file-upload/issues/950 const token = headers[XSRF_RESPONSE_HEADER.toLowerCase()]; if (isNotEmpty(token)) { this.saveXsrfToken(token); + this.uploader.options.headers = [{ name: XSRF_REQUEST_HEADER, value: this.tokenExtractor.getToken() }]; } if (isNotEmpty(response)) { @@ -169,13 +170,15 @@ export class UploaderComponent { } }; this.uploader.onErrorItem = (item: any, response: any, status: any, headers: any) => { - // Check for a changed XSRF token in response & save new token if found + // Check for a changed XSRF token in response & save new token if found (to both cookie & header for next request) // NOTE: this is only necessary because ng2-file-upload doesn't use an Http service and therefore never // triggers our xsrf.interceptor.ts. See this bug: https://github.com/valor-software/ng2-file-upload/issues/950 const token = headers[XSRF_RESPONSE_HEADER.toLowerCase()]; if (isNotEmpty(token)) { this.saveXsrfToken(token); + this.uploader.options.headers = [{ name: XSRF_REQUEST_HEADER, value: this.tokenExtractor.getToken() }]; } + this.onUploadError.emit({ item: item, response: response, status: status, headers: headers }); this.uploader.cancelAll(); };