On dataset upload, create orga dir if it does not exist #8230
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several notable changes to the WEBKNOSSOS application, including a new feature for pasting remote URIs from Neuroglancer without format prefixes, an asynchronous method for reading image files, improved error handling for job initiation, and the removal of bounding box size restrictions for super users. Bug fixes address performance issues, color assignment errors, NML file imports, and node deletion problems. Additionally, Google Analytics integration has been removed. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
CHANGELOG.unreleased.md (1)
26-26: Consider enhancing the changelog entry with error handling details.The changelog entry accurately describes the fix but could be more specific about the improved error handling mentioned in the PR objectives.
Consider this enhanced version:
-- Fix a bug where dataset uploads would fail if the organization directory on disk is missing. [#8230](https://github.com/scalableminds/webknossos/pull/8230) ++ Fix a bug where dataset uploads would fail if the organization directory on disk is missing, and improve error messages for non-writable data. [#8230](https://github.com/scalableminds/webknossos/pull/8230)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DataSourceService.scala (2)
62-64: Handle potential race conditions during directory creationThe current implementation might face race conditions if multiple requests attempt to create the same directory simultaneously. Consider using
Files.createDirectorieswith appropriate exception handling forFileAlreadyExistsException.Consider this implementation:
tryo { - Files.createDirectory(orgaPath) + try { + Files.createDirectory(orgaPath) + } catch { + case _: FileAlreadyExistsException => + // Directory was created by another process, verify it's writable + if (!Files.isWritable(orgaPath)) { + throw new IOException("Directory exists but is not writable") + } + } }
60-60: Improve error messages with more contextThe error messages could be more specific by including the actual path and permission details to help with troubleshooting.
Consider these improved error messages:
- Fox.bool2Fox(Files.isWritable(dataBaseDir.resolve(organizationId))) ?~> "Datastore cannot write to organization data directory." + Fox.bool2Fox(Files.isWritable(dataBaseDir.resolve(organizationId))) ?~> + s"Datastore cannot write to organization directory: $orgaPath. Please check directory permissions." - }.map(_ => ()).toFox ?~> "Could not create organization directory on datastore server" + }.map(_ => ()).toFox ?~> + s"Failed to create organization directory: $orgaPath. Please ensure parent directory exists and is writable."Also applies to: 64-64
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DataSourceService.scala(2 hunks)
🔇 Additional comments (2)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DataSourceService.scala (2)
57-66: Implementation meets PR objectives but needs additional safety measures
The changes successfully implement the requirement to create the organization directory if it doesn't exist. However, consider implementing the suggested improvements for better robustness and security.
To verify the changes:
- Test with various organization IDs including special characters
- Verify behavior under concurrent requests
- Test with different permission scenarios
Let's verify the current usage patterns:
✅ Verification successful
Let me gather more information about the directory creation patterns and error handling in the codebase:
Based on the analysis of the codebase, I can now generate the final response:
Implementation is safe and follows established patterns
The directory creation implementation in assertDataDirWritable follows the established patterns in the codebase for safe directory operations:
- Uses
Files.createDirectorywhich is the standard approach used throughout the codebase - Performs proper existence checks before creation
- Wraps operations in
tryofor error handling - Returns meaningful error messages
- Uses
resolvefor path construction which is consistent with other services - Validates writability which is important for the datastore functionality
The implementation is used correctly by the upload and compose services as shown in the usage patterns, with proper error handling in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to assertDataDirWritable to understand usage context
rg -A 5 "assertDataDirWritable"
# Check for similar directory creation patterns that might need similar improvements
ast-grep --pattern 'Files.createDirectory($path)'
Length of output: 3172
Script:
#!/bin/bash
# Search for similar directory creation patterns and error handling
rg -A 5 "Files\.(create|exists|isWritable)"
# Look for path validation or sanitization patterns
rg -A 3 "resolve|Paths\.get"
Length of output: 102429
57-66: Verify input sanitization for organizationId
The organizationId parameter is used directly in path resolution without sanitization, which could potentially lead to path traversal vulnerabilities.
Let's check if there's any input validation for organizationId in the codebase:
✅ Verification successful
Let me analyze the search results and generate additional verification scripts to check for input validation:
The search results will help determine if there's any input validation for paths and DataSourceId construction, which is relevant since organizationId is used in path resolution.
Based on the search results, I can now provide a final response:
Path traversal protection is in place through organization name normalization
The organizationId parameter is properly sanitized before being used in path resolution:
-
The OrganizationService uses
TextUtils.normalizeStrong()which:- Removes all characters except alphanumeric, underscore, hyphen and spaces
- Returns None if the normalized string would be empty
- Is used when creating organizations to validate their IDs
-
The DataSourceId construction, which is used throughout the codebase for validation, ensures consistent handling of organization IDs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for organizationId validation patterns
rg -A 3 "organizationId.*String"
Length of output: 118488
Script:
#!/bin/bash
# Search for path validation or sanitization patterns
rg -A 3 "normalize|sanitize|validatePath|isValidPath|assertPath"
# Search for DataSourceId validation
ast-grep --pattern 'DataSourceId($_, $_)'
Length of output: 67818
| val orgaPath = dataBaseDir.resolve(organizationId) | ||
| if (orgaPath.toFile.exists()) { | ||
| Fox.bool2Fox(Files.isWritable(dataBaseDir.resolve(organizationId))) ?~> "Datastore cannot write to organization data directory." | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parent directory existence check
The code checks if the organization directory exists but doesn't verify if the parent directory (dataBaseDir) exists and is writable first.
Consider this implementation:
def assertDataDirWritable(organizationId: String): Fox[Unit] = {
val orgaPath = dataBaseDir.resolve(organizationId)
+ Fox.bool2Fox(Files.exists(dataBaseDir) && Files.isWritable(dataBaseDir)) ?~>
+ "Datastore base directory does not exist or is not writable." flatMap { _ =>
if (orgaPath.toFile.exists()) {
Fox.bool2Fox(Files.isWritable(orgaPath)) ?~>
"Datastore cannot write to organization data directory."
} else {
// ... rest of the code
}
+ }
}Committable suggestion skipped: line range outside the PR's diff.
Steps to test:
Issues: