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

fix: Remove all permission checks #2529

Merged
merged 4 commits into from
Dec 14, 2022

Conversation

kevinrenskers
Copy link
Contributor

@kevinrenskers kevinrenskers commented Dec 13, 2022

📜 Description

Remove the permissions. Not worth the risk of apps getting denied by Apple.

💡 Motivation and Context

Closes #2528
Closes #2527

💚 How did you test it?

Removing code and making the tests pass again.

📝 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

* 8.0.0:
  Remove the automatic `viewAppearing` span (#2511)
  Fix and reenable testANRDetected_UpdatesAppStateToTrue (#2526)
  fix: Don't add out of date context for crashes (#2523)
  ref: Rename isOOM to watchdog in Client (#2520)

# Conflicts:
#	Sources/Sentry/SentryClient.m
#	Tests/SentryTests/SentryClientTests.swift
@brustolin
Copy link
Contributor

Should we include a manual way to report permissions?

@kevinrenskers
Copy link
Contributor Author

I think as long as we have any code in the SDK at all that deal with these permissions people will keep getting these review problems. So I don't think we can ship this feature at all, unless it becomes like a separate package / pod.

@github-actions
Copy link

github-actions bot commented Dec 13, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1190.08 ms 1221.44 ms 31.36 ms
Size 20.75 KiB 401.81 KiB 381.05 KiB

Baseline results on branch: 8.0.0

Startup times

Revision Plain With Sentry Diff
91350d7 1230.82 ms 1253.26 ms 22.44 ms
f8045b6 1226.25 ms 1252.41 ms 26.16 ms
9be1db2 1219.42 ms 1245.66 ms 26.24 ms
6b8ec2a 1229.15 ms 1253.80 ms 24.65 ms
58ec104 1244.29 ms 1269.67 ms 25.39 ms
2ae7db9 1231.37 ms 1239.98 ms 8.61 ms
4dc66f6 1202.59 ms 1228.34 ms 25.75 ms
ae5ecd1 1226.73 ms 1250.96 ms 24.23 ms
dafc0fa 1254.41 ms 1274.31 ms 19.90 ms
4e1c9c0 1206.94 ms 1224.20 ms 17.26 ms

App size

Revision Plain With Sentry Diff
91350d7 20.75 KiB 383.89 KiB 363.14 KiB
f8045b6 20.75 KiB 404.83 KiB 384.08 KiB
9be1db2 20.75 KiB 373.94 KiB 353.19 KiB
6b8ec2a 20.75 KiB 405.10 KiB 384.34 KiB
58ec104 20.75 KiB 379.11 KiB 358.36 KiB
2ae7db9 20.75 KiB 381.87 KiB 361.12 KiB
4dc66f6 20.75 KiB 381.81 KiB 361.06 KiB
ae5ecd1 20.75 KiB 383.31 KiB 362.55 KiB
dafc0fa 20.75 KiB 383.87 KiB 363.12 KiB
4e1c9c0 20.75 KiB 383.87 KiB 363.12 KiB

Previous results on branch: fix/2528-remove-all-permission-checks

Startup times

Revision Plain With Sentry Diff
dca1617 1227.34 ms 1240.84 ms 13.50 ms
fae7433 1206.08 ms 1225.20 ms 19.12 ms

App size

Revision Plain With Sentry Diff
dca1617 20.75 KiB 401.81 KiB 381.05 KiB
fae7433 20.75 KiB 401.81 KiB 381.05 KiB

@codecov
Copy link

codecov bot commented Dec 13, 2022

Codecov Report

Merging #2529 (f66a5c8) into 8.0.0 (0c17d16) will increase coverage by 0.32%.
The diff coverage is 100.00%.

❗ Current head f66a5c8 differs from pull request most recent head d5b8106. Consider uploading reports for the commit d5b8106 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##            8.0.0    #2529      +/-   ##
==========================================
+ Coverage   78.46%   78.78%   +0.32%     
==========================================
  Files         241      241              
  Lines       22267    22129     -138     
  Branches     9839     9776      -63     
==========================================
- Hits        17471    17435      -36     
+ Misses       4345     4249      -96     
+ Partials      451      445       -6     
Impacted Files Coverage Δ
Sources/Sentry/SentryClient.m 98.44% <100.00%> (+2.66%) ⬆️
Sources/Sentry/SentryRequestOperation.m 67.56% <0.00%> (-10.82%) ⬇️
Sources/Sentry/SentryNetworkTracker.m 94.48% <0.00%> (-0.87%) ⬇️
Sources/Sentry/include/SentryHexAddressFormatter.h 100.00% <0.00%> (ø)
Sources/Sentry/SentryBacktrace.cpp 88.39% <0.00%> (+1.78%) ⬆️
Sources/Sentry/SentryThreadInspector.m 100.00% <0.00%> (+4.12%) ⬆️

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 0c17d16...d5b8106. Read the comment docs.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @kevinrenskers 👍. Please add a short entry to https://github.com/getsentry/sentry-cocoa/tree/8.0.0/develop-docs#decision-log why we removed the permissions. It can be one sentence. Do we need to update the docs?

@kevinrenskers kevinrenskers merged commit 22952c8 into 8.0.0 Dec 14, 2022
@kevinrenskers kevinrenskers deleted the fix/2528-remove-all-permission-checks branch December 14, 2022 07:41
@kevinrenskers
Copy link
Contributor Author

Done

@github-actions
Copy link

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Remove all permission checks ([#2529](https://github.com/getsentry/sentry-cocoa/pull/2529))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against d5b8106

@kevinrenskers
Copy link
Contributor Author

kevinrenskers commented Dec 14, 2022

Do we need to update the docs

I'll see if we have anything about that feature in the docs, and if so, create a docs PR.

Edit: it was never in the docs

kevinrenskers added a commit that referenced this pull request Dec 16, 2022
* 8.0.0: (31 commits)
  tests: Reenable testAddAndRemoveData (#2533)
  feat: support SENTRY_DSN environment var on macOS (#2534)
  Remove duplicate entry (#2532)
  fix: ARC issue for FileManager (#2525)
  feat: Add SwiftUI performance tracking (#2271)
  fix: Remove all permission checks (#2529)
  Remove the automatic `viewAppearing` span (#2511)
  Fix and reenable testANRDetected_UpdatesAppStateToTrue (#2526)
  fix: Don't add out of date context for crashes (#2523)
  ref: Rename isOOM to watchdog in Client (#2520)
  test: Fix disabled failing watchdog test (#2521)
  build(deps): bump github/codeql-action from 2.1.35 to 2.1.36 (#2516)
  Rename the watchdog option and integration (#2513)
  feat: Enable CaptureFailedRequests by default (#2507)
  test: Fix asserts for SentryCrashTestInstallation (#2500)
  meta: User interaction tracing enabled per default (#2506)
  ref: Rename OOM to Watchdog Terminations (#2499)
  feat: enableUserInteractionTracing is GA (#2503)
  build(deps): bump nokogiri from 1.13.9 to 1.13.10 (#2505)
  perf: Don't attach headers for SentryNoOpSpan (#2498)
  ...
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.

Apps required to have NSLocationAlwaysAndWhenInUseUsageDescription Serialize permissions to disk
3 participants