CSRF bugfix

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: {} },

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<CoreState>;
   let mockStore: MockStore<CoreState>;
+  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<CoreState>;
     mockStore.setState(initialState);
+    xsrfService = {
+      tokenInitialized$: new BehaviorSubject(false),
+    } as XSRFService;
+
     service = new RequestService(
       objectCache,
       uuidService,
       store,
+      xsrfService,
       undefined
     );
     serviceAsAny = service as any;

src/app/core/data/request.service.ts

@@ -25,6 +25,7 @@
 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 @@
   constructor(private objectCache: ObjectCacheService,
               private uuidService: UUIDService,
               private store: Store<CoreState>,
+              protected xsrfService: XSRFService,
               private indexStore: Store<MetaIndexState>) {
   }
 
@@ -419,7 +421,17 @@
    */
   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));
+      });
+    }
   }
 
   /**

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();
+      });
+    });
+
+  });
+});

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<any> {
+    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);
+    });
+  }
+}

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();
+      });
+    });
+  });
+});

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<any> {
+    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);
+    });
+  }
+}

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, {

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;
 

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<any> {
+    return () => null;
+  }
+}
+
+describe(`XSRFService`, () => {
+  let service: XSRFService;
+
+  beforeEach(() => {
+    service = new XSRFServiceImpl();
+  });
+
+  it(`should start with tokenInitialized$.hasValue() === false`, () => {
+    expect(service.tokenInitialized$.getValue()).toBeFalse();
+  });
+});

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<boolean> = new BehaviorSubject(false);
+
+  abstract initXSRFToken(httpClient: HttpClient): () => Promise<any>;
+}