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

[Auth] Fix unexpected nil in fetchSignInMethods success case #13561

Merged
merged 5 commits into from
Aug 31, 2024
Merged

[Auth] Fix unexpected nil in fetchSignInMethods success case #13561

merged 5 commits into from
Aug 31, 2024

Conversation

ncooke3
Copy link
Member

@ncooke3 ncooke3 commented Aug 31, 2024 •
edited
Loading

  • Revert the URL to match Firebase 10 (

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

    Line 676 in eca84fd

    continueURI:@"http://www.google.com/"
    ). This actually didn't make a difference and I was able to get valid sign in methods when using both forms. LMK if there is a reason to not change it back.
  • The below line lead to crash as response.signinMethods is nil so the continuation throws an implicitly unwrapped error which crashes.

    firebase-ios-sdk/FirebaseAuth/Sources/Swift/Auth/Auth.swift

    Line 298 in 486a0a0

    Auth.wrapMainAsync(callback: completion, withParam: response.signinMethods, error: nil)
    • This can be reproduced in a scenario where email enumeration protection is turned on. The unit test added will fail without the corresponding changes in CreateAuthURIResponse.swift

Fix #13550

@ncooke3 ncooke3 added this to the 11.2.0 - M153 milestone Aug 31, 2024
paulb777
paulb777 approved these changes Aug 31, 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.

Good find and thanks for adding the tests!

FirebaseAuth/Tests/SampleSwift/SwiftApiTests/EmailPasswordTests.swift Show resolved Hide resolved
FirebaseAuth/Sources/Swift/Backend/RPC/CreateAuthURIResponse.swift
@@ -45,6 +45,6 @@ class CreateAuthURIResponse: AuthRPCResponse {
registered = dictionary["registered"] as? Bool ?? false
forExistingProvider = dictionary["forExistingProvider"] as? Bool ?? false
allProviders = dictionary["allProviders"] as? [String]
signinMethods = dictionary["signinMethods"] as? [String]
signinMethods = dictionary["signinMethods"] as? [String] ?? []
Copy link
Member

Choose a reason for hiding this comment

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

It's technically an API change to return an empty array instead of nil, so worth calling out in the release notes.

Copy link
Member Author

@ncooke3 ncooke3 Aug 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this PR may actually be re-aligning the behavior with the API contract:

firebase-ios-sdk/FirebaseAuth/Sources/Swift/Auth/Auth.swift

Lines 279 to 281 in 486a0a0

/// - Parameter completion: Optionally; a block which is invoked when the list of sign in methods
/// for the specified email address is ready or an error was encountered. Invoked asynchronously
/// on the main thread in the future.

Specifically, the list of sign in methods for the specified email address is ready or an error was encountered part.

In the current behavior, (nil, nil) was being passed to the completion handler which caused the crash.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will merge this now, but can address anything else post-merge.

Copy link
Member

Choose a reason for hiding this comment

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

Won't those using the callback version of the API now get [], nil instead of nil,nil

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, [], nil. I'm thinking that this is a valid combo as opposed to nil, nil.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, but it's possible there may be apps checking for nil,nil since it doesn't crash when using the callback api

Copy link
Member Author

Choose a reason for hiding this comment

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

True, ok I'll point this out in release notes.

One different approach to this PR is to preserve nil, nil as a valid combo and change the async wrapper to return a [String]? instead [String].

I think this PR with the corresponding release note is the better option.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. I would suspect that in practice, code would just not exercise the nil case and just loop zero times iterating the empty list.

Copy link
Member

Choose a reason for hiding this comment

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

So unlikely anything will break if the release note gets missed

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in d855c25

FirebaseAuth/Sources/Swift/Backend/RPC/CreateAuthURIResponse.swift Outdated Show resolved Hide resolved
@ncooke3 ncooke3 merged commit 853d83c into main Aug 31, 2024
56 checks passed
@ncooke3 ncooke3 deleted the nc/fix13550 branch August 31, 2024 22:32
@ncooke3 ncooke3 mentioned this pull request Aug 31, 2024
Merged
mergify bot referenced this pull request in cgrindel/rules_swift_package_manager Sep 16, 2024
@cgrindel-self-hosted-renovate @cgrindel @mergify
chore(deps): update dependency firebase/firebase-ios-sdk to from: "11…
3368822 
….2.0" (#1228)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
|
[firebase/firebase-ios-sdk](https://redirect.github.com/firebase/firebase-ios-sdk)
| minor | `from: "11.1.0"` -> `from: "11.2.0"` |

---

### Release Notes

<details>
<summary>firebase/firebase-ios-sdk (firebase/firebase-ios-sdk)</summary>

###
[`v11.2.0`](https://redirect.github.com/firebase/firebase-ios-sdk/releases/tag/11.2.0):
Firebase Apple 11.2.0

[Compare
Source](https://redirect.github.com/firebase/firebase-ios-sdk/compare/11.1.0...11.2.0)

The Firebase Apple SDK (11.2.0) is now available. For more details, see
the [Firebase Apple SDK release
notes.](https://firebase.google.com/support/release-notes/ios#11.2.0)

To install this SDK, see [Add Firebase to your
project](https://firebase.google.com/docs/ios/setup).

#### What's Changed

- \[Auth] Phone Auth – Fallback to reCATCHA flow when "invalid app
credential" error is thrown by
[@&#8203;ncooke3](https://redirect.github.com/ncooke3) in
[https://github.com/firebase/firebase-ios-sdk/pull/13519](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13519)
- \[Auth] Fix Xcode 16 continuation crashes by
[@&#8203;paulb777](https://redirect.github.com/paulb777) in
[https://github.com/firebase/firebase-ios-sdk/pull/13521](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13521)
- \[Auth] Fix Phone Auth via APNS for Sandbox Tokens and update Sample's
Firebase app by [@&#8203;paulb777](https://redirect.github.com/paulb777)
in
[https://github.com/firebase/firebase-ios-sdk/pull/13539](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13539)
- \[Auth] Add background modes capability to plist by
[@&#8203;ncooke3](https://redirect.github.com/ncooke3) in
[https://github.com/firebase/firebase-ios-sdk/pull/13548](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13548)
- \[Auth] When swizzling is disabled, open URLs via SceneDelegate by
[@&#8203;ncooke3](https://redirect.github.com/ncooke3) in
[https://github.com/firebase/firebase-ios-sdk/pull/13557](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13557)
- \[Auth] Fix unexpected nil in fetchSignInMethods success case by
[@&#8203;ncooke3](https://redirect.github.com/ncooke3) in
[https://github.com/firebase/firebase-ios-sdk/pull/13561](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13561)
- \[Auth] Fix user session persistence in multi tenant projects by
[@&#8203;paulb777](https://redirect.github.com/paulb777) in
[https://github.com/firebase/firebase-ios-sdk/pull/13567](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13567)
- \[Crashlytics] Fix Firebase/Crashlytics min iOS version by
[@&#8203;paulb777](https://redirect.github.com/paulb777) in
[https://github.com/firebase/firebase-ios-sdk/pull/13580](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13580)
- \[Database] Fix temporary disconnect when app goes inactive by
[@&#8203;paulb777](https://redirect.github.com/paulb777) in
[https://github.com/firebase/firebase-ios-sdk/pull/13564](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13564)
- \[Firestore] Mark readonly public classes as Sendable by
[@&#8203;paulb777](https://redirect.github.com/paulb777) in
[https://github.com/firebase/firebase-ios-sdk/pull/13453](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13453)
- \[Firestore] Support hex strings to prevent broken log lines by
[@&#8203;ls-todd-lunter](https://redirect.github.com/ls-todd-lunter) in
[https://github.com/firebase/firebase-ios-sdk/pull/13128](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13128)
- \[Functions] `FunctionsContext` Updates by
[@&#8203;yakovmanshin](https://redirect.github.com/yakovmanshin) in
[https://github.com/firebase/firebase-ios-sdk/pull/13531](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13531)
- \[Functions] Updated Functions Errors by
[@&#8203;yakovmanshin](https://redirect.github.com/yakovmanshin) in
[https://github.com/firebase/firebase-ios-sdk/pull/13535](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13535)
- \[Testing] Update OCMock dependency to v3.9.4 by
[@&#8203;andrewheard](https://redirect.github.com/andrewheard) in
[https://github.com/firebase/firebase-ios-sdk/pull/13536](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13536)
- \[Vertex AI] Make `uri` optional in `Citation` and add `title` field
by [@&#8203;andrewheard](https://redirect.github.com/andrewheard) in
[https://github.com/firebase/firebase-ios-sdk/pull/13520](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13520)
- \[Vertex AI] Add `Sendable` conformance to types by
[@&#8203;andrewheard](https://redirect.github.com/andrewheard) in
[https://github.com/firebase/firebase-ios-sdk/pull/13560](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13560)
- \[Vertex AI] Make `Logger` properties constants by
[@&#8203;andrewheard](https://redirect.github.com/andrewheard) in
[https://github.com/firebase/firebase-ios-sdk/pull/13570](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13570)
- \[Vertex AI] Make `GenerativeModel` and `Chat` into Swift actors by
[@&#8203;andrewheard](https://redirect.github.com/andrewheard) in
[https://github.com/firebase/firebase-ios-sdk/pull/13545](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13545)
- \[Vertex AI] Make generateContentStream/sendMessageStream throws by
[@&#8203;andrewheard](https://redirect.github.com/andrewheard) in
[https://github.com/firebase/firebase-ios-sdk/pull/13573](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13573)
- \[Vertex AI] Add `SourceImage` enum to `ImageConversionError` by
[@&#8203;andrewheard](https://redirect.github.com/andrewheard) in
[https://github.com/firebase/firebase-ios-sdk/pull/13575](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13575)
- \[Vertex AI] Add `responseSchema` to `GenerationConfig` by
[@&#8203;andrewheard](https://redirect.github.com/andrewheard) in
[https://github.com/firebase/firebase-ios-sdk/pull/13576](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13576)

#### New Contributors

- [@&#8203;ls-todd-lunter](https://redirect.github.com/ls-todd-lunter)
made their first contribution in
[https://github.com/firebase/firebase-ios-sdk/pull/13128](https://redirect.github.com/firebase/firebase-ios-sdk/pull/13128)

**Full Changelog**:
firebase/firebase-ios-sdk@11.1.0...11.2.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config
help](https://redirect.github.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://redirect.github.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC44MC4wIiwidXBkYXRlZEluVmVyIjoiMzguODAuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->

Co-authored-by: cgrindel-self-hosted-renovate[bot] <139595543+cgrindel-self-hosted-renovate[bot]@users.noreply.github.com>
Co-authored-by: Chuck Grindel <chuck.grindel@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
This was referenced Sep 26, 2024
Closed
Merged
@firebase firebase locked and limited conversation to collaborators Oct 1, 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

Assignees
No one assigned
Labels
api: auth
Projects
None yet
Milestone
11.2.0 - M153
3 participants
@ncooke3 @paulb777 @google-oss-bot