Fix inconsistent EMM error handling

CHANGELOG.md

@@ -1,6 +1,8 @@
 # Unreleased
 - Fix a crash when a server error response carries a non-string value under its `error` key. The EMM error handler sent `-hasPrefix:` to whatever value was present, raising an unrecognized selector exception on a number, array or object.
 - Fix a custom `nonce` and requested token `claims` being dropped when a sign-in is continued after a Device Policy app restart.
+- 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 it completed off the main thread.
 
 # 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. If |response| carries an EMM error and no EMM dialog is already pending,
+// |completion| is called asynchronously on the main thread with |YES| — after the user
+// dismisses the remediation dialog, or immediately if no dialog could be presented.
+// Otherwise |completion| is called with |NO| before this method returns.
+- (void)handleErrorFromResponse:(NSDictionary<NSString *, id> *)response
+                     completion:(void (^)(BOOL handled))completion;
 
 @end
 

GoogleSignIn/Sources/GIDEMMErrorHandler.m

@@ -56,8 +56,8 @@ + (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;
   @synchronized(self) {  // for accessing _pendingDialog
@@ -90,15 +90,19 @@ - (BOOL)handleErrorFromResponse:(NSDictionary<NSString *, id> *)response
     }
   }
   if (!errorCode) {
-    completion();
-    return NO;
+    completion(NO);
+    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 +125,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 +149,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,21 @@ - (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;
-
 #if TARGET_OS_IOS && !TARGET_OS_MACCATALYST
       if (authFlow.emmSupport) {
         [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];
+        }];
+      } else
 #endif // TARGET_OS_IOS && !TARGET_OS_MACCATALYST
-      errorString = (NSString *)params[kOAuth2ErrorKeyName];
-      if ([errorString isEqualToString:kOAuth2AccessDenied]) {
-        errorCode = kGIDSignInErrorCodeCanceled;
+      {
+        authFlow.error = [self authorizationErrorWithParameters:params isEMMError:NO];
       }
-
-      authFlow.error = [self errorWithString:errorString code:errorCode];
     }
   } else {
     NSString *errorString = [error localizedDescription];
@@ -1289,6 +1281,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";