getsentry / sentry-cocoa

The official Sentry SDK for iOS, tvOS, macOS, watchOS.
https://sentry.io/for/cocoa/
MIT License
797 stars 315 forks source link

Include app permissions in events #1014

Closed bruno-garcia closed 2 years ago

bruno-garcia commented 3 years ago

Events should include App's allowed/disallowed permissions.

Android counterpart: https://github.com/getsentry/sentry-java/issues/1347

brustolin commented 2 years ago

We have permission levels on iOS. Lets say, location permission, it may be "all the time" or "only while using the app" Photos permissions may be "all" our "restricted".

How are we dealing with this nuances? Treat it as granted?

brustolin commented 2 years ago

In iOS we don't have a collection of permissions that can be query all at once. We need to use each permission API to check for it. To run all checks it can take up to 20ms, which is unacceptable to run for every event.

This are the permissions in iOS that we can check:

//location service
CLLocationManager.authorizationStatus

//Push notification 
//Deprecated from iOS 10, but we are targeting iOS 9.
//But the new solution with UNUserNotificationService uses an async method to query for permission
UIApplication.sharedApplication.currentUserNotificationSettings

//bluetooth
@available(iOS 13.1,macOS 10.15, *)
CBManager.authorization

//microphone
[AVCaptureDevice authorizationStatusForMediaType:AVMediaTypeAudio]

//camera
[AVCaptureDevice authorizationStatusForMediaType:AVMediaTypeVideo]

//contacts    
[CNContactStore authorizationStatusForEntityType:CNEntityTypeContacts]

//calendar    
[EKEventStore authorizationStatusForEntityType:EKEntityTypeEvent]

//media library    
MPMediaLibrary.authorizationStatus

//motion
@available(iOS 11.0, *)
CMMotionActivityManager.authorizationStatus

//photo library
PHPhotoLibrary.authorizationStatus

//speech
SFSpeechRecognizer.authorizationStatus

//tracking
@available(iOS 14, *)
ATTrackingManager.trackingAuthorizationStatus

this are the imports required

#import <CoreLocation/CoreLocation.h>
#import <CoreBluetooth/CoreBluetooth.h>
#import <AVFoundation/AVFoundation.h>
#import <Contacts/Contacts.h>
#import <EventKit/EventKit.h>
#import <MediaPlayer/MediaPlayer.h>
#import <CoreMotion/CoreMotion.h>
#import <Photos/Photos.h>
#import <Intents/Intents.h>
#import <Speech/Speech.h>
#import <AppTrackingTransparency/AppTrackingTransparency.h>
#import <UserNotifications/UserNotifications.h>
philipphofmann commented 2 years ago

Android already has this. They add this to the app context:

"contexts": {
    "app": {
      "app_start_time": "2022-07-14T12:38:41.995Z",
      "app_identifier": "io.sentry.samples.android",
      "app_name": "Sentry sample",
      "app_version": "1.1.0",
      "app_build": "2",
      "permissions": {
        "ACCESS_NETWORK_STATE": "granted",
        "CAMERA": "not_granted",
        "FOREGROUND_SERVICE": "granted",
        "INTERNET": "granted",
        "READ_EXTERNAL_STORAGE": "not_granted",
        "READ_PHONE_STATE": "not_granted",
        "WRITE_EXTERNAL_STORAGE": "not_granted"
      },
      "type": "app"
    },
    ...

Example event https://sentry.io/organizations/sentry-sdks/issues/3338677814/events/6b5164b6990546e6823c7711e15bbcb0/?project=5428559

kevinrenskers commented 2 years ago

To run all checks it can take up to 20ms, which is unacceptable to run for every event.

What would be acceptable? If we can define a subset of the most important permissions (I'd say location, push permission, photo library), and we measure how long that takes, what does it need to stay under to be acceptable?

In another issue I saw something about fetching some info taking 5 ms and that was considered unacceptable, which leads me to think that none of these permission checks are going to be acceptable? And it's not like we can fetch these once at SDK start and use that for every event, as obviously these permissions can change during the lifetime of the app. Unless there's some kind of system notification that tells us that a certain permission has been changed? 🤔

brustolin commented 2 years ago

Maybe reading this permissions is not the way to got, we should use some kind of cache and listening to permission changes during the execution.

philipphofmann commented 2 years ago

Maybe reading this permissions is not the way to got, we should use some kind of cache and listening to permission changes during the execution.

Yes, I agree. If we can't listen for changes, I would say it's not worth the effort at the moment. Enriching the event with all permissions on a background thread would work, but this would require some refactoring. We want to write the event to disk as quickly as possible to avoid losing it if the app crashes. We could write the event to disk quickly and then add further information to it on a background thread, but this is a major change in our current architecture and would require some rework, which is not worth the effort for just this feature.

It's also fine to say this issue is not worth the effort at the moment, and we lower the priority.

kevinrenskers commented 2 years ago

I had the same idea about listening for changes, and am looking into that now.