WorryFree Computers   »   [go: up one dir, main page]
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.

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

fix for _userAccessGroup being nil when getStoredUserForAccessGroup is called #7472

Merged
merged 11 commits into from
Mar 2, 2021
Merged

fix for _userAccessGroup being nil when getStoredUserForAccessGroup is called #7472

merged 11 commits into from
Mar 2, 2021

Conversation

joehinkle11
Copy link
Contributor

This is a small change which fixes a significant bug. Basically the app doesn't let users share access groups until second launch

To reproduce:

  1. build 2 apps with a shared access group (useUserAccessGroup)
  2. sign in the first app
  3. open the second app (for the first time! if you already opened it, delete and reinstall)
  4. notice the second app is unable to load the access group
  5. force close the second app
  6. reopen the second app
  7. notice the second app is now able to load the access group

The cause: _userAccessGroup is set AFTER getStoredUserForAccessGroup is called, and getStoredUserForAccessGroup depends on _userAccessGroup not being null

Solution: swap the order of when _userAccessGroup is set and when getStoredUserForAccessGroup is called

@google-cla google-cla bot added the cla: yes label Feb 6, 2021
@paulb777 paulb777 mentioned this pull request Feb 8, 2021
Closed
@paulb777
Copy link
Member
paulb777 commented Feb 8, 2021

@joehinkle11 Thanks for the fix and PR.

@rosalyntan Please take a look.

Note the CI failure can be disregarded. (#7476)

rosalyntan
rosalyntan reviewed Feb 9, 2021
View reviewed changes
Copy link
Member
@rosalyntan rosalyntan 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 finding and fixing this issue! Your fix should work, but I took a look into the code, and I think you'll actually want to make a change in

firebase-ios-sdk/FirebaseAuth/Sources/Auth/FIRAuth.m

Line 2261 in 025eecd

user = [self.storedUserManager getStoredUserForAccessGroup:self.userAccessGroup
instead -- I believe we should be using the passed-in accessGroup rather than self.userAccessGroup there.

@joehinkle11
Copy link
Contributor Author

@rosalyntan updated!

@paulb777 paulb777 requested a review from rosalyntan March 1, 2021 21:50
rosalyntan
rosalyntan approved these changes Mar 2, 2021
View reviewed changes
Copy link
Member
@rosalyntan rosalyntan left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@rosalyntan rosalyntan merged commit b807226 into firebase:master Mar 2, 2021
@rosalyntan rosalyntan mentioned this pull request Mar 2, 2021
Merged
This was referenced Mar 10, 2021
Closed
Closed
@mikehardy
Copy link
Contributor

Irony: react-native-firebase was inadvertently relying on the old behavior in our E2E test suite, and now that this is working our test suite broke when I attempted to adopt v7.8.0. To be clear: as far as I can tell our test suite was not using the API correctly, but the way unintended consequences ripple out always fascinates me. Cheers all

@joehinkle11
Copy link
Contributor Author

@mikehardy Glad I was a service in breaking something 😅

@peterfriese
Copy link
Contributor

@mikehardy Glad I was a service in breaking something 😅

Breaking Stuff as a Service ;-)

@firebase firebase locked and limited conversation to collaborators Apr 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rosalyntan rosalyntan rosalyntan approved these changes

Assignees

@rosalyntan rosalyntan

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@joehinkle11 @paulb777 @mikehardy @peterfriese @rosalyntan