Skip to content

Commit

Permalink
AWSMobile Client fixes - Balance semaphore's wait and signal calls. (a…
Browse files Browse the repository at this point in the history 
…ws-amplify#1824)

GetIdentity inside AWSIdentityProvider.m process one request at a time. If a new request comes in while we are processing another, we ask the new request to wait using semaphore dispatch_semaphore_wait(self.semaphore, dispatch_time(DISPATCH_TIME_NOW, 5 * NSEC_PER_SEC));

This semaphore is signaled when we receive a request back for identity dispatch_semaphore_signal(self.semaphore);. But in the case where there is only one getIdentityId request, we signal the semaphore without calling a wait and thus increasing the semaphore value. So now when a new concurrent getIdentity is encountered, the semaphore is not decreased to a negative value and it wont wait.
  • Loading branch information
@royjit
royjit authored Sep 3, 2019
1 parent ef151d7 commit 955384d
Show file tree
Hide file tree
Showing 6 changed files with 231 additions and 21 deletions.
2 changes: 1 addition & 1 deletion AWSAuthSDK/Sources/AWSMobileClient/AWSMobileClientExtensions.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -489,11 +489,11 @@ extension AWSMobileClient {
_AWSMobileClient.sharedInstance().setCustomRoleArnInternal(nil, for: self)
self.saveLoginsMapInKeychain()
self.setLoginProviderMetadataAndSaveInKeychain(provider: .none)
self.internalCredentialsProvider?.clearKeychain()
self.performUserPoolSignOut()
self.internalCredentialsProvider?.identityProvider.identityId = nil
self.internalCredentialsProvider?.clearKeychain()
self.mobileClientStatusChanged(userState: .signedOut, additionalInfo: [:])
self.federationProvider = .none
}

internal func performUserPoolSignOut() {
Expand Down
37 changes: 37 additions & 0 deletions AWSAuthSDK/Tests/AWSMobileClientTests/AWSMobileClientCredentialsTest.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,4 +262,41 @@ class AWSMobileClientCredentialsTest: AWSMobileClientBaseTests {
AWSMobileClient.sharedInstance().removeUserStateListener(self)
}


func testMultipleGetCredentials() {
AWSDDLog.sharedInstance.logLevel = .verbose
AWSDDLog.add(AWSDDTTYLogger.sharedInstance)
let username = "testUser" + UUID().uuidString
let credentialFetchBeforeSignIn = expectation(description: "Request to getAWSCredentials before signIn")
let credentialFetchAfterSignIn = expectation(description: "Request to getAWSCredentials after signIn")
let credentialFetchAfterSignIn2 = expectation(description: "Request to getAWSCredentials after signIn")
let credentialFetchAfterSignOut = expectation(description: "Request to getAWSCredentials after signOut")
AWSMobileClient.sharedInstance().getAWSCredentials({ (awscredentials, error) in
XCTAssertNil(error)
XCTAssertNotNil(awscredentials, "Credentials should not return nil.")
credentialFetchBeforeSignIn.fulfill()
})
signUpAndVerifyUser(username: username)
signIn(username: username)
AWSMobileClient.sharedInstance().getAWSCredentials({ (awscredentials, error) in
XCTAssertNil(error)
XCTAssertNotNil(awscredentials, "Credentials should not return nil.")
credentialFetchAfterSignIn.fulfill()
})
wait(for: [credentialFetchAfterSignIn], timeout: 15)

AWSMobileClient.sharedInstance().getAWSCredentials({ (awscredentials, error) in
// We do not need to check the values here, this can succeed
// or fail based on whether this method completes before the below signOut.
credentialFetchAfterSignIn2.fulfill()
})
AWSMobileClient.sharedInstance().signOut()
AWSMobileClient.sharedInstance().getAWSCredentials({ (awscredentials, error) in
XCTAssertNil(error)
XCTAssertNotNil(awscredentials, "Credentials should not return nil.")
credentialFetchAfterSignOut.fulfill()
})
wait(for: [credentialFetchAfterSignOut, credentialFetchAfterSignIn2, credentialFetchBeforeSignIn],
timeout: 15)
}
}
124 changes: 121 additions & 3 deletions AWSAuthSDK/Tests/AWSMobileClientTests/AWSMobileClientTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ class AWSMobileClientTests: AWSMobileClientBaseTests {
print("^^^^")
tokensExpectation.fulfill()
}
wait(for: [tokensExpectation], timeout: 5000)
wait(for: [tokensExpectation], timeout: 15)
}

