From 78779f3ca401659a508259161280e2398cb87b13 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Tue, 9 Oct 2012 18:51:26 -0700 Subject: [PATCH] Don't allow clients to set fields in the createUser method that they can't later modify with the default Meteor.users update allow rule. This does mean that it's difficult for trusted server code calling Accounts.createUser, even with a custom onCreateUser hook, to set values on the new user that can't also be set by arbitrary clients. For now, server code needing to do this can just set it with a post-create update; later we might add another parameter to onCreateUser. --- packages/accounts-base/accounts_server.js | 14 +++++++---- packages/accounts-base/accounts_tests.js | 22 ++++++++++-------- .../accounts-oauth1-helper/oauth1_tests.js | 4 ++-- .../accounts-oauth2-helper/oauth2_tests.js | 4 ++-- .../accounts-password/passwords_server.js | 5 ++++ packages/accounts-password/passwords_tests.js | 23 +++++++++++-------- .../passwords_tests_setup.js | 4 ++-- 7 files changed, 46 insertions(+), 30 deletions(-) diff --git a/packages/accounts-base/accounts_server.js b/packages/accounts-base/accounts_server.js index 96534070a5..3c28e28704 100644 --- a/packages/accounts-base/accounts_server.js +++ b/packages/accounts-base/accounts_server.js @@ -114,12 +114,18 @@ onCreateUserHook = func; }; + // XXX see comment on Accounts.createUser in passwords_server about adding a + // third "server options" argument. var defaultCreateUserHook = function (options, extra, user) { - if (!_.isEmpty( - _.intersection( - _.keys(extra), - ['services', 'username', 'email', 'emails']))) + // 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"); + } if (Accounts._options.requireEmail && (!user.emails || !user.emails.length)) diff --git a/packages/accounts-base/accounts_tests.js b/packages/accounts-base/accounts_tests.js index fe3fb0b85f..d350d811d6 100644 --- a/packages/accounts-base/accounts_tests.js +++ b/packages/accounts-base/accounts_tests.js @@ -6,17 +6,19 @@ Tinytest.add('accounts - updateOrCreateUserFromExternalService', function (test) // create an account with facebook var uid1 = Accounts.updateOrCreateUserFromExternalService( - 'facebook', {id: facebookId}, {foo: 1}).id; + '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}).foo, 1); + test.equal(Meteor.users.findOne({"services.facebook.id": facebookId}).profile.foo, 1); - // create again with the same id, see that we get the same user + // 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). var uid2 = Accounts.updateOrCreateUserFromExternalService( - 'facebook', {id: facebookId}, {foo: 1000, bar: 2}).id; // foo: 1000 shouldn't overwrite + 'facebook', {id: facebookId}, {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).foo, 1); - test.equal(Meteor.users.findOne(uid1).bar, 2); + test.equal(Meteor.users.findOne(uid1).profile.foo, 1); + test.equal(Meteor.users.findOne(uid1).profile.bar, undefined); // cleanup Meteor.users.remove(uid1); @@ -24,13 +26,13 @@ Tinytest.add('accounts - updateOrCreateUserFromExternalService', function (test) // users that have different service ids get different users uid1 = Accounts.updateOrCreateUserFromExternalService( - 'weibo', {id: weiboId1}, {foo: 1}).id; + 'weibo', {id: weiboId1}, {profile: {foo: 1}}).id; uid2 = Accounts.updateOrCreateUserFromExternalService( - 'weibo', {id: weiboId2}, {bar: 2}).id; + 'weibo', {id: weiboId2}, {profile: {bar: 2}}).id; test.equal(Meteor.users.find({"services.weibo.id": {$in: [weiboId1, weiboId2]}}).count(), 2); - test.equal(Meteor.users.findOne({"services.weibo.id": weiboId1}).foo, 1); + test.equal(Meteor.users.findOne({"services.weibo.id": weiboId1}).profile.foo, 1); test.equal(Meteor.users.findOne({"services.weibo.id": weiboId1}).emails, undefined); - test.equal(Meteor.users.findOne({"services.weibo.id": weiboId2}).bar, 2); + test.equal(Meteor.users.findOne({"services.weibo.id": weiboId2}).profile.bar, 2); test.equal(Meteor.users.findOne({"services.weibo.id": weiboId2}).emails, undefined); // cleanup diff --git a/packages/accounts-oauth1-helper/oauth1_tests.js b/packages/accounts-oauth1-helper/oauth1_tests.js index d28bccf365..4929fc9b8b 100644 --- a/packages/accounts-oauth1-helper/oauth1_tests.js +++ b/packages/accounts-oauth1-helper/oauth1_tests.js @@ -93,7 +93,7 @@ Tinytest.add("oauth1 - error in user creation", function (test) { accessTokenSecret: twitterfailAccessTokenSecret }, extra: { - invalid: true + profile: {invalid: true} } }; }); @@ -101,7 +101,7 @@ Tinytest.add("oauth1 - error in user creation", function (test) { // a way to fail new users. duplicated from passwords_tests, but // shouldn't hurt. Accounts.validateNewUser(function (user) { - return !user.invalid; + return !(user.profile && user.profile.invalid); }); // simulate logging in with failure diff --git a/packages/accounts-oauth2-helper/oauth2_tests.js b/packages/accounts-oauth2-helper/oauth2_tests.js index bc34842735..6e513e999c 100644 --- a/packages/accounts-oauth2-helper/oauth2_tests.js +++ b/packages/accounts-oauth2-helper/oauth2_tests.js @@ -56,7 +56,7 @@ Tinytest.add("oauth2 - error in user creation", function (test) { id: failbookId }, extra: { - invalid: true + profile: {invalid: true} } }; }); @@ -64,7 +64,7 @@ Tinytest.add("oauth2 - error in user creation", function (test) { // a way to fail new users. duplicated from passwords_tests, but // shouldn't hurt. Accounts.validateNewUser(function (user) { - return !user.invalid; + return !(user.profile && user.profile.invalid); }); // simulate logging in with failure diff --git a/packages/accounts-password/passwords_server.js b/packages/accounts-password/passwords_server.js index 861ad9dc0d..9771eadbd9 100644 --- a/packages/accounts-password/passwords_server.js +++ b/packages/accounts-password/passwords_server.js @@ -381,6 +381,11 @@ // after creation. // // returns userId or throws an error if it can't create + // + // XXX add another argument ("server options") that gets sent to onCreateUser, + // 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) { options = _.clone(options); options.generateLoginToken = false; diff --git a/packages/accounts-password/passwords_tests.js b/packages/accounts-password/passwords_tests.js index d647c7f269..ab401d4d9c 100644 --- a/packages/accounts-password/passwords_tests.js +++ b/packages/accounts-password/passwords_tests.js @@ -200,18 +200,21 @@ if (Meteor.isClient) (function () { // test Accounts.validateNewUser function(test, expect) { Accounts.createUser({username: username3, password: password3}, - {invalid: true}, // should fail the new user validators - expect(function (error) { - test.equal(error.error, 403); - test.equal( - error.reason, - "User validation failed"); - })); + // should fail the new user validators + {profile: {invalid: true}}, + expect(function (error) { + test.equal(error.error, 403); + test.equal( + error.reason, + "User validation failed"); + })); }, logoutStep, function(test, expect) { Accounts.createUser({username: username3, password: password3}, - {invalidAndThrowException: true}, // should fail the new user validator with a special exception + // should fail the new user validator with a special + // exception + {profile: {invalidAndThrowException: true}}, expect(function (error) { test.equal( error.reason, @@ -271,8 +274,8 @@ if (Meteor.isServer) (function () { function (test) { var email = Meteor.uuid() + '@example.com'; test.throws(function () { - Accounts.createUser({email: email}, - {invalid: true}); // should fail the new user validators + // should fail the new user validators + Accounts.createUser({email: email}, {profile: {invalid: true}}); }); // disable sending emails diff --git a/packages/accounts-password/passwords_tests_setup.js b/packages/accounts-password/passwords_tests_setup.js index 0d4fa072ec..19b685a48d 100644 --- a/packages/accounts-password/passwords_tests_setup.js +++ b/packages/accounts-password/passwords_tests_setup.js @@ -1,7 +1,7 @@ Accounts.validateNewUser(function (user) { - if (user.invalidAndThrowException) + if (user.profile && user.profile.invalidAndThrowException) throw new Meteor.Error(403, "An exception thrown within Accounts.validateNewUser"); - return !user.invalid; + return !(user.profile && user.profile.invalid); }); Accounts.onCreateUser(function (options, extra, user) {