Implement Firebase Auth UseUserAccessGroup for C++ iOS SDK - #1760
Draft
jonsimantov wants to merge 5 commits into
Draft
Implement Firebase Auth UseUserAccessGroup for C++ iOS SDK#1760jonsimantov wants to merge 5 commits into
jonsimantov wants to merge 5 commits into
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
This commit introduces the `UseUserAccessGroup` method to the Firebase Authentication C++ SDK. This method is specific to iOS and allows developers to specify a keychain access group for user data. It calls the underlying Objective-C method `[FIRAuth useUserAccessGroup:error:]`. On Android and desktop platforms, this method is a no-op stub and returns `kAuthErrorNone` as per its documented behavior for non-iOS platforms. Key changes: - Added `Auth::UseUserAccessGroup(const char* access_group)` to the public header `auth/src/include/firebase/auth.h` with Doxygen comments. - Implemented the iOS-specific logic in `auth/src/ios/auth_ios.mm`, including error conversion from `NSError` to `AuthError`. - Added stub implementations in `auth/src/desktop/auth_desktop.cc` and `auth/src/android/auth_android.cc` returning `kAuthErrorNone`.
jonsimantov
commented
Jun 27, 2025
jonsimantov
left a comment
Copy link
Copy Markdown
Contributor
Author
There was a problem hiding this comment.
Please add a very simple integration test to the auth/integration_test/src/integration_test.cc test file. This should just call the function and ensure it doesn't crash.
Sorry, something went wrong.
Comment thread
auth/src/include/firebase/auth.h
Outdated
| /// @brief Modifies this Auth instance to use the specified keychain access | ||
| /// group. | ||
| /// | ||
| /// Accessing the keychain requires that the application has the keychain |
Copy link
Copy Markdown
Contributor
Author
There was a problem hiding this comment.
Please remove all of this and instead tell the developer to refer to the Firebase iOS SDK documentation.
Sorry, something went wrong.
Comment thread
auth/src/ios/auth_ios.mm
Outdated
|
|
||
| AuthError Auth::UseUserAccessGroup(const char* access_group) { | ||
| if (!auth_data_) { | ||
| return kAuthErrorUninitialized; |
Copy link
Copy Markdown
Contributor
Author
There was a problem hiding this comment.
This error code doesn't exist, please check the AuthError enum and choose a more suitable one.
Sorry, something went wrong.
This commit incorporates feedback from the code review: - Updated Doxygen comments in `auth.h` for `UseUserAccessGroup` to refer to Firebase iOS SDK documentation for keychain details, removing the extensive inline explanation. - Corrected the error code in `auth_ios.mm` from the non-existent `kAuthErrorUninitialized` to `kAuthErrorFailure` when `auth_data_` is null. - Added a new integration test `TestUseUserAccessGroupDoesNotCrash` in `auth/integration_test/src/integration_test.cc`. This test calls the function with a sample group and nullptr, checking for `kAuthErrorNone` or `kAuthErrorKeychainError` on iOS (to ensure no crash even if keychain isn't fully set up in test env) and `kAuthErrorNone` on other platforms, primarily ensuring the calls do not crash.
jonsimantov
commented
Jun 27, 2025
Comment thread
auth/integration_test/src/integration_test.cc
| LogDebug("Calling UseUserAccessGroup with a test group name."); | ||
| firebase::auth::AuthError error = | ||
| auth_->UseUserAccessGroup("com.google.firebase.test.accessgroup"); | ||
| #if TARGET_OS_IPHONE |
Copy link
Copy Markdown
Contributor
Author
There was a problem hiding this comment.
No need to check platform since their are stubs. And no need for such verbose debug messages. Just make the calls and return.
Sorry, something went wrong.
This commit incorporates feedback from the code review regarding the integration test for `UseUserAccessGroup`: - Simplified the `TestUseUserAccessGroupDoesNotCrash` test in `auth/integration_test/src/integration_test.cc`. - Removed platform-specific checks (`#if TARGET_OS_IPHONE`) and associated `EXPECT_THAT` calls. - Removed `LogDebug` messages from the test. - Both calls to `UseUserAccessGroup` now uniformly expect `firebase::auth::kAuthErrorNone`. This aligns with the stub behavior on non-iOS platforms and simplifies the test as requested by the reviewer, focusing on ensuring the calls do not crash and stubs return the expected no-op error code.
jonsimantov
commented
Jun 27, 2025
Comment thread
auth/integration_test/src/integration_test.cc
Outdated
| // on any platform and that stubs return kAuthErrorNone. | ||
| firebase::auth::AuthError error = | ||
| auth_->UseUserAccessGroup("com.google.firebase.test.accessgroup"); | ||
| // On non-iOS, this is a stub and returns kAuthErrorNone. |
Copy link
Copy Markdown
Contributor
Author
There was a problem hiding this comment.
This long comment is unnecessary, please remove the entire thing.
Sorry, something went wrong.
jonsimantov
commented
Jun 27, 2025
Comment thread
auth/integration_test/src/integration_test.cc
| // This implies we should expect the stub behavior (kAuthErrorNone) or simply ensure no crash. | ||
| // Let's stick to expecting kAuthErrorNone as stubs should return this. | ||
| // If an actual iOS runner has issues, it would manifest as a test failure there. | ||
| EXPECT_EQ(error, firebase::auth::kAuthErrorNone); |
Copy link
Copy Markdown
Contributor
Author
There was a problem hiding this comment.
When checking stub return values, only check that on Android and Desktop - on iOS, it's possible that this call might return an error (since the integration test might not be set up for keychain sharing) but the test should still pass in that case.
Sorry, something went wrong.
Copy link
Copy Markdown
❌ Integration test FAILEDRequested by @jonsimantov on commit 0929f11
Add flaky tests to go/fpl-cpp-flake-tracker |
Sorry, something went wrong.
This commit incorporates further feedback on the integration test for
`UseUserAccessGroup`:
- Removed the verbose explanatory comment block from
`TestUseUserAccessGroupDoesNotCrash` in
`auth/integration_test/src/integration_test.cc`.
- Reinstated platform-specific error checking within the test:
- On iOS, the test now uses `EXPECT_THAT` to allow either
`kAuthErrorNone` or `kAuthErrorKeychainError`. This handles
potential keychain configuration issues in test environments while
ensuring the call does not crash.
- On other platforms (Android/Desktop), the test continues to expect
`kAuthErrorNone`, verifying the stub implementation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit introduces the UseUserAccessGroup method to the Firebase Authentication C++ SDK.
This method is specific to iOS and allows developers to specify a keychain access group for user data. It calls the underlying Objective-C method [FIRAuth useUserAccessGroup:error:].
On Android and desktop platforms, this method is a no-op stub and returns kAuthErrorNone as per its documented behavior for non-iOS platforms.
Key changes:
Description
Testing
Type of Change
Place an x the applicable box:
Notes