From 6c8dc63538aa9fd05fbcc34986a848826cf75a36 Mon Sep 17 00:00:00 2001 From: Khalid Awwad Date: Tue, 24 Oct 2017 09:19:04 +0300 Subject: [PATCH 1/2] FIX(iOS): NSDictionary of contexts is not thread safe Issue #166 NSDictionary is not thread safe, but its accessed in a background thread. This can cause crashes. The crash is seen in our telemetry. Please see issue logs --- src/ios/CordovaAdalPlugin.h | 5 +- src/ios/CordovaAdalPlugin.m | 200 +++++++++++++++++++----------------- 2 files changed, 105 insertions(+), 100 deletions(-) diff --git a/src/ios/CordovaAdalPlugin.h b/src/ios/CordovaAdalPlugin.h index fcfe9d7..85a2bc8 100644 --- a/src/ios/CordovaAdalPlugin.h +++ b/src/ios/CordovaAdalPlugin.h @@ -12,6 +12,8 @@ // Implements Apache Cordova plugin for Microsoft Azure ADAL @interface CordovaAdalPlugin : CDVPlugin +- (void)pluginInitialize; + // AuthenticationContext methods - (void)createAsync:(CDVInvokedUrlCommand *)command; - (void)acquireTokenAsync:(CDVInvokedUrlCommand *)command; @@ -22,9 +24,6 @@ - (void)tokenCacheReadItems:(CDVInvokedUrlCommand *)command; - (void)tokenCacheDeleteItem:(CDVInvokedUrlCommand *)command; -+ (ADAuthenticationContext *)getOrCreateAuthContext:(NSString *)authority - validateAuthority:(BOOL)validate; - - (void)setLogger:(CDVInvokedUrlCommand *)command; - (void)setLogLevel:(CDVInvokedUrlCommand *) command; @end diff --git a/src/ios/CordovaAdalPlugin.m b/src/ios/CordovaAdalPlugin.m index 4f3bd4e..b7e0412 100644 --- a/src/ios/CordovaAdalPlugin.m +++ b/src/ios/CordovaAdalPlugin.m @@ -9,8 +9,19 @@ #import +@interface CordovaAdalPlugin() { + NSMutableDictionary *existingContexts; +} + +@end + @implementation CordovaAdalPlugin +- (void)pluginInitialize +{ + existingContexts = [NSMutableDictionary dictionaryWithCapacity:0]; +} + - (void)createAsync:(CDVInvokedUrlCommand *)command { [self.commandDelegate runInBackground:^{ @@ -18,12 +29,12 @@ - (void)createAsync:(CDVInvokedUrlCommand *)command { NSString *authority = ObjectOrNil([command.arguments objectAtIndex:0]); BOOL validateAuthority = [[command.arguments objectAtIndex:1] boolValue]; - - [CordovaAdalPlugin getOrCreateAuthContext:authority - validateAuthority:validateAuthority]; - + + [self getOrCreateAuthContext:authority + validateAuthority:validateAuthority]; + CDVPluginResult *pluginResult = [CDVPluginResult resultWithStatus:CDVCommandStatus_OK]; - + [self.commandDelegate sendPluginResult:pluginResult callbackId:command.callbackId]; } @catch (ADAuthenticationError *error) @@ -47,14 +58,14 @@ - (void)acquireTokenAsync:(CDVInvokedUrlCommand *)command NSURL *redirectUri = [NSURL URLWithString:[command.arguments objectAtIndex:4]]; NSString *userId = ObjectOrNil([command.arguments objectAtIndex:5]); NSString *extraQueryParameters = ObjectOrNil([command.arguments objectAtIndex:6]); - - ADAuthenticationContext *authContext = [CordovaAdalPlugin getOrCreateAuthContext:authority - validateAuthority:validateAuthority]; + + ADAuthenticationContext *authContext = [self getOrCreateAuthContext:authority + validateAuthority:validateAuthority]; // `x-msauth-` redirect url prefix means we should use brokered authentication // https://github.com/AzureAD/azure-activedirectory-library-for-objc#brokered-authentication authContext.credentialsType = (redirectUri.scheme && [redirectUri.scheme hasPrefix: @"x-msauth-"]) ? - AD_CREDENTIALS_AUTO : AD_CREDENTIALS_EMBEDDED; - + AD_CREDENTIALS_AUTO : AD_CREDENTIALS_EMBEDDED; + // TODO iOS sdk requires user name instead of guid so we should map provided id to a known user name userId = [CordovaAdalUtils mapUserIdToUserName:authContext userId:userId]; @@ -67,7 +78,7 @@ - (void)acquireTokenAsync:(CDVInvokedUrlCommand *)command userId:userId extraQueryParameters:extraQueryParameters completionBlock:^(ADAuthenticationResult *result) { - + NSMutableDictionary *msg = [CordovaAdalUtils ADAuthenticationResultToDictionary: result]; CDVCommandStatus status = (AD_SUCCEEDED != result.status) ? CDVCommandStatus_ERROR : CDVCommandStatus_OK; CDVPluginResult *pluginResult = [CDVPluginResult resultWithStatus:status messageAsDictionary: msg]; @@ -94,14 +105,14 @@ - (void)acquireTokenSilentAsync:(CDVInvokedUrlCommand *)command NSString *resourceId = ObjectOrNil([command.arguments objectAtIndex:2]); NSString *clientId = ObjectOrNil([command.arguments objectAtIndex:3]); NSString *userId = ObjectOrNil([command.arguments objectAtIndex:4]); - - ADAuthenticationContext *authContext = [CordovaAdalPlugin getOrCreateAuthContext:authority - validateAuthority:validateAuthority]; - + + ADAuthenticationContext *authContext = [self getOrCreateAuthContext:authority + validateAuthority:validateAuthority]; + // TODO iOS sdk requires user name instead of guid so we should map provided id to a known user name userId = [CordovaAdalUtils mapUserIdToUserName:authContext userId:userId]; - + [authContext acquireTokenSilentWithResource:resourceId clientId:clientId redirectUri:nil @@ -128,23 +139,23 @@ - (void)tokenCacheClear:(CDVInvokedUrlCommand *)command @try { ADAuthenticationError *error; - + ADKeychainTokenCache* cacheStore = [ADKeychainTokenCache new]; - + NSArray *cacheItems = [cacheStore allItems:&error]; - + for (int i = 0; i < cacheItems.count; i++) { [cacheStore removeItem: cacheItems[i] error: &error]; } - + if (error != nil) { @throw(error); } - + CDVPluginResult *pluginResult = [CDVPluginResult resultWithStatus:CDVCommandStatus_OK]; - + [self.commandDelegate sendPluginResult:pluginResult callbackId:command.callbackId]; } @catch (ADAuthenticationError *error) @@ -162,27 +173,27 @@ - (void)tokenCacheReadItems:(CDVInvokedUrlCommand *)command @try { ADAuthenticationError *error; - + ADKeychainTokenCache* cacheStore = [ADKeychainTokenCache new]; - + //get all items from cache NSArray *cacheItems = [cacheStore allItems:&error]; - + NSMutableArray *items = [NSMutableArray arrayWithCapacity:cacheItems.count]; - + if (error != nil) { @throw(error); } - + for (id obj in cacheItems) { [items addObject:[CordovaAdalUtils ADTokenCacheStoreItemToDictionary:obj]]; } - + CDVPluginResult *pluginResult = [CDVPluginResult resultWithStatus:CDVCommandStatus_OK messageAsArray:items]; - + [self.commandDelegate sendPluginResult:pluginResult callbackId:command.callbackId]; } @catch (ADAuthenticationError *error) @@ -199,55 +210,55 @@ - (void)tokenCacheDeleteItem:(CDVInvokedUrlCommand *)command @try { ADAuthenticationError *error; - + NSString *authority = ObjectOrNil([command.arguments objectAtIndex:0]); BOOL validateAuthority = [[command.arguments objectAtIndex:1] boolValue]; NSString *itemAuthority = ObjectOrNil([command.arguments objectAtIndex:2]); NSString *resourceId = ObjectOrNil([command.arguments objectAtIndex:3]); NSString *clientId = ObjectOrNil([command.arguments objectAtIndex:4]); NSString *userId = ObjectOrNil([command.arguments objectAtIndex:5]); - - ADAuthenticationContext *authContext = [CordovaAdalPlugin getOrCreateAuthContext:authority - validateAuthority:validateAuthority]; - + + ADAuthenticationContext *authContext = [self getOrCreateAuthContext:authority + validateAuthority:validateAuthority]; + // TODO iOS sdk requires user name instead of guid so we should map provided id to a known user name userId = [CordovaAdalUtils mapUserIdToUserName:authContext userId:userId]; - + ADKeychainTokenCache* cacheStore = [ADKeychainTokenCache new]; - + //get all items from cache NSArray *cacheItems = [cacheStore allItems:&error]; - + if (error != nil) { @throw(error); } - + for (ADTokenCacheItem* item in cacheItems) { NSDictionary *itemAllClaims = [[item userInformation] allClaims]; - + NSString * userUniqueName = (itemAllClaims && itemAllClaims[@"unique_name"]) ? itemAllClaims[@"unique_name"] : nil; - + if ([itemAuthority isEqualToString:[item authority]] && ((userUniqueName != nil && [userUniqueName isEqualToString:userId]) || [userId isEqualToString:[[item userInformation] userId]]) && [clientId isEqualToString:[item clientId]] // resource could be nil which is fine && ((!resourceId && ![item resource]) || [resourceId isEqualToString:[item resource]])) { - + //remove item [cacheStore removeItem:item error: &error]; - + if (error != nil) { @throw(error); } } - + } - + CDVPluginResult *pluginResult = [CDVPluginResult resultWithStatus:CDVCommandStatus_OK]; [self.commandDelegate sendPluginResult:pluginResult callbackId:command.callbackId]; } @@ -262,70 +273,65 @@ - (void)tokenCacheDeleteItem:(CDVInvokedUrlCommand *)command - (void) setLogger:(CDVInvokedUrlCommand *)command { - [self.commandDelegate runInBackground:^{ - [ADLogger setLogCallBack:^(ADAL_LOG_LEVEL logLevel, NSString *message, NSString *additionalInfo, NSInteger errorCode, NSDictionary *userInfo) { - NSMutableDictionary *logItem = [[NSMutableDictionary alloc] init]; - - [logItem setObject:[NSNumber numberWithInt:logLevel] forKey:@"level"]; - [logItem setValue:message forKey:@"message"]; - [logItem setValue:additionalInfo forKey:@"additionalInfo"]; - [logItem setValue:[NSNumber numberWithInteger:errorCode] forKey:@"errorCode"]; - - CDVPluginResult *pluginResult = [CDVPluginResult resultWithStatus:CDVCommandStatus_OK - messageAsDictionary:logItem]; - [pluginResult setKeepCallbackAsBool:YES]; - [self.commandDelegate sendPluginResult:pluginResult callbackId:command.callbackId]; - }]; - }]; + [self.commandDelegate runInBackground:^{ + [ADLogger setLogCallBack:^(ADAL_LOG_LEVEL logLevel, NSString *message, NSString *additionalInfo, NSInteger errorCode, NSDictionary *userInfo) { + NSMutableDictionary *logItem = [[NSMutableDictionary alloc] init]; + + [logItem setObject:[NSNumber numberWithInt:logLevel] forKey:@"level"]; + [logItem setValue:message forKey:@"message"]; + [logItem setValue:additionalInfo forKey:@"additionalInfo"]; + [logItem setValue:[NSNumber numberWithInteger:errorCode] forKey:@"errorCode"]; + + CDVPluginResult *pluginResult = [CDVPluginResult resultWithStatus:CDVCommandStatus_OK + messageAsDictionary:logItem]; + [pluginResult setKeepCallbackAsBool:YES]; + [self.commandDelegate sendPluginResult:pluginResult callbackId:command.callbackId]; + }]; + }]; } - (void) setLogLevel:(CDVInvokedUrlCommand *)command { - [self.commandDelegate runInBackground:^{ - @try { - NSNumber *logLevel = [command.arguments objectAtIndex:0]; - [ADLogger setLevel:[logLevel intValue]]; - CDVPluginResult *pluginResult = [CDVPluginResult resultWithStatus:CDVCommandStatus_OK]; - - [self.commandDelegate sendPluginResult:pluginResult callbackId:command.callbackId]; - } - - @catch(ADAuthenticationError* error) { - CDVPluginResult *pluginResult = [CDVPluginResult resultWithStatus:CDVCommandStatus_ERROR - messageAsDictionary:[CordovaAdalUtils ADAuthenticationErrorToDictionary:error]]; - [self.commandDelegate sendPluginResult:pluginResult callbackId:command.callbackId]; - } - }]; + [self.commandDelegate runInBackground:^{ + @try { + NSNumber *logLevel = [command.arguments objectAtIndex:0]; + [ADLogger setLevel:[logLevel intValue]]; + CDVPluginResult *pluginResult = [CDVPluginResult resultWithStatus:CDVCommandStatus_OK]; + + [self.commandDelegate sendPluginResult:pluginResult callbackId:command.callbackId]; + } + + @catch(ADAuthenticationError* error) { + CDVPluginResult *pluginResult = [CDVPluginResult resultWithStatus:CDVCommandStatus_ERROR + messageAsDictionary:[CordovaAdalUtils ADAuthenticationErrorToDictionary:error]]; + [self.commandDelegate sendPluginResult:pluginResult callbackId:command.callbackId]; + } + }]; } -static NSMutableDictionary *existingContexts = nil; - -+ (ADAuthenticationContext *)getOrCreateAuthContext:(NSString *)authority +- (ADAuthenticationContext *)getOrCreateAuthContext:(NSString *)authority validateAuthority:(BOOL)validate { - if (!existingContexts) - { - existingContexts = [NSMutableDictionary dictionaryWithCapacity:1]; - } - - ADAuthenticationContext *authContext = [existingContexts objectForKey:authority]; - - if (!authContext) - { - ADAuthenticationError *error; - - authContext = [ADAuthenticationContext authenticationContextWithAuthority:authority - validateAuthority:validate - error:&error]; - if (error != nil) + @synchronized(self->existingContexts) { + ADAuthenticationContext *authContext = [existingContexts objectForKey:authority]; + + if (!authContext) { - @throw(error); + ADAuthenticationError *error; + + authContext = [ADAuthenticationContext authenticationContextWithAuthority:authority + validateAuthority:validate + error:&error]; + if (error != nil) + { + @throw(error); + } + + [existingContexts setObject:authContext forKey:authority]; } - - [existingContexts setObject:authContext forKey:authority]; + + return authContext; } - - return authContext; } static id ObjectOrNil(id object) From 424f990b9144296322679e337c4692478961aba3 Mon Sep 17 00:00:00 2001 From: Khalid Awwad Date: Tue, 24 Oct 2017 09:27:19 +0300 Subject: [PATCH 2/2] change how intialize NSDictionary --- src/ios/CordovaAdalPlugin.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ios/CordovaAdalPlugin.m b/src/ios/CordovaAdalPlugin.m index b7e0412..fc5b693 100644 --- a/src/ios/CordovaAdalPlugin.m +++ b/src/ios/CordovaAdalPlugin.m @@ -19,7 +19,7 @@ @implementation CordovaAdalPlugin - (void)pluginInitialize { - existingContexts = [NSMutableDictionary dictionaryWithCapacity:0]; + existingContexts = [NSMutableDictionary new]; } - (void)createAsync:(CDVInvokedUrlCommand *)command