OAuth login to an existing account now *does* update the services.PROVIDER

field (eg, to refresh access tokens) but does *not* update anything else (which is effectively a no-op change because in practice the only thing that it attempted to update was "profile" but because existing fields were not overridden, this never did anything). Remove the "extra" argument from createUser and related functions. Add a new "profile" option to the main options dictionary, interpreted by defaultCreateUserHook.
2026-05-02 03:01:46 -04:00 · 2012-10-10 15:44:58 -07:00
parent 65872c3c1c
commit 00eda43420
17 changed files with 84 additions and 100 deletions
--- a/packages/accounts-base/accounts_server.js
+++ b/packages/accounts-base/accounts_server.js
@@ -115,21 +115,13 @@
  };

  // XXX see comment on Accounts.createUser in passwords_server about adding a
-  // third "server options" argument.
-  var defaultCreateUserHook = function (options, extra, user) {
-    // This hook gets 'extra' directly from the createUser method, so make sure
-    // we don't allow users to set any fields at creation time that they won't
-    // later be able to set according to the default Meteor.users.allow. Set
-    // your own onCreateUser if you want users to be able to specify other
-    // fields at creation time.
-    if (_.any(extra, function(value, key) {return key != 'profile';})) {
-      console.log(JSON.stringify(extra));
-      throw new Meteor.Error(400, "Disallowed fields in extra");
-    }
-
-    return _.extend(user, extra);
+  // second "server options" argument.
+  var defaultCreateUserHook = function (options, user) {
+    if (options.profile)
+      user.profile = options.profile;
+    return user;
  };
-  Accounts.insertUserDoc = function (options, extra, user) {
+  Accounts.insertUserDoc = function (options, user) {
    // add created at timestamp (and protect passed in user object from
    // modification)
    user = _.extend({createdAt: +(new Date)}, user);
@@ -137,15 +129,15 @@
    var fullUser;

    if (onCreateUserHook) {
-      fullUser = onCreateUserHook(options, extra, user);
+      fullUser = onCreateUserHook(options, user);

      // This is *not* part of the API. We need this because we can't isolate
      // the global server environment between tests, meaning we can't test
      // both having a create user hook set and not having one set.
      if (fullUser === 'TEST DEFAULT HOOK')
-        fullUser = defaultCreateUserHook(options, extra, user);
+        fullUser = defaultCreateUserHook(options, user);
    } else {
-      fullUser = defaultCreateUserHook(options, extra, user);
+      fullUser = defaultCreateUserHook(options, user);
    }

    _.each(validateNewUserHooks, function (hook) {
@@ -199,13 +191,13 @@
  // @param serviceData {Object} Data to store in the user's record
  //        under services[serviceName]. Must include an "id" field
  //        which is a unique identifier for the user in the service.
-  // @param extra {Object, optional} Any additional fields to place on the user
-  //        object
+  // @param options {Object, optional} Other options to pass to insertUserDoc
+  //        (eg, profile)
  // @returns {Object} Object with token and id keys, like the result
  //        of the "login" method.
  Accounts.updateOrCreateUserFromExternalService = function(
-    serviceName, serviceData, extra) {
-    extra = extra || {};
+    serviceName, serviceData, options) {
+    options = _.clone(options || {});

    if (serviceName === "password" || serviceName === "resume")
      throw new Error(
@@ -221,26 +213,28 @@
    var user = Meteor.users.findOne(selector);

    if (user) {
-      // don't overwrite existing fields
-      // XXX subobjects (aka 'profile', 'services')?
-      var newKeys = _.difference(_.keys(extra), _.keys(user));
-      var newAttrs = _.pick(extra, newKeys);
+      // We *don't* process options (eg, profile) for update, but we do replace
+      // the serviceData (eg, so that we keep an unexpired access token and
+      // don't cache old email addresses in serviceData.email).
+      // XXX provide an onUpdateUser hook which would let apps update
+      //     the profile too
      var stampedToken = Accounts._generateStampedLoginToken();
-      var result = {token: stampedToken.token};
+      var setAttrs = {};
+      setAttrs["services." + serviceName] = serviceData;
+      // XXX Maybe we should re-use the selector above and notice if the update
+      //     touches nothing?
      Meteor.users.update(
        user._id,
-        {$set: newAttrs, $push: {'services.resume.loginTokens': stampedToken}});
-      result.id = user._id;
-      return result;
+        {$set: setAttrs,
+         $push: {'services.resume.loginTokens': stampedToken}});
+      return {token: stampedToken.token, id: user._id};
    } else {
-      // Create a new user.
-      var servicesClause = {};
-      servicesClause[serviceName] = serviceData;
-      var insertOptions = {services: servicesClause, generateLoginToken: true};
-      // Build a user doc; clone to make sure sure mutating
-      // insertOptions.services doesn't affect user.services or vice versa.
-      user = {services: JSON.parse(JSON.stringify(servicesClause))};
-      return Accounts.insertUserDoc(insertOptions, extra, user);
+      // Create a new user with the service data. Pass other options through to
+      // insertUserDoc.
+      user = {services: {}};
+      user.services[serviceName] = serviceData;
+      options.generateLoginToken = true;
+      return Accounts.insertUserDoc(options, user);
    }
  };

--- a/packages/accounts-base/accounts_tests.js
+++ b/packages/accounts-base/accounts_tests.js
@@ -6,20 +6,24 @@ Tinytest.add('accounts - updateOrCreateUserFromExternalService', function (test)

  // create an account with facebook
  var uid1 = Accounts.updateOrCreateUserFromExternalService(
-    'facebook', {id: facebookId}, {profile: {foo: 1}}).id;
-  test.equal(Meteor.users.find({"services.facebook.id": facebookId}).count(), 1);
-  test.equal(Meteor.users.findOne({"services.facebook.id": facebookId}).profile.foo, 1);
+    'facebook', {id: facebookId, monkey: 42}, {profile: {foo: 1}}).id;
+  var users = Meteor.users.find({"services.facebook.id": facebookId}).fetch();
+  test.length(users, 1);
+  test.equal(users[0].profile.foo, 1);
+  test.equal(users[0].services.facebook.monkey, 42);

-  // create again with the same id, see that we get the same user. profile
-  // doesn't get overwritten in this implementation (though we should do
-  // something better with merging later).
+  // create again with the same id, see that we get the same user.
+  // it should update services.facebook but not profile.
  var uid2 = Accounts.updateOrCreateUserFromExternalService(
-    'facebook', {id: facebookId}, {profile: {foo: 1000, bar: 2}}).id;
+    'facebook', {id: facebookId, llama: 50},
+    {profile: {foo: 1000, bar: 2}}).id;
  test.equal(uid1, uid2);
-  test.equal(Meteor.users.find({"services.facebook.id": facebookId}).count(), 1);
-  test.equal(Meteor.users.findOne(uid1).profile.foo, 1);
-  test.equal(Meteor.users.findOne(uid1).profile.bar, undefined);
-
+  users = Meteor.users.find({"services.facebook.id": facebookId}).fetch();
+  test.length(users, 1);
+  test.equal(users[0].profile.foo, 1);
+  test.equal(users[0].profile.bar, undefined);
+  test.equal(users[0].services.facebook.llama, 50);
+  test.equal(users[0].services.facebook.monkey, undefined);
  // cleanup
  Meteor.users.remove(uid1);

@@ -48,7 +52,6 @@ Tinytest.add('accounts - insertUserDoc username', function (test) {

  // user does not already exist. create a user object with fields set.
  var result = Accounts.insertUserDoc(
-    userIn,
    {profile: {name: 'Foo Bar'}},
    userIn
  );
@@ -61,7 +64,6 @@ Tinytest.add('accounts - insertUserDoc username', function (test) {
  // run the hook again. now the user exists, so it throws an error.
  test.throws(function () {
    Accounts.insertUserDoc(
-      userIn,
      {profile: {name: 'Foo Bar'}},
      userIn
    );
@@ -83,7 +85,6 @@ Tinytest.add('accounts - insertUserDoc email', function (test) {

  // user does not already exist. create a user object with fields set.
  var result = Accounts.insertUserDoc(
-    userIn,
    {profile: {name: 'Foo Bar'}},
    userIn
  );
@@ -97,7 +98,6 @@ Tinytest.add('accounts - insertUserDoc email', function (test) {
  // run the hook again. now the user exists, so it throws an error.
  test.throws(function () {
    Accounts.insertUserDoc(
-      userIn,
      {profile: {name: 'Foo Bar'}},
      userIn
    );
@@ -106,20 +106,20 @@ Tinytest.add('accounts - insertUserDoc email', function (test) {
  // now with only one of them.
  test.throws(function () {
    Accounts.insertUserDoc(
-      {}, {}, {emails: [{address: email1}]}
+      {}, {emails: [{address: email1}]}
    );
  });

  test.throws(function () {
    Accounts.insertUserDoc(
-      {}, {}, {emails: [{address: email2}]}
+      {}, {emails: [{address: email2}]}
    );
  });


  // a third email works.
  var result3 = Accounts.insertUserDoc(
-      {}, {}, {emails: [{address: email3}]}
+      {}, {emails: [{address: email3}]}
  );
  var user3 = Meteor.users.findOne(result3.id);
  test.equal(typeof user3.createdAt, 'number');
--- a/packages/accounts-facebook/facebook_server.js
+++ b/packages/accounts-facebook/facebook_server.js
@@ -11,7 +11,7 @@
        accessToken: accessToken,
        email: identity.email
      },
-      extra: {profile: {name: identity.name}}
+      options: {profile: {name: identity.name}}
    };
  });

--- a/packages/accounts-github/github_server.js
+++ b/packages/accounts-github/github_server.js
@@ -11,7 +11,7 @@
 	email: identity.email,
 	username: identity.login
      },
-      extra: {profile: {name: identity.name}}
+      options: {profile: {name: identity.name}}
    };
  });

--- a/packages/accounts-google/google_server.js
+++ b/packages/accounts-google/google_server.js
@@ -11,7 +11,7 @@
        accessToken: accessToken,
        email: identity.email
      },
-      extra: {profile: {name: identity.name}}
+      options: {profile: {name: identity.name}}
    };
  });

--- a/packages/accounts-oauth-helper/oauth_server.js
+++ b/packages/accounts-oauth-helper/oauth_server.js
@@ -14,7 +14,7 @@
  //   - (For OAuth1 only) oauthBinding {OAuth1Binding} bound to the appropriate provider
  //   - (For OAuth2 only) query {Object} parameters passed in query string
  //   - return value is:
-  //     - {serviceData, (optional extra)} where serviceData should end
+  //     - {serviceData:, (optional options:)} where serviceData should end
  //       up in the user's services[name] field
  //     - `null` if the user declined to give permissions
  Accounts.oauth.registerService = function (name, version, handleOauthRequest) {
--- a/packages/accounts-oauth1-helper/oauth1_server.js
+++ b/packages/accounts-oauth1-helper/oauth1_server.js
@@ -55,7 +55,7 @@
        // Get or create user doc and login token for reconnect.
        Accounts.oauth._loginResultForState[query.state] =
          Accounts.updateOrCreateUserFromExternalService(
-            service.serviceName, oauthResult.serviceData, oauthResult.extra);
+            service.serviceName, oauthResult.serviceData, oauthResult.options);
      }
    }

--- a/packages/accounts-oauth1-helper/oauth1_tests.js
+++ b/packages/accounts-oauth1-helper/oauth1_tests.js
@@ -92,7 +92,7 @@ Tinytest.add("oauth1 - error in user creation", function (test) {
        accessToken: twitterfailAccessToken,
        accessTokenSecret: twitterfailAccessTokenSecret
      },
-      extra: {
+      options: {
        profile: {invalid: true}
      }
    };
--- a/packages/accounts-oauth2-helper/oauth2_server.js
+++ b/packages/accounts-oauth2-helper/oauth2_server.js
@@ -14,7 +14,7 @@
      // Get or create user doc and login token for reconnect.
      Accounts.oauth._loginResultForState[query.state] =
        Accounts.updateOrCreateUserFromExternalService(
-          service.serviceName, oauthResult.serviceData, oauthResult.extra);
+          service.serviceName, oauthResult.serviceData, oauthResult.options);
    }

    // Either close the window, redirect, or render nothing
--- a/packages/accounts-oauth2-helper/oauth2_tests.js
+++ b/packages/accounts-oauth2-helper/oauth2_tests.js
@@ -55,7 +55,7 @@ Tinytest.add("oauth2 - error in user creation", function (test) {
      serviceData: {
        id: failbookId
      },
-      extra: {
+      options: {
        profile: {invalid: true}
      }
    };
--- a/packages/accounts-password/email_tests.js
+++ b/packages/accounts-password/email_tests.js
@@ -17,9 +17,9 @@
    function (test, expect) {
      email1 = Meteor.uuid() + "-intercept@example.com";
      Accounts.createUser({email: email1, password: 'foobar'},
-                        expect(function (error) {
-                          test.equal(error, undefined);
-                        }));
+                          expect(function (error) {
+                            test.equal(error, undefined);
+                          }));
    },
    function (test, expect) {
      Accounts.forgotPassword({email: email1}, expect(function (error) {
--- a/packages/accounts-password/passwords_client.js
+++ b/packages/accounts-password/passwords_client.js
@@ -1,12 +1,7 @@
 (function () {
-  Accounts.createUser = function (options, extra, callback) {
+  Accounts.createUser = function (options, callback) {
    options = _.clone(options); // we'll be modifying options

-    if (typeof extra === "function") {
-      callback = extra;
-      extra = {};
-    }
-
    if (!options.password)
      throw new Error("Must set options.password");
    var verifier = Meteor._srp.generateVerifier(options.password);
@@ -14,7 +9,7 @@
    delete options.password;
    options.srp = verifier;

-    Meteor.apply('createUser', [options, extra], {wait: true},
+    Meteor.apply('createUser', [options], {wait: true},
                 function (error, result) {
                   if (error || !result) {
                     error = error || new Error("No result");
--- a/packages/accounts-password/passwords_server.js
+++ b/packages/accounts-password/passwords_server.js
@@ -355,8 +355,7 @@
  //
  // returns an object with id: userId, and (if options.generateLoginToken is
  // set) token: loginToken.
-  var createUser = function (options, extra) {
-    extra = extra || {};
+  var createUser = function (options) {
    var username = options.username;
    var email = options.email;
    if (!username && !email)
@@ -379,19 +378,19 @@
    if (email)
      user.emails = [{address: email, verified: false}];

-    return Accounts.insertUserDoc(options, extra, user);
+    return Accounts.insertUserDoc(options, user);
  };

  // method for create user. Requests come from the client.
  Meteor.methods({
-    createUser: function (options, extra) {
+    createUser: function (options) {
      options = _.clone(options);
      options.generateLoginToken = true;
      if (Accounts._options.forbidClientAccountCreation)
        throw new Meteor.Error(403, "Signups forbidden");

      // Create user. result contains id and token.
-      var result = createUser(options, extra);
+      var result = createUser(options);
      // safety belt. createUser is supposed to throw on error. send 500 error
      // instead of sending a verification email with empty userid.
      if (!result.id)
@@ -420,20 +419,16 @@
  // which is always empty when called from the createUser method? eg, "admin:
  // true", which we want to prevent the client from setting, but which a custom
  // method calling Accounts.createUser could set?
-  Accounts.createUser = function (options, extra, callback) {
+  Accounts.createUser = function (options, callback) {
    options = _.clone(options);
    options.generateLoginToken = false;
-    if (typeof extra === "function") {
-      callback = extra;
-      extra = {};
-    }

    // XXX allow an optional callback?
    if (callback) {
      throw new Error("Meteor.createUser with callback not supported on the server yet.");
    }

-    var userId = createUser(options, extra).id;
+    var userId = createUser(options).id;

    return userId;
  };
--- a/packages/accounts-password/passwords_tests.js
+++ b/packages/accounts-password/passwords_tests.js
@@ -199,9 +199,9 @@ if (Meteor.isClient) (function () {
    logoutStep,
    // test Accounts.validateNewUser
    function(test, expect) {
-      Accounts.createUser({username: username3, password: password3},
-                          // should fail the new user validators
-                          {profile: {invalid: true}},
+      Accounts.createUser({username: username3, password: password3,
+                           // should fail the new user validators
+                           profile: {invalid: true}},
                          expect(function (error) {
                            test.equal(error.error, 403);
                            test.equal(
@@ -211,10 +211,10 @@ if (Meteor.isClient) (function () {
    },
    logoutStep,
    function(test, expect) {
-      Accounts.createUser({username: username3, password: password3},
+      Accounts.createUser({username: username3, password: password3,
                           // should fail the new user validator with a special
                           // exception
-                          {profile: {invalidAndThrowException: true}},
+                           profile: {invalidAndThrowException: true}},
                        expect(function (error) {
                          test.equal(
                            error.reason,
@@ -224,8 +224,8 @@ if (Meteor.isClient) (function () {
    // test Accounts.onCreateUser
    function(test, expect) {
      Accounts.createUser(
-        {username: username3, password: password3},
-        {testOnCreateUserHook: true},
+        {username: username3, password: password3,
+         testOnCreateUserHook: true},
        loggedInAs(username3, test, expect));
    },
    function(test, expect) {
@@ -282,14 +282,14 @@ if (Meteor.isServer) (function () {
      var email = Meteor.uuid() + '@example.com';
      test.throws(function () {
        // should fail the new user validators
-        Accounts.createUser({email: email}, {profile: {invalid: true}});
+        Accounts.createUser({email: email, profile: {invalid: true}});
        });

      // disable sending emails
      var oldEmailSend = Email.send;
      Email.send = function() {};
-      var userId = Accounts.createUser({email: email},
-                                       {testOnCreateUserHook: true});
+      var userId = Accounts.createUser({email: email,
+                                        testOnCreateUserHook: true});
      Email.send = oldEmailSend;

      test.isTrue(userId);
@@ -303,7 +303,7 @@ if (Meteor.isServer) (function () {
    function (test) {
      var username = Meteor.uuid();

-      var userId = Accounts.createUser({username: username}, {});
+      var userId = Accounts.createUser({username: username});

      var user = Meteor.users.findOne(userId);
      // no services yet.
--- a/packages/accounts-password/passwords_tests_setup.js
+++ b/packages/accounts-password/passwords_tests_setup.js
@@ -4,9 +4,9 @@ Accounts.validateNewUser(function (user) {
  return !(user.profile && user.profile.invalid);
 });

-Accounts.onCreateUser(function (options, extra, user) {
-  if (extra.testOnCreateUserHook) {
-    user.profile = (user.profile || {});
+Accounts.onCreateUser(function (options, user) {
+  if (options.testOnCreateUserHook) {
+    user.profile = user.profile || {};
    user.profile.touchedByOnCreateUser = true;
    return user;
  } else {
--- a/packages/accounts-twitter/twitter_server.js
+++ b/packages/accounts-twitter/twitter_server.js
@@ -10,7 +10,7 @@
        accessToken: oauthBinding.accessToken,
        accessTokenSecret: oauthBinding.accessTokenSecret
      },
-      extra: {
+      options: {
        profile: {
          name: identity.name
        }
--- a/packages/accounts-weibo/weibo_server.js
+++ b/packages/accounts-weibo/weibo_server.js
@@ -11,7 +11,7 @@
        accessToken: accessToken.access_token,
        screenName: identity.screen_name
      },
-      extra: {profile: {name: identity.screen_name}}
+      options: {profile: {name: identity.screen_name}}
    };
  });