Skip to content

Commit c83683f

Browse files
jonsimantova-maurice
authored andcommitted
In OS X secure storage, set a non-secure flag (using NSUserDefaults) that we check before trying to access the keychain, so the user isn't prompted to unlock the keychain
unless we know there is useful data in it for the given app. OS X will prompt the user if they try to read a given keychain entry having never written to it, so this prevents that prompt (which would be an unfortunate user experience the first time they run a desktop app). PiperOrigin-RevId: 246215475
1 parent 614c00d commit c83683f

File tree

2 files changed

+54
-3
lines changed

2 files changed

+54
-3
lines changed

auth/src/desktop/secure/user_secure_darwin_internal.h

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,13 @@ namespace auth {
2424
namespace secure {
2525

2626
// Darwin-specific implementation for the secure manager of user data.
27+
//
28+
// Stores the secure data in the user's default keychain.
29+
//
30+
// Also sets an entry in NSUserDefaults for the app the first time data is
31+
// written; unless that entry is set, we won't check the keychain (if we do,
32+
// their system will prompt the user for a password if we try to access the
33+
// keychain before writing to it, which is not a great user experience).
2734
class UserSecureDarwinInternal : public UserSecureInternal {
2835
public:
2936
explicit UserSecureDarwinInternal(const char* service);
@@ -40,13 +47,20 @@ class UserSecureDarwinInternal : public UserSecureInternal {
4047
void DeleteAllData() override;
4148

4249
private:
50+
// Does the user have secure data set for this app? Don't access the keychain
51+
// unless this is true.
52+
bool UserHasSecureData();
53+
// Set whether the user has secure data set for this app.
54+
void SetUserHasSecureData(bool b);
55+
4356
// Delete either a single key, or (if app_name is null) all keys.
4457
// func_name is used for error messages.
4558
void DeleteData(const char* app_name, const char* func_name);
4659

4760
std::string GetKeystoreLocation(const std::string& app);
4861

4962
std::string service_;
63+
std::string user_defaults_key_;
5064
};
5165

5266
} // namespace secure

auth/src/desktop/secure/user_secure_darwin_internal.mm

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,19 @@
2121
namespace auth {
2222
namespace secure {
2323

24+
// Prefix and suffix to add to keychain service name.
2425
static const char kServicePrefix[] = "";
2526
static const char kServiceSuffix[] = " [Firebase Auth]";
2627
static const int kMaxAllowedKeychainEntries = INT_MAX;
2728

29+
// Prefix and suffix for the key for NSUserDefaults.
30+
static const char kUserDefaultsPrefix[] = "com.google.firebase.auth.";
31+
static const char kUserDefaultsSuffix[] = ".has_saved_user";
32+
2833
UserSecureDarwinInternal::UserSecureDarwinInternal(const char* service) {
2934
service_ = std::string(kServicePrefix) + service + std::string(kServiceSuffix);
35+
user_defaults_key_ =
36+
std::string(kUserDefaultsPrefix) + service + std::string(kUserDefaultsSuffix);
3037
}
3138

3239
UserSecureDarwinInternal::~UserSecureDarwinInternal() {}
@@ -47,7 +54,27 @@
4754
return service_ + "/" + app;
4855
}
4956

57+
bool UserSecureDarwinInternal::UserHasSecureData() {
58+
NSUserDefaults* defaults = [NSUserDefaults standardUserDefaults];
59+
if (!defaults) {
60+
// For some reason we can't get NSUserDefaults, so just err on the safe side and return true so
61+
// we check the keychain directly.
62+
return true;
63+
}
64+
return [defaults boolForKey:@(user_defaults_key_.c_str())] ? true : false;
65+
}
66+
67+
void UserSecureDarwinInternal::SetUserHasSecureData(bool b) {
68+
NSUserDefaults* defaults = [NSUserDefaults standardUserDefaults];
69+
[defaults setBool:(b ? YES : NO) forKey:@(user_defaults_key_.c_str())];
70+
[defaults synchronize];
71+
}
72+
5073
std::string UserSecureDarwinInternal::LoadUserData(const std::string& app_name) {
74+
if (!UserHasSecureData()) {
75+
LogDebug("LoadUserData: User has no data stored.");
76+
return "";
77+
}
5178
NSMutableDictionary* query = GetQueryForApp(service_.c_str(), app_name.c_str());
5279
std::string keystore_location = GetKeystoreLocation(app_name);
5380
// We want to return the data and attributes.
@@ -111,9 +138,16 @@
111138
error_string.UTF8String);
112139
return;
113140
}
141+
142+
SetUserHasSecureData(true);
114143
}
115144

116145
void UserSecureDarwinInternal::DeleteData(const char* app_name, const char* func_name) {
146+
if (!UserHasSecureData()) {
147+
LogDebug("%s: User has no data stored.", func_name);
148+
return;
149+
}
150+
117151
NSMutableDictionary* query = GetQueryForApp(service_.c_str(), app_name);
118152
std::string keystore_location = app_name ? GetKeystoreLocation(app_name) : service_;
119153

@@ -125,11 +159,11 @@
125159
OSStatus status = SecItemDelete((__bridge CFDictionaryRef)query);
126160

127161
if (status == errSecItemNotFound) {
128-
LogDebug("%s: Key %s not found", keystore_location.c_str());
162+
LogDebug("%s: Key %s not found", func_name, keystore_location.c_str());
129163
return;
130164
} else if (status != noErr) {
131165
NSString* error_string = (__bridge_transfer NSString*)SecCopyErrorMessageString(status, NULL);
132-
LogError("%s: Error %d deleting %s: %s", status, keystore_location.c_str(),
166+
LogError("%s: Error %d deleting %s: %s", func_name, status, keystore_location.c_str(),
133167
error_string.UTF8String);
134168
return;
135169
}
@@ -139,7 +173,10 @@
139173
DeleteData(app_name.c_str(), "DeleteUserData");
140174
}
141175

142-
void UserSecureDarwinInternal::DeleteAllData() { DeleteData(nullptr, "DeleteAllData"); }
176+
void UserSecureDarwinInternal::DeleteAllData() {
177+
DeleteData(nullptr, "DeleteAllData");
178+
SetUserHasSecureData(false);
179+
}
143180

144181
} // namespace secure
145182
} // namespace auth

0 commit comments

Comments
 (0)