Added some safety features from permissions.

KambizAsadzadeh commented at 1:49 pm on December 2, 2021: none

Users can now view the permissions requests themselves.

In the Android section, it was observed that no application for a permission has been applied! The codes I sent are enough to solve this problem.

Users can now view the permissions requests themselves.

023025f33a

hebasto commented at 2:21 pm on December 2, 2021: member

cc @icota

hebasto added the label Android on Dec 2, 2021

in src/qt/android/src/org/bitcoincore/qt/BitcoinQtActivity.java:42 in 023025f33a outdated

37+@Override
38+public void onRequestPermissionsResult(int requestCode, String[] permissions, int[] grantResults) {
39+    switch (requestCode) {
40+        case REQUEST_CODE_ASK_MULTIPLE_PERMISSIONS: {
41+            Map<String, Integer> perms = new HashMap<String, Integer>();
42+            perms.put(Manifest.permission.ACCESS_FINE_LOCATION, PackageManager.PERMISSION_GRANTED);

icota commented at 4:53 pm on December 2, 2021:

Why do we need ACCESS_FINE_LOCATION?

KambizAsadzadeh commented at 5:02 pm on December 2, 2021:

Ops! This is my mistake, We don’t need to this permission I’ll remove this in the next commit.

Can you tell me all permissions that used in bitcoin core? I’ll set them all in the next level.

Of course I have a plan for iOS version too.

in src/qt/android/src/org/bitcoincore/qt/BitcoinQtActivity.java:61 in 023025f33a outdated

56+            super.onRequestPermissionsResult(requestCode, permissions, grantResults);
57     }
58 }
59+
60+@TargetApi(Build.VERSION_CODES.M)
61+private void fuckMarshMallow() {

icota commented at 4:54 pm on December 2, 2021:

I like swearing too but it belongs in the comments. Name the function after what it does.

KambizAsadzadeh commented at 8:37 am on December 3, 2021:

This was my mistake, I didn’t pay attention to it, you are right.

in src/qt/android/src/org/bitcoincore/qt/BitcoinQtActivity.java:65 in 023025f33a outdated

60+@TargetApi(Build.VERSION_CODES.M)
61+private void fuckMarshMallow() {
62+    List<String> permissionsNeeded = new ArrayList<String>();
63+    final List<String> permissionsList = new ArrayList<String>();
64+    if (!addPermission(permissionsList, Manifest.permission.ACCESS_FINE_LOCATION))
65+        permissionsNeeded.add("Show Location");

icota commented at 4:54 pm on December 2, 2021:

Again I don’t understand why we need location.

in src/qt/bitcoin.cpp:492 in 023025f33a outdated

485@@ -486,6 +486,14 @@ int GuiMain(int argc, char* argv[])
486     QApplication::setAttribute(Qt::AA_DontUseNativeMenuBar);
487     QApplication::setAttribute(Qt::AA_DontCreateNativeWidgetSiblings);
488     QApplication::setAttribute(Qt::AA_DontUseNativeDialogs);
489+
490+    // Add new safety native permission dialog for R/W actions.
491+    static const QString write_permission = "android.permission.WRITE_EXTERNAL_STORAGE";
492+    if (QtAndroid::checkPermission(write_permission) == QtAndroid::PermissionResult::Denied)

icota commented at 4:55 pm on December 2, 2021:

Is the rationale for this to be able to write to SD card?

KambizAsadzadeh commented at 5:07 pm on December 2, 2021:

Yes, we need to this for internal and removable volumes, such as an SD card, appear in the file system as part of external storage.

KambizAsadzadeh commented at 8:03 pm on December 2, 2021:

@icota Thank you for review! I checked the code again, there is no need for the Java method, and the last changes I made are enough on the C++ side.

icota changes_requested

Changed the method.

Java method removed from BitcoinQtActivity.

We use the QtAndroid method instead.

Note: The androidextras module must be added into the project.

This method is enough.

450c5464bf

icota commented at 7:52 am on December 4, 2021: contributor

Thank you for cleaning up the Java part @KambizAsadzadeh.

Can you please fix the CI and give us a brief rationale for the PR? What are you able to do with these changes that you weren’t able to do before?

KambizAsadzadeh commented at 9:18 am on December 4, 2021: none

Thank you for cleaning up the Java part @KambizAsadzadeh.

You’re welcome!

Can you please fix the CI and give us a brief rationale for the PR? What are you able to do with these changes that you weren’t able to do before?

Check this please: https://github.com/bitcoin-core/gui/blob/57982f419e36d0023c83af2dd0d683ca3160dc2a/src/qt/android/AndroidManifest.xml#L6

In this section there is a request for permission to write on storage right? Well, In this case a native dialog must be displayed to accept or reject this request and this is important in terms of privacy. This is not possible by default and you must do this manually. :)

Like this one: https://github.com/KambizAsadzadeh/gui/blob/450c5464bfd8ee57d2eb7594b2ba207ee6eca64a/src/qt/bitcoin.cpp#L492

We have two ways to do this, one is to use Java code or JNI inside C++ and the other (I suggest the C++ method) is to use method checkPermission on QtAndroid class. this method checks if the permission was granted or not. This function should be called every time when the application starts for needed permissions, as the users might disable them from Android Settings.

icota commented at 10:31 am on December 5, 2021: contributor

Thank you for clarifying @KambizAsadzadeh. This is something that is needed but if we follow good practices probably doesn’t belong at startup as not all users will opt to use external storage.

According to the docs:

Wait for the user to invoke the task or action in your app that requires access to specific private user data. At that time, your app can request the runtime permission that’s required for accessing that data.

IMO there should be a GUI prompt/explainer and only then should we trigger the OS modal to ask for these permissions. Something to discuss with the design team?

KambizAsadzadeh commented at 7:23 pm on December 5, 2021: none

Thank you for clarifying @KambizAsadzadeh. This is something that is needed but if we follow good practices probably doesn’t belong at startup as not all users will opt to use external storage.

According to the docs:

Wait for the user to invoke the task or action in your app that requires access to specific private user data. At that time, your app can request the runtime permission that’s required for accessing that data.

You’re welcome, Thank you for your good comments. I agree with you regarding UX terms, In my opinion, it’s better to call the QtAndroid::checkPermission method where this permission is needed instead of startup. Can you show me the part you used for writing external storage?

Rule 4:

Check whether the user has already granted the runtime permission that your app requires. If so, your app can access the private user data. If not, continue to the next step. You must check whether you have that permission every time you perform an operation that requires that permission.

icota commented at 8:24 am on December 12, 2021: contributor

Can you show me the part you used for writing external storage?

I never used external storage, my hardware doesn’t even have such an option.

As it stands right now we use the .bitcoin folder in app sandbox. I think I enabled WRITE_EXTERNAL_STORAGE because I believed this would be granted without user intervention. In that case one could change the conf and use the SD.

I realise that external storage is a big use-case (people repurposing old phones) so moving forward I suggest we either:

Document how to enable this manually (long press on the app icon -> go to permissions)
Design some sort of UI around this (prompt user if she wants to use external storage -> trigger permissions modal)

DrahtBot commented at 9:47 pm on June 12, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#149 (Intro: Have user choose assumevalid by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

KambizAsadzadeh closed this on Jun 13, 2022

bitcoin-core locked this on Jun 13, 2023

Added some safety features from permissions. #496

Conflicts