fix(users): Duplicate Provider Accounts (#1767)

Fixes an issue where a provider account could be added to the logged in user's additionalProviderData, when another user has already connected that same account. Refactors the saveOAuthUserProfile method to consider the results of the provider search query, whether or not the current user is logged in.
meanjs · Jun 11, 2017 · e4a3d38 · e4a3d38
1 parent 7b91f76
commit e4a3d38
Showing 1 changed file with 62 additions and 55 deletions.
diff --git a/modules/users/server/controllers/users/users.authentication.server.controller.js b/modules/users/server/controllers/users/users.authentication.server.controller.js
@@ -122,72 +122,81 @@ exports.oauthCallback = function (strategy) {
  * Helper function to save or update a OAuth user profile
  */
 exports.saveOAuthUserProfile = function (req, providerUserProfile, done) {
-  // Setup info object
+  // Setup info and user objects
   var info = {};
+  var user;
 
   // Set redirection path on session.
   // Do not redirect to a signin or signup page
-  if (noReturnUrls.indexOf(req.session.redirect_to) === -1)
+  if (noReturnUrls.indexOf(req.session.redirect_to) === -1) {
     info.redirect_to = req.session.redirect_to;
+  }
 
-  if (!req.user) {
-    // Define a search query fields
-    var searchMainProviderIdentifierField = 'providerData.' + providerUserProfile.providerIdentifierField;
-    var searchAdditionalProviderIdentifierField = 'additionalProvidersData.' + providerUserProfile.provider + '.' + providerUserProfile.providerIdentifierField;
+  // Define a search query fields
+  var searchMainProviderIdentifierField = 'providerData.' + providerUserProfile.providerIdentifierField;
+  var searchAdditionalProviderIdentifierField = 'additionalProvidersData.' + providerUserProfile.provider + '.' + providerUserProfile.providerIdentifierField;
 
-    // Define main provider search query
-    var mainProviderSearchQuery = {};
-    mainProviderSearchQuery.provider = providerUserProfile.provider;
-    mainProviderSearchQuery[searchMainProviderIdentifierField] = providerUserProfile.providerData[providerUserProfile.providerIdentifierField];
+  // Define main provider search query
+  var mainProviderSearchQuery = {};
+  mainProviderSearchQuery.provider = providerUserProfile.provider;
+  mainProviderSearchQuery[searchMainProviderIdentifierField] = providerUserProfile.providerData[providerUserProfile.providerIdentifierField];
 
-    // Define additional provider search query
-    var additionalProviderSearchQuery = {};
-    additionalProviderSearchQuery[searchAdditionalProviderIdentifierField] = providerUserProfile.providerData[providerUserProfile.providerIdentifierField];
+  // Define additional provider search query
+  var additionalProviderSearchQuery = {};
+  additionalProviderSearchQuery[searchAdditionalProviderIdentifierField] = providerUserProfile.providerData[providerUserProfile.providerIdentifierField];
 
-    // Define a search query to find existing user with current provider profile
-    var searchQuery = {
-      $or: [mainProviderSearchQuery, additionalProviderSearchQuery]
-    };
+  // Define a search query to find existing user with current provider profile
+  var searchQuery = {
+    $or: [mainProviderSearchQuery, additionalProviderSearchQuery]
+  };
 
-    User.findOne(searchQuery, function (err, user) {
-      if (err) {
-        return done(err);
-      } else {
-        if (!user) {
-          var possibleUsername = providerUserProfile.username || ((providerUserProfile.email) ? providerUserProfile.email.split('@')[0] : '');
-
-          User.findUniqueUsername(possibleUsername, null, function (availableUsername) {
-            user = new User({
-              firstName: providerUserProfile.firstName,
-              lastName: providerUserProfile.lastName,
-              username: availableUsername,
-              displayName: providerUserProfile.displayName,
-              profileImageURL: providerUserProfile.profileImageURL,
-              provider: providerUserProfile.provider,
-              providerData: providerUserProfile.providerData
-            });
-
-            // Email intentionally added later to allow defaults (sparse settings) to be applid.
-            // Handles case where no email is supplied.
-            // See comment: https://github.com/meanjs/mean/pull/1495#issuecomment-246090193
-            user.email = providerUserProfile.email;
-
-            // And save the user
-            user.save(function (err) {
-              return done(err, user, info);
-            });
+  // Find existing user with this provider account
+  User.findOne(searchQuery, function (err, existingUser) {
+    if (err) {
+      return done(err);
+    }
+
+    if (!req.user) {
+      if (!existingUser) {
+        var possibleUsername = providerUserProfile.username || ((providerUserProfile.email) ? providerUserProfile.email.split('@')[0] : '');
+
+        User.findUniqueUsername(possibleUsername, null, function (availableUsername) {
+          user = new User({
+            firstName: providerUserProfile.firstName,
+            lastName: providerUserProfile.lastName,
+            username: availableUsername,
+            displayName: providerUserProfile.displayName,
+            profileImageURL: providerUserProfile.profileImageURL,
+            provider: providerUserProfile.provider,
+            providerData: providerUserProfile.providerData
           });
-        } else {
-          return done(err, user, info);
+
+          // Email intentionally added later to allow defaults (sparse settings) to be applid.
+          // Handles case where no email is supplied.
+          // See comment: https://github.com/meanjs/mean/pull/1495#issuecomment-246090193
+          user.email = providerUserProfile.email;
+
+          // And save the user
+          user.save(function (err) {
+            return done(err, user, info);
+          });
+        });
+      } else {
+        return done(err, existingUser, info);
+      }
+    } else {
+      // User is already logged in, join the provider data to the existing user
+      user = req.user;
+
+      // Check if an existing user was found for this provider account
+      if (existingUser) {
+        if (user.id !== existingUser.id) {
+          return done(new Error('Account is already connected to another user'), user, info);
         }
+
+        return done(new Error('User is already connected using this provider'), user, info);
       }
-    });
-  } else {
-    // User is already logged in, join the provider data to the existing user
-    var user = req.user;
 
-    // Check if user exists, is not signed in using this provider, and doesn't have that provider data already configured
-    if (user.provider !== providerUserProfile.provider && (!user.additionalProvidersData || !user.additionalProvidersData[providerUserProfile.provider])) {
       // Add the provider data to the additional provider data field
       if (!user.additionalProvidersData) {
         user.additionalProvidersData = {};
@@ -202,10 +211,8 @@ exports.saveOAuthUserProfile = function (req, providerUserProfile, done) {
       user.save(function (err) {
         return done(err, user, info);
       });
-    } else {
-      return done(new Error('User is already connected using this provider'), user);
     }
-  }
+  });
 };
 
 /**