rRemix / APlayer

Android Music Player
GNU General Public License v3.0
1.34k stars 176 forks source link

Question on permissions #227

Closed IzzySoft closed 6 months ago

IzzySoft commented 7 months ago

My scanner recently got additional checks implemented, and on your latest release reported:

! repo/remix.myplayer_16201.apk declares sensitive permission(s):
  android.permission.MANAGE_EXTERNAL_STORAGE android.permission.REQUEST_INSTALL_PACKAGES
  android.permission.READ_EXTERNAL_STORAGE android.permission.SYSTEM_ALERT_WINDOW
  android.permission.READ_MEDIA_AUDIO android.permission.READ_MEDIA_IMAGES
! repo/remix.myplayer_16201.apk contains signature block blobs: 0x504b4453 (DEPENDENCY_INFO_BLOCK; GOOGLE)

Now I could easily align READ_EXTERNAL_STORAGE and READ_MEDIA_AUDIO being needed to access the audio files to play. But could you please clarify what the other permissions are needed for? Thanks in advance!

Btw, the DEPENDENCY_INFO_BLOCK can easily be avoided:

android {
    dependenciesInfo {
        // Disables dependency metadata when building APKs.
        includeInApk = false
        // Disables dependency metadata when building Android App Bundles.
        includeInBundle = false
    }
}

For some background: that BLOB is supposed to be just a binary representation of your app's dependency tree. But as it's encrypted with a public key belonging to Google, only Google can read it – and nobody else can even verify what it really contains.

rRemix commented 7 months ago

android.permission.MANAGE_EXTERNAL_STORAGE, such as reading lyric file, blacklist android.permission.SYSTEM_ALERT_WINDOW, desktop lyric android.permission.REQUEST_INSTALL_PACKAGES, in-app upgrade, but it's only available in non-google channel

Henry-ZHR commented 7 months ago

MANAGE_EXTERNAL_STORAGE is initially for "Manual scan" function (users may want to scan files outside standard directories), and it may also solve some permission-related issues on some Chinese systems. It's not necessary for everyone.

IzzySoft commented 7 months ago

in-app upgrade, but it's only available in non-google channel

It's been reported for the APK used in my repo. And self-updater are not in accordance with the repo's inclusion criteria – as those updates would bypass the checks performed in the repo. May I ask how it is configured? Enabled by default? Opt-in (so with clear consent)? Explaining the source of the update and the implications (such as bypassing repo-checks)?

Henry-ZHR commented 6 months ago

DEPENDENCY_INFO_BLOCK excluded in b1f990191175ba7fb30a371b17e70017a3d7dec8

IzzySoft commented 6 months ago

Wonderful, thanks! So what's with REQUEST_INSTALL_PACKAGES now (see my last comment)?

Henry-ZHR commented 6 months ago

I suggest adding a new flavor which disables the self-updater (and maybe the non-free dependencies?)

Anyway, @rRemix is the only owner of this repo :)

IzzySoft commented 6 months ago

Thanks Henry! OK, so let's see what the owner says :smile:

rRemix commented 6 months ago

i'll add it

IzzySoft commented 6 months ago

Thanks, @rRemix! Please let me know when that flavor is available. I'd suggest to switch your app to that in my repo then. If it involves a new applicationId/packageName, is there a "migration path" (e.g. export/import settings)?

rRemix commented 6 months ago

@IzzySoft you can use flavor 'noUpdater'

Henry-ZHR commented 6 months ago

@rRemix 等下 我说开flavor是方便你那边直接release加一个 因为我以为他们是直接用你的apk 但是他们好像是自己build 那感觉不如开buildConfigField

@IzzySoft Are you building the app from source or using the binary from GitHub Release?

rRemix commented 6 months ago

先看他们怎么说,感觉是要自己build

IzzySoft commented 6 months ago

Are you building the app from source or using the binary from GitHub Release?

I use the APKs provided by their authors – so yes, I'd need the corresponding APK at releases. Thanks!

Henry-ZHR commented 6 months ago

@rRemix 那就还是放flavor吧

但是还有些小问题

  1. 大小写不能随便动 如果要动源码目录得跟着改

  2. 现在命名有点怪 感觉这仨flavor不是同级的 以及可以放另外的dimension?

rRemix commented 6 months ago

@rRemix 那就还是放flavor吧

但是还有些小问题

  1. 大小写不能随便动 如果要动源码目录得跟着改
  2. 现在命名有点怪 感觉这仨flavor不是同级的 以及可以放另外的dimension?

@Henry-ZHR 1.大小写的问题我已经改回去了 2.是有那么点怪,但是google渠道的又不存在是否有自动更新

Henry-ZHR commented 6 months ago

@rRemix

  1. 其实我也觉得用大小写分隔单词更合理 只是提醒你记得改源码目录 Linux文件系统一般区分大小写(

  2. https://developer.android.com/build/build-variants#filter-variants 可以filter掉google+updater组合?

以及 忘说了 应该其他都算非google渠道 如果在同一个dimension应该不用依赖billingclient?(在新的dimension就没有这个问题

IzzySoft commented 5 months ago

Today's release still has my scanner report

android.permission.REQUEST_INSTALL_PACKAGES android.permission.READ_MEDIA_IMAGES

So what did I miss concerning the latter? And will there be an APK for the NoUpdater flavor?

Henry-ZHR commented 5 months ago

Sorry, I forgot android.permission.REQUEST_INSTALL_PACKAGES. It will be included whether self-updater is enabled or not until #248 is merged.

IzzySoft commented 5 months ago

Thanks! So that means after that PR is merged, android.permission.REQUEST_INSTALL_PACKAGES will be gone? And what about android.permission.READ_MEDIA_IMAGES?

Henry-ZHR commented 5 months ago

READ_MEDIA_IMAGES may be needed to read the cover image of music

We need @rRemix or someone else who is familiar with Android permission system to check whether it's really necessary

IzzySoft commented 5 months ago

That would be one way to find out. If you have the chance, you could just compile an APK with that permission commented out and try that on-device (or EMU). AFAIK READ_MEDIA_IMAGES refers to photos (DCIM) and maybe screenshots, but not to images embedded in IDv3 or such – but I'm not 100% sure and thus would appreciate if someone could "tell definitely". Thanks for taking care!

Henry-ZHR commented 5 months ago

APlayer does read the cover not only directly from ID3 tag but also from MediaStore. For example: https://github.com/rRemix/APlayer/blob/3a0f4613523be47f7f169b36106e24ee4da6da7e/app/src/main/java/remix/myplayer/bean/mp3/Song.kt#L107

It should require READ_MEDIA_AUDIO only, but I'm not sure.