Question on permissions

IzzySoft commented 9 months ago

My APK checker just got a few improvements last week, so with today's update of PA it yielded the following warning:

! repo/com.github.frimtec.android.pikettassist_1634610.apk declares risky permission(s): android.permission.SYSTEM_ALERT_WINDOW android.permission.READ_CONTACTS android.permission.ACCESS_COARSE_LOCATION android.permission.READ_EXTERNAL_STORAGE*

Could you please clarify what those permissions are needed for? In case you wonder: READ_EXTERNAL_STORAGE is granted implicitly if WRITE_EXTERNAL_STORAGE is requested. As your app requires at least Android 8: are those older permissions still needed (e.g. because you need to access media files "owned" by a different app – which I doubt as I see no *_MEDIA_* permissions being requested)?

Thanks in advance for clarification!

frimtec commented 9 months ago

Hi @IzzySoft

Great to see, that you scan even more stuff in the apps on your app-store. Thanks a lot for the report of the permission warnings.

Let me go through the warned permissions one by one and explain their need for PAssist:

android.permission.SYSTEM_ALERT_WINDOW: PAssist alerts the user in case of an on-call alert, triggered by a received SMS from an operations center. The alert is raced similar to an incoming phone call. This alert screen must be shown on top of all other apps even on top of a potential lock screen. The user can then confirm the alert with a swipe button similar to accepting an incoming phone call (see screen-shot https://github.com/frimtec/pikett-assist/blob/master/images/PAssist_on_call_alert.png).
android.permission.READ_CONTACTS: This permission must be granted by the user during the setup process. PAssists uses a configured contact as operation center. All received SMS from phone numbers of this contact, trigger an on-call alert. Secondly on-call shifts from the calendar can contain partner ID's. PAssist is looking them up via the contact nicknames and displays the names of the on-call partners during the on-call shift (see screen-shot https://github.com/frimtec/pikett-assist/blob/master/images/PAssist_overview_activated.png).
android.permission.ACCESS_COARSE_LOCATION: This permission is only used until android 8.1 as configured in the AndroidManifest.xml. Maybe your scan could considering the android:maxSdkVersion attribute to prevent false positives. The permission on android prior 9 is required to supervise the telephone signal level during on-call service. PAssist alerts the user in case he is in a dead spot. As of android 9 there is a new API to gather the signal level without requesting any permissions.
android.permission.READ_EXTERNAL_STORAGE/WRITE_EXTERNAL_STORAGE This permission is only used until android 9 as configured in the AndroidManifest.xml. Maybe your scan could considering the android:maxSdkVersion attribute to prevent false positives. The permission is used to import/export call logs from/to the filesystem. As of android 10 this can be done without any special permissions.

Please let me know if there are any further clarifications required.

Best regards frimtec

IzzySoft commented 9 months ago

Great to see, that you scan even more stuff in the apps on your app-store. Thanks a lot for the report of the permission warnings.

:star_struck: Only positive responses up to now, but this one is the best yet, thanks! :star_struck:

SYSTEM_ALERT_WINDOW: PAssist alerts the user in case of an on-call alert

Ah, I should have thought of that. Yes, of course! => allow-list

PAssists uses a configured contact as operation center.

OK, makes sense. I thought those details would be stored in the app itself, but I can see it makes sense this way. => allow-list

Maybe your scan could considering the android:maxSdkVersion attribute to prevent false positives.

This is intentionally not done. Sensitive permissions are always leading to a warning, as the possibility of abuse/misuse/forgotten exists there as well. That's what the per-app allow-lists are for. Will be hard to track which permissions should be removed then as they were just added for compatibility (e.g. if one day PAssist sets minSdk to Android 10 and you forget to remove this permission), but I don't want to add too much complexity here.

The permission on android prior 9 is required

Prior to? I didn't check this specific one, but going by a lot of other ones (see android.permission.READ_EXTERNAL_STORAGE/WRITE_EXTERNAL_STORAGE) I had expected "Android 9 and below". But yeah: "PAssist alerts the user in case he is in a dead spot." => allow-list

android.permission.READ_EXTERNAL_STORAGE/WRITE_EXTERNAL_STORAGE: … used to import/export call logs from/to the filesystem

Apart from using "media storage" for that (which then with Android < 10 indeed would need these permissions), wouldn't this be covered by SAF without these permissions? And where to the exported call logs come from (or the imported go to) if the app doesn't request *_CALL_LOG? I guess you are speaking of something PAssist handles internally (i.e. its own records of events)?

So for now:

    PermIgnore:
      android.permission.SYSTEM_ALERT_WINDOW: This is needed to show a pop-up on incoming SMS alerts.
      android.permission.READ_CONTACTS: PAssists uses a configured contact as operation center.
      android.permission.ACCESS_COARSE_LOCATION: Up to Android 9, this is required to supervise the telephone signal level during on-call service. PAssist alerts the user in case they are in a dead spot.

Waiting for your confirmation on the call logs, then will add them there, too. Thanks a lot!

frimtec commented 9 months ago

Thanks for your feedback.

Here the precision on the permission READ_EXTERNAL_STORAGE/WRITE_EXTERNAL_STORAGE: You are right, call-log is a bad term in this context and can lead to misunderstandings. Even in the PAssist app itself, an other term is used "alert-log". As you guessed, it has nothing to do with the android call-logs and are PAssist internal records of all occurred alerts including all details to each case (see screen-shots https://github.com/frimtec/pikett-assist/blob/master/images/PAssist_alert_log_overview.png and https://github.com/frimtec/pikett-assist/blob/master/images/PAssist_alert_log_detail.png). The complete alert-log can be exported as a json file to the file system and re-imported from the file-system. I personally use it to automate time booking for my on-site efforts with my employer.

IzzySoft commented 9 months ago

And SAF cannot be used for this without the *_EXTERNAL_STORAGE permissions? Is it bound to a specific location in the file system not directly accessible this way?

frimtec commented 9 months ago

SAF says nothing to me right now. I will check this week if that would be an option.

frimtec commented 9 months ago

I indeed use SAF :-) I guess that is the reason why I don't need the permissions anymore as of android 10. I will test if the export/import also works without the permissions on android 8 and 9. If so I will remove the permission and build a new release.

frimtec commented 9 months ago

Tests were positive :-)

