Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include application permissions in Android events #2018

Merged
merged 12 commits into from
May 13, 2022
Merged

Conversation

romtsn
Copy link
Member

@romtsn romtsn commented May 4, 2022

📜 Description

  • Reads granted/non-granted permissions in the event processor
  • Adds them as part of the App object of the protocol. For now uses the unknown field.

image

Some open questions

  • Should we change protocol to account for permissions field?
  • Should this target 6.x.x or rather main? (would require some changed for the unknown map)
  • Is it fine to have full permission name (as in, android.permission.CAMERA) as key for the map or should make it pretty?
  • Should we change granted/not_granted to something different?

💡 Motivation and Context

Closes #1347

@marandaneto
Copy link
Contributor

I'd target every new PR into 6.x.x.

@marandaneto
Copy link
Contributor

Once this is done/merged, let's add this to the feature list in the docs https://docs.sentry.io/platforms/android/

@codecov-commenter
Copy link

Codecov Report

Merging #2018 (9288b35) into 6.x.x (e91f397) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              6.x.x    #2018   +/-   ##
=========================================
  Coverage     80.79%   80.79%           
  Complexity     3146     3146           
=========================================
  Files           228      228           
  Lines         11645    11645           
  Branches       1565     1565           
=========================================
  Hits           9409     9409           
  Misses         1649     1649           
  Partials        587      587           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e91f397...9288b35. Read the comment docs.

CHANGELOG.md Outdated Show resolved Hide resolved
@romtsn romtsn marked this pull request as ready for review May 12, 2022 09:11
@romtsn
Copy link
Member Author

romtsn commented May 12, 2022

@marandaneto there were no folks from ingest in the TSC, but the general vibe was that it should be part of the app context (since it was designed for those kind of things). Also agreed that it's fine to send it as other|unknown for now, since it's forwards compatible and if a new field will be introduced later on, we will just switch to sending this as the new field.

2 additional things:

  • Test how long does it take to retrieve the packageInfo and permissions - takes half-a-millisecond, so I think we are fine here
  • Switched back to granted|not_granted to align with iOS; if iOS ever decides to implement this, they will have those two + partial in addition

@marandaneto marandaneto changed the title Feat/report permissions Include application permissions in Android events May 13, 2022
@romtsn romtsn merged commit d4c250a into 6.x.x May 13, 2022
@romtsn romtsn deleted the feat/report-permissions branch May 13, 2022 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants