Skip to content

Commit

Permalink
Merge pull request #3063 from tdonohue/port_new_csrf_fixes_to_7x
Browse files Browse the repository at this point in the history
[Port dspace-7_x] Ensures CSRF token is initialized prior to first modifying (non-GET) request
  • Loading branch information
tdonohue authored Jul 1, 2024
2 parents 735a3af + 2d81a48 commit 28bd6a5
Show file tree
Hide file tree
Showing 34 changed files with 265 additions and 21 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 @@ -23,6 +23,7 @@ import { DSpaceObject } from '../../../core/shared/dspace-object.model';
import { HALEndpointService } from '../../../core/shared/hal-endpoint.service';
import { PageInfo } from '../../../core/shared/page-info.model';
import { UUIDService } from '../../../core/shared/uuid.service';
import { XSRFService } from '../../../core/xsrf/xsrf.service';
import { FormBuilderService } from '../../../shared/form/builder/form-builder.service';
import { NotificationsService } from '../../../shared/notifications/notifications.service';
import { GroupMock, GroupMock2 } from '../../../shared/testing/group-mock';
Expand Down Expand Up @@ -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: {} },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { NotificationsService } from '../../../shared/notifications/notification
import { NotificationsServiceStub } from '../../../shared/testing/notifications-service.stub';
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 { cold, getTestScheduler, hot } from 'jasmine-marbles';
import { TestScheduler } from 'rxjs/testing';
import {
Expand Down Expand Up @@ -108,7 +109,8 @@ describe('BitstreamFormatsComponent', () => {
{ provide: BitstreamFormatDataService, useValue: bitstreamFormatService },
{ provide: HostWindowService, useValue: new HostWindowServiceStub(0) },
{ provide: NotificationsService, useValue: notificationsServiceStub },
{ provide: PaginationService, useValue: paginationService }
{ provide: PaginationService, useValue: paginationService },
{ provide: XSRFService, useValue: {} },
]
}).compileComponents();
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { CollectionElementLinkType } from '../../../../../shared/object-collecti
import { ViewMode } from '../../../../../core/shared/view-mode.model';
import { RouterTestingModule } from '@angular/router/testing';
import { WorkflowItem } from '../../../../../core/submission/models/workflowitem.model';
import { XSRFService } from '../../../../../core/xsrf/xsrf.service';
import {
WorkflowItemSearchResultAdminWorkflowListElementComponent
} from './workflow-item-search-result-admin-workflow-list-element.component';
Expand Down Expand Up @@ -58,7 +59,8 @@ describe('WorkflowItemSearchResultAdminWorkflowListElementComponent', () => {
{ provide: TruncatableService, useValue: mockTruncatableService },
{ provide: LinkService, useValue: linkService },
{ provide: DSONameService, useClass: DSONameServiceMock },
{ provide: APP_CONFIG, useValue: environment }
{ provide: APP_CONFIG, useValue: environment },
{ provide: XSRFService, useValue: {} },
],
schemas: [NO_ERRORS_SCHEMA]
})
Expand Down
13 changes: 10 additions & 3 deletions src/app/core/data/request.effects.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
import { Injectable, Injector } from '@angular/core';

import { Actions, createEffect, ofType } from '@ngrx/effects';
import { catchError, filter, map, mergeMap, take } from 'rxjs/operators';
import { catchError, filter, map, mergeMap, take, withLatestFrom } from 'rxjs/operators';

