Merge pull request #1146 from AzureAD/oldalton/logger_callback_deprec…

…ation

 Deprecate old logger callback properly

ADAL/ADALAutomation/ADAutoMainViewController.m

@@ -48,7 +48,7 @@ - (void)viewDidLoad
     [super viewDidLoad];
     // Do any additional setup after loading the view, typically from a nib.
 
-    [ADLogger setLogCallBack:^(ADAL_LOG_LEVEL __unused logLevel, NSString *message, BOOL __unused containsPii)
+    [ADLogger setLoggerCallback:^(ADAL_LOG_LEVEL __unused logLevel, NSString *message, BOOL __unused containsPii)
     {
         if (_resultLogs)
         {

ADAL/src/ADLogger+Internal.h

@@ -75,9 +75,6 @@
 
 + (NSString*)getCPUInfo;
 
-/*! Returns previously set callback call or nil, if the user has not set such callback. */
-+ (LogCallback)getLogCallBack;
-
 + (void)log:(ADAL_LOG_LEVEL)level
     context:(id)context
 correlationId:(NSUUID *)correlationId

ADAL/src/ADLogger.m

@@ -37,7 +37,8 @@ - (NSString *)component;
 
 static ADAL_LOG_LEVEL s_LogLevel = ADAL_LOG_LEVEL_ERROR;
 static BOOL s_piiEnabled = NO;
-static LogCallback s_LogCallback = nil;
+static LogCallback s_OldCallback = nil;
+static ADLoggerCallback s_LoggerCallback = nil;
 static BOOL s_NSLogging = YES;
 static NSString* s_OSString = @"UnkOS";
 
@@ -79,9 +80,17 @@ + (ADAL_LOG_LEVEL)getLevel
 
 + (void)setLogCallBack:(LogCallback)callback
 {
-    @synchronized(self)//Avoid changing to null while attempting to call it.
+    @synchronized (self)
     {
-        s_LogCallback = [callback copy];
+        s_OldCallback = [callback copy];
+    }
+}
+
++ (void)setLoggerCallback:(ADLoggerCallback)callback
+{
+    @synchronized (self)
+    {
+        s_LoggerCallback = [callback copy];
     }
 }
 
@@ -109,14 +118,6 @@ + (BOOL)getPiiEnabled
 
 @implementation ADLogger (Internal)
 
-+ (LogCallback)getLogCallBack
-{
-    @synchronized(self)
-    {
-        return s_LogCallback;
-    }
-}
-
 + (NSString*)stringForLevel:(ADAL_LOG_LEVEL)level
 {
     switch (level)
@@ -167,7 +168,7 @@ + (void)log:(ADAL_LOG_LEVEL)level
 
     @synchronized(self)//Guard against thread-unsafe callback and modification of sLogCallback after the check
     {
-        if (!(level <= s_LogLevel && (s_LogCallback || s_NSLogging)))
+        if (!(level <= s_LogLevel && (s_LoggerCallback || s_OldCallback || s_NSLogging)))
         {
             return;
         }
@@ -200,11 +201,18 @@ + (void)log:(ADAL_LOG_LEVEL)level
             NSLog(@"%@", msg);
         }
 
-        if (s_LogCallback)
+        NSString* msg = [NSString stringWithFormat:@"ADAL " ADAL_VERSION_STRING " %@ [%@%@]%@ %@", s_OSString, dateString, correlationIdStr, component, message];
+
+        if (s_LoggerCallback)
+        {
+            s_LoggerCallback(level, msg, isPii);
+        }
+        else if (s_OldCallback)
         {
-            NSString* msg = [NSString stringWithFormat:@"ADAL " ADAL_VERSION_STRING " %@ [%@%@]%@ %@", s_OSString, dateString, correlationIdStr, component, message];
+            NSString *message = isPii ? @"PII message" : msg;
+            NSString *additionalMessage = isPii ? msg : nil;
 
-            s_LogCallback(level, msg, isPii);
+            s_OldCallback(level, message, additionalMessage, 0, nil);
         }
     }
 }

ADAL/src/public/ADLogger.h

@@ -61,18 +61,37 @@ typedef enum
  
  @param  logLevel        The level of the log message
  @param  message         A short log message describing the event that occurred, this string will not contain PII.
- @param  containsPii     If the message might contain Personally Identifiable Information (PII) this will be true. Log messages possibly containing PII will not be sent to the callback unless piiEnabled is set to YES on the logger.
+ @param  additionalInfo  A longer message that may contain PII and other details relevant to the event.
+ @param  errorCode       An integer error code if the log message is an error.
+ @param  userInfo        A dictionary with other information relevant to the log message. The information varies,
+ for most error messages the error object will be in the "error" key.
  */
 typedef void (^LogCallback)(ADAL_LOG_LEVEL logLevel,
                             NSString *message,
-                            BOOL containsPii);
+                            NSString *additionalInfo,
+                            NSInteger errorCode,
+                            NSDictionary *userInfo);
 
 /*!
     Sets a block for the ADAL logger to use to send log messages to.
  
     @param callback     The block log messages are sent to. See the documentation for LogCallback for more information.
  */
-+ (void)setLogCallBack:(LogCallback)callback;
++ (void)setLogCallBack:(LogCallback)callback __attribute((deprecated("Use the setLoggerCallback: method instead.")));
+
+
+/*!
+ The LogCallback block for the ADAL logger
+ 
+ @param  logLevel        The level of the log message
+ @param  message         A short log message describing the event that occurred, this string will not contain PII.
+ @param  containsPii     If the message might contain Personally Identifiable Information (PII) this will be true. Log messages possibly containing PII will not be sent to the callback unless piiEnabled is set to YES on the logger.
+ */
+typedef void (^ADLoggerCallback)(ADAL_LOG_LEVEL logLevel,
+                                 NSString *message,
+                                 BOOL containsPii);
+
++ (void)setLoggerCallback:(ADLoggerCallback)callback;
 
 /*!
     Turns on or off ADAL printing log messages to the console via NSLog. On by default.

ADAL/tests/unit/ADLoggerTests.m

@@ -44,7 +44,14 @@ - (void)tearDown
     [super tearDown];
 
     [ADLogger setNSLogging:self.enableNSLogging];
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
     [ADLogger setLogCallBack:nil];
+#pragma clang diagnostic pop
+
+    [ADLogger setLoggerCallback:nil];
+    [ADLogger setPiiEnabled:NO];
 }
 
 #pragma mark - setNSLogging
@@ -75,11 +82,31 @@ - (void)testLog_whenLogLevelErrorMessageNilInfoValid_shouldNotThrow
     XCTAssertNoThrow([ADLogger log:ADAL_LOG_LEVEL_ERROR context:nil correlationId:nil isPii:NO format:nil]);
 }
 
-- (void)testLog_whenPiiEnabled_shouldReturnMessageInCallback
+- (void)testLog_whenPiiNotEnabled_andLogMessage_shouldReturnMessageInCallback
+{
+    XCTestExpectation* expectation = [self expectationWithDescription:@"Validate logger callback."];
+
+    [ADLogger setLoggerCallback:^(ADAL_LOG_LEVEL logLevel, NSString *message, BOOL containsPii)
+     {
+         XCTAssertNotNil(message);
+         XCTAssertEqual(logLevel, ADAL_LOG_LEVEL_ERROR);
+         XCTAssertFalse(containsPii);
+
+         [expectation fulfill];
+     }];
+
+    [ADLogger log:ADAL_LOG_LEVEL_ERROR context:nil correlationId:nil isPii:NO format:@"message"];
+
+    [self waitForExpectationsWithTimeout:1 handler:nil];
+}
+
+- (void)testLog_whenPiiEnabled_andLogPii_shouldReturnMessageInCallback
 {
+    [ADLogger setPiiEnabled:YES];
+
     XCTestExpectation* expectation = [self expectationWithDescription:@"Validate logger callback."];
 
-    [ADLogger setLogCallBack:^(ADAL_LOG_LEVEL logLevel, NSString *message, BOOL containsPii)
+    [ADLogger setLoggerCallback:^(ADAL_LOG_LEVEL logLevel, NSString *message, BOOL containsPii)
      {
          XCTAssertNotNil(message);
          XCTAssertEqual(logLevel, ADAL_LOG_LEVEL_ERROR);
@@ -93,12 +120,12 @@ - (void)testLog_whenPiiEnabled_shouldReturnMessageInCallback
     [self waitForExpectationsWithTimeout:1 handler:nil];
 }
 
-- (void)testLog_whenPiiNotEnabled_shouldNotInvokeCallback
+- (void)testLog_whenPiiNotEnabled_andLogPii_shouldNotInvokeCallback
 {
     XCTestExpectation* expectation = [self expectationWithDescription:@"Validate logger callback."];
     expectation.inverted = YES;
 
-    [ADLogger setLogCallBack:^(ADAL_LOG_LEVEL __unused logLevel, NSString __unused *message, BOOL __unused containsPii)
+    [ADLogger setLoggerCallback:^(ADAL_LOG_LEVEL __unused logLevel, NSString __unused *message, BOOL __unused containsPii)
      {
          [expectation fulfill];
      }];
@@ -108,4 +135,107 @@ - (void)testLog_whenPiiNotEnabled_shouldNotInvokeCallback
     [self waitForExpectationsWithTimeout:1 handler:nil];
 }
 
+- (void)testLog_whenPiiNotEnabled_andOldCallback_andLogMessage_shouldReturnMessageInCallback
+{
+    XCTestExpectation* expectation = [self expectationWithDescription:@"Validate logger callback."];
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+
+    [ADLogger setLogCallBack:^(ADAL_LOG_LEVEL logLevel, NSString *message, NSString *additionalInfo, __unused NSInteger errorCode, __unused NSDictionary *userInfo)
+    {
+         XCTAssertNotNil(message);
+         XCTAssertEqual(logLevel, ADAL_LOG_LEVEL_ERROR);
+         XCTAssertNil(additionalInfo);
+
+         [expectation fulfill];
+     }];
+
+#pragma clang diagnostic pop
+
+    [ADLogger log:ADAL_LOG_LEVEL_ERROR context:nil correlationId:nil isPii:NO format:@"message"];
+
+    [self waitForExpectationsWithTimeout:1 handler:nil];
+}
+
+- (void)testLog_whenPiiEnabled_andOldCallback_andLogPii_shouldReturnAdditionalMessageInCallback
+{
+    [ADLogger setPiiEnabled:YES];
+
+    XCTestExpectation* expectation = [self expectationWithDescription:@"Validate logger callback."];
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+
+    [ADLogger setLogCallBack:^(ADAL_LOG_LEVEL logLevel, NSString *message, NSString *additionalInfo, NSInteger errorCode, NSDictionary *userInfo)
+     {
+         XCTAssertNil(userInfo);
+         XCTAssertEqual(errorCode, 0);
+         XCTAssertNotNil(additionalInfo);
+         XCTAssertEqualObjects(message, @"PII message");
+         XCTAssertEqual(logLevel, ADAL_LOG_LEVEL_ERROR);
+         XCTAssertNil(userInfo);
+
+         [expectation fulfill];
+     }];
+
+#pragma clang diagnostic pop
+
+    [ADLogger log:ADAL_LOG_LEVEL_ERROR context:nil correlationId:nil isPii:YES format:@"message"];
+
+    [self waitForExpectationsWithTimeout:1 handler:nil];
+}
+
+- (void)testLog_whenPiiNotEnabled_andOldCallback_andLogPii_shouldNotInvokeCallback
+{
+    XCTestExpectation* expectation = [self expectationWithDescription:@"Validate logger callback."];
+    expectation.inverted = YES;
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+
+    [ADLogger setLogCallBack:^(__unused ADAL_LOG_LEVEL logLevel, __unused NSString *message, __unused NSString *additionalInfo, __unused NSInteger errorCode, __unused NSDictionary *userInfo)
+    {
+         [expectation fulfill];
+     }];
+
+#pragma clang diagnostic pop
+
+    [ADLogger log:ADAL_LOG_LEVEL_ERROR context:nil correlationId:nil isPii:YES format:@"message"];
+
+    [self waitForExpectationsWithTimeout:1 handler:nil];
+}
+
+- (void)testLog_whenBothCallbacksSet_shouldCallNewOne
+{
+    XCTestExpectation *oldExpectation = [self expectationWithDescription:@"Validate old logger callback."];
+    oldExpectation.inverted = YES;
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+
+    [ADLogger setLogCallBack:^(__unused ADAL_LOG_LEVEL logLevel, __unused NSString *message, __unused NSString *additionalInfo, __unused NSInteger errorCode, __unused NSDictionary *userInfo)
+     {
+         [oldExpectation fulfill];
+     }];
+
+#pragma clang diagnostic pop
+
+    XCTestExpectation *newExpectation = [self expectationWithDescription:@"Validate new logger callback."];
+
+    [ADLogger setLoggerCallback:^(ADAL_LOG_LEVEL logLevel, NSString *message, BOOL containsPii)
+     {
+         XCTAssertNotNil(message);
+         XCTAssertEqual(logLevel, ADAL_LOG_LEVEL_ERROR);
+         XCTAssertFalse(containsPii);
+
+         [newExpectation fulfill];
+     }];
+
+
+    [ADLogger log:ADAL_LOG_LEVEL_ERROR context:nil correlationId:nil isPii:NO format:@"message"];
+
+    [self waitForExpectationsWithTimeout:1 handler:nil];
+}
+
 @end

Samples/MyTestMacOSApp/MyTestMacOSApp/main.m

@@ -27,7 +27,7 @@
 
 int main(int argc, const char * argv[])
 {
-    [ADLogger setLogCallBack:^(ADAL_LOG_LEVEL __unused logLevel, NSString *message, BOOL __unused containsPii)
+    [ADLogger setLoggerCallback:^(ADAL_LOG_LEVEL __unused logLevel, NSString *message, BOOL __unused containsPii)
      {
          NSLog(@"%@", message);
      }];

Samples/MyTestiOSApp/MyTestiOSApp/ADTestAppLogViewController.m

@@ -55,7 +55,7 @@ - (id)init
 
     [self setEdgesForExtendedLayout:UIRectEdgeNone];
 
-    [ADLogger setLogCallBack:^(ADAL_LOG_LEVEL logLevel, NSString *message, BOOL containsPii)
+    [ADLogger setLoggerCallback:^(ADAL_LOG_LEVEL logLevel, NSString *message, BOOL containsPii)
      {
         [self appendNewLogLine:message];
     }];