-
Notifications
You must be signed in to change notification settings - Fork 10
AccessRepository.ts omits credentials if using Bearer Token Authorization #438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
62748a8
5be90ea
d497b2b
a4458ff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,11 @@ | ||
| import { ApiConfig, DataverseApiAuthMechanism } from '../../../core/infra/repositories/ApiConfig' | ||
| import { WriteError } from '../../../core/domain/repositories/WriteError' | ||
| import { ApiConstants } from '../../../core/infra/repositories/ApiConstants' | ||
| import { ApiRepository } from '../../../core/infra/repositories/ApiRepository' | ||
| import { | ||
| buildRequestConfig, | ||
| buildRequestUrl | ||
| } from '../../../core/infra/repositories/apiConfigBuilders' | ||
| import { GuestbookResponseDTO } from '../../domain/dtos/GuestbookResponseDTO' | ||
| import { IAccessRepository } from '../../domain/repositories/IAccessRepository' | ||
|
|
||
|
|
@@ -13,14 +20,7 @@ | |
| const endpoint = this.buildApiEndpoint(`${this.accessResourceName}/datafile`, undefined, fileId) | ||
| const queryParams = format ? { signed: true, format } : { signed: true } | ||
|
|
||
| return this.doPost(endpoint, guestbookResponse, queryParams) | ||
| .then((response) => { | ||
| const signedUrl = response.data.data.signedUrl | ||
| return signedUrl | ||
| }) | ||
| .catch((error) => { | ||
| throw error | ||
| }) | ||
| return await this.submitGuestbookDownload(endpoint, guestbookResponse, queryParams) | ||
| } | ||
|
|
||
| public async submitGuestbookForDatafilesDownload( | ||
|
|
@@ -30,21 +30,14 @@ | |
| ): Promise<string> { | ||
| const queryParams = format ? { signed: true, format } : { signed: true } | ||
|
|
||
| return this.doPost( | ||
| return await this.submitGuestbookDownload( | ||
| this.buildApiEndpoint( | ||
| this.accessResourceName, | ||
| `datafiles/${Array.isArray(fileIds) ? fileIds.join(',') : fileIds}` | ||
| ), | ||
| guestbookResponse, | ||
| queryParams | ||
| ) | ||
| .then((response) => { | ||
| const signedUrl = response.data.data.signedUrl | ||
| return signedUrl | ||
| }) | ||
| .catch((error) => { | ||
| throw error | ||
| }) | ||
| } | ||
|
|
||
| public async submitGuestbookForDatasetDownload( | ||
|
|
@@ -59,14 +52,7 @@ | |
| ) | ||
| const queryParams = format ? { signed: true, format } : { signed: true } | ||
|
|
||
| return this.doPost(endpoint, guestbookResponse, queryParams) | ||
| .then((response) => { | ||
| const signedUrl = response.data.data.signedUrl | ||
| return signedUrl | ||
| }) | ||
| .catch((error) => { | ||
| throw error | ||
| }) | ||
| return await this.submitGuestbookDownload(endpoint, guestbookResponse, queryParams) | ||
| } | ||
|
|
||
| public async submitGuestbookForDatasetVersionDownload( | ||
|
|
@@ -82,13 +68,112 @@ | |
| ) | ||
| const queryParams = format ? { signed: true, format } : { signed: true } | ||
|
|
||
| return this.doPost(endpoint, guestbookResponse, queryParams) | ||
| .then((response) => { | ||
| const signedUrl = response.data.data.signedUrl | ||
| return signedUrl | ||
| }) | ||
| .catch((error) => { | ||
| throw error | ||
| }) | ||
| return await this.submitGuestbookDownload(endpoint, guestbookResponse, queryParams) | ||
| } | ||
|
|
||
| private async submitGuestbookDownload( | ||
| apiEndpoint: string, | ||
| guestbookResponse: GuestbookResponseDTO, | ||
| queryParams: object | ||
| ): Promise<string> { | ||
| const requestConfig = buildRequestConfig( | ||
| true, | ||
| queryParams, | ||
| ApiConstants.CONTENT_TYPE_APPLICATION_JSON | ||
| ) | ||
| const response = await fetch( | ||
| this.buildUrlWithQueryParams(buildRequestUrl(apiEndpoint), queryParams), | ||
| { | ||
| method: 'POST', | ||
| headers: this.buildFetchHeaders(requestConfig.headers), | ||
| credentials: this.getFetchCredentials(requestConfig.withCredentials), | ||
| body: JSON.stringify(guestbookResponse) | ||
| } | ||
| ).catch((error) => { | ||
| throw new WriteError(error instanceof Error ? error.message : String(error)) | ||
| }) | ||
|
|
||
| const responseData = await this.parseResponseBody(response) | ||
|
|
||
| if (!response.ok) { | ||
| throw new WriteError(this.buildFetchErrorMessage(response.status, responseData)) | ||
| } | ||
|
|
||
| return this.getSignedUrlOrThrow(responseData) | ||
| } | ||
|
Comment on lines
+96
to
+103
|
||
|
|
||
| private getFetchCredentials(withCredentials?: boolean): RequestCredentials | undefined { | ||
| if (ApiConfig.dataverseApiAuthMechanism === DataverseApiAuthMechanism.BEARER_TOKEN) { | ||
| return 'omit' | ||
| } | ||
|
|
||
| if (withCredentials) { | ||
| return 'include' | ||
| } | ||
|
|
||
| return undefined | ||
| } | ||
|
|
||
| private buildUrlWithQueryParams(requestUrl: string, queryParams: object): string { | ||
| const url = new URL(requestUrl) | ||
|
|
||
| Object.entries(queryParams).forEach(([key, value]) => { | ||
| if (value !== undefined && value !== null) { | ||
| url.searchParams.append(key, String(value)) | ||
| } | ||
| }) | ||
|
|
||
| return url.toString() | ||
| } | ||
|
|
||
| private buildFetchHeaders(headers?: Record<string, unknown>): Record<string, string> { | ||
| const fetchHeaders: Record<string, string> = {} | ||
|
|
||
| if (!headers) { | ||
| return fetchHeaders | ||
| } | ||
|
|
||
| Object.entries(headers).forEach(([key, value]) => { | ||
| if (value !== undefined) { | ||
| fetchHeaders[key] = String(value) | ||
| } | ||
| }) | ||
|
|
||
| return fetchHeaders | ||
| } | ||
|
|
||
| private async parseResponseBody(response: Response): Promise<any> { | ||
| const contentType = response.headers.get('content-type') ?? '' | ||
|
|
||
| if (contentType.includes('application/json')) { | ||
| return await response.json() | ||
| } | ||
|
|
||
| const responseText = await response.text() | ||
|
|
||
| try { | ||
| return JSON.parse(responseText) | ||
| } catch { | ||
| return responseText | ||
| } | ||
| } | ||
|
|
||
| private buildFetchErrorMessage(status: number, responseData: any): string { | ||
| const message = | ||
| typeof responseData === 'string' | ||
| ? responseData | ||
| : responseData?.message || responseData?.data?.message || 'unknown error' | ||
|
|
||
| return `[${status}] ${message}` | ||
| } | ||
|
|
||
| private getSignedUrlOrThrow(responseData: any): string { | ||
| const signedUrl = responseData?.data?.signedUrl | ||
|
|
||
| if (typeof signedUrl !== 'string' || signedUrl.length === 0) { | ||
| throw new WriteError('Missing signedUrl in access download response.') | ||
| } | ||
|
|
||
| return signedUrl | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| /** | ||
| * @jest-environment jsdom | ||
| */ | ||
|
|
||
| import { AccessRepository } from '../../../src/access/infra/repositories/AccessRepository' | ||
| import { GuestbookResponseDTO } from '../../../src/access/domain/dtos/GuestbookResponseDTO' | ||
| import { WriteError } from '../../../src/core/domain/repositories/WriteError' | ||
| import { | ||
| ApiConfig, | ||
| DataverseApiAuthMechanism | ||
| } from '../../../src/core/infra/repositories/ApiConfig' | ||
| import { TestConstants } from '../../testHelpers/TestConstants' | ||
|
|
||
| describe('AccessRepository', () => { | ||
| const sut = new AccessRepository() | ||
| const originalFetch = global.fetch | ||
| const guestbookResponse: GuestbookResponseDTO = { | ||
| guestbookResponse: { | ||
| answers: [{ id: 1, value: 'question 1' }] | ||
| } | ||
| } | ||
|
|
||
| beforeEach(() => { | ||
| window.localStorage.setItem( | ||
| TestConstants.TEST_BEARER_TOKEN_LOCAL_STORAGE_KEY, | ||
| JSON.stringify(TestConstants.TEST_DUMMY_BEARER_TOKEN) | ||
| ) | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| global.fetch = originalFetch | ||
| window.localStorage.clear() | ||
| }) | ||
|
|
||
| test('uses fetch with credentials omit for bearer token auth', async () => { | ||
| ApiConfig.init( | ||
| TestConstants.TEST_API_URL, | ||
| DataverseApiAuthMechanism.BEARER_TOKEN, | ||
| undefined, | ||
| TestConstants.TEST_BEARER_TOKEN_LOCAL_STORAGE_KEY | ||
| ) | ||
|
|
||
| const fetchMock = jest.fn().mockResolvedValue({ | ||
| ok: true, | ||
| status: 200, | ||
| headers: new Headers({ 'content-type': 'application/json' }), | ||
| json: jest.fn().mockResolvedValue({ | ||
| data: { | ||
| signedUrl: 'https://signed.dataset' | ||
| } | ||
| }) | ||
| } as unknown as Response) | ||
|
|
||
| global.fetch = fetchMock as typeof fetch | ||
|
|
||
|
Comment on lines
+43
to
+55
|
||
| const actual = await sut.submitGuestbookForDatasetDownload(123, guestbookResponse, 'original') | ||
|
|
||
| expect(fetchMock).toHaveBeenCalledWith( | ||
| `${TestConstants.TEST_API_URL}/access/dataset/123?signed=true&format=original`, | ||
| { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${TestConstants.TEST_DUMMY_BEARER_TOKEN}` | ||
| }, | ||
| credentials: 'omit', | ||
| body: JSON.stringify(guestbookResponse) | ||
| } | ||
| ) | ||
| expect(actual).toBe('https://signed.dataset') | ||
| }) | ||
|
|
||
| test('parses signed url from a JSON body when content-type is incorrect', async () => { | ||
| ApiConfig.init( | ||
| TestConstants.TEST_API_URL, | ||
| DataverseApiAuthMechanism.BEARER_TOKEN, | ||
| undefined, | ||
| TestConstants.TEST_BEARER_TOKEN_LOCAL_STORAGE_KEY | ||
| ) | ||
|
|
||
| const fetchMock = jest.fn().mockResolvedValue({ | ||
| ok: true, | ||
| status: 200, | ||
| headers: new Headers({ 'content-type': 'text/plain' }), | ||
| text: jest | ||
| .fn() | ||
| .mockResolvedValue(JSON.stringify({ data: { signedUrl: 'https://signed.text' } })) | ||
| } as unknown as Response) | ||
|
|
||
| global.fetch = fetchMock as typeof fetch | ||
|
|
||
| await expect(sut.submitGuestbookForDatasetDownload(123, guestbookResponse)).resolves.toBe( | ||
| 'https://signed.text' | ||
| ) | ||
| }) | ||
|
|
||
| test('throws WriteError when signedUrl is missing from a successful response', async () => { | ||
| ApiConfig.init( | ||
| TestConstants.TEST_API_URL, | ||
| DataverseApiAuthMechanism.BEARER_TOKEN, | ||
| undefined, | ||
| TestConstants.TEST_BEARER_TOKEN_LOCAL_STORAGE_KEY | ||
| ) | ||
|
|
||
| const fetchMock = jest.fn().mockResolvedValue({ | ||
| ok: true, | ||
| status: 200, | ||
| headers: new Headers({ 'content-type': 'application/json' }), | ||
| json: jest.fn().mockResolvedValue({ | ||
| data: {} | ||
| }) | ||
| } as unknown as Response) | ||
|
|
||
| global.fetch = fetchMock as typeof fetch | ||
|
|
||
| await expect(sut.submitGuestbookForDatasetDownload(123, guestbookResponse)).rejects.toThrow( | ||
| new WriteError('Missing signedUrl in access download response.') | ||
| ) | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AccessRepositoryreimplements request building (query param serialization, header conversion, body parsing, error message formatting, credentials selection) usingfetch, while the rest of the codebase standardizes HTTP behavior viaApiRepository+ axios. To reduce drift and keep error/timeout/auth behavior consistent, consider extracting these helpers intoApiRepository(e.g., a shareddoPostFetch/doRequestFetchused by this repository) or a core utility module reused across repositories.