Skip to content

Commit

Permalink
Merge pull request #2886 from tdonohue/csrf_fixes
Browse files Browse the repository at this point in the history
Ensures CSRF token is initialized prior to first modifying (non-`GET`) request
  • Loading branch information
tdonohue authored Apr 3, 2024
2 parents b3b3ef8 + bbab56a commit 86ddd45
Show file tree
Hide file tree
Showing 33 changed files with 269 additions and 7 deletions.
10 changes: 5 additions & 5 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ jobs:
#CHROME_VERSION: "90.0.4430.212-1"
# Bump Node heap size (OOM in CI after upgrading to Angular 15)
NODE_OPTIONS: '--max-old-space-size=4096'
# Project name to use when running docker-compose prior to e2e tests
# Project name to use when running "docker compose" prior to e2e tests
COMPOSE_PROJECT_NAME: 'ci'
strategy:
# Create a matrix of Node versions to test against (in parallel)
Expand Down Expand Up @@ -108,12 +108,12 @@ jobs:
path: 'coverage/dspace-angular/lcov.info'
retention-days: 14

# Using docker-compose start backend using CI configuration
# Using "docker compose" start backend using CI configuration
# and load assetstore from a cached copy
- name: Start DSpace REST Backend via Docker (for e2e tests)
run: |
docker-compose -f ./docker/docker-compose-ci.yml up -d
docker-compose -f ./docker/cli.yml -f ./docker/cli.assetstore.yml run --rm dspace-cli
docker compose -f ./docker/docker-compose-ci.yml up -d
docker compose -f ./docker/cli.yml -f ./docker/cli.assetstore.yml run --rm dspace-cli
docker container ls
# Run integration tests via Cypress.io
Expand Down Expand Up @@ -182,7 +182,7 @@ jobs:
run: kill -9 $(lsof -t -i:4000)

- name: Shutdown Docker containers
run: docker-compose -f ./docker/docker-compose-ci.yml down
run: docker compose -f ./docker/docker-compose-ci.yml down

# Codecov upload is a separate job in order to allow us to restart this separate from the entire build/test
# job above. This is necessary because Codecov uploads seem to randomly fail at times.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import { HALEndpointService } from '../../../core/shared/hal-endpoint.service';
import { NoContent } from '../../../core/shared/NoContent.model';
import { PageInfo } from '../../../core/shared/page-info.model';
import { UUIDService } from '../../../core/shared/uuid.service';
import { XSRFService } from '../../../core/xsrf/xsrf.service';
import { AlertComponent } from '../../../shared/alert/alert.component';
import { ContextHelpDirective } from '../../../shared/context-help.directive';
import { FormBuilderService } from '../../../shared/form/builder/form-builder.service';
Expand Down Expand Up @@ -244,6 +245,7 @@ describe('GroupFormComponent', () => {
{ provide: HttpClient, useValue: {} },
{ provide: ObjectCacheService, useValue: {} },
{ provide: UUIDService, useValue: {} },
{ provide: XSRFService, useValue: {} },
{ provide: Store, useValue: {} },
{ provide: RemoteDataBuildService, useValue: {} },
{ provide: HALEndpointService, useValue: {} },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import { GroupDataService } from '../../../core/eperson/group-data.service';
import { PaginationService } from '../../../core/pagination/pagination.service';
import { BitstreamFormat } from '../../../core/shared/bitstream-format.model';
import { BitstreamFormatSupportLevel } from '../../../core/shared/bitstream-format-support-level';
import { XSRFService } from '../../../core/xsrf/xsrf.service';
import { HostWindowService } from '../../../shared/host-window.service';
import { NotificationsService } from '../../../shared/notifications/notifications.service';
import { PaginationComponent } from '../../../shared/pagination/pagination.component';
Expand Down Expand Up @@ -143,6 +144,7 @@ describe('BitstreamFormatsComponent', () => {
{ provide: PaginationService, useValue: paginationService },
{ provide: GroupDataService, useValue: groupDataService },
{ provide: ConfigurationDataService, useValue: configurationDataService },
{ provide: XSRFService, useValue: {} },
],
schemas: [NO_ERRORS_SCHEMA],
}).compileComponents();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { AuthorizationDataService } from '../../../../../core/data/feature-autho
import { Item } from '../../../../../core/shared/item.model';
import { ViewMode } from '../../../../../core/shared/view-mode.model';
import { WorkflowItem } from '../../../../../core/submission/models/workflowitem.model';
import { XSRFService } from '../../../../../core/xsrf/xsrf.service';
import { AuthServiceMock } from '../../../../../shared/mocks/auth.service.mock';
import { DSONameServiceMock } from '../../../../../shared/mocks/dso-name.service.mock';
import { getMockLinkService } from '../../../../../shared/mocks/link-service.mock';
Expand Down Expand Up @@ -67,6 +68,7 @@ describe('WorkflowItemSearchResultAdminWorkflowListElementComponent', () => {
{ provide: ThemeService, useValue: getMockThemeService() },
{ provide: AuthService, useValue: new AuthServiceMock() },
{ provide: AuthorizationDataService, useValue: {} },
{ provide: XSRFService, useValue: {} },
],
schemas: [NO_ERRORS_SCHEMA],
})
Expand Down
8 changes: 8 additions & 0 deletions src/app/core/data/request.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
getTestScheduler,
} from 'jasmine-marbles';
import {
BehaviorSubject,
EMPTY,
Observable,
of as observableOf,
Expand All @@ -32,6 +33,7 @@ import { ObjectCacheService } from '../cache/object-cache.service';
import { coreReducers } from '../core.reducers';
import { CoreState } from '../core-state.model';
import { UUIDService } from '../shared/uuid.service';
import { XSRFService } from '../xsrf/xsrf.service';
import {
RequestConfigureAction,
RequestExecuteAction,
Expand Down Expand Up @@ -59,6 +61,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';
Expand Down Expand Up @@ -104,10 +107,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;
Expand Down
14 changes: 13 additions & 1 deletion src/app/core/data/request.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import {
requestIndexSelector,
} from '../index/index.selectors';
import { UUIDService } from '../shared/uuid.service';
import { XSRFService } from '../xsrf/xsrf.service';
import {
RequestConfigureAction,
RequestExecuteAction,
Expand Down Expand Up @@ -168,6 +169,7 @@ export class RequestService {
constructor(private objectCache: ObjectCacheService,
private uuidService: UUIDService,
private store: Store<CoreState>,
protected xsrfService: XSRFService,
private indexStore: Store<MetaIndexState>) {
}

Expand Down Expand Up @@ -450,7 +452,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));
});
}
}