import { hasValue, isNotEmpty } from '../../shared/empty.util';
import { StoreActionTypes } from '../../store.actions';
import { getClassForType } from '../cache/builders/build-decorators';
import { RawRestResponse } from '../dspace-rest/raw-rest-response.model';
import { DspaceRestService } from '../dspace-rest/dspace-rest.service';
import { DSpaceSerializer } from '../dspace-rest/dspace.serializer';
import { XSRFService } from '../xsrf/xsrf.service';
import {
RequestActionTypes,
RequestErrorAction,
Expand All @@ -19,6 +20,7 @@ import {
import { RequestService } from './request.service';
import { ParsedResponse } from '../cache/response.models';
import { RequestError } from './request-error.model';
import { RestRequestMethod } from './rest-request-method';
import { RestRequestWithResponseParser } from './rest-request-with-response-parser.model';
import { RequestEntry } from './request-entry.model';

Expand All @@ -33,7 +35,11 @@ export class RequestEffects {
);
}),
filter((entry: RequestEntry) => hasValue(entry)),
map((entry: RequestEntry) => entry.request),
withLatestFrom(this.xsrfService.tokenInitialized$),
// If it's a GET request, or we have an XSRF token, dispatch it immediately
// Otherwise wait for the XSRF token first
filter(([entry, tokenInitialized]: [RequestEntry, boolean]) => entry.request.method === RestRequestMethod.GET || tokenInitialized === true),
map(([entry, tokenInitialized]: [RequestEntry, boolean]) => entry.request),
mergeMap((request: RestRequestWithResponseParser) => {
let body = request.body;
if (isNotEmpty(request.body) && !request.isMultipart) {
Expand Down Expand Up @@ -73,7 +79,8 @@ export class RequestEffects {
private actions$: Actions,
private restApi: DspaceRestService,
private injector: Injector,
protected requestService: RequestService
protected requestService: RequestService,
protected xsrfService: XSRFService,
) { }

}
2 changes: 1 addition & 1 deletion src/app/core/data/request.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,11 @@ describe('RequestService', () => {
store = TestBed.inject(Store);
mockStore = store as MockStore<CoreState>;
mockStore.setState(initialState);

service = new RequestService(
objectCache,
uuidService,
store,
undefined
);
serviceAsAny = service as any;
});
Expand Down
5 changes: 2 additions & 3 deletions src/app/core/data/request.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import cloneDeep from 'lodash/cloneDeep';
import { hasValue, isEmpty, isNotEmpty, hasNoValue } from '../../shared/empty.util';
import { ObjectCacheEntry } from '../cache/object-cache.reducer';
import { ObjectCacheService } from '../cache/object-cache.service';
import { IndexState, MetaIndexState } from '../index/index.reducer';
import { IndexState } from '../index/index.reducer';
import { requestIndexSelector, getUrlWithoutEmbedParams } from '../index/index.selectors';
import { UUIDService } from '../shared/uuid.service';
import {
Expand Down Expand Up @@ -136,8 +136,7 @@ export class RequestService {

constructor(private objectCache: ObjectCacheService,
private uuidService: UUIDService,
private store: Store<CoreState>,
private indexStore: Store<MetaIndexState>) {
private store: Store<CoreState>) {
}

generateRequestId(): string {
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 @@ -20,6 +20,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 { NotificationsService } from '../../../../../shared/notifications/notifications.service';
import { ItemSearchResult } from '../../../../../shared/object-collection/shared/item-search-result.model';
import { SelectableListService } from '../../../../../shared/object-list/selectable-list/selectable-list.service';
Expand Down Expand Up @@ -115,6 +116,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 @@ -28,6 +28,7 @@ import { ConfigurationDataService } from '../../../../core/data/configuration-da
import { LinkHeadService } from '../../../../core/services/link-head.service';
import { SearchConfigurationService } from '../../../../core/shared/search/search-configuration.service';
import { SearchConfigurationServiceStub } from '../../../../shared/testing/search-configuration-service.stub';
import { XSRFService } from '../../../../core/xsrf/xsrf.service';
import { ConfigurationProperty } from '../../../../core/shared/configuration-property.model';
import { Router } from '@angular/router';
import { RouterMock } from '../../../../shared/mocks/router.mock';
Expand Down Expand Up @@ -235,6 +236,7 @@ describe('EditRelationshipListComponent', () => {
{ provide: ConfigurationDataService, useValue: configurationDataService },
{ provide: SearchConfigurationService, useValue: new SearchConfigurationServiceStub() },
{ provide: EditItemRelationshipsService, useValue: editItemRelationshipsService },
{ provide: XSRFService, useValue: {} },
{ provide: APP_CONFIG, useValue: environmentUseThumbs }
], schemas: [
NO_ERRORS_SCHEMA
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { of as observableOf } from 'rxjs';
import { MockBitstreamFormat1 } from '../../../../shared/mocks/item.mock';
import { FileSizePipe } from '../../../../shared/utils/file-size-pipe';
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 { createPaginatedList } from '../../../../shared/testing/utils.test';
import { NotificationsService } from '../../../../shared/notifications/notifications.service';
Expand Down Expand Up @@ -66,6 +67,7 @@ describe('FileSectionComponent', () => {
}), BrowserAnimationsModule],
declarations: [FileSectionComponent, VarDirective, FileSizePipe, MetadataFieldWrapperComponent],
providers: [
{ provide: XSRFService, useValue: {} },
{ provide: BitstreamDataService, useValue: bitstreamDataService },
{ provide: NotificationsService, useValue: new NotificationsServiceStub() },
{ provide: APP_CONFIG, useValue: environment }
Expand Down
Loading

0 comments on commit 28bd6a5

Please sign in to comment.