feat: Include app permissions with event

[kevinrenskers]

📜 Description

We're now including the status of some permissions, namely for Push Notifications, Location access, Media Library and Photo Library. This is added to the app context in every event. These permissions are cached.

We can easily add more permissions, but I figured we can do that in a separate PR, let's get this merged first if we like this approach. Also it would be a lot easier to add more permission once we drop iOS 9 😅

💡 Motivation and Context

Closes #1014

💚 How did you test it?

With the sample app and unit tests.

📝 Checklist

• I reviewed the submitted code
• I added tests to verify the changes
• I updated the docs if needed
• Review from the native team if needed
• No breaking changes

🔮 Next steps

[github-actions]

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 708286a

[kevinrenskers]

This is still very much a Work In Progress, tests won't compile etc etc. But it would good to get an initial first look at this. The way I implemented this is with a SentryPermissionsObserver class which acts as the "cache", and it updates its values when the system permissions change.

There is one problem with this though: most of the system permissions do not have a way to observe permission changes. CLLocationManager is kind of unique in that you can assign a delegate and from then on locationManagerDidChangeAuthorization will be called. But for example for Push Notification permissions, there is nothing like that, we can only fetch the status on demand, which is an async method at that. So right now I solved it by listening to UIApplicationDidBecomeActiveNotification to update the value. but that's not a full solution, because if the app asks for push permissions and the user simply gives it in the native alert, UIApplicationDidBecomeActiveNotification is not called.

And it's the same with most of the other system permissions: you can read them on demand, but there is no way to be notified of changes. So what do we do? Reading them for every event will take too long (20 ms), so that's not an option. But sending the wrong values is arguably even worse.

[kevinrenskers]

Alrighty, the UIApplicationDidBecomeActiveNotification notification is actually called when you grant permissions in the normal happy path flow. I simply was adding the permissions to the context only once at SDK start-up, instead of always for every event, so the values weren't updating. Now it all works.

[brustolin]

Alrighty, the UIApplicationDidBecomeActiveNotification notification is actually called when you grant permissions in the normal happy path flow

That's interesting. If every time a permission popup disappear we receive a UIApplicationDidBecomeActiveNotification this might be the solution for almost every permission.

If that is not the case, we could use swizzling to hook into the "notifications".

[kevinrenskers]

276 test failures in macOS? And I can't run them locally because the test always crashes with The file “io.sentry” couldn’t be saved in the folder “Caches”.

[kevinrenskers]

I am getting some UI test failures, but all the other stuff (finally!) succeeds. I am surprised by the UI test failures though, as I haven't changed those tests, and only added a button to the home screen which isn't tested in any way.

[balavor]

I don't have existing examples, but we've experienced floating crashes inside app extensions, where some features are not expected to be used. Those have been fixed by now, but there might be new cases, so I wish we could have complete control of those requests using our feature toggles on the device.

[kevinrenskers]

@philipphofmann @brustolin do you think we should add an option for this? And should it default to on or off?

[philipphofmann]

Hey there,

Could you please share if there'd be a toggle to turn off that feature? It is something that may return invalid results or even crash. Both cases are primarily applicable to iOS app extensions.

We don't have a toggle for that feature on Android getsentry/sentry-java#2018, and I think we shouldn't add one if we can safely write the code so it doesn't crash on app extensions. Would that be possible, @kevinrenskers?

[kevinrenskers]

Would these permission checks crash in extensions? Is that documented somewhere? I need to look into this. We're just reading permissions, I can hardly believe this would cause a crash 🤔

I guess I'll create an extension and read these permissions, see what happens. As I've never built an extension before, that'll be a fun exercise by itself :)

[balavor]

Yeah, we have an app extension as our primary product on iOS instead of the app itself, so I just want to highlight that 🙃

[kevinrenskers]

I've created an IntentUI extension and a Widget extension, and in neither of them did accessing any of these permissions properties cause any problems. Personally I don't think this would ever crash, but just to be sure I added a check to only read permissions when we're running the main app.

[brustolin]

I added a check to only read permissions when we're running the main app

I like this solution, but please create an issue for us to investigate this further in the future, I think this feature could be useful for extensions, especially widgets.

[armcknight]

This is looking good. I wonder if we couldn't also gather info on the existence of required Plist keys for various permissions requests. The absence of a required perm key in the plist can lead to crashes, so that'd be a quick win diagnostic.

I'm not sure if we can also check for required entitlements in the bundle at runtime, but that's another avenue worth exploring.

See

• https://developer.apple.com/documentation/uikit/protecting_the_user_s_privacy/requesting_access_to_protected_resources
• https://www.iosdev.recipes/info-plist/permissions/

[kevinrenskers]

Asking for permission without the correct things in the info.plist will cause a crash, but merely checking if we have permission doesn't cause a crash (in my testing).

[armcknight]

Oh, I wasn't worrying about us causing a crash, more thinking that we could use this as a diagnostic if the app caused such a crash. Do we [plan to] swizzle any CLLocation API?

[kevinrenskers]

That might be a nice future feature to add, can you make an issue to keep track of that? I don't think it should necessarily go into this PR. I was really hoping to merge this soon so we can build on top of it.

[kevinrenskers]

I added a check to only read permissions when we're running the main app

I like this solution, but please create an issue for us to investigate this further in the future, I think this feature could be useful for extensions, especially widgets.

#2026

[kevinrenskers]

One failure in a benchmarking test. How can I run this locally? And what is the implication? That this new feature is adding overhead?

[brustolin]

You can run the test under iOS-Swift sample project, "PerformanceBenchmarks > SentrySDKPerformanceBenchmarkTests.m".
This was added by the profiling team, Im not really sure what it does.
Can you figure it out whether this is flack and we should ping then, or is something that was added to the PR? Thanks!!

[kevinrenskers]

Damn, this is the most annoying test to run, ever. It takes forever. The error it runs into on CI is that usagePercentage is equal to zero, and it expects it to be higher than zero. This is definitely unrelated to this PR.

[kevinrenskers]

Pushed another commit without changes and yea now they succeed just fine. I think the profiling team needs to look at their tests.

[brustolin]

LGTM