/**
Expand Down
58 changes: 58 additions & 0 deletions src/app/core/xsrf/browser-xsrf.service.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import { HttpClient } from '@angular/common/http';
import {
HttpClientTestingModule,
HttpTestingController,
} from '@angular/common/http/testing';
import { TestBed } from '@angular/core/testing';

import { RESTURLCombiner } from '../url-combiner/rest-url-combiner';
import { BrowserXSRFService } from './browser-xsrf.service';

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 GET to the csrf endpoint`, (done: DoneFn) => {
service.initXSRFToken(httpClient)();

const req = httpTestingController.expectOne({
url: endpointURL,
method: 'GET',
});

req.flush({});
httpTestingController.verify();
expect().nothing();
done();
});

describe(`when the GET succeeds`, () => {
it(`should set tokenInitialized$ to true`, (done: DoneFn) => {
service.initXSRFToken(httpClient)();

const req = httpTestingController.expectOne(endpointURL);

req.flush({});
httpTestingController.verify();

expect(service.tokenInitialized$.getValue()).toBeTrue();
done();
});
});

});
});
30 changes: 30 additions & 0 deletions src/app/core/xsrf/browser-xsrf.service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { HttpClient } from '@angular/common/http';
import { Injectable } from '@angular/core';
import { take } from 'rxjs/operators';

import { RESTURLCombiner } from '../url-combiner/rest-url-combiner';
import { XSRFService } from './xsrf.service';

/**
* Browser (CSR) Service to obtain a new CSRF/XSRF token when needed by our RequestService
* to perform a modify request (e.g. POST/PUT/DELETE).
* NOTE: This is primarily necessary before the *first* modifying request, as the CSRF
* token may not yet be initialized.
*/
@Injectable()
export class BrowserXSRFService extends XSRFService {
initXSRFToken(httpClient: HttpClient): () => Promise<any> {
return () => new Promise<void>((resolve) => {
// Force a new token to be created by calling the CSRF endpoint
httpClient.get(new RESTURLCombiner('/security/csrf').toString(), undefined).pipe(
take(1),
).subscribe(() => {
// Once token is returned, set tokenInitialized to true.
this.tokenInitialized$.next(true);
});

// return immediately, the rest of the app doesn't need to wait for this to finish
resolve();
});
}
}
33 changes: 33 additions & 0 deletions src/app/core/xsrf/server-xsrf.service.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { HttpClient } from '@angular/common/http';

import { ServerXSRFService } from './server-xsrf.service';

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();
});
});
});
});
19 changes: 19 additions & 0 deletions src/app/core/xsrf/server-xsrf.service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { HttpClient } from '@angular/common/http';
import { Injectable } from '@angular/core';

