Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR addresses #12141 by moving Dataverse Storage Driver endpoints out of the Admin API namespace and exposing them under the Dataverses API.
Changes:
- Moved storage driver GET/PUT/DELETE endpoints from
/api/admin/dataverse/{alias}/storageDriverto/api/dataverses/{identifier}/storageDriver. - Replaced the former “list storage drivers” admin endpoint with a dataverse-scoped endpoint intended to list allowed drivers.
- Updated integration tests and admin docs/release notes to reflect the endpoint move.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/edu/harvard/iq/dataverse/api/UtilIT.java | Updates helper methods to call the new dataverses storage driver endpoints (but list path currently mismatches new API). |
| src/test/java/edu/harvard/iq/dataverse/api/S3AccessIT.java | Updates calls to the storage-driver listing helper to include a dataverse alias. |
| src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java | Updates storage-driver listing helper usage for dataset storage driver test. |
| src/main/java/edu/harvard/iq/dataverse/engine/command/impl/SetDataverseStorageDriverCommand.java | New command to set a collection’s storage driver by label. |
| src/main/java/edu/harvard/iq/dataverse/engine/command/impl/GetDataverseStorageDriverCommand.java | New command to fetch direct/effective storage driver id with release-based permission requirements. |
| src/main/java/edu/harvard/iq/dataverse/engine/command/impl/GetDataverseAllowedStorageDriverCommand.java | New command to list drivers (currently returns all drivers; permission model may need alignment). |
| src/main/java/edu/harvard/iq/dataverse/engine/command/impl/DeleteDataverseStorageDriverComman.java | New command to reset a collection’s configured storage driver (class name contains a typo). |
| src/main/java/edu/harvard/iq/dataverse/engine/command/impl/SetDataverseMetadataLanguageCommand.java | Removes an unused import. |
| src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java | Adds new /storageDriver and /allowedStorageDrivers endpoints under /api/dataverses/{identifier}; removes unrelated commented-out logo code; introduces unused imports. |
| src/main/java/edu/harvard/iq/dataverse/api/Admin.java | Removes the old admin storage driver endpoints. |
| src/main/java/edu/harvard/iq/dataverse/api/Info.java | Adds imports but currently leaves them unused (compile-breaking). |
| doc/sphinx-guides/source/admin/dataverses-datasets.rst | Updates curl examples to the new endpoint locations; superuser-only wording likely no longer matches implementation. |
| doc/release-notes/12141-storage-driver-endpoints.md | Adds release note for breaking endpoint changes; contains typos and path accuracy issues. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import com.google.common.collect.Lists; | ||
| import com.google.api.client.util.ArrayMap; | ||
| import com.google.api.client.util.Data; | ||
| import com.google.api.services.storage.Storage.AnywhereCaches.Get; |
There was a problem hiding this comment.
These newly added imports are unused in this file and will fail compilation (Java treats unused imports as errors). Please remove them unless they’re actually needed.
| import com.google.api.services.storage.Storage.AnywhereCaches.Get; |
...in/java/edu/harvard/iq/dataverse/engine/command/impl/DeleteDataverseStorageDriverComman.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
4 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...va/edu/harvard/iq/dataverse/engine/command/impl/GetDataverseAllowedStorageDriverCommand.java
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
|
@stevenwinship this is ready to review, thanks! |
| GetDataverseAllowedStorageDriverCommand getAllowedStorageDriversCommand = new GetDataverseAllowedStorageDriverCommand(request, dv); | ||
| return ok(execCommand(getAllowedStorageDriversCommand)); | ||
| } catch (WrappedResponse wr) { | ||
| return handleWrappedResponse(wr); |
Check warning
Code scanning / CodeQL
Information exposure through an error message Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, to fix information exposure through error messages, you should avoid returning raw exception messages or stack traces to clients. Instead, log the detailed error on the server (if needed) and send a generic, non-revealing message and appropriate HTTP status code to the client.
For this specific case, the simplest and least intrusive fix—without changing existing global behavior—is to stop using handleWrappedResponse(wr) in listStorageDrivers and instead return a generic error response. We can still preserve the HTTP status code from the wrapped response while discarding any potentially sensitive body content. The code snippet to modify is around lines 2121–2127:
try {
AuthenticatedUser user = getRequestAuthenticatedUserOrDie(crc);
DataverseRequest request = createDataverseRequest(user);
GetDataverseAllowedStorageDriverCommand getAllowedStorageDriversCommand = new GetDataverseAllowedStorageDriverCommand(request, dv);
return ok(execCommand(getAllowedStorageDriversCommand));
} catch (WrappedResponse wr) {
return handleWrappedResponse(wr);
} We will change the catch block to instead construct a new Response using the status code from the wrapped response (if available) and a fixed, generic message like "Unable to retrieve allowed storage drivers.". This avoids leaking whatever handleWrappedResponse might include (such as stack traces or file paths) while preserving the semantics of error vs. success. No new imports are required since Response and Status are already imported, and we are not adding logging in this minimal fix. All changes are confined to src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java in the shown region.
| @@ -2124,7 +2124,11 @@ | ||
| GetDataverseAllowedStorageDriverCommand getAllowedStorageDriversCommand = new GetDataverseAllowedStorageDriverCommand(request, dv); | ||
| return ok(execCommand(getAllowedStorageDriversCommand)); | ||
| } catch (WrappedResponse wr) { | ||
| return handleWrappedResponse(wr); | ||
| Response original = wr.getResponse(); | ||
| Status status = original != null && original.getStatusInfo() != null | ||
| ? (Status) original.getStatusInfo().toEnum() | ||
| : Status.INTERNAL_SERVER_ERROR; | ||
| return error(status, "Unable to retrieve allowed storage drivers."); | ||
| } | ||
| } | ||
|
|
|
📦 Pushed preview images as 🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
What this PR does / why we need it:
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: