From c28ae301af75305fd45ec645bf617cd0488c55c8 Mon Sep 17 00:00:00 2001 From: Mirko Galimberti Date: Thu, 29 Dec 2022 16:39:02 +0100 Subject: [PATCH 1/2] Support permission properties (maxSdkVersion and usesPermissionFlags) + remove WRITE_EXTERNAL_STORAGE permission, which has been previously required by default --- Makefile | 6 +- doc/source/buildoptions.rst | 21 ++++- .../bootstraps/common/build/build.py | 61 +++++++++++-- .../build/templates/AndroidManifest.tmpl.xml | 9 +- .../build/templates/AndroidManifest.tmpl.xml | 6 +- .../build/templates/AndroidManifest.tmpl.xml | 6 +- tests/test_bootstrap_build.py | 86 +++++++++++++++++++ tests/test_build.py | 8 +- 8 files changed, 172 insertions(+), 31 deletions(-) create mode 100644 tests/test_bootstrap_build.py diff --git a/Makefile b/Makefile index 97f502219e..ea5adadcd8 100644 --- a/Makefile +++ b/Makefile @@ -40,7 +40,8 @@ testapps-with-numpy: virtualenv . $(ACTIVATE) && cd testapps/on_device_unit_tests/ && \ python setup.py apk --sdk-dir $(ANDROID_SDK_HOME) --ndk-dir $(ANDROID_NDK_HOME) \ --requirements libffi,sdl2,pyjnius,kivy,python3,openssl,requests,urllib3,chardet,idna,sqlite3,setuptools,numpy \ - --arch=armeabi-v7a --arch=arm64-v8a --arch=x86_64 --arch=x86 + --arch=armeabi-v7a --arch=arm64-v8a --arch=x86_64 --arch=x86 \ + --permission "(name=android.permission.WRITE_EXTERNAL_STORAGE;maxSdkVersion=18)" --permission "(name=android.permission.INTERNET)" testapps-with-scipy: virtualenv . $(ACTIVATE) && cd testapps/on_device_unit_tests/ && \ @@ -53,7 +54,8 @@ testapps-with-numpy-aab: virtualenv . $(ACTIVATE) && cd testapps/on_device_unit_tests/ && \ python setup.py aab --sdk-dir $(ANDROID_SDK_HOME) --ndk-dir $(ANDROID_NDK_HOME) \ --requirements libffi,sdl2,pyjnius,kivy,python3,openssl,requests,urllib3,chardet,idna,sqlite3,setuptools,numpy \ - --arch=armeabi-v7a --arch=arm64-v8a --arch=x86_64 --arch=x86 --release + --arch=armeabi-v7a --arch=arm64-v8a --arch=x86_64 --arch=x86 --release \ + --permission "(name=android.permission.WRITE_EXTERNAL_STORAGE;maxSdkVersion=18)" --permission "(name=android.permission.INTERNET)" testapps-service_library-aar: virtualenv . $(ACTIVATE) && cd testapps/on_device_unit_tests/ && \ diff --git a/doc/source/buildoptions.rst b/doc/source/buildoptions.rst index 05ce4ef699..d25a529946 100644 --- a/doc/source/buildoptions.rst +++ b/doc/source/buildoptions.rst @@ -64,9 +64,24 @@ options (this list may not be exhaustive): ``android:screenOrientation`` in the `Android documentation `__. - ``--icon``: A path to the png file to use as the application icon. -- ``--permission``: A permission name for the app, - e.g. ``--permission VIBRATE``. For multiple permissions, add - multiple ``--permission`` arguments. +- ``--permission``: A permission that needs to be declared into the App ``AndroidManifest.xml``. + For multiple permissions, add multiple ``--permission`` arguments. + + .. Note :: + ``--permission`` accepts the following syntaxes: + ``--permission (name=android.permission.WRITE_EXTERNAL_STORAGE;maxSdkVersion=18)`` + or ``--permission android.permission.WRITE_EXTERNAL_STORAGE``. + + The first syntax is used to set additional properties to the permission + (``android:maxSdkVersion`` and ``android:usesPermissionFlags`` are the only ones supported for now). + + The second one can be used when there's no need to add any additional properties. + + .. Warning :: + The syntax ``--permission VIBRATE`` (only the permission name, without the prefix), + is also supported for backward compatibility, but it will be removed in the future. + + - ``--meta-data``: Custom key=value pairs to add in the application metadata. - ``--presplash``: A path to the image file to use as a screen while the application is loading. diff --git a/pythonforandroid/bootstraps/common/build/build.py b/pythonforandroid/bootstraps/common/build/build.py index c49d18fc4d..110c0464ea 100644 --- a/pythonforandroid/bootstraps/common/build/build.py +++ b/pythonforandroid/bootstraps/common/build/build.py @@ -53,10 +53,6 @@ def get_bootstrap_name(): curdir = dirname(__file__) -PYTHON = get_hostpython() -if PYTHON is not None and not exists(PYTHON): - PYTHON = None - BLACKLIST_PATTERNS = [ # code versionning '^*.hg/*', @@ -75,9 +71,19 @@ def get_bootstrap_name(): ] WHITELIST_PATTERNS = [] -if get_bootstrap_name() in ('sdl2', 'webview', 'service_only'): - WHITELIST_PATTERNS.append('pyconfig.h') +if os.environ.get("P4A_BUILD_IS_RUNNING_UNITTESTS", "0") != "1": + PYTHON = get_hostpython() + _bootstrap_name = get_bootstrap_name() +else: + PYTHON = "python3" + _bootstrap_name = "sdl2" + +if PYTHON is not None and not exists(PYTHON): + PYTHON = None + +if _bootstrap_name in ('sdl2', 'webview', 'service_only'): + WHITELIST_PATTERNS.append('pyconfig.h') environment = jinja2.Environment(loader=jinja2.FileSystemLoader( join(curdir, 'templates'))) @@ -646,6 +652,44 @@ def make_package(args): subprocess.check_output(patch_command) +def parse_permissions(args_permissions): + if args_permissions and isinstance(args_permissions[0], list): + args_permissions = [p for perm in args_permissions for p in perm] + + def _is_advanced_permission(permission): + return permission.startswith("(") and permission.endswith(")") + + def _decode_advanced_permission(permission): + SUPPORTED_PERMISSION_PROPERTIES = ["name", "maxSdkVersion", "usesPermissionFlags"] + _permission_args = permission[1:-1].split(";") + _permission_args = (arg.split("=") for arg in _permission_args) + advanced_permission = dict(_permission_args) + + if "name" not in advanced_permission: + raise ValueError("Advanced permission must have a name property") + + for key in advanced_permission.keys(): + if key not in SUPPORTED_PERMISSION_PROPERTIES: + raise ValueError( + f"Property '{key}' is not supported. " + "Advanced permission only supports: " + f"{', '.join(SUPPORTED_PERMISSION_PROPERTIES)} properties" + ) + + return advanced_permission + + _permissions = [] + for permission in args_permissions: + if _is_advanced_permission(permission): + _permissions.append(_decode_advanced_permission(permission)) + else: + if "." in permission: + _permissions.append(dict(name=permission)) + else: + _permissions.append(dict(name=f"android.permission.{permission}")) + return _permissions + + def parse_args_and_make_package(args=None): global BLACKLIST_PATTERNS, WHITELIST_PATTERNS, PYTHON @@ -918,8 +962,7 @@ def _read_configuration(): 'deprecated and does nothing.') args.sdk_version = -1 # ensure it is not used - if args.permissions and isinstance(args.permissions[0], list): - args.permissions = [p for perm in args.permissions for p in perm] + args.permissions = parse_permissions(args.permissions) if args.res_xmls and isinstance(args.res_xmls[0], list): args.res_xmls = [x for res in args.res_xmls for x in res] @@ -959,4 +1002,6 @@ def _read_configuration(): if __name__ == "__main__": + if get_bootstrap_name() in ('sdl2', 'webview', 'service_only'): + WHITELIST_PATTERNS.append('pyconfig.h') parse_args_and_make_package() diff --git a/pythonforandroid/bootstraps/sdl2/build/templates/AndroidManifest.tmpl.xml b/pythonforandroid/bootstraps/sdl2/build/templates/AndroidManifest.tmpl.xml index b5ddde3874..d33731f9a8 100644 --- a/pythonforandroid/bootstraps/sdl2/build/templates/AndroidManifest.tmpl.xml +++ b/pythonforandroid/bootstraps/sdl2/build/templates/AndroidManifest.tmpl.xml @@ -24,14 +24,9 @@ - - + {% for perm in args.permissions %} - {% if '.' in perm %} - - {% else %} - - {% endif %} + {% endfor %} {% if args.wakelock %} diff --git a/pythonforandroid/bootstraps/service_only/build/templates/AndroidManifest.tmpl.xml b/pythonforandroid/bootstraps/service_only/build/templates/AndroidManifest.tmpl.xml index d19ed32931..ab410330f2 100644 --- a/pythonforandroid/bootstraps/service_only/build/templates/AndroidManifest.tmpl.xml +++ b/pythonforandroid/bootstraps/service_only/build/templates/AndroidManifest.tmpl.xml @@ -20,11 +20,7 @@ {% for perm in args.permissions %} - {% if '.' in perm %} - - {% else %} - - {% endif %} + {% endfor %} {% if args.wakelock %} diff --git a/pythonforandroid/bootstraps/webview/build/templates/AndroidManifest.tmpl.xml b/pythonforandroid/bootstraps/webview/build/templates/AndroidManifest.tmpl.xml index f77533b1e6..d9ef93ac8d 100644 --- a/pythonforandroid/bootstraps/webview/build/templates/AndroidManifest.tmpl.xml +++ b/pythonforandroid/bootstraps/webview/build/templates/AndroidManifest.tmpl.xml @@ -21,11 +21,7 @@ {% for perm in args.permissions %} - {% if '.' in perm %} - - {% else %} - - {% endif %} + {% endfor %} {% if args.wakelock %} diff --git a/tests/test_bootstrap_build.py b/tests/test_bootstrap_build.py new file mode 100644 index 0000000000..4d699c6283 --- /dev/null +++ b/tests/test_bootstrap_build.py @@ -0,0 +1,86 @@ +import unittest +import os +import argparse + +from pythonforandroid.util import load_source + + +class TestParsePermissions(unittest.TestCase): + def test_parse_permissions_with_migrations(self): + # Test that permissions declared in the old format are migrated to the + # new format. + # (Users can new declare permissions in both formats, even a mix) + os.environ["P4A_BUILD_IS_RUNNING_UNITTESTS"] = "1" + + ap = argparse.ArgumentParser() + ap.add_argument( + "--permission", + dest="permissions", + action="append", + default=[], + help="The permissions to give this app.", + nargs="+", + ) + + args = [ + "--permission", + "INTERNET", + "--permission", + "com.android.voicemail.permission.ADD_VOICEMAIL", + "--permission", + "(name=android.permission.WRITE_EXTERNAL_STORAGE;maxSdkVersion=18)", + "--permission", + "(name=android.permission.BLUETOOTH_SCAN;usesPermissionFlags=neverForLocation)", + ] + + args = ap.parse_args(args) + + build_src = os.path.join( + os.path.dirname(os.path.abspath(__file__)), + "../pythonforandroid/bootstraps/common/build/build.py", + ) + + buildpy = load_source("buildpy", build_src) + parsed_permissions = buildpy.parse_permissions(args.permissions) + + assert parsed_permissions == [ + dict(name="android.permission.INTERNET"), + dict(name="com.android.voicemail.permission.ADD_VOICEMAIL"), + dict(name="android.permission.WRITE_EXTERNAL_STORAGE", maxSdkVersion="18"), + dict( + name="android.permission.BLUETOOTH_SCAN", + usesPermissionFlags="neverForLocation", + ), + ] + + def test_parse_permissions_invalid_property(self): + os.environ["P4A_BUILD_IS_RUNNING_UNITTESTS"] = "1" + + ap = argparse.ArgumentParser() + ap.add_argument( + "--permission", + dest="permissions", + action="append", + default=[], + help="The permissions to give this app.", + nargs="+", + ) + + args = [ + "--permission", + "(name=android.permission.BLUETOOTH_SCAN;propertyThatFails=neverForLocation)", + ] + + args = ap.parse_args(args) + + build_src = os.path.join( + os.path.dirname(os.path.abspath(__file__)), + "../pythonforandroid/bootstraps/common/build/build.py", + ) + + buildpy = load_source("buildpy", build_src) + + try: + parsed_permissions = buildpy.parse_permissions(args.permissions) # noqa: F841 + except ValueError as _e: + assert "Property 'propertyThatFails' is not supported." in str(_e) diff --git a/tests/test_build.py b/tests/test_build.py index 6d30f996e7..f386b8410f 100644 --- a/tests/test_build.py +++ b/tests/test_build.py @@ -64,7 +64,10 @@ def test_android_manifest_xml(self): args.min_sdk_version = 12 args.build_mode = 'debug' args.native_services = ['abcd', ] - args.permissions = [] + args.permissions = [ + dict(name="android.permission.INTERNET"), + dict(name="android.permission.WRITE_EXTERNAL_STORAGE", maxSdkVersion=18), + dict(name="android.permission.BLUETOOTH_SCAN", usesPermissionFlags="neverForLocation")] args.add_activity = [] args.android_used_libs = [] args.meta_data = [] @@ -91,6 +94,9 @@ def test_android_manifest_xml(self): assert xml.count('targetSdkVersion="1234"') == 1 assert xml.count('android:debuggable="true"') == 1 assert xml.count('') == 1 + assert xml.count('') == 1 + assert xml.count('') == 1 + assert xml.count('') == 1 # TODO: potentially some other checks to be added here to cover other "logic" (flags and loops) in the template From 1aa934375b1cfc177ecdde0c48a61c4eaca3502f Mon Sep 17 00:00:00 2001 From: Mirko Galimberti Date: Fri, 30 Dec 2022 13:52:17 +0100 Subject: [PATCH 2/2] Fix test for ValueError --- tests/test_bootstrap_build.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/test_bootstrap_build.py b/tests/test_bootstrap_build.py index 4d699c6283..03a5f79f3e 100644 --- a/tests/test_bootstrap_build.py +++ b/tests/test_bootstrap_build.py @@ -1,4 +1,5 @@ import unittest +import pytest import os import argparse @@ -80,7 +81,7 @@ def test_parse_permissions_invalid_property(self): buildpy = load_source("buildpy", build_src) - try: - parsed_permissions = buildpy.parse_permissions(args.permissions) # noqa: F841 - except ValueError as _e: - assert "Property 'propertyThatFails' is not supported." in str(_e) + with pytest.raises( + ValueError, match="Property 'propertyThatFails' is not supported." + ): + buildpy.parse_permissions(args.permissions)