Fix inconsistent EMM error handling

CHANGELOG.md

@@ -4,6 +4,9 @@
 - Fix a data race on `GIDGoogleUser`'s tokens. Concurrent token updates could write the access, refresh and ID tokens from different threads at once, and readers could observe a partially updated set. The three tokens are now published together as one snapshot.
 - `GIDGoogleUser`'s `accessToken`, `refreshToken` and `idToken` are now all derived from that snapshot, so key-value observers of any one of them are notified whenever any of them changes, not only when that property's own value changes.
 - Key-value observers of those properties are notified while `GIDGoogleUser` holds its internal lock. Calling back into the same user from the observer is fine, but an observer must not synchronously wait (for example with `dispatch_sync`) on another thread that is updating the same user.
+- Fix EMM remediation dialogs being suppressed for the rest of the app's lifetime after a single error arrived while no key window was available. The pending-dialog flag is now cleared on every exit path, and its clearing write is synchronized to match the read.
+- Fix a token request that failed with an EMM error sometimes being reported with the underlying OAuth error instead of `kGIDSignInErrorCodeEMM` when no remediation dialog could be presented.
+- Fix an EMM error that arrived while a remediation dialog was already showing being reported as `kGIDSignInErrorCodeUnknown` (or as the underlying OAuth error, for a token request) instead of `kGIDSignInErrorCodeEMM`. No second dialog is shown.
 
 # 10.0.0
 - **BREAKING**: Update to AppAuth 3.0.0 and GTMAppAuth 6.0.0, which raises the minimum deployment targets to iOS 15.0 and macOS 12.0, widens the `GTMSessionFetcher` dependency to allow 4.x and 5.x, and renames the version-specific Swift Package Manager manifest to `Package@swift-5.7.swift`. Projects that must keep supporting earlier OS versions should stay on GoogleSignIn 9.2.0. ([#628](https://github.com/google/GoogleSignIn-iOS/pull/628))

GoogleSignIn/Sources/GIDEMMErrorHandler.h

@@ -27,13 +27,13 @@ NS_ASSUME_NONNULL_BEGIN
 // Retrieve the shared instance of this class.
 + (instancetype)sharedInstance;
 
-// Handles EMM specific error that is returned in server response.
-// Returns whether or not an EMM-specific error is being handled by this invocation.
-// If the return value is |YES|, |completion| will be called asynchronously in the main thread
-// after the user interacts with the error dialog;
-// if the return value is |NO|, |completion| will be called before returning.
-- (BOOL)handleErrorFromResponse:(NSDictionary<NSString *, id> *)response
-                     completion:(void (^)(void))completion;
+// Handles EMM-specific errors in the server |response|. |completion| is always called
+// exactly once, asynchronously on the main thread, with |YES| if |response| carries an EMM
+// error and |NO| otherwise. For an EMM error, a remediation dialog is presented and
+// |completion| is called after the user dismisses it — unless another EMM dialog is already
+// pending or no dialog can be presented, in which case it is called as soon as possible.
+- (void)handleErrorFromResponse:(NSDictionary<NSString *, id> *)response
+                     completion:(void (^)(BOOL handled))completion;
 
 @end
 

GoogleSignIn/Sources/GIDEMMErrorHandler.m

@@ -56,13 +56,13 @@ + (instancetype)sharedInstance {
   return sharedInstance;
 }
 
-- (BOOL)handleErrorFromResponse:(NSDictionary<NSString *, id> *)response
-                     completion:(void (^)(void))completion {
+- (void)handleErrorFromResponse:(NSDictionary<NSString *, id> *)response
+                     completion:(void (^)(BOOL handled))completion {
   ErrorCode errorCode = ErrorCodeNone;
   NSURL *appVerificationURL;
+  BOOL dialogAlreadyPending = NO;
   @synchronized(self) {  // for accessing _pendingDialog
-    if (!_pendingDialog && [UIAlertController class] &&
-        [response isKindOfClass:[NSDictionary class]]) {
+    if ([UIAlertController class] && [response isKindOfClass:[NSDictionary class]]) {
       id errorValue = response[kErrorKey];
       if ([errorValue isKindOfClass:[NSString class]]) {
         if ([errorValue isEqual:kScreenlockRequiredError]) {
@@ -85,20 +85,38 @@ - (BOOL)handleErrorFromResponse:(NSDictionary<NSString *, id> *)response
         }
       }
       if (errorCode) {
-        _pendingDialog = YES;
+        if (_pendingDialog) {
+          dialogAlreadyPending = YES;
+        } else {
+          _pendingDialog = YES;
+        }
       }
     }
   }
   if (!errorCode) {
-    completion();
-    return NO;
+    dispatch_async(dispatch_get_main_queue(), ^() {
+      completion(NO);
+    });
+    return;
+  }
+  if (dialogAlreadyPending) {
+    // Another EMM dialog is on screen; report this error as EMM without stacking a second
+    // dialog on top of it.
+    dispatch_async(dispatch_get_main_queue(), ^() {
+      completion(YES);
+    });
+    return;
   }
   // All UI must happen in the main thread.
   dispatch_async(dispatch_get_main_queue(), ^() {
+    void (^clearPendingDialog)(void) = ^{
+      @synchronized(self) { self->_pendingDialog = NO; }
+    };
     UIWindow *keyWindow = [self keyWindow];
     if (!keyWindow) {
       // Shouldn't happen, just in case.
-      completion();
+      clearPendingDialog();
+      completion(YES);
       return;
     }
     UIWindow *alertWindow;
@@ -121,8 +139,8 @@ - (BOOL)handleErrorFromResponse:(NSDictionary<NSString *, id> *)response
       alertWindow.hidden = YES;
       alertWindow.rootViewController = nil;
       [keyWindow makeKeyAndVisible];
-      self->_pendingDialog = NO;
-      completion();
+      clearPendingDialog();
+      completion(YES);
     };
     UIAlertController *alert;
     switch (errorCode) {
@@ -145,7 +163,6 @@ - (BOOL)handleErrorFromResponse:(NSDictionary<NSString *, id> *)response
       finish();
     }
   });
-  return YES;
 }
 
 // This method is exposed to the unit test.

GoogleSignIn/Sources/GIDEMMSupport.m

@@ -68,9 +68,8 @@ + (void)handleTokenFetchEMMError:(nullable NSError *)error
                       completion:(void (^)(NSError *_Nullable))completion {
   NSDictionary *errorJSON = error.userInfo[OIDOAuthErrorResponseErrorKey];
   if (errorJSON) {
-    __block BOOL handled = NO;
-    handled = [[GIDEMMErrorHandler sharedInstance] handleErrorFromResponse:errorJSON
-                                                                completion:^() {
+    [[GIDEMMErrorHandler sharedInstance] handleErrorFromResponse:errorJSON
+                                                      completion:^(BOOL handled) {
       if (handled) {
         completion([NSError errorWithDomain:kGIDSignInErrorDomain
                                        code:kGIDSignInErrorCodeEMM

GoogleSignIn/Sources/GIDSignIn.m

@@ -984,29 +984,23 @@ - (void)processAuthorizationResponse:(OIDAuthorizationResponse *)authorizationRe
       [self maybeFetchToken:authFlow];
     } else {
       // There was a failure, convert to appropriate error code.
-      NSString *errorString;
-      GIDSignInErrorCode errorCode = kGIDSignInErrorCodeUnknown;
       NSDictionary<NSString *, NSObject *> *params = authorizationResponse.additionalParameters;
-
+      BOOL deferredToEMMHandler = NO;
 #if TARGET_OS_IOS && !TARGET_OS_MACCATALYST
       if (authFlow.emmSupport) {
+        deferredToEMMHandler = YES;
         [authFlow wait];
-        BOOL isEMMError = [[GIDEMMErrorHandler sharedInstance]
-            handleErrorFromResponse:params
-                         completion:^{
-                           [authFlow next];
-                         }];
-        if (isEMMError) {
-          errorCode = kGIDSignInErrorCodeEMM;
-        }
+        [[GIDEMMErrorHandler sharedInstance] handleErrorFromResponse:params
+                                                          completion:^(BOOL handled) {
+          // Set the error before -next, which runs the queued callbacks that read it.
+          authFlow.error = [self authorizationErrorWithParameters:params isEMMError:handled];
+          [authFlow next];
+        }];
       }
 #endif // TARGET_OS_IOS && !TARGET_OS_MACCATALYST
-      errorString = (NSString *)params[kOAuth2ErrorKeyName];
-      if ([errorString isEqualToString:kOAuth2AccessDenied]) {
-        errorCode = kGIDSignInErrorCodeCanceled;
+      if (!deferredToEMMHandler) {
+        authFlow.error = [self authorizationErrorWithParameters:params isEMMError:NO];
       }
-
-      authFlow.error = [self errorWithString:errorString code:errorCode];
     }
   } else {
     NSString *errorString = [error localizedDescription];
@@ -1289,6 +1283,18 @@ - (BOOL)handleDevicePolicyAppURL:(NSURL *)url {
 
 #pragma mark - Helpers
 
+// Returns the error for an authorization response that carried no authorization code.
+- (NSError *)authorizationErrorWithParameters:(NSDictionary<NSString *, NSObject *> *)params
+                                   isEMMError:(BOOL)isEMMError {
+  NSString *errorString = (NSString *)params[kOAuth2ErrorKeyName];
+  GIDSignInErrorCode errorCode =
+      isEMMError ? kGIDSignInErrorCodeEMM : kGIDSignInErrorCodeUnknown;
+  if ([errorString isEqualToString:kOAuth2AccessDenied]) {
+    errorCode = kGIDSignInErrorCodeCanceled;
+  }
+  return [self errorWithString:errorString code:errorCode];
+}
+
 - (NSError *)errorWithString:(NSString *)errorString code:(GIDSignInErrorCode)code {
   if (errorString == nil) {
     errorString = @"Unknown error";