func testFederatedSignInDeveloperAuthenticatedIdentities() {
Expand Down Expand Up @@ -100,7 +100,7 @@ class AWSMobileClientTests: AWSMobileClientBaseTests {
}
credentialsExpectation.fulfill()
}
wait(for: [credentialsExpectation], timeout: 5)
wait(for: [credentialsExpectation], timeout: 10)

AWSMobileClient.sharedInstance().signOut()

Expand All @@ -121,7 +121,7 @@ class AWSMobileClientTests: AWSMobileClientBaseTests {
}
credentialsExpectation2.fulfill()
}
wait(for: [credentialsExpectation2], timeout: 5)
wait(for: [credentialsExpectation2], timeout: 10)

AWSMobileClient.sharedInstance().signOut()
}
Expand Down Expand Up @@ -167,4 +167,122 @@ class AWSMobileClientTests: AWSMobileClientBaseTests {

XCTAssertNotNil(AWSMobileClient.sharedInstance().identityId, "Identity Id should not be nil.")
}

func testMultipleGetIdentityId() {
XCTAssertNil(AWSMobileClient.sharedInstance().identityId, "Identity Id should be nil after initialize.")

let identityIdExpectation1 = expectation(description: "Request to GetIdentityID 1 is complete")
let identityIdExpectation2 = expectation(description: "Request to GetIdentityID 2 is complete")
let identityIdExpectation3 = expectation(description: "Request to GetIdentityID 3 is complete")
let identityIdExpectation4 = expectation(description: "Request to GetIdentityID 4 is complete")
let identityIdExpectation5 = expectation(description: "Request to GetIdentityID 5 is complete")
AWSMobileClient.sharedInstance().getIdentityId().continueWith(block: { (task) -> Any? in
XCTAssertNil(task.error)
XCTAssertNotNil(task.result, "GetIdentityId should not return nil.")
identityIdExpectation1.fulfill()
return nil
})
AWSMobileClient.sharedInstance().getIdentityId().continueWith(block: { (task) -> Any? in
XCTAssertNil(task.error)
XCTAssertNotNil(task.result, "GetIdentityId should not return nil.")
identityIdExpectation2.fulfill()
return nil
})
AWSMobileClient.sharedInstance().getIdentityId().continueWith(block: { (task) -> Any? in
XCTAssertNil(task.error)
XCTAssertNotNil(task.result, "GetIdentityId should not return nil.")
identityIdExpectation3.fulfill()
return nil
})
AWSMobileClient.sharedInstance().getIdentityId().continueWith(block: { (task) -> Any? in
XCTAssertNil(task.error)
XCTAssertNotNil(task.result, "GetIdentityId should not return nil.")
identityIdExpectation4.fulfill()
return nil
})
AWSMobileClient.sharedInstance().getIdentityId().continueWith(block: { (task) -> Any? in
XCTAssertNil(task.error)
XCTAssertNotNil(task.result, "GetIdentityId should not return nil.")
identityIdExpectation5.fulfill()
return nil
})
wait(for: [identityIdExpectation1, identityIdExpectation2, identityIdExpectation3, identityIdExpectation4, identityIdExpectation5],
timeout: 15)
}

