E534 [Auth] Handle corrupt keychain value resulting from incomplete v11 port by ncooke3 · Pull Request #13643 · firebase/firebase-ios-sdk · GitHub
[go: up one dir, main page]
Skip to content

[Auth] Handle corrupt keychain value resulting from incomplete v11 port #13643

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 18, 2024
Merged

[Auth] Handle corrupt keychain value resulting from incomplete v11 port #13643

merged 4 commits into from
Sep 18, 2024

Conversation

ncooke3
Copy link
Member
@ncooke3 ncooke3 commented Sep 15, 2024 •
edited
Loading

This should be covered in the changelog for entry for the previous PR that added back the missing key.

Addressing the second case described in #13584 (comment):

There is another case I'm still investigating where a user (again with auth configured with a keychain access group and shareAuthStateAcrossDevices == true) is stored on 11.0, 11.1, or 11.2 and missing that key. In this case, such entries will not be read properly when 11.3 adds back the missing key (basically, the same problem you reported but from the upgrade path 11.0/ 11.1/11.2 -> 11.3 (fixed) rather than from 10.x -> 11.0/ 11.1/11.2). I'll leave this issue open until I have a fix for this case also staged in 11.3.

This was tricky enough where I thought it better for the explanation to be embedded next to the code for future us. See diff.

Alternatives considered

  • Rather than wrap error handling around the setter, change the code inside the setter. I chose against this as we would have to inspect the query within the setter whereas we have the context we need in the outer scope where we call it. I wanted to keep the setter's implementation unaware of the custom logic needed to solve this bug.

Risks

  • The added error handling will delete the iCloud entry from the buggy versions that is corrupting the non-iCloud bucket. Since the corrupt iCloud entry isn't really be synced across devices, it needs to be removed because it's blocking non-iCloud entries from being written. To avoid having to delete this, it would result in trickier keychain migration code that may things more complicated and prone to error.

Test observations

Current behavior on buggy versions:

  • In the buggy version, when can an iCloud entry be written?
    • Can it be written when there is already a non-iCloud entry there?
      • No, it cannot because of the conflicting accessibility flag.
    • Can it be written when there is iCloud entry there?
      • Yes, if from buggy version.
      • Maybe, if not from buggy version, because it will depend on if a non-iCloud entry is there (then no) or if the bucket is empty (then yes).
    • Can it be written when there is no entry there?
      • Yes.

Fix #13584

@google-oss-bot
Copy link
1 Warning
⚠️ Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

@ncooke3 ncooke3 added this to the 11.3.0 - M154 milestone Sep 18, 2024
@ncooke3 ncooke3 E880 marked this pull request as ready for review September 18, 2024 20:44
andrewheard
andrewheard approved these changes Sep 18, 2024
View reviewed changes
Copy link
Contributor
@andrewheard andrewheard left a comment

Choose a reason for hiding this comment

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

Risks

  • The added error handling will delete the iCloud entry from the buggy versions that is corrupting the non-iCloud bucket. Since the corrupt iCloud entry isn't really be synced across devices, it needs to be removed because it's blocking non-iCloud entries from being written. To avoid having to delete this, it would result in trickier keychain migration code that may things more complicated and prone to error.

I think the decision not to implement the trickier keychain migration code is reasonable since the situation only affects users of apps that upgraded to Firebase 11.0-11.2, deployed to the App Store, and the user had downloaded that version.

Also, AFAICT, it's annoying rather than breaking since the issue will now get fixed when the user signs in again.

LGTM on green (might be good to wait for a 2nd opinion from Paul though). I kicked off another CI run since I think the previous failure was a flake.

paulb777
paulb777 approved these changes Sep 18, 2024
View reviewed changes
Copy link
Member
@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

Thanks for writing it all down for future us!

@ncooke3 ncooke3 merged commit 602f3e7 into main Sep 18, 2024
45 of 56 checks passed
@ncooke3 ncooke3 deleted the nc/auth-corrupt-keychain branch September 18, 2024 23:12
@ncooke3 ncooke3 mentioned this pull request Sep 19, 2024
Closed
This was referenced Sep 30, 2024
Closed
Merged
Merged
@firebase firebase locked and limited conversation to collaborators Oct 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@paulb777 paulb777 paulb777 approved these changes

@andrewheard andrewheard andrewheard approved these changes

Assignees
No one assigned
Labels
api: auth
Projects
None yet
Milestone
11.3.0 - M154
Development

Successfully merging this pull request may close these issues.

Keychain error with Firebase Authentication when updating to 11.1.0
4 participants
@ncooke3 @google-oss-bot @paulb777 @andrewheard
0