Skip to content

Commit

Permalink
fix(stark-core): refactor Http Request builder, Http service and Sess…
Browse files Browse the repository at this point in the history 
…ion service to prevent undefined and null header values to be added to Angular Http headers.

ISSUES CLOSED: #856
  • Loading branch information
@christophercr
christophercr committed Nov 23, 2018
1 parent b84f8e5 commit c728f9c
Show file tree
Hide file tree
Showing 9 changed files with 153 additions and 69 deletions.
4 changes: 2 additions & 2 deletions packages/stark-core/src/modules/http/builder/http-abstract-base-request-builder.intf.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ export interface StarkHttpBaseRequestBuilder<T extends StarkResource> {
/**
* Adds a header to the request
* @param name - Header name
* @param value - Header value
* @param value - Header value. In case of multiple values, they should be provided in an array.
* @returns The current builder
*/
setHeader(name: string, value: string): this;
setHeader(name: string, value: string | string[]): this;

/**
* Adds a query parameter to the request (if the parameter already exists it will be overwritten)
Expand Down
5 changes: 3 additions & 2 deletions packages/stark-core/src/modules/http/builder/http-abstract-base-request-builder.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@ export abstract class StarkAbstractHttpBaseRequestBuilder<T extends StarkResourc
this.request = request;
}

public setHeader(name: string, value: string): this {
if (typeof value !== "undefined") {
public setHeader(name: string, value: string | string[]): this {
// in Angular, a header value can only be string or string[], not null/undefined (https://github.com/angular/angular/issues/18743)
if (name && typeof value !== "undefined" && value !== null) {
this.request.headers.set(name, value);
}
return this;
Expand Down
34 changes: 22 additions & 12 deletions packages/stark-core/src/modules/http/builder/http-request-builder.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,16 +55,16 @@ interface StarkHttpRequestBuilderSpecVariables<RequestBuilderType = StarkHttpBas
requestBuilder: RequestBuilderType;
}

function assertHeaders(requestHeaders: Map<string, string>, expectedHeaders: Map<string, string | undefined>): void {
function assertHeaders(requestHeaders: Map<string, string | string[]>, expectedHeaders: Map<string, string | string[] | undefined>): void {
expect(requestHeaders).toBeDefined();
expect(requestHeaders.size).toBe(expectedHeaders.size);

expectedHeaders.forEach((value?: string, header?: string) => {
expectedHeaders.forEach((value?: string | string[], header?: string) => {
expect(header).toBeDefined();
expect((<string>header).length).not.toBe(0);
expect(requestHeaders.has(<string>header)).toBe(true);
if (typeof value !== "undefined") {
expect(requestHeaders.get(<string>header)).toBe(value);
expect(requestHeaders.get(<string>header)).toEqual(value);
} else {
expect(requestHeaders.get(<string>header)).toBeUndefined();
}
Expand Down Expand Up @@ -123,9 +123,14 @@ function testSetHeader(beforeEachFn: () => StarkHttpRequestBuilderSpecVariables)
({ requestBuilder: builder } = beforeEachFn());
});

it("should add the header name/value only if the value is defined", () => {
it("should add the header name/value only if the name and value are defined and not null", () => {
builder.setHeader(StarkHttpHeaders.CONTENT_TYPE, contentTypeJSON);
builder.setHeader("invalidHeader", <any>undefined);
// tslint:disable-next-line:no-null-keyword
builder.setHeader("anotherInvalidHeader", <any>null);
builder.setHeader(<any>undefined, "dummy value");
// tslint:disable-next-line:no-null-keyword
builder.setHeader(<any>null, "another dummy value");

const request: StarkHttpRequest = builder.build();

Expand All @@ -134,17 +139,22 @@ function testSetHeader(beforeEachFn: () => StarkHttpRequestBuilderSpecVariables)

it("should add the header name/value and add it to existing ones", () => {
builder.setHeader(StarkHttpHeaders.CONTENT_TYPE, contentTypeJSON);
builder.setHeader(StarkHttpHeaders.ACCEPT_LANGUAGE, StarkLanguages.EN_US.isoCode);
builder.setHeader(StarkHttpHeaders.ACCEPT_LANGUAGE, [
StarkLanguages.EN_US.isoCode,
StarkLanguages.FR_BE.isoCode,
StarkLanguages.NL_BE.isoCode
]);

const request: StarkHttpRequest = builder.build();
const expectedHeaders: Map<string, string | string[]> = new Map<string, string | string[]>();
expectedHeaders.set(StarkHttpHeaders.CONTENT_TYPE, contentTypeJSON);
expectedHeaders.set(StarkHttpHeaders.ACCEPT_LANGUAGE, [
StarkLanguages.EN_US.isoCode,
StarkLanguages.FR_BE.isoCode,
StarkLanguages.NL_BE.isoCode
]);

assertHeaders(
request.headers,
new Map([
[StarkHttpHeaders.CONTENT_TYPE, contentTypeJSON],
[StarkHttpHeaders.ACCEPT_LANGUAGE, StarkLanguages.EN_US.isoCode]
])
);
assertHeaders(request.headers, expectedHeaders);
});
});
}
Expand Down
4 changes: 2 additions & 2 deletions packages/stark-core/src/modules/http/entities/http-request.entity.intf.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ export interface StarkHttpRequest<P extends StarkResource = StarkResource> {
*/
fieldsToInclude?: string[];
/**
* Map containing the headers to be sent with the request.
* Map containing the headers to be sent with the request. Multiple values for a header are supported.
*/
headers: Map<string, string>;
headers: Map<string, string | string[]>;
/**
* Map containing the parameters that will be included as query parameters.
* The query parameters might be undefined values in case the allowUndefinedQueryParams option is enabled and passed to the corresponding builder.
Expand Down
59 changes: 29 additions & 30 deletions packages/stark-core/src/modules/http/services/http.service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -501,11 +501,7 @@ describe("Service: StarkHttpService", () => {
let httpErrorResponse: Partial<HttpErrorResponse>;

beforeEach(() => {
({
starkHttpService: httpService,
httpMock: httpClientMock,
httpRequest: request
} = beforeEachFn());
({ starkHttpService: httpService, httpMock: httpClientMock, httpRequest: request } = beforeEachFn());

httpErrorResponse = {
status: StarkHttpStatusCodes.HTTP_400_BAD_REQUEST,
Expand Down Expand Up @@ -657,11 +653,7 @@ describe("Service: StarkHttpService", () => {
let httpService: HttpServiceHelper<MockResource>;

beforeEach(() => {
({
starkHttpService: httpService,
httpMock: httpClientMock,
httpRequest: request
} = beforeEachFn());
({ starkHttpService: httpService, httpMock: httpClientMock, httpRequest: request } = beforeEachFn());
});

it(
Expand Down Expand Up @@ -1100,11 +1092,7 @@ describe("Service: StarkHttpService", () => {
let httpResponse: Partial<HttpResponse<StarkResource>>;

beforeEach(() => {
({
starkHttpService: httpService,
httpMock: httpClientMock,
httpRequest: request
} = beforeEachFn());
({ starkHttpService: httpService, httpMock: httpClientMock, httpRequest: request } = beforeEachFn());

httpResponse = {
status: StarkHttpStatusCodes.HTTP_204_NO_CONTENT,
Expand Down Expand Up @@ -1444,7 +1432,7 @@ describe("Service: StarkHttpService", () => {
};
request.headers.set(dummyHeaderName, dummyHeaderValue);

const devAuthenticationHeaders: Map<string, string> = mockSessionService.devAuthenticationHeaders;
const devAuthenticationHeaders: Map<string, string | string[]> = mockSessionService.devAuthenticationHeaders;
expect(devAuthenticationHeaders.size).toBe(mockDevAuthHeaders.size);

const requestWithDevAuthHeaders: StarkHttpRequest = starkHttpService.addDevAuthenticationHeaders(request);
Expand All @@ -1453,28 +1441,28 @@ describe("Service: StarkHttpService", () => {
expect(requestWithDevAuthHeaders.headers.has(dummyHeaderName)).toBe(true);
expect(requestWithDevAuthHeaders.headers.get(dummyHeaderName)).toBe(dummyHeaderValue);

devAuthenticationHeaders.forEach((headerValue: string, header: string) => {
devAuthenticationHeaders.forEach((headerValue: string | string[], header: string) => {
expect(requestWithDevAuthHeaders.headers.has(header)).toBe(true);
expect(requestWithDevAuthHeaders.headers.get(header)).toBe(headerValue);
});
});
});

describe("addCorrelationIdentifierHeader", () => {
it("should get the correlationId from the Logging service and add it as a header to the current request headers", () => {
const dummyHeaderName: string = "some header";
const dummyHeaderValue: string = "some value";
const request: StarkHttpRequest<MockResource> = {
backend: mockBackend,
resourcePath: mockResourcePath,
headers: new Map<string, string>(),
queryParameters: new Map<string, string>(),
requestType: StarkHttpRequestType.DELETE,
item: mockResourceWithEtag,
serializer: mockResourceSerializer
};
request.headers.set(dummyHeaderName, dummyHeaderValue);
const dummyHeaderName: string = "some header";
const dummyHeaderValue: string = "some value";
const request: StarkHttpRequest<MockResource> = {
backend: mockBackend,
resourcePath: mockResourcePath,
headers: new Map<string, string>(),
queryParameters: new Map<string, string>(),
requestType: StarkHttpRequestType.DELETE,
item: mockResourceWithEtag,
serializer: mockResourceSerializer
};
request.headers.set(dummyHeaderName, dummyHeaderValue);

it("should get the correlationId from the Logging service and add it as a header to the current request headers", () => {
const correlationId: string = loggerMock.correlationId;
expect(correlationId).toBe(mockCorrelationId);

Expand All @@ -1486,6 +1474,17 @@ describe("Service: StarkHttpService", () => {
expect(requestWithCorrelationIdHeader.headers.has(loggerMock.correlationIdHttpHeaderName)).toBe(true);
expect(requestWithCorrelationIdHeader.headers.get(loggerMock.correlationIdHttpHeaderName)).toBe(correlationId);
});

it("should NOT add the correlationId header to the current request headers if the correlationId is not defined", () => {
(<any>loggerMock)["correlationId"] = undefined;

const requestWithCorrelationIdHeader: StarkHttpRequest<MockResource> = starkHttpService.addCorrelationIdentifierHeader(request);

expect(requestWithCorrelationIdHeader.headers.size).toBe(1); // just the custom header
expect(requestWithCorrelationIdHeader.headers.has(dummyHeaderName)).toBe(true);
expect(requestWithCorrelationIdHeader.headers.get(dummyHeaderName)).toBe(dummyHeaderValue);
expect(requestWithCorrelationIdHeader.headers.has(loggerMock.correlationIdHttpHeaderName)).toBe(false);
});
});
});

Expand Down
4 changes: 2 additions & 2 deletions packages/stark-core/src/modules/http/services/http.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ export class StarkHttpServiceImpl<P extends StarkResource> implements StarkHttpS
const requestCopy: StarkHttpRequest<P> = _cloneDeep(request);

// add the preAuthentication headers to the request headers
this.sessionService.devAuthenticationHeaders.forEach((value: string, header: string) => {
this.sessionService.devAuthenticationHeaders.forEach((value: string | string[], header: string) => {
requestCopy.headers.set(header, value);
});

Expand All @@ -173,7 +173,7 @@ export class StarkHttpServiceImpl<P extends StarkResource> implements StarkHttpS
* @returns The modified request object
*/
public addCorrelationIdentifierHeader(request: StarkHttpRequest<P>): StarkHttpRequest<P> {
if (this.logger.correlationIdHttpHeaderName && this.logger.correlationIdHttpHeaderName.length > 0) {
if (this.logger.correlationIdHttpHeaderName && this.logger.correlationIdHttpHeaderName.length > 0 && this.logger.correlationId) {
this.logger.debug(starkHttpServiceName + ": Adding correlation identifier header");
const requestCopy: StarkHttpRequest<P> = _cloneDeep(request);
requestCopy.headers.set(this.logger.correlationIdHttpHeaderName, this.logger.correlationId);
Expand Down
4 changes: 2 additions & 2 deletions packages/stark-core/src/modules/session/services/session.service.intf.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export interface StarkSessionService {
* Authentication headers necessary for non-production environments
* @returns A Map containing the development authentication headers
*/
readonly devAuthenticationHeaders: Map<string, string>;
readonly devAuthenticationHeaders: Map<string, string | string[]>;

/**
* Returns the session's current user
Expand Down Expand Up @@ -74,5 +74,5 @@ export interface StarkSessionService {
* Add authentication headers to the session
* They are use by the http service to authenticate the user
*/
setDevAuthenticationHeaders(devAuthenticationHeaders: Map<string, string>): void;
setDevAuthenticationHeaders(devAuthenticationHeaders: Map<string, string | string[]>): void;
}
87 changes: 76 additions & 11 deletions packages/stark-core/src/modules/session/services/session.service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -674,7 +674,6 @@ describe("Service: StarkSessionService", () => {
mockKeepaliveService.request.calls.reset();

const expectedDevAuthHeaders: Map<string, string> = new Map<string, string>();
expectedDevAuthHeaders.set(mockCorrelationIdHeaderName, mockCorrelationId);
expectedDevAuthHeaders.set("usernameTestHeader", mockUser.username);
expectedDevAuthHeaders.set("firstnameTestHeader", mockUser.firstName);
expectedDevAuthHeaders.set("lastnameTestHeader", mockUser.lastName);
Expand All @@ -687,6 +686,45 @@ describe("Service: StarkSessionService", () => {
expect(mockKeepaliveService.interval).toHaveBeenCalledWith(appConfig.keepAliveInterval);
expect(mockKeepaliveService.request).toHaveBeenCalledTimes(1);

let mockHeadersObj: HttpHeaders = new HttpHeaders();
expectedDevAuthHeaders.forEach((value: string, key: string) => (mockHeadersObj = mockHeadersObj.set(key, value)));
mockHeadersObj = mockHeadersObj.set(mockCorrelationIdHeaderName, mockCorrelationId);

expect(mockKeepaliveService.request).toHaveBeenCalledWith(
new HttpRequest<void>("GET", <string>appConfig.keepAliveUrl, { headers: mockHeadersObj })
);
});

it("should NOT set the correlationId headers to the keepalive Http request if the correlation id is undefined", () => {
const sessionServiceHelper: SessionServiceHelper = new SessionServiceHelper(
mockStore,
mockLogger,
mockRoutingService,
appConfig,
mockIdleService,
mockInjectorService,
mockTranslateService,
mockSessionConfig
);

expect(sessionServiceHelper.keepalive).toBeDefined();
mockKeepaliveService.interval.calls.reset();
mockKeepaliveService.request.calls.reset();

const expectedDevAuthHeaders: Map<string, string> = new Map<string, string>();
expectedDevAuthHeaders.set("usernameTestHeader", mockUser.username);
expectedDevAuthHeaders.set("firstnameTestHeader", mockUser.firstName);
expectedDevAuthHeaders.set("lastnameTestHeader", mockUser.lastName);
expectedDevAuthHeaders.set("emailTestHeader", <string>mockUser.email);

(<any>mockLogger)["correlationId"] = undefined;
sessionServiceHelper.setDevAuthenticationHeaders(expectedDevAuthHeaders);
sessionServiceHelper.configureKeepaliveService();

expect(mockKeepaliveService.interval).toHaveBeenCalledTimes(1);
expect(mockKeepaliveService.interval).toHaveBeenCalledWith(appConfig.keepAliveInterval);
expect(mockKeepaliveService.request).toHaveBeenCalledTimes(1);

let mockHeadersObj: HttpHeaders = new HttpHeaders();
expectedDevAuthHeaders.forEach((value: string, key: string) => (mockHeadersObj = mockHeadersObj.set(key, value)));

Expand Down Expand Up @@ -882,41 +920,68 @@ describe("Service: StarkSessionService", () => {
});

describe("setDevAuthenticationHeaders", () => {
it("should construct the authentication headers based on the http headers that are passed", () => {
it("should construct the dev authentication headers based on the http headers that are passed", () => {
expect(sessionService["_devAuthenticationHeaders"]).toBe(<any>undefined);

const expectedDevAuthHeaders: Map<string, string> = new Map<string, string>();
expectedDevAuthHeaders.set(mockCorrelationIdHeaderName, mockCorrelationId);
expectedDevAuthHeaders.set("usernameTestHeader", mockUser.username);
expectedDevAuthHeaders.set("firstnameTestHeader", mockUser.firstName);
expectedDevAuthHeaders.set("lastnameTestHeader", mockUser.lastName);
expectedDevAuthHeaders.set("emailTestHeader", <string>mockUser.email);

sessionService.setDevAuthenticationHeaders(expectedDevAuthHeaders);
expect(sessionService["_devAuthenticationHeaders"]).toBeDefined();
expect(sessionService.devAuthenticationHeaders.size).toBe(5);
expect(sessionService.devAuthenticationHeaders.size).toBe(4);

expectedDevAuthHeaders.forEach((_value: string, key: string) => {
expectedDevAuthHeaders.forEach((value: string, key: string) => {
expect(sessionService.devAuthenticationHeaders.has(key)).toBe(true);
expect(sessionService.devAuthenticationHeaders.get(key)).toBe(_value);
expect(sessionService.devAuthenticationHeaders.get(key)).toBe(value);
});
});

it("should construct the dev authentication headers excluding those http headers with undefined or null names or values", () => {
expect(sessionService["_devAuthenticationHeaders"]).toBe(<any>undefined);

const expectedDevAuthHeaders: Map<string, string> = new Map<string, string>();
expectedDevAuthHeaders.set("usernameTestHeader", mockUser.username);
expectedDevAuthHeaders.set("firstnameTestHeader", mockUser.firstName);
expectedDevAuthHeaders.set("lastnameTestHeader", <any>undefined);
// tslint:disable-next-line:no-null-keyword
expectedDevAuthHeaders.set("emailTestHeader", <any>null);
expectedDevAuthHeaders.set(<any>undefined, "dummy value");
// tslint:disable-next-line:no-null-keyword
expectedDevAuthHeaders.set(<any>null, "another dummy value");

sessionService.setDevAuthenticationHeaders(expectedDevAuthHeaders);
expect(sessionService["_devAuthenticationHeaders"]).toBeDefined();
expect(sessionService.devAuthenticationHeaders.size).toBe(2);

expectedDevAuthHeaders.forEach((value: string, key: string) => {
if (key === "usernameTestHeader" || key === "firstnameTestHeader") {
expect(sessionService.devAuthenticationHeaders.has(key)).toBe(true);
expect(sessionService.devAuthenticationHeaders.get(key)).toBe(value);
} else {
expect(sessionService.devAuthenticationHeaders.has(key)).toBe(false);
}
});
});
});

describe("devAuthenticationHeaders", () => {
it("should return the pre-authentication headers if they were constructed", () => {
const expectedDevAuthHeaders: Map<string, string> = new Map<string, string>();
const expectedDevAuthHeaders: Map<string, string | string[]> = new Map<string, string | string[]>();
expectedDevAuthHeaders.set("usernameTestHeader", "doej");
expectedDevAuthHeaders.set("firstnameTestHeader", "john");
expectedDevAuthHeaders.set("lastTestHeader", "doe");
expectedDevAuthHeaders.set("emailTestHeader", ["jdoe@mail.com", "john.d@email.com"]);

sessionService.setInternalDevAuthenticationHeaders(expectedDevAuthHeaders);

const devAuthenticationHeaders: Map<string, string> = sessionService.devAuthenticationHeaders;
const devAuthenticationHeaders: Map<string, string | string[]> = sessionService.devAuthenticationHeaders;

expect(devAuthenticationHeaders.size).toBe(expectedDevAuthHeaders.size);

expectedDevAuthHeaders.forEach((value: string, header: string) => {
expectedDevAuthHeaders.forEach((value: string | string[], header: string) => {
expect(expectedDevAuthHeaders.has(header)).toBe(true);
expect(expectedDevAuthHeaders.get(header)).toBe(value);
});
Expand All @@ -926,7 +991,7 @@ describe("Service: StarkSessionService", () => {
/* tslint:disable-next-line:no-undefined-argument */
sessionService.setInternalDevAuthenticationHeaders(undefined);

const devAuthenticationHeaders: Map<string, string> = sessionService.devAuthenticationHeaders;
const devAuthenticationHeaders: Map<string, string | string[]> = sessionService.devAuthenticationHeaders;

expect(devAuthenticationHeaders.size).toBe(0);
});
Expand Down Expand Up @@ -991,7 +1056,7 @@ class SessionServiceHelper extends StarkSessionServiceImpl {
return of(undefined);
}

public setInternalDevAuthenticationHeaders(headers?: Map<string, string>): void {
public setInternalDevAuthenticationHeaders(headers?: Map<string, string | string[]>): void {
this._devAuthenticationHeaders = <any>headers;
}
}
Loading

0 comments on commit c728f9c

Please sign in to comment.