-
Notifications
You must be signed in to change notification settings - Fork 510
android: use SAF for storing Taildropped files #632
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: main
Are you sure you want to change the base?
Conversation
Pull Request Revisions
10 more revisions
✅ AI review completed for r15 HelpReact with emojis to give feedback on AI-generated reviews:
We'd love to hear from you—reach out anytime at [email protected]. |
android/src/main/java/com/tailscale/ipn/util/ShareFileHelper.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/tailscale/ipn/util/ShareFileHelper.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/tailscale/ipn/util/ShareFileHelper.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/tailscale/ipn/ui/viewModel/MainViewModel.kt
Outdated
Show resolved
Hide resolved
0a9a3d8
to
3d78bbd
Compare
android/src/main/java/com/tailscale/ipn/util/ShareFileHelper.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/tailscale/ipn/util/ShareFileHelper.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/tailscale/ipn/util/ShareFileHelper.kt
Outdated
Show resolved
Hide resolved
3d78bbd
to
892300f
Compare
android/src/main/java/com/tailscale/ipn/util/ShareFileHelper.kt
Outdated
Show resolved
Hide resolved
892300f
to
92047bf
Compare
fun toggleVpn(desiredState: Boolean) { | ||
if (isToggleInProgress.value) { | ||
// Prevent toggling while a previous toggle is in progress | ||
return | ||
} | ||
|
||
viewModelScope.launch { | ||
showDirectoryPickerLauncher() |
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.
The showDirectoryPickerLauncher() call is made inside a viewModelScope.launch block during toggleVpn. Was this intentional? It could cause the directory picker to appear unexpectedly during VPN toggling, which might confuse users.
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.
I kind of agree with Skynet here. An actionable error message might be a less disruptive. Perhaps modelled after health warnings. Along with knowing where Taildrop files go (or don't go) and being able to change that in Settings after the fact.
...but this will do for now.
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.
100%
if (storedUri == null) { | ||
// No stored URI, so launch the directory picker. | ||
directoryPickerLauncher?.launch(null) | ||
return |
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.
After launching the directory picker, the function returns immediately. However, there's no indication that the function will be called again after the picker completes. Consider adding a callback mechanism to ensure the flow continues appropriately after directory selection.
92047bf
to
b3e9390
Compare
func SetShareFileHelper(fileHelper ShareFileHelper) { | ||
// Drain the channel if there's an old value. | ||
select { | ||
case <-onShareFileHelper: | ||
default: | ||
// Channel was already empty. | ||
} | ||
select { | ||
case onShareFileHelper <- fileHelper: | ||
default: | ||
// In the unlikely case the channel is still full, drain it and try again. | ||
<-onShareFileHelper | ||
onShareFileHelper <- fileHelper | ||
} |
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.
In SetShareFileHelper in interfaces.go, the current implementation handles a full channel by draining and adding the new value, but it could potentially lose an unprocessed helper. Is a buffered channel size of 1 sufficient, or should it be increased to handle more concurrent operations?
@@ -182,3 +201,23 @@ func SendLog(logstr []byte) { | |||
log.Printf("Log %v not sent", logstr) // missing argument in original code |
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.
There's a missing format argument in SendLog function in interfaces.go. The log.Printf call uses '%v' but doesn't include the logstr variable in the argument list, which would cause a runtime error.
b3e9390
to
a845a25
Compare
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.
Couple of nits, but looks good.
Small concern about the annoyance of popping up a forced picker, but allow users to know that the Taildrop dest is set/unset in Settings and set/change it and perhaps pop up a heath-esque warning if it's unset or unwritable but I understand that adds a bunch of complexity. Also unsure about AndroidTV and whether any of this is relevant.
fun toggleVpn(desiredState: Boolean) { | ||
if (isToggleInProgress.value) { | ||
// Prevent toggling while a previous toggle is in progress | ||
return | ||
} | ||
|
||
viewModelScope.launch { | ||
showDirectoryPickerLauncher() |
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.
I kind of agree with Skynet here. An actionable error message might be a less disruptive. Perhaps modelled after health warnings. Along with knowing where Taildrop files go (or don't go) and being able to change that in Settings after the fact.
...but this will do for now.
|
||
context.contentResolver.openInputStream(partialUriObj)?.use { input -> | ||
context.contentResolver.openOutputStream(destFile.uri)?.use { output -> | ||
input.copyTo(output) |
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.
Seems like a lot of work to move a file but I image there's no alternative?
android/src/main/java/com/tailscale/ipn/util/ShareFileHelper.kt
Outdated
Show resolved
Hide resolved
@@ -150,6 +158,41 @@ class MainActivity : ComponentActivity() { | |||
} | |||
viewModel.setVpnPermissionLauncher(vpnPermissionLauncher) | |||
|
|||
val directoryPickerLauncher = | |||
registerForActivityResult(ActivityResultContracts.OpenDocumentTree()) { uri: Uri? -> |
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.
Are incoming Taildrop files relevant on Android TV? They are not on AppleTV. If the user can actually pick a directory and use the files - all good - otherwise we should skip the dialog.
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.
good call. gating it with an AndroidTV check
data class SafStream(val uri: String, val stream: OutputStream) | ||
|
||
// Cache for streams; keyed by file name and savedUri. | ||
private val streamCache = ConcurrentHashMap<String, SafStream>() |
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.
The streamCache in ShareFileHelper could grow indefinitely since entries are never removed. This might lead to memory leaks if many files are created. Consider adding a cleanup mechanism that removes entries after they're no longer needed or implements an LRU cache with size limits.
fun init(context: Context, uri: String) { | ||
appContext = context.applicationContext | ||
savedUri = uri | ||
Libtailscale.setShareFileHelper(this) | ||
} |
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.
The init function in ShareFileHelper doesn't handle URI validation. If an invalid URI is passed, it will be stored and used, potentially causing errors later. Consider adding validation to prevent storing invalid URIs.
override fun openFileURI(fileName: String): String { | ||
val safFile = createStreamCached(fileName) | ||
return safFile.uri | ||
} |
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.
The createStreamCached method always creates a file even when merely getting the URI in openFileURI. This could lead to empty files being created unnecessarily. Consider separating the file creation from URI retrieval or implementing lazy file creation.
override fun renamePartialFile( | ||
partialUri: String, | ||
targetDirUri: String, | ||
targetName: String | ||
): String { |
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.
In the renamePartialFile method, there's no validation if the target directory is writable before attempting operations. This could lead to failures during file operations. Consider checking write permissions to the target directory before proceeding.
data class SafStream(val uri: String, val stream: OutputStream) | ||
|
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.
The SafStream class doesn't implement proper resource management. If a file is cached but not used for a while, the underlying stream may be closed by the OS or could hold system resources unnecessarily. Consider adding a mechanism to close idle streams after a timeout period.
9f5830d
to
25e3cd7
Compare
} ?: throw IOException("Unable to open output stream for URI: ${destFile.uri}") | ||
} ?: throw IOException("Unable to open input stream for URI: $partialUri") | ||
|
||
DocumentFile.fromSingleUri(context, partialUriObj)?.delete() |
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.
The renamePartialFile method could fail silently if DocumentFile.fromSingleUri(context, partialUriObj)?.delete() returns false or null. This might leave orphaned temporary files. Consider checking the return value and logging a warning.
8e19414
to
aca0993
Compare
Use Android Storage Access Framework for receiving Taildropped files. -Add a picker to allow users to select where Taildropped files go -If no directory is selected, internal app storage is used -Provide SAF API for Go to use when writing and renaming files -Provide Android FileOps implementation Updates tailscale/tailscale#15263 Signed-off-by: kari-ts <[email protected]>
fun generateNewFilename(filename: String): String { | ||
val dotIndex = filename.lastIndexOf('.') | ||
val baseName = if (dotIndex != -1) filename.substring(0, dotIndex) else filename | ||
val extension = if (dotIndex != -1) filename.substring(dotIndex) else "" | ||
|
||
val uuid = UUID.randomUUID() | ||
return "$baseName-$uuid$extension" | ||
} |
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.
The generateNewFilename method uses UUID which is better than a timestamp, but it adds the UUID after the base filename. Consider using a more human-readable format where the original filename is still easily recognizable. For example, appending a shorter unique identifier with a timestamp like $baseName-$timestamp-$shortId$extension
would make sorting and identification easier.
aca0993
to
3e15c26
Compare
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.
In libtailscale/backend.go, the directFileRoot value is set during initialization and there's a setDirectFileRoot method, but there's no synchronization mechanism when changing this value. If setDirectFileRoot is called while file operations are in progress, it could lead to race conditions or inconsistent behavior. Consider adding proper synchronization or ensuring the value can only be changed when no file operations are active.
// This method returns a SafStream containing the SAF URI and its corresponding OutputStream. | ||
override fun openFileWriter(fileName: String): libtailscale.OutputStream { | ||
val stream = createStreamCached(fileName) | ||
return OutputStreamAdapter(stream.stream) | ||
} |
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.
There's potential for resource leakage in the streamCache. The OutputStreamAdapter returned from openFileWriter has no mechanism to remove entries from the cache when the stream is closed. This can lead to accumulating stale streams and growing memory usage. Consider implementing a reference counting or purging mechanism for closed streams.
val directoryPickerLauncher = | ||
registerForActivityResult(ActivityResultContracts.OpenDocumentTree()) { uri: Uri? -> | ||
if (uri != null) { | ||
try { | ||
// Try to take persistable permissions for both read and write. | ||
contentResolver.takePersistableUriPermission( | ||
uri, | ||
Intent.FLAG_GRANT_READ_URI_PERMISSION or Intent.FLAG_GRANT_WRITE_URI_PERMISSION) | ||
} catch (e: SecurityException) { | ||
TSLog.e("MainActivity", "Failed to persist permissions: $e") | ||
} | ||
|
||
// Check if write permission is actually granted. | ||
val writePermission = | ||
this.checkUriPermission( | ||
uri, Process.myPid(), Process.myUid(), Intent.FLAG_GRANT_WRITE_URI_PERMISSION) | ||
if (writePermission == PackageManager.PERMISSION_GRANTED) { | ||
TSLog.d("MainActivity", "Write permission granted for $uri") | ||
Libtailscale.setDirectFileRoot(uri.toString()) | ||
saveFileDirectory(uri) | ||
} else { | ||
TSLog.d( | ||
"MainActivity", | ||
"Write access not granted for $uri. Falling back to internal storage.") | ||
// Don't save directory URI and fall back to internal storage. | ||
} | ||
} else { | ||
TSLog.d( | ||
"MainActivity", "Taildrop directory not saved. Will fall back to internal storage.") | ||
// Fall back to internal storage. | ||
} | ||
} |
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.
In the MainActivity.kt directoryPickerLauncher, there's a missing call to app.getStoredDirectoryUri()
when the user selects a valid URI. This might cause inconsistent behavior where the URI is stored but not properly retrieved on app restart. Consider updating App.getStoredDirectoryUri() to handle the saved URI correctly.
func SetDirectFileRoot(filePath string) { | ||
onFilePath <- filePath |
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.
The SetDirectFileRoot function sends to onFilePath channel without checking if there are receivers. Consider adding a select statement with a default case to prevent blocking if no goroutine is receiving from this channel. This could block the UI thread if the Go side isn't ready to receive.
2d76e45
to
2eacb5a
Compare
// OutputStream provides an adapter between Java's OutputStream and Go's | ||
// io.WriteCloser. | ||
type OutputStream interface { |
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.
There's a documentation issue in interfaces.go where the comment for OutputStream likely has incorrect information. It appears to be copied from InputStream without proper modification.
2eacb5a
to
77c183e
Compare
go func() { | ||
for { | ||
select { | ||
case filepath := <-onFilePath: | ||
lb.SetDirectFileRoot(filepath) | ||
} | ||
} | ||
}() |
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.
The infinite goroutine in backend.go that listens to onFilePath channel doesn't have any cancellation mechanism. This could lead to a goroutine leak if the backend is recreated multiple times during app lifecycle. Consider using a context or a done channel for proper cleanup.
func SetDirectFileRoot(filePath string) { | ||
onFilePath <- filePath | ||
} |
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.
The SetDirectFileRoot function in interfaces.go sends to onFilePath channel without a non-blocking fallback. If the receiving goroutine isn't running yet, this will block the UI thread. Consider using a buffered channel or adding a select with a default case.
private val streamCache = ConcurrentHashMap<String, SafStream>() | ||
|
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.
The streamCache in ShareFileHelper doesn't have any mechanism to remove entries when the stream is closed. This can lead to memory leaks as stale entries accumulate over time. Consider implementing a cleanup mechanism in the close() method of OutputStreamAdapter to remove its entry from the cache.
override fun write(data: ByteArray): Long { | ||
return try { | ||
outputStream.write(data) | ||
outputStream.flush() | ||
data.size.toLong() | ||
} catch (e: Exception) { |
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.
In the OutputStreamAdapter.write method, the return value is always data.size.toLong() regardless of how many bytes were actually written. Java's OutputStream.write might write fewer bytes than requested. Consider capturing the actual bytes written or ensuring the entire buffer is written (possibly with multiple calls).
Signed-off-by: kari-ts <[email protected]>
77c183e
to
fae21a0
Compare
func (ops *AndroidFileOps) RenamePartialFile(partialUri, targetDirUri, targetName string) (string, error) { | ||
newURI := ops.helper.RenamePartialFile(partialUri, targetDirUri, targetName) | ||
if newURI == "" { | ||
return "", fmt.Errorf("failed to rename partial file via SAF") | ||
} | ||
return newURI, nil |
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.
In RenamePartialFile in fileops.go, the empty string check doesn't distinguish between the file not being found and other errors. Consider returning a more specific error message to help with debugging.
// onFilePath receives the SAF path used for Taildrop | ||
onFilePath = make(chan string) |
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.
In libtailscale/callbacks.go, the onFilePath channel is unbuffered. Since SetDirectFileRoot doesn't have a non-blocking fallback, it could deadlock if called before the receiving goroutine is started. Consider using a buffered channel similar to onShareFileHelper.
Use Android Storage Access Framework for receiving Taildropped files.
-Add a picker to allow users to select where Taildropped files go
-If no directory is selected, internal app storage is used
-Provide SAF API for Go to use when writing and renaming files
-Provide Android FileOps implementation
Updates tailscale/tailscale#15263