sentry-cocoa icon indicating copy to clipboard operation
sentry-cocoa copied to clipboard

feat: reuse the vendorIdentifier if already fetched.

Open leejunhui opened this issue 2 years ago • 6 comments

:scroll: Description

Reuse the vendorIdentifier if already fetched beforehand.Due to the China mainland privacy policy, apps cannot call the UIDevice.currentDevice.identifierForVendor multiple times in a short period of time.So to simultaneously use Sentry SDK and meet the privacy policy.It's maybe the simplest solution that I could figure out right now😂

:bulb: Motivation and Context

:green_heart: How did you test it?

:pencil: Checklist

You have to check all boxes before merging:

  • [x] I reviewed the submitted code.
  • [x] I added tests to verify the changes.
  • [x] No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • [x] I updated the docs if needed.
  • [x] Review from the native team if needed.
  • [x] No breaking change or entry added to the changelog.
  • [x] No breaking change for hybrid SDKs or communicated to hybrid SDKs.

:crystal_ball: Next steps

leejunhui avatar Mar 13 '24 07:03 leejunhui

Im not familiar with China's privacy policy, but im ok with this change if it really helps, the behaviour it is the same.

Maybe we need some test to avoid regression, probably by using SentryUIDeviceWrapper. @leejunhui can you write this tests?

brustolin avatar Mar 13 '24 07:03 brustolin

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 89.330%. Comparing base (d0deebd) to head (1494a13).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3734       +/-   ##
=============================================
- Coverage   89.341%   89.330%   -0.012%     
=============================================
  Files          536       536               
  Lines        59081     59074        -7     
  Branches     21234     21230        -4     
=============================================
- Hits         52784     52771       -13     
- Misses        5257      5265        +8     
+ Partials      1040      1038        -2     
Files Coverage Δ
Sources/Sentry/SentryAppStateManager.m 100.000% <100.000%> (ø)

... and 9 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

codecov[bot] avatar Mar 13 '24 07:03 codecov[bot]

Hey @leejunhui thanks for your contribution - could you provide a source for this requirement? We'd like to make sure we understand the requirement correctly, and see if there's other things we need to be aware of.

kahest avatar Mar 13 '24 09:03 kahest

hi @kahest , the policy source are Information security technology-Basic requirements for collecting personal information in mobile internet applications and Methods for identifying illegal and illegal collection and use of personal information by apps, Please note that these are Chinese webpages, you can use Google's automatic translation function to access those content.

img_v3_028v_4341e97b-fa2a-4051-bd08-eaf5a7d4439g

leejunhui avatar Mar 14 '24 05:03 leejunhui

hi @brustolin Sorry, I am mainly engaged in React and React Native development, and I am not very skilled in writing tests of native code.😂

leejunhui avatar Mar 14 '24 05:03 leejunhui

For clarity - the passages are about ensuring that the "Frequency of automatic collection of personal information should be the minimum frequency necessary to achieve the business functions of the product or service". I wouldn't relate this to the technical implementation of whether personal information is temporarily cached in memory or retrieved from the operating system API instead - this is more about only collecting/sending data if and when you need it.

That said, it's fair to apply the change you contributed if we can ensure it doesn't break functionality. I don't think it's required to comply with the terms above.

kahest avatar Mar 14 '24 09:03 kahest

We will remove usage of the id for some use cases and we'll keep the current behaviour for the once usage in our OOM heuristics. For this, the id is not sent off device, so this PR is obsolete.

kahest avatar Jun 06 '24 14:06 kahest