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

Implement proper permission handling using SAF (fixes #1160) #1170

Merged
merged 42 commits into from Jul 15, 2018

Conversation

Projects
None yet
4 participants
@Catfriend1
Copy link
Contributor

Catfriend1 commented Jun 26, 2018

Purpose
Improves the FolderActivity UI when creating or editing a syncthing folder making it clear to the user why a folder can or cannot be written to.
Implements the Storage Access Framework API available on Android 5+ (API level 21) and gracefully falls back to the existing FolderPickerActivity implementation if the app runs on a lower API level.
Intends to fix issue #1160 by creating the ".stfolder" on folder creation properly using the Storage Access Framework on API level 21+. This helps users a lot creating sendonly folders for backup means (yes I know and fully support it is SyncThing and NOT BackupThing).

SAF-related: If a folder can be written to or not by the syncthing binary is intentionally implemented by using the shell instead of SAF as SAF cannot tell us if the native syncthing binary has access to something or not after the user granted permission by making his folder selection on the SAF UI. This also makes the new code backwards compatible as checking write permission using the shell is also available in API levels < 21.

The "Documents" Library shown in Android's native directory picker (SAF UI) is properly supported to be converted to an absolute unix path since commit c786e35 .

Related Issue
#453 We should close this issue as "invalid" as implementing this would a) risk user data on misusage, b) is a niche use case syncing the /data/ partition from Android and c) will get in our way implementing (or in a future android version maybe HAVING to implement) more SAF use in the code "when Google likes us to do so".

Testing
Verified working on device lg-h815 running Android 7.1.2 at 9ee829a .

Verified working on device samsung-i9100 running Android 8.1 9ee829a .

@Catfriend1 Catfriend1 self-assigned this Jun 26, 2018

WIP

Catfriend1 added some commits Jul 1, 2018

WIP

@Catfriend1 Catfriend1 changed the title WIP - Implement proper permission handling using SAF (fixes #1160) Implement proper permission handling using SAF (fixes #1160) Jul 1, 2018

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 1, 2018

Ok, its been tested on two devices with success now. See updated first post, section "Testing". I also verified this working correctly if the user has no external sdcard in the phone or took it out after folder creation.
On Android 8+ we use the EXTRA_INITIAL_DIR to start the folder picker in the writeable external storage "/Android/data/<package_name>/files" directory helping the user to create a new sync folder in the right place. Power users (have in the past) and can still go to another location and setup a new sync folder there. To sum up, the .stfolder marker is also created properly regardless from android's native linux process write perms when a new folder is setup by the user.
This will remove the sluggish discussion and tickets about "how and where can I setup sendreceive folders on android".
Ready for review.

Please also kindly look at : https://forum.syncthing.net/t/looking-for-feedback-from-android-4-x-users/11780

@capi

This comment has been minimized.

Copy link
Member

capi commented Jul 2, 2018

I have not yet reviewed the code itself, I'll try this patch on my devices soon. I just want to say that I think it's a sensible approach to the current situation and should improve usability. "Power-users" that want to do other stuff can still do it from the Web-UI of Syncthing, so you don't remove any use-cases but guide "default users" towards something that is supported. Great job!

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 4, 2018

Doing more testing and comparing to the old ficker mechanism, I found out, we had another problem in 0.10.11 (and before) which is fixed by this PR:
The old FolderPickerActivity (that is still in place for Android < 5) cannot create folders on Android 5+.
Here is a screenshot what happens on 0.10.11 release running Android 8.1:
image

@@ -381,7 +415,21 @@ public boolean onOptionsItemSelected(MenuItem item) {
.show();
return true;
}
getApi().addFolder(mFolder);
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP &&
mFolderUri != null) {

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 4, 2018

Member

Should we only do this for send only folders?

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 5, 2018

Author Contributor

Yes, we pre-check if syncthing can write to a folder and only then, the user can move the slider to enable sendreceive mode. If syncthing can write itself, it will create .stfolder correctly. So it is sufficient to do the creation of .stfolder from the java code only for send only folders as you suggested. I'll commit that shortly.

Thomas Reiser added some commits Jul 5, 2018

Thomas Reiser
Add constants for folder types
Constants.FOLDER_TYPE_SEND_ONLY
Constants.FOLDER_TYPE_SEND_RECEIVE
/**
* Available folder types.
*/
public static final String FOLDER_TYPE_SEND_ONLY = "readonly";

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 5, 2018

Member

I think its "sendonly"? Or atleast we were planning to rename, yet supported the old names.

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 5, 2018

Author Contributor

I was unsure if "sendonly" can be set already for 14.48. Let's put the new names when we have 14.49 (or future version) out and the rename took place.

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 5, 2018

Author Contributor

I checked how it's currently implemented upstream:
v14.48:

  • Web UI writes "readonly", "readwrite" (initially, on change)
  • syncthing understands "readonly", "readwrite", "sendonly", "sendreceive" in config.xml

v14.49-rc.3:

  • Web UI writes "sendonly", "sendreceive" (initially, on change)
  • syncthing understands "readonly", "readwrite", "sendonly", "sendreceive" in config.xml

So I would recommend to put "sendreceive" and "sendonly" when we bump syncthing to v14.49 . If we would do it now, the user can jump into a pitfall when we create a folder "sendonly" and he edits it later through the web UI to be "readwrite". Our code would - according to the constants in place - compare "readwrite" to the string "sendreceive" and assume we were "readonly" then. This can be solved by changing the constans later after 14.49 got released.

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 5, 2018

Member

So I'll only release the new version of the app once .49 rolls off the press.

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 5, 2018

Author Contributor

Ok, I prepared this PR for syncthing v0.14.49+, see commit 463d9cc

Add import of Constants
in FolderActivity.java
@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 5, 2018

@AudriusButkevicius Anything else I can do for this PR or are we ready now?

@Catfriend1 Catfriend1 added this to the syncthing_v0.14.49 milestone Jul 5, 2018

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 5, 2018

Ok let's wait what @capi says after his test. Please note that a test should be based on the commit before the constants change as we still build with 14.48 here.

Catfriend1 added some commits Jul 8, 2018

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 8, 2018

I've resolved the merge conflicts so it's ready again.

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

AudriusButkevicius commented Jul 8, 2018

@capi, can you give the all clear, or tell us if you don't have time and merge it as is?

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 15, 2018

@capi If you don't have time I'll do another test round on my second phone the next days.

@capi

This comment has been minimized.

Copy link
Member

capi commented Jul 15, 2018

I'm so sorry, my job is a little "crazy" at the moment, I hardly have time to do anything besides it. Haven't been online in quite some time. If you guys already tested it, please merge as is, it's unlikely I'll have time for anything until about 2 weeks from now. Sorry again for not commenting that sooner.

@AudriusButkevicius AudriusButkevicius merged commit 40b16cb into syncthing:master Jul 15, 2018

2 checks passed

Build (Syncthing Android) TeamCity build finished
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Catfriend1 Catfriend1 deleted the Catfriend1:createStfolderOnFolderCreation branch Jul 15, 2018

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 16, 2018

SideNote - If you are testing this code on top of the current syncthing v0.14.48 syncthing, you'll experience expected behaviour where the "sendonly" slider is not saved and restored correctly. This is because we prepared the code for the new folder types in syncthing v0.14.49+ and therefore not considered a bug.

@spaetz

This comment has been minimized.

Copy link

spaetz commented Aug 3, 2018

Just testing this in syncthing-android 0.10.13 (syncthing 0.14.49dirty): I can create and select a folder using the SAF framework just fine. However, I am still getting "permission denied" errors in /storage/1234-5678/DCIM.
Storage permission has been granted to the app.
To be clear, I was trying to create a send/receive folder, not just a send-only one for testing purposes.

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Aug 3, 2018

Removed as I had a better idea for support.

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Aug 3, 2018

@spaetz Please head to the forum and find an unofficial beta at https://forum.syncthing.net/t/syncthing-android-unofficial-beta/11920 . I'd appreciate your feedback on this if it solves your issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment