Fix credential.is_valid() crash on iOS (#1309)

jonsimantov · chkuang-g · web-flow · commit 20d0b8de06d7 · 2023-05-08T19:57:09.000-07:00
* Add test for credential.is_valid().

* Handle PhoneAuthCredentialFromImpl and others

- Handle null `impl_` for PhoneAuthCredentialFromImpl
- Handle API receiving invalid `Credential`
- Add a basic test to just verify if the app crashes.

* format code

* Explicitly ignore return value of is_valid to prevent build warning.

* Add release note.

* Clean up release note.

---------

Co-authored-by: Shawn Kuang &lt;chkuang@google.com&gt;
diff --git a/auth/integration_test/src/integration_test.cc b/auth/integration_test/src/integration_test.cc
@@ -273,6 +273,9 @@ bool FirebaseAuthTest::WaitForCompletion(
         EXPECT_TRUE(auth_->current_user().is_valid());
         EXPECT_EQ(future.result()->user.uid(), auth_->current_user().uid())
             << "User returned by Future doesn't match User in Auth";
+        // Expect no crash.
+        // TODO(b/281590792): Properly validate AuthResult fields case-by-case.
+        (void)future.result()->credential.is_valid();
         succeeded = future.result()->user.is_valid() &&
                     auth_->current_user().is_valid();
       }
@@ -310,6 +313,9 @@ bool FirebaseAuthTest::WaitForCompletion(
     EXPECT_EQ(result_ptr->additional_user_info.provider_id, provider_id);
     EXPECT_EQ(result_ptr->user.uid(), auth_->current_user().uid());
     EXPECT_TRUE(auth_->current_user().is_valid());
+    // Expect no crash.
+    // TODO(b/281590792): Properly validate AuthResult fields case-by-case.
+    (void)result_ptr->credential.is_valid();
   }
   return succeeded;
 }
@@ -368,6 +374,7 @@ TEST_F(FirebaseAuthTest, TestInitialization) {
 
 TEST_F(FirebaseAuthTest, TestAnonymousSignin) {
   // Test notification on SignIn().
+
   WaitForCompletion(auth_->SignInAnonymously(), "SignInAnonymously");
   EXPECT_TRUE(auth_->current_user().is_valid());
   EXPECT_TRUE(auth_->current_user().is_anonymous());
diff --git a/auth/src/ios/auth_ios.mm b/auth/src/ios/auth_ios.mm
@@ -455,6 +455,10 @@ void SignInResultCallback(FIRAuthDataResult *_Nullable auth_result, NSError *_Nu
 Future<User> Auth::SignInWithCredential(const Credential &credential) {
   ReferenceCountedFutureImpl &futures = auth_data_->future_impl;
   const auto handle = futures.SafeAlloc<User>(kAuthFn_SignInWithCredential, User());
+  if (!credential.is_valid()) {
+    futures.Complete(handle, kAuthErrorInvalidCredential, "Invalid credential is not allowed.");
+    return MakeFuture(&futures, handle);
+  }
 
   [AuthImpl(auth_data_)
       signInWithCredential:CredentialFromImpl(credential.impl_)
@@ -468,6 +472,10 @@ void SignInResultCallback(FIRAuthDataResult *_Nullable auth_result, NSError *_Nu
 Future<User *> Auth::SignInWithCredential_DEPRECATED(const Credential &credential) {
   ReferenceCountedFutureImpl &futures = auth_data_->future_impl;
   const auto handle = futures.SafeAlloc<User *>(kAuthFn_SignInWithCredential_DEPRECATED, nullptr);
+  if (!credential.is_valid()) {
+    futures.Complete(handle, kAuthErrorInvalidCredential, "Invalid credential is not allowed.");
+    return MakeFuture(&futures, handle);
+  }
 
   [AuthImpl(auth_data_)
       signInWithCredential:CredentialFromImpl(credential.impl_)
@@ -482,6 +490,10 @@ void SignInResultCallback(FIRAuthDataResult *_Nullable auth_result, NSError *_Nu
   ReferenceCountedFutureImpl &futures = auth_data_->future_impl;
   const auto handle =
       futures.SafeAlloc<AuthResult>(kAuthFn_SignInAndRetrieveDataWithCredential, AuthResult());
+  if (!credential.is_valid()) {
+    futures.Complete(handle, kAuthErrorInvalidCredential, "Invalid credential is not allowed.");
+    return MakeFuture(&futures, handle);
+  }
 
   [AuthImpl(auth_data_)
       signInWithCredential:CredentialFromImpl(credential.impl_)
@@ -497,6 +509,10 @@ void SignInResultCallback(FIRAuthDataResult *_Nullable auth_result, NSError *_Nu
   ReferenceCountedFutureImpl &futures = auth_data_->future_impl;
   const auto handle = futures.SafeAlloc<SignInResult>(
       kAuthFn_SignInAndRetrieveDataWithCredential_DEPRECATED, SignInResult());
+  if (!credential.is_valid()) {
+    futures.Complete(handle, kAuthErrorInvalidCredential, "Invalid credential is not allowed.");
+    return MakeFuture(&futures, handle);
+  }
 
   [AuthImpl(auth_data_)
       signInWithCredential:CredentialFromImpl(credential.impl_)
diff --git a/auth/src/ios/common_ios.h b/auth/src/ios/common_ios.h
@@ -91,14 +91,14 @@ static inline FIRAuth *_Nonnull AuthImpl(AuthData *_Nonnull auth_data) {
 
 /// Convert from the void* credential implementation pointer into the Obj-C
 /// FIRAuthCredential pointer.
-static inline FIRAuthCredential *_Nonnull CredentialFromImpl(void *_Nonnull impl) {
-  return static_cast<FIRAuthCredentialPointer *>(impl)->get();
+static inline FIRAuthCredential *_Nullable CredentialFromImpl(void *_Nullable impl) {
+  return impl ? static_cast<FIRAuthCredentialPointer *>(impl)->get() : nil;
 }
 
 /// Convert from the void* credential implementation pointer into the Obj-C
 /// FIRPhoneAuthCredential pointer.
-static inline FIRPhoneAuthCredential *_Nonnull PhoneAuthCredentialFromImpl(void *_Nonnull impl) {
-  return static_cast<FIRPhoneAuthCredentialPointer *>(impl)->get();
+static inline FIRPhoneAuthCredential *_Nullable PhoneAuthCredentialFromImpl(void *_Nullable impl) {
+  return impl ? static_cast<FIRPhoneAuthCredentialPointer *>(impl)->get() : nil;
 }
 
 AuthError AuthErrorFromNSError(NSError *_Nullable error);
diff --git a/auth/src/ios/user_ios.mm b/auth/src/ios/user_ios.mm
@@ -194,6 +194,10 @@ explicit IOSWrappedUserInfo(id<FIRUserInfo> user_info) : user_info_(user_info) {
   }
   ReferenceCountedFutureImpl &futures = auth_data_->future_impl;
   const auto handle = futures.SafeAlloc<AuthResult>(kUserFn_LinkWithCredential, AuthResult());
+  if (!credential.is_valid()) {
+    futures.Complete(handle, kAuthErrorInvalidCredential, "Invalid credential is not allowed.");
+    return MakeFuture(&futures, handle);
+  }
   AuthData *auth_data = auth_data_;
   [UserImpl(auth_data)
       linkWithCredential:CredentialFromImpl(credential.impl_)
@@ -209,6 +213,10 @@ explicit IOSWrappedUserInfo(id<FIRUserInfo> user_info) : user_info_(user_info) {
   }
   ReferenceCountedFutureImpl &futures = auth_data_->future_impl;
   const auto handle = futures.SafeAlloc<User *>(kUserFn_LinkWithCredential_DEPRECATED);
+  if (!credential.is_valid()) {
+    futures.Complete(handle, kAuthErrorInvalidCredential, "Invalid credential is not allowed.");
+    return MakeFuture(&futures, handle);
+  }
   [UserImpl(auth_data_)
       linkWithCredential:CredentialFromImpl(credential.impl_)
               completion:^(FIRAuthDataResult *_Nullable auth_result, NSError *_Nullable error) {
@@ -224,6 +232,10 @@ explicit IOSWrappedUserInfo(id<FIRUserInfo> user_info) : user_info_(user_info) {
   ReferenceCountedFutureImpl &futures = auth_data_->future_impl;
   const auto handle = auth_data_->future_impl.SafeAlloc<SignInResult>(
       kUserFn_LinkAndRetrieveDataWithCredential, SignInResult());
+  if (!credential.is_valid()) {
+    futures.Complete(handle, kAuthErrorInvalidCredential, "Invalid credential is not allowed.");
+    return MakeFuture(&futures, handle);
+  }
   AuthData *auth_data = auth_data_;
   [UserImpl(auth_data)
       linkWithCredential:CredentialFromImpl(credential.impl_)
@@ -279,6 +291,10 @@ explicit IOSWrappedUserInfo(id<FIRUserInfo> user_info) : user_info_(user_info) {
   const auto handle = futures.SafeAlloc<User>(kUserFn_UpdatePhoneNumberCredential);
 
 #if FIREBASE_PLATFORM_IOS
+  if (!credential.is_valid()) {
+    futures.Complete(handle, kAuthErrorInvalidCredential, "Invalid credential is not allowed.");
+    return MakeFuture(&futures, handle);
+  }
   FIRPhoneAuthCredential *objc_credential = PhoneAuthCredentialFromImpl(credential.impl_);
   [UserImpl(auth_data_)
       updatePhoneNumberCredential:objc_credential
@@ -302,6 +318,10 @@ explicit IOSWrappedUserInfo(id<FIRUserInfo> user_info) : user_info_(user_info) {
   const auto handle = futures.SafeAlloc<User *>(kUserFn_UpdatePhoneNumberCredential_DEPRECATED);
 
 #if FIREBASE_PLATFORM_IOS
+  if (!credential.is_valid()) {
+    futures.Complete(handle, kAuthErrorInvalidCredential, "Invalid credential is not allowed.");
+    return MakeFuture(&futures, handle);
+  }
   FIRAuthCredential *objc_credential = CredentialFromImpl(credential.impl_);
   if ([objc_credential isKindOfClass:[FIRPhoneAuthCredential class]]) {
     [UserImpl(auth_data_)
@@ -341,6 +361,10 @@ explicit IOSWrappedUserInfo(id<FIRUserInfo> user_info) : user_info_(user_info) {
   }
   ReferenceCountedFutureImpl &futures = auth_data_->future_impl;
   const auto handle = futures.SafeAlloc<void>(kUserFn_Reauthenticate);
+  if (!credential.is_valid()) {
+    futures.Complete(handle, kAuthErrorInvalidCredential, "Invalid credential is not allowed.");
+    return MakeFuture(&futures, handle);
+  }
 
   [UserImpl(auth_data_)
       reauthenticateWithCredential:CredentialFromImpl(credential.impl_)
@@ -359,6 +383,10 @@ explicit IOSWrappedUserInfo(id<FIRUserInfo> user_info) : user_info_(user_info) {
   ReferenceCountedFutureImpl &futures = auth_data_->future_impl;
   const auto handle = auth_data_->future_impl.SafeAlloc<AuthResult>(
       kUserFn_ReauthenticateAndRetrieveData, AuthResult());
+  if (!credential.is_valid()) {
+    futures.Complete(handle, kAuthErrorInvalidCredential, "Invalid credential is not allowed.");
+    return MakeFuture(&futures, handle);
+  }
   AuthData *auth_data = auth_data_;
   [UserImpl(auth_data)
       reauthenticateWithCredential:CredentialFromImpl(credential.impl_)
@@ -376,6 +404,10 @@ explicit IOSWrappedUserInfo(id<FIRUserInfo> user_info) : user_info_(user_info) {
   ReferenceCountedFutureImpl &futures = auth_data_->future_impl;
   const auto handle = auth_data_->future_impl.SafeAlloc<SignInResult>(
       kUserFn_ReauthenticateAndRetrieveData_DEPRECATED, SignInResult());
+  if (!credential.is_valid()) {
+    futures.Complete(handle, kAuthErrorInvalidCredential, "Invalid credential is not allowed.");
+    return MakeFuture(&futures, handle);
+  }
   AuthData *auth_data = auth_data_;
   [UserImpl(auth_data)
       reauthenticateWithCredential:CredentialFromImpl(credential.impl_)
diff --git a/release_build_files/readme.md b/release_build_files/readme.md
@@ -627,6 +627,11 @@ workflow use only during the development of your app, not for publicly shipping
 code.
 
 ## Release Notes
+### 11.0.1
+-   Changes
+    - Auth (iOS): Fixed a crash in `Credential::is_valid()` when an `AuthResult`
+      contains an invalid credential, such as when signing in anonymously.
+
 ### 11.0.0
 -   Changes
     - General: Update minimum supported C++ standard to C++14.