/// Test whether we are getting same identity id for unauth to auth transition.
///
/// - Given: An unauthenticated user session
/// - When:
/// - I fetch Identity Id, id1
/// - Then I signIn and fetch identity id , id2
/// - Then I signOut and fetch another id , id3
/// - Then I signIn again and fetch identity id , id4
/// - Then:
/// - All identity id1 == id2 and id2 != id3 and id3 == id3.
///
func testGetIdentityWithSignOutAndSignIn() {
XCTAssertNil(AWSMobileClient.sharedInstance().identityId, "Identity Id should be nil after initialize.")
var identityIdBeforeSignIn: String?
var identityIdAfterSignIn: String?
var identityIdAfterSignOut: String?
var identityIdAfterSignIn2: String?

let signOutIdentityIdExpectation = expectation(description: "Request to GetIdentityID before signIn is complete")
let signInIdentityIdExpectation = expectation(description: "Request to GetIdentityID after signIn is complete")
let signOutIdentityIdExpectation2 = expectation(description: "Request to GetIdentityID before signOut is complete")
let signInIdentityIdExpectation2 = expectation(description: "Request to GetIdentityID before second signIn is complete")

AWSMobileClient.sharedInstance().getIdentityId().continueWith(block: { (task) -> Any? in
XCTAssertNil(task.error)
XCTAssertNotNil(task.result, "GetIdentityId should not return nil.")
identityIdBeforeSignIn = task.result as String?
signOutIdentityIdExpectation.fulfill()
return nil
})
wait(for: [signOutIdentityIdExpectation], timeout: 5)
XCTAssertNotNil(AWSMobileClient.sharedInstance().identityId, "Identity Id should not be nil.")

let username = "testUser" + UUID().uuidString
signUpAndVerifyUser(username: username)
signIn(username: username)

AWSMobileClient.sharedInstance().getIdentityId().continueWith(block: { (task) -> Any? in
XCTAssertNil(task.error)
XCTAssertNotNil(task.result, "GetIdentityId should not return nil.")
identityIdAfterSignIn = task.result as String?
signInIdentityIdExpectation.fulfill()
return nil
})
wait(for: [signInIdentityIdExpectation], timeout: 5)
XCTAssertNotNil(AWSMobileClient.sharedInstance().identityId, "Identity Id should not be nil.")
XCTAssertEqual(identityIdBeforeSignIn, identityIdAfterSignIn)
AWSMobileClient.sharedInstance().signOut()

AWSMobileClient.sharedInstance().getIdentityId().continueWith(block: { (task) -> Any? in
XCTAssertNil(task.error)
XCTAssertNotNil(task.result, "GetIdentityId should not return nil.")
identityIdAfterSignOut = task.result as String?
signOutIdentityIdExpectation2.fulfill()
return nil
})
wait(for: [signOutIdentityIdExpectation2], timeout: 5)
XCTAssertNotNil(AWSMobileClient.sharedInstance().identityId, "Identity Id should not be nil.")
XCTAssertNotEqual(identityIdAfterSignIn, identityIdAfterSignOut)

let username2 = "testUser" + UUID().uuidString
signUpAndVerifyUser(username: username2)
signIn(username: username2)

AWSMobileClient.sharedInstance().getIdentityId().continueWith(block: { (task) -> Any? in
XCTAssertNil(task.error)
XCTAssertNotNil(task.result, "GetIdentityId should not return nil.")
identityIdAfterSignIn2 = task.result as String?
signInIdentityIdExpectation2.fulfill()
return nil
})
wait(for: [signInIdentityIdExpectation2], timeout: 5)
XCTAssertNotNil(AWSMobileClient.sharedInstance().identityId, "Identity Id should not be nil.")
XCTAssertEqual(identityIdAfterSignIn2, identityIdAfterSignOut)
}
}
53 changes: 53 additions & 0 deletions AWSAuthSDK/Tests/AWSMobileClientTests/AWSMobileClientUserAttributeTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,5 +105,58 @@ class AWSMobileClientUserAttributeTests: AWSMobileClientBaseTests {

wait(for: [getUserAttributesResultHandlerInvoked], timeout: 5)
}




