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

Bug Fix - Update deviceIdentifier to Save to Keychain #1472

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jaxdesmarais
Copy link
Contributor

@jaxdesmarais jaxdesmarais commented Nov 22, 2024

Summary of changes

Some recent conversations around our data collector had me investigating our comment "See if we already have an identifier in the keychain". As it turns out we were never actually saving the device identifier to the keychain and it seems like this logic has never worked as expected.

Updating main to do the following: let addStatus = SecItemAdd(query as CFDictionary, nil) gave me the result -50 for the OSStatus. This status means errSecParam - "one of the parameters you passed in a function was/are not valid". Digging into this a bit further in the Apple Docs for ksecvaluedata indicate that this should be passed as a Data object. Also discovered this tool for looking up OSStatus values because the Apple docs were not at all helpful for looking up error codes.

Updating this to now pass in data as well as our parsing logic to parse the data object as expected is now hitting the breakpoint in our keychain check. 🥳

Testing

To test, you will need to use a real device for keychain access, this cannot be tested on simulator.

  1. On main add a breakpoint on line 153 and 161
  2. In the Data Collector integration, hit "Collect Device Data"
  3. The first time should always hit line 161, but you should notice that subsequent attempts do not hit line 153 ever as expected
  4. Switch to this branch and run the same test but with breakpoints on line 152 and 160
  5. You should now see after the initial fetch you are hitting line 152 once the identifier is stored in the keychain

Checklist

  • [ ] Added a changelog entry
  • Tested and confirmed payment flows affected by this change are functioning as expected

Authors

@jaxdesmarais jaxdesmarais requested a review from a team as a code owner November 22, 2024 21:36
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.

1 participant