Index: oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/ExternalLoginModule.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/ExternalLoginModule.java	(revision 1747372)
+++ oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/ExternalLoginModule.java	(revision )
@@ -24,6 +24,7 @@
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
 import javax.jcr.Credentials;
+import javax.jcr.RepositoryException;
 import javax.security.auth.Subject;
 import javax.security.auth.callback.CallbackHandler;
 import javax.security.auth.login.LoginException;
@@ -199,31 +200,20 @@
         // remember identification for log-output
         Object logId = (userId != null) ? userId : credentials;
         try {
-            SyncedIdentity sId = null;
-            UserManager userMgr = getUserManager();
-            if (userId != null && userMgr != null) {
-                sId = syncHandler.findIdentity(userMgr, userId);
-                // if there exists an authorizable with the given userid but is
-                // not an external one or if it belongs to another IDP, we just ignore it.
-                if (sId != null) {
-                    ExternalIdentityRef externalIdRef = sId.getExternalIdRef();
-                    if (externalIdRef == null) {
-                        log.debug("ignoring local user: {}", sId.getId());
+            // check if there exists a user with the given ID that has been synchronized
+            // before into the repository.
+            SyncedIdentity sId = getSyncedIdentity(userId);
+
+            // if there exists an authorizable with the given userid (syncedIdentity != null),
+            // ignore it if any of the following conditions is met:
+            // - identity is local (i.e. not an external identity)
+            // - identity belongs to another IDP
+            // - identity is valid but we have a preAuthLogin and the user doesn't need an updating sync (OAK-3508)
+            if (ignore(sId, preAuthLogin)) {
-                        return false;
+                return false;
-                    } else if (!idp.getName().equals(externalIdRef.getProviderName())) {
-                        if (log.isDebugEnabled()) {
-                            log.debug("ignoring foreign identity: {} (idp={})", externalIdRef.getString(), idp.getName());
-                        }
+            }
-                        return false;
-                    }
-                }
-            }
 
             if (preAuthLogin != null) {
-                if (sId != null && !syncHandler.requiresSync(sId)) {
-                    log.debug("pre-authenticated external user {} does not require syncing.", sId);
-                    return false;
-                }
                 externalUser = idp.getUser(preAuthLogin.getUserId());
             } else {
                 externalUser = idp.authenticate(credentials);
@@ -244,9 +234,7 @@
 
                 return true;
             } else {
-                if (log.isDebugEnabled()) {
-                    log.debug("IDP {} returned null for {}", idp.getName(), logId);
-                }
+                debug("IDP {} returned null for {}", idp.getName(), logId.toString());
 
                 if (sId != null) {
                     // invalidate the user if it exists as synced variant
@@ -313,6 +301,35 @@
         }
     }
 
+    @CheckForNull
+    private SyncedIdentity getSyncedIdentity(@CheckForNull String userId) throws RepositoryException {
+        UserManager userMgr = getUserManager();
+        if (userId != null && userMgr != null) {
+            return syncHandler.findIdentity(userMgr, userId);
+        } else {
+            return null;
+        }
+    }
+
+    private boolean ignore(@CheckForNull SyncedIdentity syncedIdentity, @CheckForNull PreAuthenticatedLogin preAuthLogin) {
+        if (syncedIdentity != null) {
+            ExternalIdentityRef externalIdRef = syncedIdentity.getExternalIdRef();
+            if (externalIdRef == null) {
+                debug("ignoring local user: {}", syncedIdentity.getId());
+                return true;
+            } else if (!idp.getName().equals(externalIdRef.getProviderName())) {
+                debug("ignoring foreign identity: {} (idp={})", externalIdRef.getString(), idp.getName());
+                return true;
+            }
+
+            if (preAuthLogin != null && !syncHandler.requiresSync(syncedIdentity)) {
+                debug("pre-authenticated external user {} does not require syncing.", syncedIdentity.toString());
+                return true;
+            }
+        }
+        return false;
+    }
+
     /**
      * Initiates synchronization of the external user.
      * @param user the external user
@@ -338,9 +355,7 @@
                 timer.mark("sync");
                 root.commit();
                 timer.mark("commit");
-                if (log.isDebugEnabled()) {
-                    log.debug("syncUser({}) {}", user.getId(), timer.getString());
-                }
+                debug("syncUser({}) {}", user.getId(), timer.getString());
                 return;
             } catch (CommitFailedException e) {
                 log.warn("User synchronization failed during commit: {}. (attempt {}/{})", e.toString(), numAttempt, MAX_SYNC_ATTEMPTS);
@@ -375,9 +390,7 @@
             timer.mark("sync");
             root.commit();
             timer.mark("commit");
-            if (log.isDebugEnabled()) {
-                log.debug("validateUser({}) {}", id, timer.getString());
-            }
+            debug("validateUser({}) {}", id, timer.getString());
         } catch (CommitFailedException e) {
             throw new SyncException("User synchronization failed during commit.", e);
         } finally {
@@ -406,6 +419,12 @@
             attributes.putAll(credentialsSupport.getAttributes(creds));
         }
         return new AuthInfoImpl(userId, attributes, principals);
+    }
+
+    private static void debug(@Nonnull String msg, String... args) {
+        if (log.isDebugEnabled()) {
+            log.debug(msg, args);
+        }
     }
 
     //------------------------------------------------< AbstractLoginModule >---
\ No newline at end of file