Skip to content

Commit 993d778

Browse files
Address review comments for UseUserAccessGroup implementation
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.
1 parent 038ea43 commit 993d778

File tree

3 files changed

+34
-7
lines changed

3 files changed

+34
-7
lines changed

auth/integration_test/src/integration_test.cc

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1043,6 +1043,35 @@ TEST_F(FirebaseAuthTest, TestWithCustomEmailAndPassword) {
10431043
EXPECT_EQ(auth_->current_user().email(), kCustomTestEmail);
10441044
}
10451045

1046+
TEST_F(FirebaseAuthTest, TestUseUserAccessGroupDoesNotCrash) {
1047+
// This test primarily ensures that calling UseUserAccessGroup doesn't crash
1048+
// on any platform. On iOS, it would actually attempt to call the underlying
1049+
// OS function. On other platforms, it's a stub.
1050+
LogDebug("Calling UseUserAccessGroup with a test group name.");
1051+
firebase::auth::AuthError error =
1052+
auth_->UseUserAccessGroup("com.google.firebase.test.accessgroup");
1053+
#if TARGET_OS_IPHONE
1054+
// On iOS, this might return an error if keychain sharing isn't set up
1055+
// for the test app, but it shouldn't crash. We accept kAuthErrorNone or
1056+
// kAuthErrorKeychainError.
1057+
EXPECT_THAT(error, AnyOf(firebase::auth::kAuthErrorNone,
1058+
firebase::auth::kAuthErrorKeychainError));
1059+
#else
1060+
// On other platforms, it should be a no-op and return kAuthErrorNone.
1061+
EXPECT_EQ(error, firebase::auth::kAuthErrorNone);
1062+
#endif
1063+
1064+
LogDebug("Calling UseUserAccessGroup with nullptr.");
1065+
error = auth_->UseUserAccessGroup(nullptr);
1066+
#if TARGET_OS_IPHONE
1067+
EXPECT_THAT(error, AnyOf(firebase::auth::kAuthErrorNone,
1068+
firebase::auth::kAuthErrorKeychainError));
1069+
#else
1070+
EXPECT_EQ(error, firebase::auth::kAuthErrorNone);
1071+
#endif
1072+
LogDebug("UseUserAccessGroup calls completed.");
1073+
}
1074+
10461075
TEST_F(FirebaseAuthTest, TestAuthPersistenceWithAnonymousSignin) {
10471076
// Automated test is disabled on linux due to the need to unlock the keystore.
10481077
SKIP_TEST_ON_LINUX;

auth/src/include/firebase/auth.h

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -505,11 +505,9 @@ class Auth {
505505
/// @brief Modifies this Auth instance to use the specified keychain access
506506
/// group.
507507
///
508-
/// Accessing the keychain requires that the application has the keychain
509-
/// sharing capability and that the level of entitling access to the keychain
510-
/// is `any` (런타임에 여러 앱에서 키체인 항목을 공유하도록 허용). See
511-
/// https://developer.apple.com/documentation/security/keychain_services/keychain_items/sharing_access_to_keychain_items_among_a_collection_of_apps
512-
/// for more details.
508+
/// For more details on how to configure keychain access groups and capabilities
509+
/// on iOS, please refer to the Firebase iOS SDK documentation and Apple's
510+
/// documentation on keychain services.
513511
///
514512
/// @note This method is only functional on iOS. On other platforms, it's a
515513
/// no-op and will return kAuthErrorNone.
@@ -518,7 +516,7 @@ class Auth {
518516
/// to use the default app bundle ID access group.
519517
///
520518
/// @return kAuthErrorNone on success, or an AuthError code if an error
521-
/// occurred.
519+
/// occurred (iOS only).
522520
AuthError UseUserAccessGroup(const char* access_group);
523521

524522
/// Returns the Auth object for an App. Creates the Auth if required.

auth/src/ios/auth_ios.mm

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -592,7 +592,7 @@ void SignInCallback(FIRUser *_Nullable user, NSError *_Nullable error,
592592

593593
AuthError Auth::UseUserAccessGroup(const char* access_group) {
594594
if (!auth_data_) {
595-
return kAuthErrorUninitialized;
595+
return kAuthErrorFailure; // Or a more specific "not initialized" error if available
596596
}
597597
NSString* ns_access_group = access_group ? @(access_group) : nil;
598598
NSError* error = nil;

0 commit comments

Comments
 (0)