Add another attempt to check write permissions on a folder (fixes #1971) #2057
Conversation
tomasz1986
changed the title
| public static Boolean nativeBinaryCanWriteToPath2(File externalStorageDir, String absoluteFolderPath) { | ||
| final String TOUCH_FILE_NAME = ".stwritetest"; | ||
|
|
||
| // normalize path replacing ~ with external storage directory |
There was a problem hiding this comment.
| // normalize path replacing ~ with external storage directory | |
| // Normalize path replacing ~ with external storage directory. |
| final String TOUCH_FILE_NAME = ".stwritetest"; | ||
|
|
||
| // normalize path replacing ~ with external storage directory | ||
| // this is consistent with what $HOME is set to in SyncthingRunnable |
There was a problem hiding this comment.
| // this is consistent with what $HOME is set to in SyncthingRunnable | |
| // This is consistent with what $HOME is set to in SyncthingRunnable. |
| // this is consistent with what $HOME is set to in SyncthingRunnable | ||
| final String normalizedPath = absoluteFolderPath.replaceFirst("^~", externalStorageDir.getAbsolutePath()); | ||
|
|
||
| // Write permission test file |
There was a problem hiding this comment.
| // Write permission test file | |
| // Write permission test file. |
| * Returns if the syncthing binary would be able to write a file into | ||
| * the given folder given the configured access level. | ||
| * | ||
| * This uses the Android-native File API |
There was a problem hiding this comment.
| * This uses the Android-native File API | |
| * This uses the Android-native File API. |
| @@ -9,6 +9,8 @@ | |||
| import android.os.Build; | |||
| import android.os.Bundle; | |||
| import androidx.documentfile.provider.DocumentFile; | |||
|
|
|||
There was a problem hiding this comment.
Please remove the new line.
| @@ -537,6 +539,10 @@ private void checkWriteAndUpdateUI() { | |||
| * Access level readwrite: folder can be configured "sendonly" or "sendreceive". | |||
| */ | |||
| mCanWriteToPath = Util.nativeBinaryCanWriteToPath(FolderActivity.this, mFolder.path); | |||
| if(!mCanWriteToPath){ | |||
There was a problem hiding this comment.
| if(!mCanWriteToPath){ | |
| if (!mCanWriteToPath){ |
| // Write permission test file | ||
| File touchFile = new File(normalizedPath, TOUCH_FILE_NAME); | ||
| final String touchFilePath = touchFile.getAbsolutePath(); | ||
| try{ |
There was a problem hiding this comment.
| try{ | |
| try { |
There was a problem hiding this comment.
Please double-check whether the code follows the same convention as the current one (spacing, new lines, etc.).
There was a problem hiding this comment.
@tomasz1986 thank you for the review. I addressed the formatting issues you highlighted.
There was a problem hiding this comment.
Thanks for working on this! And sorry for the long wait on the review.
This looks good. One question inline and a concern below, just in case you have any insight on it.
I am a somewhat worried that Android handles IO differently when executing shell command vs using the java file API. However I think it's better to potentially have some false positives than the current state, where the detection always fails and users need to know they can workaround it by creating a folder in the web UI.
Also FYI it might be a while (at worst never) until this reaches users due to google, see #2064
| // Normalize path replacing ~ with external storage directory. | ||
| // This is consistent with what $HOME is set to in SyncthingRunnable. | ||
| final String normalizedPath = absoluteFolderPath.replaceFirst("^~", externalStorageDir.getAbsolutePath()); |
There was a problem hiding this comment.
Did you test explicitly if this part alone helped?
Seems like we should use the same environment, including setting HOME, when executing the shell command as we do in SyncthingRunnable.
Description
Fixes issue #1971
Changes
~toexternalStorageDirectory.Constants.PREF_USE_ROOTjava.io.Fileto check access instead of shelling out.Notes
This an unobtrusive change, leaving the existing write-access check in place.
I tested this on a Pixel 6 Pro with Android 14.