AccessRepository.ts omits credentials if using Bearer Token Authorization#438
AccessRepository.ts omits credentials if using Bearer Token Authorization#438ekraffmiller wants to merge 4 commits intodevelopfrom
Conversation
ekraffmiller
added
GREI Re-arch
There was a problem hiding this comment.
Pull request overview
This PR updates AccessRepository guestbook-download calls to avoid sending session cookies when Bearer Token authentication is configured, preventing unintended authentication via existing Dataverse session cookies.
Changes:
- Replaced axios-based
doPostusage inAccessRepositoryguestbook download methods withfetch, selectingcredentials: 'omit'for bearer-token auth. - Added a unit test asserting
fetchis called withcredentials: 'omit'under bearer-token auth. - Strengthened an integration test to validate the returned signed URL is actually downloadable.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/access/infra/repositories/AccessRepository.ts |
Implements fetch-based POST for guestbook download endpoints and omits credentials for bearer-token auth. |
test/unit/access/AccessRepository.test.ts |
Adds unit coverage to ensure credentials: 'omit' is used with bearer tokens. |
test/integration/access/AccessRepository.test.ts |
Verifies the signed URL can be fetched and returns expected file content. |
Comments suppressed due to low confidence (1)
src/access/infra/repositories/AccessRepository.ts:37
submitGuestbookForDatafilesDownloadtakesfileIds: Array<number>but still builds the endpoint usingArray.isArray(fileIds) ? fileIds.join(',') : fileIds. TheArray.isArraybranch is dead code with the current signature and makes the endpoint construction harder to read. Simplify to alwaysfileIds.join(',')(or widen the parameter type if non-array input is actually supported).
return await this.submitGuestbookDownload(
this.buildApiEndpoint(
this.accessResourceName,
`datafiles/${Array.isArray(fileIds) ? fileIds.join(',') : fileIds}`
),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const responseData = await this.parseResponseBody(response) | ||
|
|
||
| if (!response.ok) { | ||
| throw new WriteError(this.buildFetchErrorMessage(response.status, responseData)) | ||
| } | ||
|
|
||
| return responseData.data.signedUrl as string | ||
| } |
There was a problem hiding this comment.
After parseResponseBody, the code assumes responseData is an object with data.signedUrl. If the response has a missing/incorrect content-type header (so it’s parsed as text) or the JSON shape differs, this will throw a runtime TypeError instead of a controlled WriteError. Add validation for the expected response shape (and/or a fallback JSON parse attempt) and throw a WriteError with a clear message when signedUrl is absent.
| 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) => { |
There was a problem hiding this comment.
AccessRepository reimplements request building (query param serialization, header conversion, body parsing, error message formatting, credentials selection) using fetch, while the rest of the codebase standardizes HTTP behavior via ApiRepository + axios. To reduce drift and keep error/timeout/auth behavior consistent, consider extracting these helpers into ApiRepository (e.g., a shared doPostFetch/doRequestFetch used by this repository) or a core utility module reused across repositories.
| 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 | ||
|
|
There was a problem hiding this comment.
This test overwrites global.fetch but never restores it. That can leak into other unit tests running in the same worker and make failures order-dependent. Capture the original global.fetch and restore it in afterEach (or use jest.spyOn(global, 'fetch') and mockRestore()).
github-project-automation
bot
moved this from Ready for Review ⏩
to Ready for QA ⏩
in IQSS Dataverse Project
What this PR does / why we need it:
The Access API for download sometimes uses session cookies for authentication, which can be a problem if the user is running JSF and SPA in the same browser. To work around this, this change omits session cookies for all API calls in AccessRepository.
Which issue(s) this PR closes:
Related Dataverse PRs:
(IQSS/dataverse#12267)
Suggestions on how to test this:
Review tests