Leonidius20 / RecordingStudio

Audio recorder app for Android
GNU General Public License v3.0
95 stars 4 forks source link

Remove unnecessary permissions #1

Open Glitchy-Tozier opened 2 months ago

Glitchy-Tozier commented 2 months ago

Hi, your app looks great! :) I was wondering whether all these permissions are required. Especially "view network connections" seems unnecessary. There's also this one where I have no idea what it does "io.github.leonidius20.recorder.lite.DYNAMIC_RECEIVER_NOT_EXPORTED_PERMISSION"

I suggest removing as many unnecessary permissions as possible, for increased transparency and simplicity.

In case it helps, here's the full list as shown on F-Droid: Screenshot_20240907-144034_Fennec.jpg

Leonidius20 commented 2 months ago

Hello and thank you for your feedback! Let's tackle permissions one by one: RECORD_AUDIO - record audio :) FOREGROUND_SERVICE - needed so that sound recording and playback happens in a foreground service and doesn't get killed by the system if the user puts the app in the background FOREGROUND_SERVICE_MICROPHONE - on newer Android versions, apart from the foreground service permission, the app also has to declare a permission for a specific type of service that it starts. In this case, sound recording service FOREGROUND_SERVICE_MEDIA_PLAYBACK - same thing but for recordings playback READ_EXTERNAL_STORAGE - needed on older Android versions to read the list of recordings. On newer Android versions the app does not request this permission in runtime. WRITE_EXTERNAL_STORAGE - needed on older Android versions to save recording files. Similarly on newer Android versions the app doesn't request it in runtime POST_NOTIFICATIONS - needed so that a notification can be shown if a recording was stopped because of low battery/storage/incoming call. I believe on newer Android versions it is also needed to display the persistent notification while recording is in progress READ_PHONE_STATE - needed so that the app can get a broadcast when there is an incoming phone call and pause the recording. It is only requested if you activate the corresponding feature in the settings, it is off by default. ACCESS_NETWORK_STATE - is in fact not needed. It was added by some library. I am going to release an update that removes this permission soon. DYNAMIC_RECEIVER_NOT_EXPORTED_PERMISSION - is not a real permission. It means that the app registers a broadcast receiver (in this case, for controlling recording from the persistent notification), that does not accept broadcasts from other apps, only from the app itself. You can read more about it here https://gitlab.com/fdroid/fdroidserver/-/merge_requests/1336

Glitchy-Tozier commented 2 months ago

Thank you for the full explanation! :) I wasn't aware that some older android versions required additional permissions.

ACCESS_NETWORK_STATE - is in fact not needed. It was added by some library. I am going to release an update that removes this permission soon.

For some reason, libraries adding network permission happens quite frequently. I think in the case of Flutter it's to enable live debugging, but it might be a different story with Kotlin. Anyway, since this commit sounds like the network permission was removed, should we close the issue?

Leonidius20 commented 2 months ago

i will close the issue. the new version is now available to download in the Releases section and will be available on f-droid at some point (it seems to take a long time to update there) if you find any other issues, feel free to open a new issue.

IzzySoft commented 1 month ago

READ_PHONE_STATE - needed so that the app can get a broadcast when there is an incoming phone call and pause the recording. It is only requested if you activate the corresponding feature in the settings, it is off by default.

Well, that's one way to do it – with the side-effect of getting access to sensible information the app won't need, and thus "look suspicious for no reason". An alternative would be to register to the OnAudioFocusChangeListener – as the audio channel switches to "ringer" on incoming calls. Quoting:

The focusChange value indicates whether the focus was gained, whether the focus was lost, and whether that loss is transient, or whether the new focus holder will hold it for an unknown amount of time. When losing focus, listeners can use the focus change information to decide what behavior to adopt when losing focus. A music player could for instance elect to lower the volume of its music stream (duck) for transient focus losses, and pause otherwise.

I'd say that exactly describes your use case :wink:

Reason of my chiming in to this already closed issue: The scanners at IzzyOnDroid just reported this:

! repo/io.github.leonidius20.recorder.lite_5.apk declares sensitive permission(s):
  android.permission.READ_PHONE_STATE

Would you mind switching to this "less invasive" approach, @Leonidius20? Thanks in advance!

Leonidius20 commented 1 month ago

@IzzySoft thank you for the suggestion, I will look into it. It probably won't be in the next version though, as there are some higher priority plans i wanted to realize.

It is weird that the scanner was only triggered now though, as the app was using this approach since the very first version

IzzySoft commented 1 month ago

I will look into it.

Thanks!

It probably won't be in the next version though, as there are some higher priority plans i wanted to realize.

Your app – your pace, of course. It's not that it's an extremely critical thing.

It is weird that the scanner was only triggered now though, as the app was using this approach since the very first version

Not weird at all, as that scan is still triggered only when a new version comes in – to not flood my inbox: with more than 1.2k apps, a full scan would be hard to handle, "information overflow". So until that has "simmered down", it will only be detected with new releases. We'll do another "full scan" later, as soon as time permits, to see what can be done for "older apps" – and once that's done, the approach of "with new releases only" is fine as long as the list of "sensitive permissions" stays unchanged.

Sensitive permissions are always highlighted (with the repo browser at least), so this is more about transparency ("why is this permission needed?") – of course with the not-so-rare side effect of "is it indeed needed?", which in many cases lead to getting permissions removed. Good again for privacy and security :smiley: