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

[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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7d9b617
[Auth] Fix unexpected nil in fetchSignInMethods success case
ncooke3 Aug 31, 2024
9dda4aa
Add changelog
ncooke3 Aug 31, 2024
256a48b
[skip ci] Remove whitespace in comment
ncooke3 Aug 31, 2024
9919971
[skip ci] Update comment in EmailPasswordTests.swift
ncooke3 Aug 31, 2024
d855c25
review
ncooke3 Aug 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion FirebaseAuth/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
# 11.2.0
# Unreleased
- [Fixed] Fixed crashes that could occur in Swift continuation blocks running in the Xcode 16
betas. (#13480)
- [Fixed] Fixed Phone Auth via Sandbox APNS tokens that broke in 11.0.0. (#13479)
- [Fixed] Fix crash when fetching sign in methods due to unexpected nil.
Previously, fetching sign in methods could return both a `nil` array of sign
in methods and a `nil` error. In such cases, an empty array is instead
returned with the `nil` error. (#13550)


# 11.1.0
- [fixed] Fixed `Swift.error` conformance for `AuthErrorCode`. (#13430)
Expand Down
2 changes: 1 addition & 1 deletion FirebaseAuth/Sources/Swift/Auth/Auth.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ extension Auth: AuthInterop {
completion: (([String]?, Error?) -> Void)? = nil) {
kAuthGlobalWorkQueue.async {
let request = CreateAuthURIRequest(identifier: email,
continueURI: "http:www.google.com",
continueURI: "http://www.google.com/",
requestConfiguration: self.requestConfiguration)
Task {
do {
Expand Down
6 changes: 3 additions & 3 deletions FirebaseAuth/Sources/Swift/Backend/RPC/CreateAuthURIResponse.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ class CreateAuthURIResponse: AuthRPCResponse {
/// A list of provider IDs the passed identifier could use to sign in with.
var allProviders: [String]?

/// A list of sign-in methods available for the passed identifier.
var signinMethods: [String]?
/// A list of sign-in methods available for the passed identifier.
var signinMethods: [String] = []

/// Bare initializer.
required init() {}
Expand All @@ -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

}
}
5 changes: 4 additions & 1 deletion FirebaseAuth/Tests/SampleSwift/SwiftApiTests/EmailPasswordTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,15 @@ class EmailPasswordTests: TestsBase {
waitForExpectations(timeout: TestsBase.kExpectationsTimeout)
}

@available(iOS 13, tvOS 13, macOS 10.15, macCatalyst 13, watchOS 7, *)
func testSignInExistingUserWithEmailAndPasswordAsync() async throws {
let auth = Auth.auth()
try await auth.signIn(withEmail: kExistingEmailToSignIn, password: "password")
XCTAssertEqual(auth.currentUser?.email,
kExistingEmailToSignIn,
"Signed user does not match request.")
// Regression test for #13550. Auth enumeration protection is enabled for
ncooke3 marked this conversation as resolved.
Show resolved Hide resolved
// the test project, so no sign in methods should be returned.
let signInMethods = try await auth.fetchSignInMethods(forEmail: kExistingEmailToSignIn)
XCTAssertEqual(signInMethods, [])
}
}
Loading