/// Test to verify that tokens are valid after an update attritbue
/// Note: This test relies on the configuration of the test UserPools to have two mutable custom attributes:
/// custom:mutableStringAttr1;
///
/// - Given: An authenticated user session
/// - When:
/// - I update user attritbutes
/// - Then:
/// - Get token on the same user session should have valid values
///
func testTokenAfterUpdateAttributes() {
let username = "testUser" + UUID().uuidString
signUpAndVerifyUser(username: username)
signIn(username: username)

let updateUserAttributesResultHandlerInvoked = expectation(description: "updateUserAttributes result handler should be invoked")
let newUserAttributes = ["custom:mutableStringAttr1": "new value for previously set attribute"]

AWSMobileClient.sharedInstance().updateUserAttributes(attributeMap: newUserAttributes) { result, error in
defer {
updateUserAttributesResultHandlerInvoked.fulfill()
}
guard error == nil else {
XCTFail("Received un-expected error: \(error.debugDescription)")
return
}
guard let result = result else {
XCTFail("updateUserAttributes result unexpectedtly nil")
return
}
XCTAssertEqual(result.count, 0)
}
wait(for: [updateUserAttributesResultHandlerInvoked], timeout: 5)

let getTokenExpectation = expectation(description: "Get token result handler should be invoked")
AWSMobileClient.sharedInstance().getTokens { (token, error) in
defer {
getTokenExpectation.fulfill()
}
guard error == nil else {
XCTFail("Received un-expected error: \(error.debugDescription)")
return
}
XCTAssertNotNil(token!.accessToken, "Access token should not be nil for a signed in user")
XCTAssertNotNil(token!.idToken, "Id token should not be nil for a signed in user")
XCTAssertNotNil(token!.expiration, "Expiration date should not be nil for a signed in user")
}
wait(for: [getTokenExpectation], timeout: 10)
}

}
35 changes: 18 additions & 17 deletions AWSCore/Authentication/AWSIdentityProvider.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ - (void)postIdentityIdChangedNotification:(NSString *)newId {
[userInfo setObject:self.identityId forKey:AWSCognitoNotificationPreviousId];
}
[userInfo setObject:newId forKey:AWSCognitoNotificationNewId];

dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
[[NSNotificationCenter defaultCenter] postNotificationName:AWSCognitoIdentityIdChangedNotification
object:self
Expand Down Expand Up @@ -133,13 +133,13 @@ - (instancetype)initWithRegionType:(AWSRegionType)regionType
_useEnhancedFlow = useEnhancedFlow;
self.identityPoolId = identityPoolId;
self.identityProviderManager = identityProviderManager;

AWSAnonymousCredentialsProvider *credentialsProvider = [AWSAnonymousCredentialsProvider new];
AWSServiceConfiguration *configuration = [[AWSServiceConfiguration alloc] initWithRegion:regionType
credentialsProvider:credentialsProvider];
_cognitoIdentity = [[AWSCognitoIdentity alloc] initWithConfiguration:configuration];
}

return self;
}

Expand All @@ -159,7 +159,7 @@ - (NSString *)identityProviderName {
code:AWSCognitoCredentialsProviderHelperErrorTypeIdentityIsNil
userInfo:@{NSLocalizedDescriptionKey: @"identityId shouldn't be nil"}]];
}

if (self.identityProviderManager) {
return [self.identityProviderManager logins];
} else {
Expand All @@ -177,36 +177,36 @@ - (NSString *)identityProviderName {
return [AWSTask taskWithResult:[task.result objectForKey:[self identityProviderName]]];
}
}

