feat: support folder selection from other apps via Storage Access Framework - #202
feat: support folder selection from other apps via Storage Access Framework#202alvaroemtnez wants to merge 2 commits into
Conversation
…k, include a toggle to pretend to be local storage and solve COLUMN_FLAGS being declared twice in RootCursor.kt
…ulti-threaded file creation
|
@zerox80 ^ |
|
ill take a look |
| val parentFile = getFileByIdOrException(parentIdInt) | ||
|
|
||
| // Check if the child belongs to the same account and its path sits inside the parent's path | ||
| childFile.owner == parentFile.owner && childFile.remotePath.startsWith(parentFile.remotePath) |
There was a problem hiding this comment.
Keep tree grants within the selected space
isChildDocument() is part of SAF's permission boundary: Android calls it before allowing access to a document through a tree URI.
OpenCloud paths are relative to a space, so two spaces belonging to the same account can both contain paths such as / or /Documents/file.txt. The current check only compares owner and remotePath. Consequently, after the user grants access to Space A, a client can construct a URI containing a document ID from Space B and this method will incorrectly accept it as a descendant.
At minimum, the space must be part of the comparison:
| childFile.owner == parentFile.owner && childFile.remotePath.startsWith(parentFile.remotePath) | |
| childFile.owner == parentFile.owner && | |
| childFile.spaceId == parentFile.spaceId && | |
| childFile.remotePath.startsWith(parentFile.remotePath) |
| newFile.parentFile?.mkdirs() | ||
| fileToUpload = OCFile( | ||
|
|
||
| val pendingId = "pending_" + UUID.randomUUID().toString() |
There was a problem hiding this comment.
Make the temporary upload path unique as well
The UUID currently makes only the map key unique. The backing file is still created as tempDir/displayName.
Therefore, two concurrent creations with the same basename in different remote folders, for example folderA/config.json and folderB/config.json, point to the same local file. Their writes can overwrite each other, and the two queued MOVE uploads then race for the same source path. This can result in incorrect contents or a missing second upload; it only takes two concurrent files, not thousands of operations.
A UUID-specific staging directory keeps the original filename while making the complete path unique:
| val pendingId = "pending_" + UUID.randomUUID().toString() | |
| val pendingId = "pending_${UUID.randomUUID()}" | |
| val tempDir = File(FileStorageUtils.getTemporalPath(parentDocument.owner, parentDocument.spaceId)) | |
| val newFile = File(File(tempDir, pendingId), displayName) | |
| newFile.parentFile?.mkdirs() |
| Timber.d("A file with id $documentId has been closed! Time to synchronize it with server.") | ||
| // If only needs to upload that file | ||
| if (uploadOnly) { | ||
| pendingUploads.remove(documentId) |
There was a problem hiding this comment.
Keep the document ID valid after the first close
createDocument() returns pending_<uuid> as the document's ID, but this entry is removed as soon as the first descriptor closes. There is no mapping from that UUID to the final database file ID.
Any later queryDocument() or openDocument() using the URI returned by createDocument() therefore fails. openDocument() can additionally reach the uninitialized fileToUpload fallback. Losing the map after process recreation causes the same problem.
There is a related metadata issue: queryDocument(pendingId) calls addFile() with an OCFile whose id is null, so FileCursor reports "null" as COLUMN_DOCUMENT_ID instead of the returned UUID.
Android requires a document ID returned by createDocument() to remain stable because it can be used by persistent URI grants:
https://developer.android.com/reference/android/provider/DocumentsProvider#createDocument(java.lang.String,%20java.lang.String,%20java.lang.String)
This should use a persisted UUID-to-file mapping rather than a process-local map. The cursor API could then explicitly expose the provider ID:
fun addFile(
file: OCFile,
documentId: String = requireNotNull(file.id).toString(),
) {
// ...
newRow()
.add(Document.COLUMN_DOCUMENT_ID, documentId)
// ...
}
Summary
OpenCloud has a correct implementation to select its locations from the system files app via the Storage Access Framework. However, as stated in #99 and #45, selecting a folder from an app was not supported, as the
ACTION_OPEN_DOCUMENT_TREEintent of the Android Storage Access Framework (SAF) needed to be implemented.This PR adds support for that intent, and also, as some apps filter the SAF locations and allow only local storage (which OpenCloud can be, if a folder is selected as "available offline", so filtering it makes no sense but hardcoding it as local storage doesn't either), adds a toggle in the Advanced section of settings to pretend to be so, by adding the FLAG_LOCAL_ONLY flag, as other apps like RSAF do, and the corresponding translations for the settings item.
As granting an app access to a location in OpenCloud can mean that the app in question does multiple concurrent operations, light changes have been made, replacing
fileToUploadsingleton with ConcurrentHashMap and UUIDs to support multi-threaded file creation.Code changes
isChildDocumentfunction override toopencloudApp/src/main/java/eu/opencloud/android/presentation/documentsprovider/DocumentsStorageProvider.ktto implement the ACTION_OPEN_DOCUMENT_TREE intent.opencloudApp/src/main/java/eu/opencloud/android/presentation/documentsprovider/DocumentsStorageProvider.kt, minor changes to use ConcurrentHashMap and random UUIDs for a more robust upload queue.addRootfunction inopencloudApp/src/main/java/eu/opencloud/android/presentation/documentsprovider/cursors/RootCursor.ktto pass a boolean to enable pretending to be local storage.opencloudApp/src/main/res/xml/settings_advanced.xmlandopencloudApp/src/main/java/eu/opencloud/android/presentation/settings/advanced/SettingsAdvancedFragment.kt)Limitations
content://eu.opencloud.documents/tree/[ID], where[ID]is an integer defining the directory or file in the internal app tree. As far as I know, displaying the full URI that the user sees in their cloud web UI would require deep and breaking changes (which would probably not be worth, at the moment).AI use
LLMs (Gemini) were used for assistance in code generation and for the translations. All generated code was reviewed and tested and can be easily audited.