diff --git a/src/app/services/baw-api/api.interceptor.service.spec.ts b/src/app/services/baw-api/api.interceptor.service.spec.ts index c3ed2c11f..e45e7d8a5 100644 --- a/src/app/services/baw-api/api.interceptor.service.spec.ts +++ b/src/app/services/baw-api/api.interceptor.service.spec.ts @@ -1,276 +1,343 @@ -// /* eslint-disable @typescript-eslint/naming-convention */ -// import { HttpClient, HttpClientModule, HttpParams } from "@angular/common/http"; -// import { TestRequest } from "@angular/common/http/testing"; -// import { ApiErrorDetails } from "@baw-api/api.interceptor.service"; -// import { MockBawApiModule } from "@baw-api/baw-apiMock.module"; -// import { SecurityService } from "@baw-api/security/security.service"; -// import { API_ROOT } from "@helpers/app-initializer/app-initializer"; -// import { Session } from "@models/User"; -// import { -// createHttpFactory, -// HttpMethod, -// SpectatorHttp, -// } from "@ngneat/spectator"; -// import { generateApiErrorResponse } from "@test/fakes/ApiErrorDetails"; -// import { generateSessionUser } from "@test/fakes/User"; -// import { noop } from "rxjs"; -// import { ApiResponse } from "./baw-api.service"; -// import { shouldNotFail, shouldNotSucceed } from "./baw-api.service.spec"; - -// describe("BawApiInterceptor", () => { -// let apiRoot: string; -// let http: HttpClient; -// let spec: SpectatorHttp; -// const createService = createHttpFactory({ -// service: SecurityService, -// imports: [HttpClientModule, MockBawApiModule], -// }); - -// function getPath(path: string) { -// return apiRoot + path; -// } - -// function setLoggedOut() { -// spyOn(spec.service, "isLoggedIn").and.callFake(() => false); -// spyOn(spec.service, "getLocalUser").and.callFake(() => undefined); -// } - -// function setLoggedIn(authToken?: string) { -// spyOn(spec.service, "isLoggedIn").and.callFake(() => true); -// spyOn(spec.service, "getLocalUser").and.callFake(() => -// authToken -// ? new Session(generateSessionUser({ authToken })) -// : new Session(generateSessionUser()) -// ); -// } - -// beforeEach(() => { -// spec = createService(); -// http = spec.inject(HttpClient); -// apiRoot = spec.inject(API_ROOT); -// }); - -// describe("error handling", () => { -// function convertErrorResponseToDetails( -// response: ApiResponse -// ): ApiErrorDetails { -// return { -// status: response.meta.status, -// message: response.meta.error.details, -// info: response.meta.error.info, -// }; -// } - -// beforeEach(() => setLoggedOut()); - -// it("should handle api error response", () => { -// const error = generateApiErrorResponse("Unauthorized", { -// message: "Incorrect user name", -// }); - -// http -// .get(getPath("/brokenapiroute")) -// .subscribe(shouldNotSucceed, (err) => { -// expect(err).toEqual(convertErrorResponseToDetails(error)); -// }); - -// spec.expectOne(getPath("/brokenapiroute"), HttpMethod.GET).flush(error, { -// status: error.meta.status, -// statusText: error.meta.message, -// }); -// }); - -// it("should handle api error response with info", () => { -// const error = generateApiErrorResponse("Unprocessable Entity", { -// info: { name: ["has already been taken"] }, -// }); - -// http -// .get(getPath("/brokenapiroute")) -// .subscribe(shouldNotSucceed, (err) => { -// expect(err).toEqual(convertErrorResponseToDetails(error)); -// }); - -// spec.expectOne(getPath("/brokenapiroute"), HttpMethod.GET).flush(error, { -// status: error.meta.status, -// statusText: error.meta.message, -// }); -// }); - -// it("should handle http error response", () => { -// http -// .get(getPath("/brokenapiroute")) -// .subscribe(shouldNotSucceed, (err) => { -// expect(err).toEqual({ -// status: 404, -// message: `Http failure response for ${apiRoot}/brokenapiroute: 404 Page Not Found`, -// }); -// }); - -// spec -// .expectOne(getPath("/brokenapiroute"), HttpMethod.GET) -// .flush({}, { status: 404, statusText: "Page Not Found" }); -// }); -// }); - -// describe("non baw api traffic", () => { -// describe("outgoing data", () => { -// function makeRequest() { -// http.get("https://brokenlink").subscribe(noop, noop); -// return spec.expectOne("https://brokenlink", HttpMethod.GET); -// } - -// it("should not set Authorization header when not logged in", () => { -// setLoggedOut(); -// const req = makeRequest(); -// expect(req.request.headers.has("Authorization")).toBeFalsy(); -// }); - -// it("should not set Authorization header when logged in", () => { -// setLoggedIn(); -// const req = makeRequest(); -// expect(req.request.headers.has("Authorization")).toBeFalsy(); -// }); - -// it("should not convert into snake case for GET requests", () => { -// setLoggedOut(); -// http -// .get("https://brokenlink/brokenapiroute", { -// params: new HttpParams().set("shouldNotConvert", "true"), -// }) -// .subscribe(noop, noop); -// expect( -// spec.expectOne( -// "https://brokenlink/brokenapiroute?shouldNotConvert=true", -// HttpMethod.GET -// ) -// ).toBeInstanceOf(TestRequest); -// }); - -// it("should not convert into snake case for POST requests", () => { -// setLoggedOut(); -// http -// .post("https://brokenlink/brokenapiroute", { -// shouldNotConvert: true, -// }) -// .subscribe(noop, noop); - -// const req = spec.expectOne( -// "https://brokenlink/brokenapiroute", -// HttpMethod.POST -// ); -// expect(req.request.body).toEqual({ shouldNotConvert: true }); -// }); -// }); - -// describe("incoming data", () => { -// beforeEach(() => setLoggedOut()); - -// it("should not convert into camel case for GET requests", () => { -// http -// .get("https://brokenlink/brokenapiroute") -// .subscribe( -// (response) => expect(response).toEqual({ dummy_response: true }), -// shouldNotFail -// ); -// spec -// .expectOne("https://brokenlink/brokenapiroute", HttpMethod.GET) -// .flush({ dummy_response: true }); -// }); - -// it("should not convert into camel case for POST requests", () => { -// http -// .post("https://brokenlink/brokenapiroute", {}) -// .subscribe( -// (response) => expect(response).toEqual({ dummy_response: true }), -// shouldNotFail -// ); -// spec -// .expectOne("https://brokenlink/brokenapiroute", HttpMethod.POST) -// .flush({ dummy_response: true }); -// }); -// }); -// }); - -// describe("baw api traffic", () => { -// describe("outgoing data", () => { -// it("should set cookies", () => { -// setLoggedOut(); -// http.get(getPath("/brokenapiroute")).subscribe(noop, noop); -// const req = spec.expectOne(getPath("/brokenapiroute"), HttpMethod.GET); -// expect(req.request.withCredentials).toBeTrue(); -// }); - -// it("should convert into snake case for GET requests", () => { -// setLoggedOut(); -// http -// .get(getPath("/brokenapiroute"), { -// params: new HttpParams().set("shouldConvert", "true"), -// }) -// .subscribe(noop, noop); - -// expect( -// spec.expectOne( -// getPath("/brokenapiroute?should_convert=true"), -// HttpMethod.GET -// ) -// ).toBeInstanceOf(TestRequest); -// }); - -// it("should convert into snake case for POST requests", () => { -// setLoggedOut(); -// http -// .post(getPath("/brokenapiroute"), { shouldConvert: true }) -// .subscribe(noop, noop); -// const req = spec.expectOne(getPath("/brokenapiroute"), HttpMethod.POST); -// expect(req.request.body).toEqual({ should_convert: true }); -// }); - -// it("should not attach Authorization when unauthenticated", () => { -// setLoggedOut(); -// http.get(getPath("/brokenapiroute")).subscribe(noop, noop); -// const req = spec.expectOne(getPath("/brokenapiroute"), HttpMethod.GET); -// expect(req.request.headers.has("Authorization")).toBeFalsy(); -// }); - -// it("should attach Authorization when authenticated", () => { -// setLoggedIn("xxxxxxxxxxxxxxxxxxxx"); -// http.get(getPath("/brokenapiroute")).subscribe(noop, noop); -// const req = spec.expectOne(getPath("/brokenapiroute"), HttpMethod.GET); -// expect(req.request.headers.get("Authorization")).toBe( -// 'Token token="xxxxxxxxxxxxxxxxxxxx"' -// ); -// }); -// }); - -// describe("incoming data", () => { -// beforeEach(() => setLoggedOut()); - -// it("should convert into camel case for GET requests", () => { -// http -// .get(getPath("/brokenapiroute")) -// .subscribe( -// (response) => expect(response).toEqual({ dummyResponse: true }), -// shouldNotFail -// ); - -// spec -// .expectOne(getPath("/brokenapiroute"), HttpMethod.GET) -// .flush({ dummy_response: true }); -// }); - -// it("should convert incoming data from baw api into camel case for POST requests", () => { -// http -// .post(getPath("/brokenapiroute"), {}) -// .subscribe( -// (response) => expect(response).toEqual({ dummyResponse: true }), -// shouldNotFail -// ); - -// spec -// .expectOne(getPath("/brokenapiroute"), HttpMethod.POST) -// .flush({ dummy_response: true }); -// }); -// }); -// }); -// }); +import { + HttpClient, + HttpClientModule, + HttpContext, + HttpParams, +} from "@angular/common/http"; +import { TestRequest } from "@angular/common/http/testing"; +import { MockBawApiModule } from "@baw-api/baw-apiMock.module"; +import { User } from "@models/User"; +import { + createHttpFactory, + HttpMethod, + SpectatorHttp, +} from "@ngneat/spectator"; +import { noop } from "rxjs"; +import { generateUser } from "@test/fakes/User"; +import { API_ROOT } from "@services/config/config.tokens"; +import { + ApiErrorDetails, + BawApiError, +} from "@helpers/custom-errors/baw-api-error"; +import { generateBawApiError } from "@test/fakes/BawApiError"; +import { NOT_FOUND, UNPROCESSABLE_ENTITY } from "http-status"; +import { BawSessionService } from "./baw-session.service"; +import { shouldNotFail, shouldNotSucceed } from "./baw-api.service.spec"; +import { CREDENTIALS_CONTEXT } from "./api.interceptor.service"; + +describe("BawApiInterceptor", () => { + let apiRoot: string; + let http: HttpClient; + let spec: SpectatorHttp; + const createService = createHttpFactory({ + service: BawSessionService, + imports: [HttpClientModule, MockBawApiModule], + }); + + function getPath(path: string) { + return apiRoot + path; + } + + function setLoggedOut() { + spyOnProperty(spec.service, "isLoggedIn", "get").and.returnValue(false); + spyOnProperty(spec.service, "loggedInUser", "get").and.returnValue( + undefined + ); + } + + function setLoggedIn(authToken?: string) { + spyOnProperty(spec.service, "isLoggedIn", "get").and.returnValue(true); + spyOnProperty(spec.service, "authToken", "get").and.returnValue(authToken); + spyOnProperty(spec.service, "loggedInUser", "get").and.returnValue( + new User(generateUser()) + ); + } + + beforeEach(() => { + spec = createService(); + http = spec.inject(HttpClient); + apiRoot = spec.inject(API_ROOT); + }); + + describe("error handling", () => { + function convertErrorResponseToDetails( + response: BawApiError + ): ApiErrorDetails { + return { + status: response.status, + message: response.message, + info: response.info as any, + }; + } + + beforeEach(() => setLoggedOut()); + + it("should handle api error response", () => { + const error = generateBawApiError( + NOT_FOUND, + "The following action does not exist, if you believe this is an error please report a problem." + ); + + http.get(getPath("/brokenapiroute")).subscribe({ + next: shouldNotSucceed, + error: (err) => { + expect(err).toEqual(error); + }, + }); + + spec + .expectOne(getPath("/brokenapiroute"), HttpMethod.GET) + .flush({}, { status: 404, statusText: "" }); + }); + + xit("should handle api error response with info", () => { + const error = generateBawApiError(UNPROCESSABLE_ENTITY, "", { + name: ["has already been taken"], + }) as any; + + http.get(getPath("/brokenapiroute")).subscribe({ + next: shouldNotSucceed, + error: (err) => { + expect(err).toEqual(convertErrorResponseToDetails(error)); + }, + }); + + spec.expectOne(getPath("/brokenapiroute"), HttpMethod.GET).flush(error, { + status: error.meta.status, + statusText: error.meta.message, + }); + }); + + xit("should handle http error response", () => { + http.get(getPath("/brokenapiroute")).subscribe({ + next: shouldNotSucceed, + error: (err) => { + expect(err).toEqual({ + status: 404, + message: `Http failure response for ${apiRoot}/brokenapiroute: 404 Page Not Found`, + }); + }, + }); + + spec + .expectOne(getPath("/brokenapiroute"), HttpMethod.GET) + .flush({}, { status: 404, statusText: "Page Not Found" }); + }); + }); + + describe("non baw api traffic", () => { + describe("outgoing data", () => { + function makeRequest() { + http.get("https://brokenlink").subscribe(noop); + return spec.expectOne("https://brokenlink", HttpMethod.GET); + } + + it("should not set Authorization header when not logged in", () => { + setLoggedOut(); + const req = makeRequest(); + expect(req.request.headers.has("Authorization")).toBeFalsy(); + }); + + it("should not set Authorization header when logged in", () => { + setLoggedIn(); + const req = makeRequest(); + expect(req.request.headers.has("Authorization")).toBeFalsy(); + }); + + it("should not convert into snake case for GET requests", () => { + setLoggedOut(); + http + .get("https://brokenlink/brokenapiroute", { + params: new HttpParams().set("shouldNotConvert", "true"), + }) + .subscribe(noop); + expect( + spec.expectOne( + "https://brokenlink/brokenapiroute?shouldNotConvert=true", + HttpMethod.GET + ) + ).toBeInstanceOf(TestRequest); + }); + + it("should not convert into snake case for POST requests", () => { + setLoggedOut(); + http + .post("https://brokenlink/brokenapiroute", { + shouldNotConvert: true, + }) + .subscribe(noop); + + const req = spec.expectOne( + "https://brokenlink/brokenapiroute", + HttpMethod.POST + ); + expect(req.request.body).toEqual({ shouldNotConvert: true }); + }); + }); + + describe("incoming data", () => { + beforeEach(() => setLoggedOut()); + + it("should not convert into camel case for GET requests", () => { + http.get("https://brokenlink/brokenapiroute").subscribe({ + next: (response) => + expect(response).toEqual({ dummy_response: true }), + error: shouldNotFail, + }); + spec + .expectOne("https://brokenlink/brokenapiroute", HttpMethod.GET) + .flush({ dummy_response: true }); + }); + + it("should not convert into camel case for POST requests", () => { + http.post("https://brokenlink/brokenapiroute", {}).subscribe({ + next: (response) => + expect(response).toEqual({ dummy_response: true }), + error: shouldNotFail, + }); + spec + .expectOne("https://brokenlink/brokenapiroute", HttpMethod.POST) + .flush({ dummy_response: true }); + }); + }); + }); + + describe("baw api traffic", () => { + describe("outgoing data", () => { + it("should set cookies", () => { + setLoggedOut(); + http.get(getPath("/brokenapiroute")).subscribe(noop); + const req = spec.expectOne(getPath("/brokenapiroute"), HttpMethod.GET); + expect(req.request.withCredentials).toBeTrue(); + }); + + it("should convert into snake case for GET requests", () => { + setLoggedOut(); + http + .get(getPath("/brokenapiroute"), { + params: new HttpParams().set("shouldConvert", "true"), + }) + .subscribe(noop); + + expect( + spec.expectOne( + getPath("/brokenapiroute?should_convert=true"), + HttpMethod.GET + ) + ).toBeInstanceOf(TestRequest); + }); + + it("should convert into snake case for POST requests", () => { + setLoggedOut(); + http + .post(getPath("/brokenapiroute"), { shouldConvert: true }) + .subscribe(noop); + const req = spec.expectOne(getPath("/brokenapiroute"), HttpMethod.POST); + expect(req.request.body).toEqual({ should_convert: true }); + }); + + it("should not attach Authorization when unauthenticated", () => { + setLoggedOut(); + http.get(getPath("/brokenapiroute")).subscribe(noop); + const req = spec.expectOne(getPath("/brokenapiroute"), HttpMethod.GET); + expect(req.request.headers.has("Authorization")).toBeFalsy(); + }); + + it("should not attach Authorization when unauthenticated and the credentials context is explicitly set", () => { + const mockContext = new HttpContext().set(CREDENTIALS_CONTEXT, true); + setLoggedOut(); + + http + .get(getPath("/brokenapiroute"), { + context: mockContext, + }) + .subscribe(noop); + + const req = spec.expectOne(getPath("/brokenapiroute"), HttpMethod.GET); + + expect(req.request.headers.has("Authorization")).toBeFalsy(); + }); + + it("should attach Authorization when authenticated and the credentials context is not set", () => { + setLoggedIn("xxxxxxxxxxxxxxxxxxxx"); + http.get(getPath("/brokenapiroute")).subscribe(noop); + const req = spec.expectOne(getPath("/brokenapiroute"), HttpMethod.GET); + expect(req.request.headers.get("Authorization")).toBe( + 'Token token="xxxxxxxxxxxxxxxxxxxx"' + ); + }); + + it("should not attach Authorization when the credentials context has set to false", () => { + const mockContext = new HttpContext().set(CREDENTIALS_CONTEXT, false); + + setLoggedIn("xxxxxxxxxxxxxxxxxxxx"); + + http + .get(getPath("/brokenapiroute"), { + context: mockContext, + }) + .subscribe(noop); + + const req = spec.expectOne(getPath("/brokenapiroute"), HttpMethod.GET); + + expect(req.request.headers.has("Authorization")).toBeFalse(); + }); + + it("should attach Authorization when the credentials context is explicitly set to true", () => { + const mockContext = new HttpContext().set(CREDENTIALS_CONTEXT, true); + + setLoggedIn("xxxxxxxxxxxxxxxxxxxx"); + + http + .get(getPath("/brokenapiroute"), { + context: mockContext, + }) + .subscribe(noop); + + const req = spec.expectOne(getPath("/brokenapiroute"), HttpMethod.GET); + + expect(req.request.headers.get("Authorization")).toBe( + 'Token token="xxxxxxxxxxxxxxxxxxxx"' + ); + }); + + it("should default to attaching Authorization when the credentials context is not set", () => { + setLoggedIn("xxxxxxxxxxxxxxxxxxxx"); + + http.get(getPath("/brokenapiroute")).subscribe(noop); + + const req = spec.expectOne(getPath("/brokenapiroute"), HttpMethod.GET); + + expect(req.request.headers.get("Authorization")).toBe( + 'Token token="xxxxxxxxxxxxxxxxxxxx"' + ); + }); + }); + + describe("incoming data", () => { + beforeEach(() => setLoggedOut()); + + it("should convert into camel case for GET requests", () => { + http.get(getPath("/brokenapiroute")).subscribe({ + next: (response) => expect(response).toEqual({ dummyResponse: true }), + error: shouldNotFail, + }); + + spec + .expectOne(getPath("/brokenapiroute"), HttpMethod.GET) + .flush({ dummy_response: true }); + }); + + it("should convert incoming data from baw api into camel case for POST requests", () => { + http.post(getPath("/brokenapiroute"), {}).subscribe({ + next: (response) => expect(response).toEqual({ dummyResponse: true }), + error: shouldNotFail, + }); + + spec + .expectOne(getPath("/brokenapiroute"), HttpMethod.POST) + .flush({ dummy_response: true }); + }); + }); + }); +}); diff --git a/src/app/services/baw-api/api.interceptor.service.ts b/src/app/services/baw-api/api.interceptor.service.ts index 66485a4d4..800c688f2 100644 --- a/src/app/services/baw-api/api.interceptor.service.ts +++ b/src/app/services/baw-api/api.interceptor.service.ts @@ -1,4 +1,5 @@ import { + HttpContextToken, HttpErrorResponse, HttpEvent, HttpHandler, @@ -25,6 +26,10 @@ import { BawSessionService } from "./baw-session.service"; export const CLIENT_TIMEOUT = 0; +// the authentication context token can be used to hide/show the authentication in requests +// a true value will include the authentication header and cookies +export const CREDENTIALS_CONTEXT = new HttpContextToken(() => true); + /** * BAW API Interceptor. * This handles intercepting http requests to the BAW API server and manages @@ -53,8 +58,10 @@ export class BawApiInterceptor implements HttpInterceptor { return next.handle(request); } + const shouldSendCredentials = request.context.get(CREDENTIALS_CONTEXT); + // If logged in, add authorization token - if (this.session.isLoggedIn) { + if (this.session.isLoggedIn && shouldSendCredentials) { request = request.clone({ setHeaders: { // eslint-disable-next-line @typescript-eslint/naming-convention @@ -64,8 +71,9 @@ export class BawApiInterceptor implements HttpInterceptor { } // Convert outgoing data + // if withCredentials is false, the request will not include cookies (such as the _baw_session cookie) request = request.clone({ - withCredentials: true, + withCredentials: shouldSendCredentials, body: toSnakeCase(request.body), }); diff --git a/src/app/services/baw-api/baw-api.service.spec.ts b/src/app/services/baw-api/baw-api.service.spec.ts index b239a3812..fa791f631 100644 --- a/src/app/services/baw-api/baw-api.service.spec.ts +++ b/src/app/services/baw-api/baw-api.service.spec.ts @@ -1,10 +1,14 @@ import { HTTP_INTERCEPTORS } from "@angular/common/http"; import { TestRequest } from "@angular/common/http/testing"; import { Injector } from "@angular/core"; -import { BawApiInterceptor } from "@baw-api/api.interceptor.service"; +import { + CREDENTIALS_CONTEXT, + BawApiInterceptor, +} from "@baw-api/api.interceptor.service"; import { ApiResponse, BawApiService, + BawServiceOptions, defaultApiHeaders, Filters, Meta, @@ -277,7 +281,7 @@ describe("BawApiService", () => { httpMethods.forEach((httpMethod) => { describe(httpMethod.functionName, () => { function functionCall( - opts: (string | number)[] = [], + opts: unknown[] = [], next: (value: any) => void = noop, error: (err: any) => void = noop, complete: () => void = noop @@ -314,6 +318,74 @@ describe("BawApiService", () => { verifyAuthHeaders(catchFunctionCall(), defaultAuthToken); }); + it("should create a request with authentication headers when withCredentials is not set", () => { + signIn(defaultUser, defaultAuthToken); + + let functionCallOptions: unknown[]; + + // we have a special case for GET and GET because these function signatures don't take a body argument + // (the third argument in every other function) + if (httpMethod.method === HttpMethod.DELETE || httpMethod.method === HttpMethod.GET) { + functionCallOptions = [undefined, { withCredentials: false }]; + } else { + functionCallOptions = [ + undefined, + undefined, + { withCredentials: false }, + ]; + } + + functionCall(functionCallOptions); + + const request = catchFunctionCall(); + verifyHeaders(request); + expect(request.request.headers.get("Authorization")).toBeNull(); + }); + + it("should create a request with authentication headers when withCredentials is explicitly set to true", () => { + signIn(defaultUser, defaultAuthToken); + + let functionCallOptions: unknown[]; + + if (httpMethod.method === HttpMethod.DELETE || httpMethod.method === HttpMethod.GET) { + functionCallOptions = [undefined, { withCredentials: true }]; + } else { + functionCallOptions = [ + undefined, + undefined, + { withCredentials: true }, + ]; + } + + functionCall(functionCallOptions); + + const request = catchFunctionCall(); + verifyAuthHeaders(request, defaultAuthToken); + expect(request.request.headers.get("Authorization")).toBeDefined(); + }); + + it("should not create a request with authentication headers when withCredentials is set to false", () => { + signIn(defaultUser, defaultAuthToken); + + let functionCallOptions: unknown[]; + + if (httpMethod.method === HttpMethod.DELETE || httpMethod.method === HttpMethod.GET) { + functionCallOptions = [undefined, { withCredentials: false }]; + } else { + functionCallOptions = [ + undefined, + undefined, + { withCredentials: false }, + ]; + } + + functionCall(functionCallOptions); + + const request = catchFunctionCall(); + verifyHeaders(request); + expect(request.request.headers.get("Authorization")).toBeNull(); + }); + it("should return single response", (done) => { const response = { meta: meta.single, data: responses.single }; functionCall( @@ -423,34 +495,63 @@ describe("BawApiService", () => { }); it("should cache results when given", () => { + const cacheOptions = { cache: true }; + service - .httpGet("/broken_link", defaultApiHeaders, { cache: true }) + .httpGet("/broken_link", defaultApiHeaders, { cacheOptions }) .subscribe(); + const context = catchFunctionCall().request.context; - expect(context).toEqual(withCache({ cache: true, ...defaultCache })); + const expectedContext = withCache({ + cache: true, + ...defaultCache, + }).set(CREDENTIALS_CONTEXT, true); + + expect(context).toEqual(expectedContext); }); it("should override default cache settings", () => { const cacheOptions: ContextOptions = { cache: true, ttl: 10000 }; service - .httpGet("/broken_link", defaultApiHeaders, cacheOptions) + .httpGet("/broken_link", defaultApiHeaders, { cacheOptions }) .subscribe(); const context = catchFunctionCall().request.context; - expect(context).toEqual( - withCache({ ...defaultCache, ...cacheOptions }) - ); + const expectedContext = withCache({ + ...defaultCache, + ...cacheOptions, + }).set(CREDENTIALS_CONTEXT, true); + + expect(context).toEqual(expectedContext); }); it("should not cache results when not given", () => { service.httpGet("/broken_link").subscribe(); const context = catchFunctionCall().request.context; - expect(context).toEqual( - withCache({ - cache: false, - ttl: cacheSettings.httpGetTtlMs, - context: withCacheLogging(), - }) - ); + const expectedContext = withCache({ + cache: false, + ttl: cacheSettings.httpGetTtlMs, + context: withCacheLogging(), + }).set(CREDENTIALS_CONTEXT, true); + + expect(context).toEqual(expectedContext); + }); + + it("should allow settings both cache and authentication contexts to non-default values", () => { + const cacheOptions: ContextOptions = { cache: true, ttl: 10000 }; + const options: BawServiceOptions = { + cacheOptions, + withCredentials: false, + }; + + service.httpGet("/broken_link", defaultApiHeaders, options).subscribe(); + + const context = catchFunctionCall().request.context; + const expectedContext = withCache({ + ...defaultCache, + ...cacheOptions, + }).set(CREDENTIALS_CONTEXT, false); + + expect(context).toEqual(expectedContext); }); }); }); @@ -504,10 +605,17 @@ describe("BawApiService", () => { describe(method, () => { let defaultBody: IMockModel; let defaultFilter: Filters; + // the default options for the baw service methods eg. show, create, list, filterShow, filter, etc... + // because the default options are applied in lower level methods eg. httpGet, httpPost, etc... + // we expect that low level methods are called by high level methods with an empty options object as their default + // (indicating no options were passed in by the programmer) + // if options are passed in by the programmer, the options object will be a partial options object + let defaultBawMethodOptions: BawServiceOptions; beforeEach(() => { defaultBody = { id: 1, name: "test", caseConversion: {} }; defaultFilter = { paging: { page: 1 } }; + defaultBawMethodOptions = { }; }); function errorRequest(error: BawApiError): jasmine.Spy { @@ -574,21 +682,35 @@ describe("BawApiService", () => { expect(spy).toHaveBeenCalledWith( "/broken_link", defaultApiHeaders, - { cache: true } + defaultBawMethodOptions ); break; case "filter": - expect(spy).toHaveBeenCalledWith("/broken_link", defaultFilter); + expect(spy).toHaveBeenCalledWith( + "/broken_link", + defaultFilter, + undefined, + defaultBawMethodOptions + ); break; case "create": case "update": - expect(spy).toHaveBeenCalledWith("/broken_link", { - // eslint-disable-next-line @typescript-eslint/naming-convention - "Mock Model": defaultBody, - }); + expect(spy).toHaveBeenCalledWith( + "/broken_link", + { + // eslint-disable-next-line @typescript-eslint/naming-convention + "Mock Model": defaultBody, + }, + undefined, + defaultBawMethodOptions + ); break; case "destroy": - expect(spy).toHaveBeenCalledWith("/broken_link"); + expect(spy).toHaveBeenCalledWith( + "/broken_link", + undefined, + defaultBawMethodOptions + ); break; } }); diff --git a/src/app/services/baw-api/baw-api.service.ts b/src/app/services/baw-api/baw-api.service.ts index ae500508c..8d62ac8df 100644 --- a/src/app/services/baw-api/baw-api.service.ts +++ b/src/app/services/baw-api/baw-api.service.ts @@ -1,4 +1,4 @@ -import { HttpClient, HttpHeaders } from "@angular/common/http"; +import { HttpClient, HttpContext, HttpHeaders } from "@angular/common/http"; import { Inject, Injectable, Injector } from "@angular/core"; import { KeysOfType, Writeable, XOR } from "@helpers/advancedTypes"; import { toSnakeCase } from "@helpers/case-converter/case-converter"; @@ -13,7 +13,6 @@ import { AbstractModelWithoutId, } from "@models/AbstractModel"; import { HttpCacheManager, withCache } from "@ngneat/cashew"; -import { ContextOptions } from "@ngneat/cashew/lib/cache-context"; import { withCacheLogging } from "@services/cache/cache-logging.service"; import { CacheSettings, CACHE_SETTINGS } from "@services/cache/cache-settings"; import { API_ROOT } from "@services/config/config.tokens"; @@ -21,17 +20,22 @@ import { ToastrService } from "ngx-toastr"; import { Observable, iif, of, throwError } from "rxjs"; import { catchError, concatMap, map, switchMap, tap } from "rxjs/operators"; import { IS_SERVER_PLATFORM } from "src/app/app.helper"; +import { ContextOptions } from "@ngneat/cashew/lib/cache-context"; import { BawSessionService } from "./baw-session.service"; +import { CREDENTIALS_CONTEXT } from "./api.interceptor.service"; export const defaultApiPageSize = 25; export const unknownErrorCode = -1; -interface NotificationsOpt { +export interface BawServiceOptions { + /** If set and a request fails, an error notification won't be shown to the user */ disableNotification?: boolean; -} -interface CacheOpt { - disableCache?: boolean; + /** If set, requests will include the users authentication token and cookies */ + withCredentials?: boolean; + + /** Allows you to modify the cashew cache options per request */ + cacheOptions?: ContextOptions; } /** Default headers for API requests */ @@ -48,6 +52,11 @@ export const multiPartApiHeaders = new HttpHeaders({ // eslint-disable-next-line @typescript-eslint/naming-convention Accept: "application/json", }); +export const defaultBawServiceOptions: Required = { + disableNotification: false, + withCredentials: true, + cacheOptions: { cache: false }, +}; /** * Interface with BAW Server Rest API @@ -101,6 +110,36 @@ export class BawApiService< this.manager.delete(path); }; + // because users can create a partial options object, we need to merge the partial options with the default options + // so that we don't have "undefined" values being passed as options + private buildServiceOptions( + options: Partial + ): Required { + return { + ...defaultBawServiceOptions, + ...options, + }; + } + + // the "context" headers are passed to the interceptor to determine if the request should be cached and if + // the Authentication token and cookies should be sent in requests + private credentialsHttpContext( + options: Required, + baseContext: HttpContext = new HttpContext() + ): HttpContext { + return baseContext.set(CREDENTIALS_CONTEXT, options.withCredentials); + } + + // because users can input a partial options object, it is possible for the disableNotification property to be undefined + // this can be a problem because it will default to a falsy value, not the default option + // therefore, this method should be used to check if we should show a toast notification if the request fails + private shouldNotify(options: Partial): boolean { + return ( + options?.disableNotification ?? + defaultBawServiceOptions.disableNotification + ); + } + public constructor( @Inject(API_ROOT) protected apiRoot: string, @Inject(IS_SERVER_PLATFORM) protected isServer: boolean, @@ -167,16 +206,12 @@ export class BawApiService< public list( classBuilder: ClassBuilder, path: string, - opts?: CacheOpt & NotificationsOpt + options: BawServiceOptions = {} ): Observable { return this.session.authTrigger.pipe( - switchMap(() => - this.httpGet(path, defaultApiHeaders, { - cache: this.cacheSettings.enabled && !opts?.disableCache, - }) - ), + switchMap(() => this.httpGet(path, defaultApiHeaders, options)), map(this.handleCollectionResponse(classBuilder)), - catchError((err) => this.handleError(err, opts?.disableNotification)) + catchError((err) => this.handleError(err, this.shouldNotify(options))) ); } @@ -191,12 +226,12 @@ export class BawApiService< classBuilder: ClassBuilder, path: string, filters: Filters, - opts?: NotificationsOpt + options: BawServiceOptions = {} ): Observable { return this.session.authTrigger.pipe( - switchMap(() => this.httpPost(path, filters)), + switchMap(() => this.httpPost(path, filters, undefined, options)), map(this.handleCollectionResponse(classBuilder)), - catchError((err) => this.handleError(err, opts?.disableNotification)) + catchError((err) => this.handleError(err, this.shouldNotify(options))) ); } @@ -211,12 +246,12 @@ export class BawApiService< classBuilder: ClassBuilder, path: string, filters: Filters, - opts?: NotificationsOpt + options: BawServiceOptions = {} ): Observable { return this.session.authTrigger.pipe( - switchMap(() => this.httpPost(path, filters)), + switchMap(() => this.httpPost(path, filters, undefined, options)), map(this.handleSingleResponse(classBuilder)), - catchError((err) => this.handleError(err, opts?.disableNotification)) + catchError((err) => this.handleError(err, this.shouldNotify(options))) ); } @@ -229,16 +264,12 @@ export class BawApiService< public show( classBuilder: ClassBuilder, path: string, - opts?: CacheOpt & NotificationsOpt + options: BawServiceOptions = {} ): Observable { return this.session.authTrigger.pipe( - switchMap(() => - this.httpGet(path, defaultApiHeaders, { - cache: this.cacheSettings.enabled && !opts?.disableCache, - }) - ), + switchMap(() => this.httpGet(path, defaultApiHeaders, options)), map(this.handleSingleResponse(classBuilder)), - catchError((err) => this.handleError(err, opts?.disableNotification)) + catchError((err) => this.handleError(err, this.shouldNotify(options))) ); } @@ -256,7 +287,7 @@ export class BawApiService< createPath: string, updatePath: (model: Model) => string, model: AbstractModel, - opts?: NotificationsOpt + options: BawServiceOptions = {} ): Observable { const jsonData = model.getJsonAttributes?.({ create: true }); const formData = model.getFormDataOnlyAttributes({ create: true }); @@ -271,7 +302,7 @@ export class BawApiService< return of(null).pipe( concatMap( model.hasJsonOnlyAttributes({ create: true }) - ? () => this.httpPost(createPath, body).pipe() + ? () => this.httpPost(createPath, body, undefined, options).pipe() : (data) => of(data) ), // we create a class from the POST response so that we can construct an update route for the formData PUT request @@ -284,15 +315,18 @@ export class BawApiService< // using ternary logic here (similar to the update function) would result in poor readability and a lot of nesting iif( () => model.hasFormDataOnlyAttributes({ create: true }), - this.httpPut(updatePath(data), formData, multiPartApiHeaders).pipe( - map(this.handleSingleResponse(classBuilder)) - ), + this.httpPut( + updatePath(data), + formData, + multiPartApiHeaders, + options + ).pipe(map(this.handleSingleResponse(classBuilder))), of(data) ) ), // there is no map function here, because the handleSingleResponse method is invoked on the POST and PUT requests // individually. Moving the handleSingleResponse mapping here would result in the response object being instantiated twice - catchError((err) => this.handleError(err, opts?.disableNotification)) + catchError((err) => this.handleError(err, this.shouldNotify(options))) ); } @@ -308,7 +342,7 @@ export class BawApiService< classBuilder: ClassBuilder, path: string, model: AbstractModel, - opts?: NotificationsOpt + options: BawServiceOptions = {} ): Observable { const jsonData = model.getJsonAttributes?.({ update: true }); const formData = model.getFormDataOnlyAttributes({ update: true }); @@ -326,16 +360,16 @@ export class BawApiService< // returns a value, and not an observable. Because we use concatMap below, we need the existing // value to be emitted as an observable instead. Therefore, we create a static observable using of() model.hasJsonOnlyAttributes({ update: true }) - ? () => this.httpPatch(path, body) + ? () => this.httpPatch(path, body, undefined, options) : (data) => of(data) ), concatMap( model.hasFormDataOnlyAttributes({ update: true }) - ? () => this.httpPut(path, formData, multiPartApiHeaders) + ? () => this.httpPut(path, formData, multiPartApiHeaders, options) : (data) => of(data) ), map(this.handleSingleResponse(classBuilder)), - catchError((err) => this.handleError(err, opts?.disableNotification)) + catchError((err) => this.handleError(err, this.shouldNotify(options))) ); } @@ -344,11 +378,14 @@ export class BawApiService< * * @param path API path */ - public destroy(path: string, opts?: NotificationsOpt): Observable { - return this.httpDelete(path).pipe( + public destroy( + path: string, + options: BawServiceOptions = {} + ): Observable { + return this.httpDelete(path, undefined, options).pipe( map(this.handleEmptyResponse), tap(() => this.clearCache(path)), - catchError((err) => this.handleError(err, opts?.disableNotification)) + catchError((err) => this.handleError(err, this.shouldNotify(options))) ); } @@ -358,20 +395,31 @@ export class BawApiService< * whenever the users authenticated state changes. * * @param path API path + * @param headers Request headers + * + * @param options Options to modify the request + * eg. `{ withCredentials: false }` will not include the users authentication token and cookies in the request */ public httpGet( path: string, - options: any = defaultApiHeaders, - cacheOptions: ContextOptions = { cache: false } + headers: HttpHeaders = defaultApiHeaders, + options: BawServiceOptions = {} ): Observable> { + const fullOptions = this.buildServiceOptions(options); + + const cacheContext: HttpContext = withCache({ + ttl: this.cacheSettings.httpGetTtlMs, + context: withCacheLogging(), + ...options.cacheOptions, + cache: this.cacheSettings.enabled && fullOptions.cacheOptions.cache, + }); + + const context = this.credentialsHttpContext(fullOptions, cacheContext); + return this.http.get>(this.getPath(path), { responseType: "json", - headers: options, - context: withCache({ - ttl: this.cacheSettings.httpGetTtlMs, - context: withCacheLogging(), - ...cacheOptions, - }), + headers, + context, }); } @@ -381,14 +429,24 @@ export class BawApiService< * whenever the users authenticated state changes. * * @param path API path + * @param headers Request headers + * + * @param options Options to modify the request + * eg. `{ withCredentials: false }` will not include the users authentication token and cookies in the request */ public httpDelete( path: string, - options: any = defaultApiHeaders + headers: HttpHeaders = defaultApiHeaders, + options: BawServiceOptions = {} ): Observable> { + const fullOptions = this.buildServiceOptions(options); + + const context = this.credentialsHttpContext(fullOptions); + return this.http.delete>(this.getPath(path), { responseType: "json", - headers: options, + headers, + context, }); } @@ -399,18 +457,28 @@ export class BawApiService< * * @param path API path * @param body Request body + * @param headers Request headers + * + * @param options Options to modify the request + * eg. `{ withCredentials: false }` will not include the users authentication token and cookies in the request */ public httpPost( path: string, body?: any, - options: any = defaultApiHeaders + headers: HttpHeaders = defaultApiHeaders, + options: BawServiceOptions = {} ): Observable> { + const fullOptions = this.buildServiceOptions(options); + + const context = this.credentialsHttpContext(fullOptions); + return this.http.post>( this.getPath(path), body, { responseType: "json", - headers: options, + headers, + context, } ); } @@ -422,15 +490,25 @@ export class BawApiService< * * @param path API path * @param body Request body + * @param headers Request headers + * + * @param options Options to modify the request + * eg. `{ withCredentials: false }` will not include the users authentication token and cookies in the request */ public httpPut( path: string, body?: any, - options: any = defaultApiHeaders + headers: HttpHeaders = defaultApiHeaders, + options: BawServiceOptions = {} ): Observable> { + const fullOptions = this.buildServiceOptions(options); + + const context = this.credentialsHttpContext(fullOptions); + return this.http.put>(this.getPath(path), body, { responseType: "json", - headers: options, + headers, + context, }); } @@ -441,19 +519,33 @@ export class BawApiService< * * @param path API path * @param body Request body + * @param headers Request headers + * + * @param options Options to modify the request + * eg. `{ withCredentials: false }` will not include the users authentication token and cookies in the request */ public httpPatch( path: string, body?: any, - options: any = defaultApiHeaders + headers: HttpHeaders = defaultApiHeaders, + options: BawServiceOptions = {} ): Observable> { + const fullOptions = this.buildServiceOptions(options); + + const context = this.credentialsHttpContext(fullOptions); + return this.http.patch>(this.getPath(path), body, { responseType: "json", - headers: options, + headers, + context, }); } - public encodeFilter(filter: Filters, disablePaging?: boolean): string { + public encodeFilter( + filter: Filters, + disablePaging?: boolean, + withCredentials: boolean = true + ): string { const body: Record = { // Base64 RFC 4648 ยง5 encoding filterEncoded: toBase64Url(JSON.stringify(toSnakeCase(filter))), @@ -463,7 +555,7 @@ export class BawApiService< body["disablePaging"] = "true"; } - if (this.session.isLoggedIn) { + if (this.session.isLoggedIn && withCredentials) { body["authToken"] = this.session.authToken; } diff --git a/src/app/services/baw-api/harvest/harvest.service.ts b/src/app/services/baw-api/harvest/harvest.service.ts index 06c45c51e..098992613 100644 --- a/src/app/services/baw-api/harvest/harvest.service.ts +++ b/src/app/services/baw-api/harvest/harvest.service.ts @@ -61,7 +61,7 @@ export class HarvestsService implements StandardApi]> { project: IdOr ): Observable { return this.api.show(Harvest, endpoint(project, model, emptyParam), { - disableCache: true, + cacheOptions: { cache: false }, }); } diff --git a/src/app/services/baw-api/website-status/website-status.service.spec.ts b/src/app/services/baw-api/website-status/website-status.service.spec.ts index f1635f79b..c173713de 100644 --- a/src/app/services/baw-api/website-status/website-status.service.spec.ts +++ b/src/app/services/baw-api/website-status/website-status.service.spec.ts @@ -1,12 +1,20 @@ -import { SpectatorService, createServiceFactory } from "@ngneat/spectator"; +import { + SpectatorService, + SpyObject, + createServiceFactory, +} from "@ngneat/spectator"; import { mockServiceImports, mockServiceProviders, } from "@test/helpers/api-common"; +import { BawApiService } from "@baw-api/baw-api.service"; +import { WebsiteStatus } from "@models/WebsiteStatus"; +import { of } from "rxjs"; import { WebsiteStatusService } from "./website-status.service"; describe("WebsiteStatusService", () => { let spec: SpectatorService; + let mockApi: SpyObject>; const createService = createServiceFactory({ service: WebsiteStatusService, @@ -15,7 +23,11 @@ describe("WebsiteStatusService", () => { }); function setup(): void { - spec = createService(); + spec = createService({}); + + mockApi = spec.inject(BawApiService); + mockApi.httpGet = jasmine.createSpy("httpGet") as any; + mockApi.httpGet.and.returnValue(of()); jasmine.clock().install(); jasmine.clock().mockDate(new Date("2020-01-01T00:00:00.000+09:30")); @@ -30,4 +42,20 @@ describe("WebsiteStatusService", () => { it("should create", () => { expect(spec.service).toBeInstanceOf(WebsiteStatusService); }); + + it("should call httpGet with the correct options", () => { + const expectedHeaders = spec.service["requestHeaders"]; + const expectedOptions = { + cacheOptions: { cache: false }, + withCredentials: false, + }; + + spec.service["show"]().subscribe(); + + expect(mockApi.httpGet).toHaveBeenCalledWith( + "/status", + expectedHeaders, + expectedOptions + ); + }); }); diff --git a/src/app/services/baw-api/website-status/website-status.service.ts b/src/app/services/baw-api/website-status/website-status.service.ts index ce3b7f3a1..2ef2468f8 100644 --- a/src/app/services/baw-api/website-status/website-status.service.ts +++ b/src/app/services/baw-api/website-status/website-status.service.ts @@ -61,9 +61,12 @@ export class WebsiteStatusService { private show(): Observable { const endpoint = "/status"; - // we use our custom api.httpGet so that this service using our site wide - // caching config (and can be overwritten by the caller in a uniform manner) - return (this.api.httpGet(endpoint, this.requestHeaders) as any).pipe( + return ( + this.api.httpGet(endpoint, this.requestHeaders, { + cacheOptions: { cache: false }, + withCredentials: false, + }) as any + ).pipe( map((response: IWebsiteStatus) => new WebsiteStatus(response)), catchError((err: BawApiError | Error) => { const bawError = isBawApiError(err)