import { XSRFService } from './xsrf.service';

/**
* Server (SSR) Service to obtain a new CSRF/XSRF token. Because SSR only triggers GET
* requests a CSRF token is never needed.
*/
@Injectable()
export class ServerXSRFService extends XSRFService {
initXSRFToken(httpClient: HttpClient): () => Promise<any> {
return () => new Promise<void>((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();
});
}
}
21 changes: 21 additions & 0 deletions src/app/core/xsrf/xsrf.service.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { HttpClient } from '@angular/common/http';

import { XSRFService } from './xsrf.service';

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();
});
});
15 changes: 15 additions & 0 deletions src/app/core/xsrf/xsrf.service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { HttpClient } from '@angular/common/http';
import { Injectable } from '@angular/core';
import { BehaviorSubject } from 'rxjs';

/**
* Abstract CSRF/XSRF Service used to track whether a CSRF token has been received
* from the DSpace REST API. Once it is received, the "tokenInitialized$" flag will
* be set to "true".
*/
@Injectable()
export abstract class XSRFService {
public tokenInitialized$: BehaviorSubject<boolean> = new BehaviorSubject(false);

abstract initXSRFToken(httpClient: HttpClient): () => Promise<any>;
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import { Bitstream } from '../../../../../core/shared/bitstream.model';
import { HALEndpointService } from '../../../../../core/shared/hal-endpoint.service';
import { Item } from '../../../../../core/shared/item.model';
import { UUIDService } from '../../../../../core/shared/uuid.service';
import { XSRFService } from '../../../../../core/xsrf/xsrf.service';
import { getMockThemeService } from '../../../../../shared/mocks/theme-service.mock';
import { CollectionElementLinkType } from '../../../../../shared/object-collection/collection-element-link.type';
import { ItemSearchResult } from '../../../../../shared/object-collection/shared/item-search-result.model';
Expand Down Expand Up @@ -138,6 +139,7 @@ describe('PersonSearchResultListElementSubmissionComponent', () => {
{ provide: Store, useValue: {} },
{ provide: ObjectCacheService, useValue: {} },
{ provide: UUIDService, useValue: {} },
{ provide: XSRFService, useValue: {} },
{ provide: RemoteDataBuildService, useValue: {} },
{ provide: CommunityDataService, useValue: {} },
{ provide: HALEndpointService, useValue: {} },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import { ItemType } from '../../../../core/shared/item-relationships/item-type.m
import { Relationship } from '../../../../core/shared/item-relationships/relationship.model';
import { RelationshipType } from '../../../../core/shared/item-relationships/relationship-type.model';
import { SearchConfigurationService } from '../../../../core/shared/search/search-configuration.service';
import { XSRFService } from '../../../../core/xsrf/xsrf.service';
import { HostWindowService } from '../../../../shared/host-window.service';
import { RouterMock } from '../../../../shared/mocks/router.mock';
import { SelectableListService } from '../../../../shared/object-list/selectable-list/selectable-list.service';
Expand Down Expand Up @@ -257,6 +258,7 @@ describe('EditRelationshipListComponent', () => {
{ provide: ActivatedRoute, useValue: new ActivatedRouteStub() },
{ provide: AuthRequestService, useValue: new AuthRequestServiceStub() },
{ provide: HardRedirectService, useValue: hardRedirectService },
{ provide: XSRFService, useValue: {} },
{ provide: APP_CONFIG, useValue: environmentUseThumbs },
{ provide: REQUEST, useValue: {} },
CookieService,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
import { BitstreamDataService } from '../../../../core/data/bitstream-data.service';
import { Bitstream } from '../../../../core/shared/bitstream.model';
import { PageInfo } from '../../../../core/shared/page-info.model';
import { XSRFService } from '../../../../core/xsrf/xsrf.service';
import { MetadataFieldWrapperComponent } from '../../../../shared/metadata-field-wrapper/metadata-field-wrapper.component';
import { MockBitstreamFormat1 } from '../../../../shared/mocks/item.mock';
import { getMockThemeService } from '../../../../shared/mocks/theme-service.mock';
Expand Down Expand Up @@ -83,6 +84,7 @@ describe('FileSectionComponent', () => {
}), BrowserAnimationsModule, FileSectionComponent, VarDirective, FileSizePipe],
providers: [
{ provide: APP_DATA_SERVICES_MAP, useValue: {} },
{ provide: XSRFService, useValue: {} },
{ provide: BitstreamDataService, useValue: bitstreamDataService },
{ provide: NotificationsService, useValue: new NotificationsServiceStub() },
{ provide: APP_CONFIG, useValue: environment },
Expand Down
Loading

0 comments on commit 86ddd45

Please sign in to comment.