8000 Fix bug where multiple keychain entries would result in user persistence failure by morganchen12 · Pull Request #5930 · firebase/firebase-ios-sdk · GitHub
[go: up one dir, main page]
Skip to content

Fix bug where multiple keychain entries would result in user persistence failure #5930

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 8 commits into from
Jul 30, 2020
Merged

Fix bug where multiple keychain entries would result in user persistence failure #5930

merged 8 commits into from
Jul 30, 2020

Conversation

morganchen12
Copy link
Contributor

Fixes #5906.

@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

paulb777
paulb777 reviewed Jun 30, 2020
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.

What are the risks of continuing with the wrong keychain?

rosalyntan
rosalyntan reviewed Jun 30, 2020
View reviewed changes
FirebaseAuth/Sources/Storage/FIRAuthKeychainServices.m
// More than one keychain item was found, all but the first will be ignored.
FIRLogError(
kFIRLoggerAuth, @"I-AUT000005",
@"Keychain query returned multiple results, all but the first will be ignored: %@",
Copy link
Member

Choose a reason for hiding this comment

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

Should the match limit of 2 above be removed, in order to get all the ignored results?

@rosalyntan
Copy link
Member

Is there a way to delete the duplicate users from the keychain? (I'm still new to iOS so don't know exactly how the keychain works)

@morganchen12
Copy link
Contributor Author

One auth session will be discarded. Based on my understanding of the code, the two auth sessions are actually the same user, one from the legacy keychain service (with no service specified) and one from the up-to-date keychain service.

The right thing to do is probably to delete the legacy entry if we encounter both a legacy and non-legacy keychain entry.

@paulb777 paulb777 mentioned this pull request Jul 24, 2020
Closed
@morganchen12
Copy link
Contributor Author

I've modified the code to always return the non-legacy item if it exists. The way keychain deletions work, I'm not sure it's practical to delete the old keychain value without making significant modifications to the class.

Given the relatively limited impact of the bug I'm ok with not removing the legacy entry in this method.

@paulb777
Copy link
Member

Fix CI , then LGTM

@morganchen12 morganchen12 merged commit f56576f into master Jul 30, 2020
@morganchen12 morganchen12 deleted the morganchen/auth branch July 30, 2020 22:56
@firebase firebase locked and limited conversation to collaborators Aug 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@paulb777 paulb777 paulb777 approved these changes

@rosalyntan rosalyntan Awaiting requested review from rosalyntan

Assignees
No one assigned
Labels
api: auth cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Keychain duplicate item error causing new Anonymous user on every login. No way to recover.
5 participants
@morganchen12 @google-oss-bot @rosalyntan @paulb777 @googlebot
0