AWSCognitoIdentityGetOpenIdTokenInput *getTokenInput = [AWSCognitoIdentityGetOpenIdTokenInput new];
getTokenInput.identityId = self.identityId;
getTokenInput.logins = logins;

return [[[self.cognitoIdentity getOpenIdToken:getTokenInput] continueWithBlock:^id(AWSTask *task) {
// When an invalid identityId is cached in the keychain for auth,
// we will refresh the identityId and try to get OpenID token again.
if (task.error) {
AWSDDLogError(@"GetOpenIdToken failed. Error is [%@]", task.error);

// If it's auth or we caught a not found or validation error
// we want to reset the identity id, otherwise, just return
// the error to our caller
if (![AWSCognitoCredentialsProvider shouldResetIdentityId:task.error
authenticated:[self isAuthenticated]]) {
return task;
}

if (self.hasClearedIdentityId) {
return [AWSTask taskWithError:[NSError errorWithDomain:AWSCognitoCredentialsProviderErrorDomain
code:AWSCognitoCredentialsProviderInvalidConfiguration
userInfo:@{NSLocalizedDescriptionKey : @"GetCredentialsForIdentity keeps failing. Clearing identityId did not help. Please check your Amazon Cognito Identity configuration."}]];
}

AWSDDLogDebug(@"Resetting identity Id and calling getIdentityId");
// if it's auth, reset id and refetch
self.identityId = nil;
self.hasClearedIdentityId = YES;

return [[self getIdentityId] continueWithSuccessBlock:^id(AWSTask *task) {
// This should never happen, but just in case
if (!self.identityId) {
Expand All @@ -217,14 +217,14 @@ - (NSString *)identityProviderName {
userInfo:@{NSLocalizedDescriptionKey: @"identityId shouldn't be nil"}]
];
}

AWSDDLogDebug(@"Retrying GetOpenIdToken");

// retry get token
AWSCognitoIdentityGetOpenIdTokenInput *tokenRetry = [AWSCognitoIdentityGetOpenIdTokenInput new];
tokenRetry.identityId = self.identityId;
tokenRetry.logins = self.cachedLogins;

return [self.cognitoIdentity getOpenIdToken:tokenRetry];
}];
}
Expand All @@ -234,7 +234,7 @@ - (NSString *)identityProviderName {
AWSCognitoIdentityGetOpenIdTokenResponse *getTokenResponse = task.result;
NSString *token = getTokenResponse.token;
NSString *identityIdFromToken = getTokenResponse.identityId;

// This should never happen, but just in case
if (!identityIdFromToken) {
AWSDDLogError(@"identityId from getOpenIdToken is nil");
Expand All @@ -243,11 +243,11 @@ - (NSString *)identityProviderName {
userInfo:@{NSLocalizedDescriptionKey: @"identityId shouldn't be nil"}]
];
}

if (![self.identityId isEqualToString:identityIdFromToken]) {
self.identityId = identityIdFromToken;
}

return [AWSTask taskWithResult:token];
}];
}];
Expand All @@ -267,7 +267,7 @@ - (NSString *)identityProviderName {
}
}];
}

return [[self token] continueWithSuccessBlock:^id _Nullable(AWSTask<NSString *> * _Nonnull task) {
if (!task.result) {
return [AWSTask taskWithResult:nil];
Expand Down Expand Up @@ -295,6 +295,7 @@ - (NSString *)identityProviderName {
// Create an identity id via GetID if the call to logins didn't set it which DevAuth does
// And there are no other calls in flight to create one
if (!self.identityId && self.count <= 1) {
dispatch_semaphore_wait(self.semaphore, dispatch_time(DISPATCH_TIME_NOW, 0));
AWSCognitoIdentityGetIdInput *getIdInput = [AWSCognitoIdentityGetIdInput new];
getIdInput.identityPoolId = self.identityPoolId;
getIdInput.logins = logins;
Expand Down
Loading

0 comments on commit 955384d

Please sign in to comment.