-
Notifications
You must be signed in to change notification settings - Fork 280
Feature/add sdkwrapper identifier #625
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
44a7b2f
0c09749
18ad3f2
d3e09a2
046a58e
5d587a2
9d4641c
f75d076
51e8d8b
505676b
fa51e74
f5741c2
158f549
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,8 @@ | |
|
|
||
| #import "GoogleSignIn/Sources/GIDSignInPreferences.h" | ||
|
|
||
| #import <os/lock.h> | ||
|
|
||
| NS_ASSUME_NONNULL_BEGIN | ||
|
|
||
| static NSString *const kLSOServer = @"accounts.google.com"; | ||
|
|
@@ -26,6 +28,12 @@ | |
| // The name of the query parameter used for logging the Apple execution environment. | ||
| NSString *const kEnvironmentLoggingParameter = @"gidenv"; | ||
|
|
||
| // The name of the query parameter used for logging the SDK wrapper. | ||
| NSString *const kSDKWrapperLoggingParameter = @"gidwrapper"; | ||
|
|
||
| static NSString *gWrapperIdentifier = nil; | ||
| static os_unfair_lock gWrapperIdentifierLock = OS_UNFAIR_LOCK_INIT; | ||
|
|
||
| // Supported Apple execution environments | ||
| static NSString *const kAppleEnvironmentUnknown = @"unknown"; | ||
| static NSString *const kAppleEnvironmentIOS = @"ios"; | ||
|
|
@@ -44,6 +52,36 @@ | |
| #define STR(x) STR_EXPAND(x) | ||
| #define STR_EXPAND(x) #x | ||
|
|
||
| // Enforces the format documented on `GIDSignIn.wrapperIdentifier`: returns the accepted value, | ||
| // or nil if `candidate` must be rejected. `candidate` is non-nil. | ||
| static NSString * _Nullable GIDSanitizedWrapperIdentifier(NSString *candidate) { | ||
| static NSCharacterSet *allowedSet; | ||
| static dispatch_once_t onceToken; | ||
| dispatch_once(&onceToken, ^{ | ||
| // Printable ASCII is U+0020 through U+007E: length 0x5F starting at 0x20 (this is a | ||
| // length, not an end index). | ||
| // Note that, if this set changes, the substringToIndex: in the truncation | ||
| // may also need to change. | ||
| allowedSet = [NSCharacterSet characterSetWithRange:NSMakeRange(0x20, 0x5F)]; | ||
| }); | ||
|
|
||
| // The whole value is validated before truncating. | ||
| if ([candidate rangeOfCharacterFromSet:[allowedSet invertedSet]].location != NSNotFound) { | ||
| return nil; | ||
| } | ||
|
|
||
| if (candidate.length == 0) { | ||
| return nil; | ||
| } | ||
|
|
||
| if (candidate.length > 100) { | ||
| // This cannot split a surrogate pair because each value is single-unit ASCII. | ||
| return [candidate substringToIndex:100]; | ||
| } | ||
|
|
||
| return candidate; | ||
| } | ||
|
|
||
| @implementation GIDSignInPreferences | ||
|
|
||
| + (NSString *)sdkVersion { | ||
|
|
@@ -79,11 +117,55 @@ + (NSString *)environment { | |
| return appleEnvironment; | ||
| } | ||
|
|
||
| + (nullable NSString *)wrapperIdentifier { | ||
| os_unfair_lock_lock(&gWrapperIdentifierLock); | ||
| NSString *wrapper = [gWrapperIdentifier copy]; | ||
| os_unfair_lock_unlock(&gWrapperIdentifierLock); | ||
| return wrapper; | ||
| } | ||
|
|
||
| + (void)setWrapperIdentifier:(nullable NSString *)wrapperIdentifier { | ||
| if (wrapperIdentifier == nil) { | ||
| return; | ||
| } | ||
|
|
||
| NSString *sanitized = GIDSanitizedWrapperIdentifier(wrapperIdentifier); | ||
| if (sanitized == nil) { | ||
| NSLog(@"[Google Sign-In iOS]: the SDK wrapper identifier '%@' was rejected, because it must be " | ||
| "non-empty and contain only printable ASCII characters (U+0020 to U+007E).", | ||
| wrapperIdentifier); | ||
| return; | ||
| } | ||
|
|
||
| os_unfair_lock_lock(&gWrapperIdentifierLock); | ||
| NSString *current = gWrapperIdentifier; | ||
| if (current != nil && ![current isEqualToString:sanitized]) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, the "set once" enforcement is here. That's a little surprising, but I think I understand the reasoning. I don't know if this can be addressed, but I do still feel like it's confusing to see the doc comment on the property of the other class, and then for the actual enforcement to be here. ¯_(ツ)_/¯ Probably no big deal. Feel free to close. |
||
| os_unfair_lock_unlock(&gWrapperIdentifierLock); | ||
| NSLog(@"[Google Sign-In iOS]: the SDK wrapper identifier is already set to '%@', so '%@' was " | ||
| "ignored; more than one wrapper appears to be present.", current, sanitized); | ||
| return; | ||
| } | ||
| gWrapperIdentifier = [sanitized copy]; | ||
| os_unfair_lock_unlock(&gWrapperIdentifierLock); | ||
| } | ||
|
|
||
| + (void)resetWrapperIdentifier { | ||
| os_unfair_lock_lock(&gWrapperIdentifierLock); | ||
| gWrapperIdentifier = nil; | ||
| os_unfair_lock_unlock(&gWrapperIdentifierLock); | ||
| } | ||
|
|
||
| + (NSDictionary<NSString *, NSString *> *)loggingParameters { | ||
| return @{ | ||
| NSMutableDictionary<NSString *, NSString *> *parameters = [@{ | ||
| kSDKVersionLoggingParameter : [self sdkVersion], | ||
| kEnvironmentLoggingParameter : [self environment], | ||
| }; | ||
| kEnvironmentLoggingParameter : [self environment] | ||
| } mutableCopy]; | ||
|
|
||
| NSString *wrapperIdentifier = [self wrapperIdentifier]; | ||
| if (wrapperIdentifier) { | ||
| parameters[kSDKWrapperLoggingParameter] = wrapperIdentifier; | ||
| } | ||
| return [parameters copy]; | ||
| } | ||
|
|
||
| + (NSString *)googleAuthorizationServer { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ | |
| #import "GoogleSignIn/Sources/Public/GoogleSignIn/GIDToken.h" | ||
|
|
||
| #import "GoogleSignIn/Sources/GIDGoogleUser_Private.h" | ||
| #import "GoogleSignIn/Sources/GIDSignInPreferences.h" | ||
| #import "GoogleSignIn/Tests/Unit/GIDGoogleUser+Testing.h" | ||
| #import "GoogleSignIn/Tests/Unit/GIDProfileData+Testing.h" | ||
| #import "GoogleSignIn/Tests/Unit/OIDAuthState+Testing.h" | ||
|
|
@@ -67,10 +68,13 @@ @interface GIDGoogleUserTest : XCTestCase | |
| @implementation GIDGoogleUserTest { | ||
| // The saved token fetch handler. | ||
| OIDTokenCallback _tokenFetchHandler; | ||
| // The saved token request. | ||
| OIDTokenRequest *_savedTokenRequest; | ||
| } | ||
|
|
||
| - (void)setUp { | ||
| _tokenFetchHandler = nil; | ||
| _savedTokenRequest = nil; | ||
|
|
||
| // We need to use swizzle here because OCMock can not stub class method with arguments. | ||
| [GULSwizzler swizzleClass:[OIDAuthorizationService class] | ||
|
|
@@ -80,7 +84,8 @@ - (void)setUp { | |
| OIDTokenRequest *request, | ||
| OIDAuthorizationResponse *authorizationResponse, | ||
| OIDTokenCallback callback) { | ||
| // Save the OIDTokenCallback. | ||
| // Save the OIDTokenRequest and OIDTokenCallback. | ||
| self->_savedTokenRequest = request; | ||
| self->_tokenFetchHandler = [callback copy]; | ||
| }]; | ||
| } | ||
|
|
@@ -89,6 +94,7 @@ - (void)tearDown { | |
| [GULSwizzler unswizzleClass:[OIDAuthorizationService class] | ||
| selector:@selector(performTokenRequest:originalAuthorizationResponse:callback:) | ||
| isClassSelector:YES]; | ||
| [GIDSignInPreferences resetWrapperIdentifier]; | ||
| } | ||
|
|
||
| #pragma mark - Tests | ||
|
|
@@ -478,6 +484,95 @@ - (void)testRefreshTokensIfNeededWithCompletion_noRefresh_givenRefreshTokenExpir | |
| [self waitForExpectationsWithTimeout:1 handler:nil]; | ||
| } | ||
|
|
||
| - (void)testWrapperIdentifier_PresentOnRefreshRequestWhenSet { | ||
| GIDSignIn.wrapperIdentifier = @"firebase"; | ||
|
|
||
| // Both tokens expired 10 seconds ago. | ||
| GIDGoogleUser *user = [self googleUserWithAccessTokenExpiresIn:-10 idTokenExpiresIn:-10]; | ||
|
|
||
| XCTestExpectation *expectation = [self expectationWithDescription:@"Callback is called"]; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make the description more descriptive? I know the pattern in the library is to use "Callback is called," but I've had trouble in past with debugging failed tests when I see a wall of that output. It's hard to find which callback wasn't called. |
||
|
|
||
| // Save the intermediate states. | ||
| [user refreshTokensIfNeededWithCompletion:^(GIDGoogleUser * _Nullable user, | ||
| NSError * _Nullable error) { | ||
| [expectation fulfill]; | ||
| }]; | ||
|
|
||
| XCTAssertEqualObjects(_savedTokenRequest.additionalParameters[@"gidwrapper"], @"firebase"); | ||
|
|
||
| // Clean up the handler by providing a fake response to fulfill any internal state. | ||
| OIDTokenResponse *fakeResponse = [OIDTokenResponse testInstanceWithIDToken:nil | ||
| accessToken:kNewAccessToken | ||
| expiresIn:@(kAccessTokenExpiresIn) | ||
| refreshToken:kRefreshToken | ||
| tokenRequest:_savedTokenRequest]; | ||
| _tokenFetchHandler(fakeResponse, nil); | ||
| [self waitForExpectationsWithTimeout:1 handler:nil]; | ||
| } | ||
|
|
||
| - (void)testWrapperIdentifier_AbsentOnRefreshRequestWhenUnset { | ||
| [GIDSignInPreferences resetWrapperIdentifier]; | ||
|
|
||
| // Both tokens expired 10 seconds ago. | ||
| GIDGoogleUser *user = [self googleUserWithAccessTokenExpiresIn:-10 idTokenExpiresIn:-10]; | ||
|
|
||
| XCTestExpectation *expectation = [self expectationWithDescription:@"Callback is called"]; | ||
|
|
||
| // Save the intermediate states. | ||
| [user refreshTokensIfNeededWithCompletion:^(GIDGoogleUser * _Nullable user, | ||
| NSError * _Nullable error) { | ||
| [expectation fulfill]; | ||
| }]; | ||
|
|
||
| XCTAssertNil(_savedTokenRequest.additionalParameters[@"gidwrapper"]); | ||
|
|
||
| // Clean up the handler by providing a fake response. | ||
| OIDTokenResponse *fakeResponse = [OIDTokenResponse testInstanceWithIDToken:nil | ||
| accessToken:kNewAccessToken | ||
| expiresIn:@(kAccessTokenExpiresIn) | ||
| refreshToken:kRefreshToken | ||
| tokenRequest:_savedTokenRequest]; | ||
| _tokenFetchHandler(fakeResponse, nil); | ||
| [self waitForExpectationsWithTimeout:1 handler:nil]; | ||
| } | ||
|
|
||
| - (void)testWrapperIdentifier_AbsentOnRefreshRequestWhenDropped { | ||
| // Assert that attempting to set a dropped identifier is ignored. | ||
| XCTAssertNoThrow(GIDSignIn.wrapperIdentifier = @"firebasé"); | ||
|
|
||
| // The rejection leaves the store nil. | ||
| XCTAssertNil(GIDSignIn.wrapperIdentifier); | ||
|
|
||
| // Both tokens expired 10 seconds ago. | ||
| GIDGoogleUser *user = [self googleUserWithAccessTokenExpiresIn:-10 idTokenExpiresIn:-10]; | ||
|
|
||
| XCTestExpectation *expectation = [self expectationWithDescription:@"Callback is called"]; | ||
|
|
||
| // Save the intermediate states. | ||
| [user refreshTokensIfNeededWithCompletion:^(GIDGoogleUser * _Nullable user, | ||
| NSError * _Nullable error) { | ||
| [expectation fulfill]; | ||
| }]; | ||
|
|
||
| // Assert the captured token request additionalParameters does NOT contain key @"gidwrapper". | ||
| XCTAssertNil(_savedTokenRequest.additionalParameters[@"gidwrapper"]); | ||
|
|
||
| // Assert it DOES contain kSDKVersionLoggingParameter and kEnvironmentLoggingParameter. | ||
| XCTAssertEqualObjects(_savedTokenRequest.additionalParameters[kSDKVersionLoggingParameter], | ||
| [GIDSignInPreferences sdkVersion]); | ||
| XCTAssertEqualObjects(_savedTokenRequest.additionalParameters[kEnvironmentLoggingParameter], | ||
| [GIDSignInPreferences environment]); | ||
|
|
||
| // Clean up the handler by providing a fake response. | ||
| OIDTokenResponse *fakeResponse = [OIDTokenResponse testInstanceWithIDToken:nil | ||
| accessToken:kNewAccessToken | ||
| expiresIn:@(kAccessTokenExpiresIn) | ||
| refreshToken:kRefreshToken | ||
| tokenRequest:_savedTokenRequest]; | ||
| _tokenFetchHandler(fakeResponse, nil); | ||
| [self waitForExpectationsWithTimeout:1 handler:nil]; | ||
| } | ||
|
|
||
| # pragma mark - Test `addScopes:` | ||
|
|
||
| - (void)testAddScopes_success { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to enforce the "set once" language in property's doc comment?