A new release https://github.com/frimtec/pikett-assist/releases/tag/2.11.4 is built with dropped permission for WRITE_EXTERNAL_STORAGE.

Thanks a lot for this improvement 👍

IzzySoft commented 9 months ago

Yay! And looks good:

$ iod repo get com.github.frimtec.android.pikettassist
com.github.frimtec.android.pikettassist: looking for 'https://api.github.com/repos/frimtec/pikett-assist/releases'
com.github.frimtec.android.pikettassist: checking tag '2.11.4'
com.github.frimtec.android.pikettassist: lastRelNo set to '2.11.4', checking for files
com.github.frimtec.android.pikettassist: Upstream file date (2024-01-23 15:11) is newer than ours (2024-01-22 19:43).
com.github.frimtec.android.pikettassist: returning ['2.11.4','https://github.com/frimtec/pikett-assist/releases/download/2.11.4/app-release.apk',1706019091]
com.github.frimtec.android.pikettassist: 2.11.3-Roboto/2.11.4, https://github.com/frimtec/pikett-assist/releases: https://github.com/frimtec/pikett-assist/releases/download/2.11.4/app-release.apk
- Grabbing update for com.github.frimtec.android.pikettassist: OK
- Checking 'repo/com.github.frimtec.android.pikettassist_1635849.apk' for libraries and malware …
- Checking the app's AndroidManifest.xml …
com.github.frimtec.android.pikettassist: check if repo contains FUNDING.yml
com.github.frimtec.android.pikettassist: looking for 'https://api.github.com/repos/frimtec/pikett-assist/contents/.github'
com.github.frimtec.android.pikettassist: looking for 'https://api.github.com/repos/frimtec/pikett-assist/contents/'
com.github.frimtec.android.pikettassist: looking for 'https://api.github.com/repos/frimtec/.github/contents/'
com.github.frimtec.android.pikettassist: Github reports "Not Found" for https://api.github.com/repos/frimtec/.github/contents/
com.github.frimtec.android.pikettassist: no FUNDING.yml detected.
com.github.frimtec.android.pikettassist: checking Fastlane for per-release changelogs
com.github.frimtec.android.pikettassist: cleaning up older changelogs
com.github.frimtec.android.pikettassist: fetched 'https://github.com/frimtec/pikett-assist/raw/master/fastlane/metadata/android/en-US/changelogs/1635849.txt'
com.github.frimtec.android.pikettassist: calling 'getFastlaneMeta(github,[host:github.com,owner:frimtec,repo:pikett-assist,path:/fastlane/metadata/android])'
com.github.frimtec.android.pikettassist: FastlaneFeatures title,shortdesc,fulldescMD,icon,screenshotsJPG
com.github.frimtec.android.pikettassist: looking for 'https://api.github.com/repos/frimtec/pikett-assist/contents/fastlane%2Fmetadata%2Fandroid'
com.github.frimtec.android.pikettassist: looking for 'https://api.github.com/repos/frimtec/pikett-assist/contents/fastlane%2Fmetadata%2Fandroid%2Fde'
com.github.frimtec.android.pikettassist: looking for 'https://api.github.com/repos/frimtec/pikett-assist/contents/fastlane%2Fmetadata%2Fandroid%2Fen-US'
com.github.frimtec.android.pikettassist: looking for 'https://api.github.com/repos/frimtec/pikett-assist/contents/fastlane%2Fmetadata%2Fandroid%2Fen-US%2Fchangelogs'
com.github.frimtec.android.pikettassist: looking for 'https://api.github.com/repos/frimtec/pikett-assist/contents/fastlane%2Fmetadata%2Fandroid%2Fen-US%2Fimages'
com.github.frimtec.android.pikettassist: looking for 'https://api.github.com/repos/frimtec/pikett-assist/contents/fastlane%2Fmetadata%2Fandroid%2Fen-US%2Fimages%2FphoneScreenshots'
com.github.frimtec.android.pikettassist: checking locale 'de'
com.github.frimtec.android.pikettassist: checking locale 'en-US'
com.github.frimtec.android.pikettassist: updating 'repo/com.github.frimtec.android.pikettassist/en-US/phoneScreenshots/PAssist_01.png' as JPG
com.github.frimtec.android.pikettassist: updating 'repo/com.github.frimtec.android.pikettassist/en-US/phoneScreenshots/PAssist_02.png' as JPG
com.github.frimtec.android.pikettassist: updating 'repo/com.github.frimtec.android.pikettassist/en-US/phoneScreenshots/PAssist_03.png' as JPG
com.github.frimtec.android.pikettassist: updating 'repo/com.github.frimtec.android.pikettassist/en-US/phoneScreenshots/PAssist_04.png' as JPG
com.github.frimtec.android.pikettassist: updating 'repo/com.github.frimtec.android.pikettassist/en-US/phoneScreenshots/PAssist_05.png' as JPG
com.github.frimtec.android.pikettassist: updating 'repo/com.github.frimtec.android.pikettassist/en-US/phoneScreenshots/PAssist_06.png' as JPG
com.github.frimtec.android.pikettassist: updating 'repo/com.github.frimtec.android.pikettassist/en-US/phoneScreenshots/PAssist_07.png' as JPG
com.github.frimtec.android.pikettassist: cross-checking for obsolete screenshots
com.github.frimtec.android.pikettassist: screenshots in Fastlane: PAssist_01,PAssist_02,PAssist_03,PAssist_04,PAssist_05,PAssist_06,PAssist_07
com.github.frimtec.android.pikettassist: local screenshots checked: PAssist_01,PAssist_02,PAssist_03,PAssist_04,PAssist_05,PAssist_06,PAssist_07

No more warnings at "Checking the app's AndroidManifest.xml", so all solved – thanks! :star_struck:

frimtec / pikett-assist

Question on permissions #508