diff --git a/src/app/access-control/group-registry/group-form/group-form.component.spec.ts b/src/app/access-control/group-registry/group-form/group-form.component.spec.ts index f8c5f3cd870..6adf5e2a5b3 100644 --- a/src/app/access-control/group-registry/group-form/group-form.component.spec.ts +++ b/src/app/access-control/group-registry/group-form/group-form.component.spec.ts @@ -38,6 +38,7 @@ import { ValidateGroupExists } from './validators/group-exists.validator'; import { NoContent } from '../../../core/shared/NoContent.model'; import { DSONameService } from '../../../core/breadcrumbs/dso-name.service'; import { DSONameServiceMock } from '../../../shared/mocks/dso-name.service.mock'; +import { XSRFService } from '../../../core/xsrf/xsrf.service'; describe('GroupFormComponent', () => { let component: GroupFormComponent; @@ -211,6 +212,7 @@ describe('GroupFormComponent', () => { { provide: HttpClient, useValue: {} }, { provide: ObjectCacheService, useValue: {} }, { provide: UUIDService, useValue: {} }, + { provide: XSRFService, useValue: {} }, { provide: Store, useValue: {} }, { provide: RemoteDataBuildService, useValue: {} }, { provide: HALEndpointService, useValue: {} }, diff --git a/src/app/core/data/request.service.spec.ts b/src/app/core/data/request.service.spec.ts index 61091c79404..c02bfaf60fe 100644 --- a/src/app/core/data/request.service.spec.ts +++ b/src/app/core/data/request.service.spec.ts @@ -1,6 +1,6 @@ import { Store, StoreModule } from '@ngrx/store'; import { cold, getTestScheduler } from 'jasmine-marbles'; -import { EMPTY, Observable, of as observableOf } from 'rxjs'; +import { EMPTY, Observable, of as observableOf, BehaviorSubject } from 'rxjs'; import { TestScheduler } from 'rxjs/testing'; import { getMockObjectCacheService } from '../../shared/mocks/object-cache.service.mock'; @@ -26,6 +26,7 @@ import { RequestEntryState } from './request-entry-state.model'; import { RestRequest } from './rest-request.model'; import { CoreState } from '../core-state.model'; import { RequestEntry } from './request-entry.model'; +import { XSRFService } from '../xsrf/xsrf.service'; describe('RequestService', () => { let scheduler: TestScheduler; @@ -35,6 +36,7 @@ describe('RequestService', () => { let uuidService: UUIDService; let store: Store; let mockStore: MockStore; + let xsrfService: XSRFService; const testUUID = '5f2a0d2a-effa-4d54-bd54-5663b960f9eb'; const testHref = 'https://rest.api/endpoint/selfLink'; @@ -80,10 +82,15 @@ describe('RequestService', () => { store = TestBed.inject(Store); mockStore = store as MockStore; mockStore.setState(initialState); + xsrfService = { + tokenInitialized$: new BehaviorSubject(false), + } as XSRFService; + service = new RequestService( objectCache, uuidService, store, + xsrfService, undefined ); serviceAsAny = service as any; diff --git a/src/app/core/data/request.service.ts b/src/app/core/data/request.service.ts index 9f43c3f5992..1e5fd2adfb0 100644 --- a/src/app/core/data/request.service.ts +++ b/src/app/core/data/request.service.ts @@ -25,6 +25,7 @@ import { RestRequest } from './rest-request.model'; import { CoreState } from '../core-state.model'; import { RequestState } from './request-state.model'; import { RequestEntry } from './request-entry.model'; +import { XSRFService } from '../xsrf/xsrf.service'; /** * The base selector function to select the request state in the store @@ -137,6 +138,7 @@ export class RequestService { constructor(private objectCache: ObjectCacheService, private uuidService: UUIDService, private store: Store, + protected xsrfService: XSRFService, private indexStore: Store) { } @@ -419,7 +421,17 @@ export class RequestService { */ private dispatchRequest(request: RestRequest) { this.store.dispatch(new RequestConfigureAction(request)); - this.store.dispatch(new RequestExecuteAction(request.uuid)); + // If it's a GET request, or we have an XSRF token, dispatch it immediately + if (request.method === RestRequestMethod.GET || this.xsrfService.tokenInitialized$.getValue() === true) { + this.store.dispatch(new RequestExecuteAction(request.uuid)); + } else { + // Otherwise wait for the XSRF token first + this.xsrfService.tokenInitialized$.pipe( + find((hasInitialized: boolean) => hasInitialized === true) + ).subscribe(() => { + this.store.dispatch(new RequestExecuteAction(request.uuid)); + }); + } } /** diff --git a/src/app/core/xsrf/browser-xsrf.service.spec.ts b/src/app/core/xsrf/browser-xsrf.service.spec.ts new file mode 100644 index 00000000000..378df0e46ba --- /dev/null +++ b/src/app/core/xsrf/browser-xsrf.service.spec.ts @@ -0,0 +1,64 @@ +import { BrowserXSRFService } from './browser-xsrf.service'; +import { HttpClient } from '@angular/common/http'; +import { RESTURLCombiner } from '../url-combiner/rest-url-combiner'; +import { TestBed } from '@angular/core/testing'; +import { HttpClientTestingModule, HttpTestingController } from '@angular/common/http/testing'; + +describe(`BrowserXSRFService`, () => { + let service: BrowserXSRFService; + let httpClient: HttpClient; + let httpTestingController: HttpTestingController; + + const endpointURL = new RESTURLCombiner('/security/csrf').toString(); + + beforeEach(() => { + TestBed.configureTestingModule({ + imports: [ HttpClientTestingModule ], + providers: [ BrowserXSRFService ] + }); + httpClient = TestBed.inject(HttpClient); + httpTestingController = TestBed.inject(HttpTestingController); + service = TestBed.inject(BrowserXSRFService); + }); + + describe(`initXSRFToken`, () => { + it(`should perform a POST to the csrf endpoint`, () => { + service.initXSRFToken(httpClient)(); + + const req = httpTestingController.expectOne({ + url: endpointURL, + method: 'POST' + }); + + req.flush({}); + httpTestingController.verify(); + }); + + describe(`when the POST succeeds`, () => { + it(`should set tokenInitialized$ to true`, () => { + service.initXSRFToken(httpClient)(); + + const req = httpTestingController.expectOne(endpointURL); + + req.flush({}); + httpTestingController.verify(); + + expect(service.tokenInitialized$.getValue()).toBeTrue(); + }); + }); + + describe(`when the POST fails`, () => { + it(`should set tokenInitialized$ to true`, () => { + service.initXSRFToken(httpClient)(); + + const req = httpTestingController.expectOne(endpointURL); + + req.error(new ErrorEvent('415')); + httpTestingController.verify(); + + expect(service.tokenInitialized$.getValue()).toBeTrue(); + }); + }); + + }); +}); diff --git a/src/app/core/xsrf/browser-xsrf.service.ts b/src/app/core/xsrf/browser-xsrf.service.ts new file mode 100644 index 00000000000..f7ae93f360c --- /dev/null +++ b/src/app/core/xsrf/browser-xsrf.service.ts @@ -0,0 +1,26 @@ +import { HttpClient } from '@angular/common/http'; +import { Injectable } from '@angular/core'; +import { RESTURLCombiner } from '../url-combiner/rest-url-combiner'; +import { take, catchError } from 'rxjs/operators'; +import { of as observableOf } from 'rxjs'; +import { XSRFService } from './xsrf.service'; + +@Injectable() +export class BrowserXSRFService extends XSRFService { + initXSRFToken(httpClient: HttpClient): () => Promise { + return () => new Promise((resolve) => { + httpClient.post(new RESTURLCombiner('/security/csrf').toString(), undefined).pipe( + // errors are to be expected if the token and the cookie don't match, that's what we're + // trying to fix for future requests, so just emit any observable to end up in the + // subscribe + catchError(() => observableOf(null)), + take(1), + ).subscribe(() => { + this.tokenInitialized$.next(true); + }); + + // return immediately, the rest of the app doesn't need to wait for this to finish + resolve(undefined); + }); + } +} diff --git a/src/app/core/xsrf/server-xsrf.service.spec.ts b/src/app/core/xsrf/server-xsrf.service.spec.ts new file mode 100644 index 00000000000..b2ace67dd0c --- /dev/null +++ b/src/app/core/xsrf/server-xsrf.service.spec.ts @@ -0,0 +1,32 @@ +import { ServerXSRFService } from './server-xsrf.service'; +import { HttpClient } from '@angular/common/http'; + +describe(`ServerXSRFService`, () => { + let service: ServerXSRFService; + let httpClient: HttpClient; + + beforeEach(() => { + httpClient = jasmine.createSpyObj(['post', 'get', 'request']); + service = new ServerXSRFService(); + }); + + describe(`initXSRFToken`, () => { + it(`shouldn't perform any requests`, (done: DoneFn) => { + service.initXSRFToken(httpClient)().then(() => { + for (const prop in httpClient) { + if (httpClient.hasOwnProperty(prop)) { + expect(httpClient[prop]).not.toHaveBeenCalled(); + } + } + done(); + }); + }); + + it(`should leave tokenInitialized$ on false`, (done: DoneFn) => { + service.initXSRFToken(httpClient)().then(() => { + expect(service.tokenInitialized$.getValue()).toBeFalse(); + done(); + }); + }); + }); +}); diff --git a/src/app/core/xsrf/server-xsrf.service.ts b/src/app/core/xsrf/server-xsrf.service.ts new file mode 100644 index 00000000000..5dcc74a73da --- /dev/null +++ b/src/app/core/xsrf/server-xsrf.service.ts @@ -0,0 +1,14 @@ +import { HttpClient } from '@angular/common/http'; +import { Injectable } from '@angular/core'; +import { XSRFService } from './xsrf.service'; + +@Injectable() +export class ServerXSRFService extends XSRFService { + initXSRFToken(httpClient: HttpClient): () => Promise { + return () => new Promise((resolve) => { + // return immediately, and keep tokenInitialized$ false. The server side can make only GET + // requests, since it can never get a valid XSRF cookie + resolve(undefined); + }); + } +} diff --git a/src/app/core/xsrf/xsrf.interceptor.spec.ts b/src/app/core/xsrf/xsrf.interceptor.spec.ts index 4a78b60fc17..0b8459740fd 100644 --- a/src/app/core/xsrf/xsrf.interceptor.spec.ts +++ b/src/app/core/xsrf/xsrf.interceptor.spec.ts @@ -7,11 +7,13 @@ import { CookieService } from '../services/cookie.service'; import { CookieServiceMock } from '../../shared/mocks/cookie.service.mock'; import { XsrfInterceptor } from './xsrf.interceptor'; import { HttpXsrfTokenExtractorMock } from '../../shared/mocks/http-xsrf-token-extractor.mock'; +import { RESTURLCombiner } from '../url-combiner/rest-url-combiner'; describe(`XsrfInterceptor`, () => { let service: DspaceRestService; let httpMock: HttpTestingController; let cookieService: CookieService; + const restURL = new RESTURLCombiner('server/api/core/items').toString(); // mock XSRF token const testToken = 'test-token'; @@ -45,24 +47,24 @@ describe(`XsrfInterceptor`, () => { }); it('should change withCredentials to true at all times', (done) => { - service.request(RestRequestMethod.POST, 'server/api/core/items', 'test', { withCredentials: false }).subscribe((response) => { + service.request(RestRequestMethod.POST, restURL, 'test', { withCredentials: false }).subscribe((response) => { expect(response).toBeTruthy(); done(); }); - const httpRequest = httpMock.expectOne('server/api/core/items'); + const httpRequest = httpMock.expectOne(restURL); expect(httpRequest.request.withCredentials).toBeTrue(); httpRequest.flush(mockPayload, { status: mockStatusCode, statusText: mockStatusText }); }); it('should add an X-XSRF-TOKEN header when we are sending an HTTP POST request', (done) => { - service.request(RestRequestMethod.POST, 'server/api/core/items', 'test').subscribe((response) => { + service.request(RestRequestMethod.POST, restURL, 'test').subscribe((response) => { expect(response).toBeTruthy(); done(); }); - const httpRequest = httpMock.expectOne('server/api/core/items'); + const httpRequest = httpMock.expectOne(restURL); expect(httpRequest.request.headers.has('X-XSRF-TOKEN')).toBeTrue(); expect(httpRequest.request.withCredentials).toBeTrue(); @@ -73,16 +75,20 @@ describe(`XsrfInterceptor`, () => { httpRequest.flush(mockPayload, { status: mockStatusCode, statusText: mockStatusText }); }); - it('should NOT add an X-XSRF-TOKEN header when we are sending an HTTP GET request', (done) => { - service.request(RestRequestMethod.GET, 'server/api/core/items').subscribe((response) => { + it('should add an X-XSRF-TOKEN header when we are sending an HTTP GET request', (done) => { + service.request(RestRequestMethod.GET, restURL).subscribe((response) => { expect(response).toBeTruthy(); done(); }); - const httpRequest = httpMock.expectOne('server/api/core/items'); + const httpRequest = httpMock.expectOne(restURL); - expect(httpRequest.request.headers.has('X-XSRF-TOKEN')).toBeFalse(); + expect(httpRequest.request.headers.has('X-XSRF-TOKEN')).toBeTrue(); expect(httpRequest.request.withCredentials).toBeTrue(); + const token = httpRequest.request.headers.get('X-XSRF-TOKEN'); + expect(token).toBeDefined(); + expect(token).toBe(testToken.toString()); + httpRequest.flush(mockPayload, { status: mockStatusCode, statusText: mockStatusText }); }); @@ -106,7 +112,7 @@ describe(`XsrfInterceptor`, () => { // Create a mock XSRF token to be returned in response within DSPACE-XSRF-TOKEN header const mockNewXSRFToken = '123456789abcdefg'; - service.request(RestRequestMethod.GET, 'server/api/core/items').subscribe((response) => { + service.request(RestRequestMethod.GET, restURL).subscribe((response) => { expect(response).toBeTruthy(); // ensure mock data (added in below flush() call) is returned. @@ -126,7 +132,7 @@ describe(`XsrfInterceptor`, () => { done(); }); - const httpRequest = httpMock.expectOne('server/api/core/items'); + const httpRequest = httpMock.expectOne(restURL); // Flush & create mock response (including sending back a new XSRF token in header) httpRequest.flush(mockPayload, { @@ -144,7 +150,7 @@ describe(`XsrfInterceptor`, () => { const mockErrorText = 'Forbidden'; const mockErrorMessage = 'CSRF token mismatch'; - service.request(RestRequestMethod.GET, 'server/api/core/items').subscribe({ + service.request(RestRequestMethod.GET, restURL).subscribe({ error: (error) => { expect(error).toBeTruthy(); @@ -160,7 +166,7 @@ describe(`XsrfInterceptor`, () => { } }); - const httpRequest = httpMock.expectOne('server/api/core/items'); + const httpRequest = httpMock.expectOne(restURL); // Flush & create mock error response (including sending back a new XSRF token in header) httpRequest.flush(mockErrorMessage, { diff --git a/src/app/core/xsrf/xsrf.interceptor.ts b/src/app/core/xsrf/xsrf.interceptor.ts index c728a5cbd09..cafe2f7d96f 100644 --- a/src/app/core/xsrf/xsrf.interceptor.ts +++ b/src/app/core/xsrf/xsrf.interceptor.ts @@ -66,10 +66,8 @@ export class XsrfInterceptor implements HttpInterceptor { // Get root URL of configured REST API const restUrl = new RESTURLCombiner('/').toString().toLowerCase(); - // Skip any non-mutating request. This is because our REST API does NOT - // require CSRF verification for read-only requests like GET or HEAD - // Also skip any request which is NOT to our trusted/configured REST API - if (req.method !== 'GET' && req.method !== 'HEAD' && reqUrl.startsWith(restUrl)) { + // Only add the XSRF token to requests to dspace's configured REST API + if (reqUrl.startsWith(restUrl)) { // parse token from XSRF-TOKEN (client-side) cookie const token = this.tokenExtractor.getToken() as string; diff --git a/src/app/core/xsrf/xsrf.service.spec.ts b/src/app/core/xsrf/xsrf.service.spec.ts new file mode 100644 index 00000000000..a7c5c01cb7d --- /dev/null +++ b/src/app/core/xsrf/xsrf.service.spec.ts @@ -0,0 +1,20 @@ +import { XSRFService } from './xsrf.service'; +import { HttpClient } from '@angular/common/http'; + +class XSRFServiceImpl extends XSRFService { + initXSRFToken(httpClient: HttpClient): () => Promise { + return () => null; + } +} + +describe(`XSRFService`, () => { + let service: XSRFService; + + beforeEach(() => { + service = new XSRFServiceImpl(); + }); + + it(`should start with tokenInitialized$.hasValue() === false`, () => { + expect(service.tokenInitialized$.getValue()).toBeFalse(); + }); +}); diff --git a/src/app/core/xsrf/xsrf.service.ts b/src/app/core/xsrf/xsrf.service.ts new file mode 100644 index 00000000000..fb8dfe74b33 --- /dev/null +++ b/src/app/core/xsrf/xsrf.service.ts @@ -0,0 +1,10 @@ +import { HttpClient } from '@angular/common/http'; +import { Injectable } from '@angular/core'; +import { BehaviorSubject } from 'rxjs'; + +@Injectable() +export abstract class XSRFService { + public tokenInitialized$: BehaviorSubject = new BehaviorSubject(false); + + abstract initXSRFToken(httpClient: HttpClient): () => Promise; +} diff --git a/src/modules/app/browser-app.module.ts b/src/modules/app/browser-app.module.ts index d6295cf791d..51a581dacad 100644 --- a/src/modules/app/browser-app.module.ts +++ b/src/modules/app/browser-app.module.ts @@ -1,5 +1,5 @@ import { HttpClient, HttpClientModule } from '@angular/common/http'; -import { NgModule } from '@angular/core'; +import { NgModule, APP_INITIALIZER } from '@angular/core'; import { BrowserModule, BrowserTransferStateModule, makeStateKey, TransferState } from '@angular/platform-browser'; import { BrowserAnimationsModule } from '@angular/platform-browser/animations'; import { REQUEST } from '@nguniversal/express-engine/tokens'; @@ -33,6 +33,8 @@ import { BrowserAuthRequestService } from '../../app/core/auth/browser-auth-requ import { BrowserInitService } from './browser-init.service'; import { ReferrerService } from '../../app/core/services/referrer.service'; import { BrowserReferrerService } from '../../app/core/services/browser.referrer.service'; +import { XSRFService } from '../../app/core/xsrf/xsrf.service'; +import { BrowserXSRFService } from '../../app/core/xsrf/browser-xsrf.service'; export const REQ_KEY = makeStateKey('req'); @@ -73,6 +75,16 @@ export function getRequest(transferState: TransferState): any { useFactory: getRequest, deps: [TransferState] }, + { + provide: APP_INITIALIZER, + useFactory: (xsrfService: XSRFService, httpClient: HttpClient) => xsrfService.initXSRFToken(httpClient), + deps: [ XSRFService, HttpClient ], + multi: true, + }, + { + provide: XSRFService, + useClass: BrowserXSRFService + }, { provide: AuthService, useClass: AuthService diff --git a/src/modules/app/server-app.module.ts b/src/modules/app/server-app.module.ts index b3d718b0b2b..b1de4259e1b 100644 --- a/src/modules/app/server-app.module.ts +++ b/src/modules/app/server-app.module.ts @@ -37,6 +37,8 @@ import { XhrFactory } from '@angular/common'; import { ServerXhrService } from '../../app/core/services/server-xhr.service'; import { ReferrerService } from '../../app/core/services/referrer.service'; import { ServerReferrerService } from '../../app/core/services/server.referrer.service'; +import { XSRFService } from '../../app/core/xsrf/xsrf.service'; +import { ServerXSRFService } from '../../app/core/xsrf/server-xsrf.service'; export function createTranslateLoader(transferState: TransferState) { return new TranslateServerLoader(transferState, 'dist/server/assets/i18n/', '.json'); @@ -94,6 +96,10 @@ export function createTranslateLoader(transferState: TransferState) { provide: AuthRequestService, useClass: ServerAuthRequestService, }, + { + provide: XSRFService, + useClass: ServerXSRFService + }, { provide: LocaleService, useClass: ServerLocaleService