From eba872966dc7482cc00cad1318abe0815350ef66 Mon Sep 17 00:00:00 2001 From: Chris Morison Date: Sat, 14 Dec 2019 05:56:02 +1200 Subject: [PATCH 1/8] accounts-base: limit finds to required fields #10469 (non-breaking, no new api) --- packages/accounts-base/accounts_server.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/accounts-base/accounts_server.js b/packages/accounts-base/accounts_server.js index fc2a89dd2f..00e5bac206 100644 --- a/packages/accounts-base/accounts_server.js +++ b/packages/accounts-base/accounts_server.js @@ -1150,9 +1150,9 @@ export class AccountsServer extends AccountsCommon { Meteor.startup(() => { this.users.find({ "services.resume.haveLoginTokensToDelete": true - }, { + }, {fields: { "services.resume.loginTokensToDelete": 1 - }).forEach(user => { + }}).forEach(user => { this._deleteSavedTokensForUser( user._id, user.services.resume.loginTokensToDelete @@ -1322,7 +1322,8 @@ const defaultResumeLoginHandler = (accounts, options) => { // sending the unhashed token to the database in a query if we don't // need to. let user = accounts.users.findOne( - {"services.resume.loginTokens.hashedToken": hashedToken}); + {"services.resume.loginTokens.hashedToken": hashedToken}, + {fields: {"services.resume.loginTokens": 1}}); if (! user) { // If we didn't find the hashed login token, try also looking for @@ -1335,7 +1336,8 @@ const defaultResumeLoginHandler = (accounts, options) => { {"services.resume.loginTokens.hashedToken": hashedToken}, {"services.resume.loginTokens.token": options.resume} ] - }); + }, + {fields: {"services.resume.loginTokens": 1}}); } if (! user) From 21c1ca052d3a7094d75b5fdf5b0b5eeea7bb4419 Mon Sep 17 00:00:00 2001 From: Chris Morison Date: Sun, 15 Dec 2019 08:46:04 +1200 Subject: [PATCH 2/8] Added `defaultFieldSelector` and used it in Accounts callbacks. #10469 --- packages/accounts-base/accounts_common.js | 6 +- packages/accounts-base/accounts_server.js | 8 +-- packages/accounts-base/accounts_tests.js | 68 ++++++++++++++++++++++- 3 files changed, 75 insertions(+), 7 deletions(-) diff --git a/packages/accounts-base/accounts_common.js b/packages/accounts-base/accounts_common.js index d89ed40fa1..bd8ede56ca 100644 --- a/packages/accounts-base/accounts_common.js +++ b/packages/accounts-base/accounts_common.js @@ -132,6 +132,7 @@ export class AccountsCommon { * @param {Number} options.passwordResetTokenExpirationInDays The number of days from when a link to reset password is sent until token expires and user can't reset password with the link anymore. Defaults to 3. * @param {Number} options.passwordEnrollTokenExpirationInDays The number of days from when a link to set inital password is sent until token expires and user can't set password with the link anymore. Defaults to 30. * @param {Boolean} options.ambiguousErrorMessages Return ambiguous error messages from login failures to prevent user enumeration. Defaults to false. + * @param {Object} options.defaultFieldSelector Default Mongo field selector, to exclude large custom fields from `Meteor.user()` & `Meteor.findUserBy...()` functions when called without a field selector and `onLogin`, `onLoginFailure` & `onLogout` callbacks. Example: `Accounts.config({ defaultFieldSelector: { myBigArray: 0 }})`. */ config(options) { // We don't want users to accidentally only call Accounts.config on the @@ -166,7 +167,8 @@ export class AccountsCommon { // validate option keys const VALID_KEYS = ["sendVerificationEmail", "forbidClientAccountCreation", "passwordEnrollTokenExpirationInDays", "restrictCreationByEmailDomain", "loginExpirationInDays", "passwordResetTokenExpirationInDays", - "ambiguousErrorMessages", "bcryptRounds"]; + "ambiguousErrorMessages", "bcryptRounds", "defaultFieldSelector"]; + Object.keys(options).forEach(key => { if (!VALID_KEYS.includes(key)) { throw new Error(`Accounts.config: Invalid key: ${key}`); @@ -321,4 +323,4 @@ export const EXPIRE_TOKENS_INTERVAL_MS = 600 * 1000; // 10 minutes export const CONNECTION_CLOSE_DELAY_MS = 10 * 1000; // A large number of expiration days (approximately 100 years worth) that is // used when creating unexpiring tokens. -const LOGIN_UNEXPIRING_TOKEN_DAYS = 365 * 100; +const LOGIN_UNEXPIRING_TOKEN_DAYS = 365 * 100; \ No newline at end of file diff --git a/packages/accounts-base/accounts_server.js b/packages/accounts-base/accounts_server.js index 00e5bac206..e12e54e2bd 100644 --- a/packages/accounts-base/accounts_server.js +++ b/packages/accounts-base/accounts_server.js @@ -188,7 +188,7 @@ export class AccountsServer extends AccountsCommon { }; _successfulLogout(connection, userId) { - const user = userId && this.users.findOne(userId); + const user = userId && this.users.findOne(userId, {fields: this._options.defaultFieldSelector}); this._onLogoutHook.each(callback => { callback({ user, connection }); return true; @@ -317,7 +317,7 @@ export class AccountsServer extends AccountsCommon { let user; if (result.userId) - user = this.users.findOne(result.userId); + user = this.users.findOne(result.userId, {fields: this._options.defaultFieldSelector}); const attempt = { type: result.type || "unknown", @@ -398,7 +398,7 @@ export class AccountsServer extends AccountsCommon { }; if (result.userId) { - attempt.user = this.users.findOne(result.userId); + attempt.user = this.users.findOne(result.userId, {fields: this._options.defaultFieldSelector}); } this._validateLogin(methodInvocation.connection, attempt); @@ -1576,4 +1576,4 @@ const setupUsersCollection = users => { users._ensureIndex("services.resume.loginTokens.when", { sparse: 1 }); // For expiring password tokens users._ensureIndex('services.password.reset.when', { sparse: 1 }); -}; +}; \ No newline at end of file diff --git a/packages/accounts-base/accounts_tests.js b/packages/accounts-base/accounts_tests.js index fb4165a96c..7a079dca17 100644 --- a/packages/accounts-base/accounts_tests.js +++ b/packages/accounts-base/accounts_tests.js @@ -57,6 +57,15 @@ Tinytest.add('accounts - config - default token lifetime', test => { Accounts._options = options; }); +Tinytest.add('accounts - config - defaultFieldSelector', test => { + const options = Accounts._options; + Accounts._options = {}; + const setValue = {bigArray: 0}; + Accounts.config({defaultFieldSelector: setValue}); + test.equal(Accounts._options.defaultFieldSelector, setValue); + Accounts._options = options; +}); + const idsInValidateNewUser = {}; Accounts.validateNewUser(user => { idsInValidateNewUser[user._id] = true; @@ -451,6 +460,63 @@ Tinytest.add( } ); +Tinytest.add( + 'accounts - hook callbacks obey options.defaultFieldSelector', + test => { + const ignoreFieldName = "bigArray"; + const userId = Accounts.insertUserDoc({}, { username: Random.id(), [ignoreFieldName]: [1] }); + const stampedToken = Accounts._generateStampedLoginToken(); + Accounts._insertLoginToken(userId, stampedToken); + const options = Accounts._options; + Accounts._options = {}; + Accounts.config({defaultFieldSelector: {[ignoreFieldName]: 0}}); + test.equal(Accounts._options.defaultFieldSelector, {[ignoreFieldName]: 0}, 'defaultFieldSelector'); + + const validateStopper = Accounts.validateLoginAttempt(attempt => { + test.isUndefined(allowLogin != 'bogus' ? attempt.user[ignoreFieldName] : attempt.user, "validateLoginAttempt") + return allowLogin; + }); + const onLoginStopper = Accounts.onLogin(attempt => + test.isUndefined(attempt.user[ignoreFieldName], "onLogin") + ); + const onLogoutStopper = Accounts.onLogout(logoutContext => + test.isUndefined(logoutContext.user[ignoreFieldName], "onLogout") + ); + const onLoginFailureStopper = Accounts.onLoginFailure(attempt => + test.isUndefined(allowLogin != 'bogus' ? attempt.user[ignoreFieldName] : attempt.user, "onLoginFailure") + ); + + const conn = DDP.connect(Meteor.absoluteUrl()); + + // test a new connection + let allowLogin = true; + conn.call('login', { resume: stampedToken.token }); + + // Now that the user is logged in on the connection, Meteor.userId() should + // return that user. + conn.call('login', { resume: stampedToken.token }); + + // Trigger onLoginFailure callbacks, this will not include the user object + allowLogin = 'bogus'; + test.throws(() => conn.call('login', { resume: "bogus" }), '403'); + + // test a forced login fail which WILL include the user object + allowLogin = false; + test.throws(() => conn.call('login', { resume: stampedToken.token }), '403'); + + // Trigger onLogout callbacks + const onLogoutExpectedUserId = userId; + conn.call('logout'); + + Accounts._options = options; + conn.disconnect(); + validateStopper.stop(); + onLoginStopper.stop(); + onLogoutStopper.stop(); + onLoginFailureStopper.stop(); + } +); + Tinytest.add( 'accounts - verify onExternalLogin hook can update oauth user profiles', test => { @@ -499,4 +565,4 @@ Tinytest.add( Meteor.users.remove(uid2); Accounts._onExternalLoginHook = null; } -); +); \ No newline at end of file From 2ddec43befe7f799bba60e668ee9ea2b011e36d3 Mon Sep 17 00:00:00 2001 From: Chris Morison Date: Mon, 16 Dec 2019 11:51:44 +1200 Subject: [PATCH 3/8] Added field selector and defaultFieldSelector to many user queries --- packages/accounts-base/accounts_common.js | 16 +++- packages/accounts-base/accounts_server.js | 2 +- packages/accounts-base/accounts_tests.js | 51 +++++++++++- packages/accounts-password/password_server.js | 79 ++++++++++++++----- packages/accounts-password/password_tests.js | 59 ++++++++++++-- 5 files changed, 174 insertions(+), 33 deletions(-) diff --git a/packages/accounts-base/accounts_common.js b/packages/accounts-base/accounts_common.js index bd8ede56ca..8a601cbb42 100644 --- a/packages/accounts-base/accounts_common.js +++ b/packages/accounts-base/accounts_common.js @@ -77,13 +77,22 @@ export class AccountsCommon { throw new Error("userId method not implemented"); } + // merge the defaultFieldSelector with an existing options object + _addDefaultFieldSelector(options = {}) { + return options.fields || !this._options.defaultFieldSelector ? options : { + ...options, + fields: this._options.defaultFieldSelector, + }; + } + /** * @summary Get the current user record, or `null` if no user is logged in. A reactive data source. * @locus Anywhere + * @param {Object} [options] `options` parameter to be passed to `Meteor.user.findOne(selector, options)`. Can be used to limit the returned fields. */ - user() { + user(options) { const userId = this.userId(); - return userId ? this.users.findOne(userId) : null; + return userId ? this.users.findOne(userId, this._addDefaultFieldSelector(options)) : null; } // Set up config for the accounts system. Call this on both the client @@ -303,8 +312,9 @@ Meteor.userId = () => Accounts.userId(); * @summary Get the current user record, or `null` if no user is logged in. A reactive data source. * @locus Anywhere but publish functions * @importFromPackage meteor + * @param {Object} [options] `options` parameter to be passed to `Meteor.user.findOne(selector, options)`. Can be used to limit the returned fields. */ -Meteor.user = () => Accounts.user(); +Meteor.user = (options) => Accounts.user(options); // how long (in days) until a login token expires const DEFAULT_LOGIN_EXPIRATION_DAYS = 90; diff --git a/packages/accounts-base/accounts_server.js b/packages/accounts-base/accounts_server.js index e12e54e2bd..aced00f4f2 100644 --- a/packages/accounts-base/accounts_server.js +++ b/packages/accounts-base/accounts_server.js @@ -1212,7 +1212,7 @@ export class AccountsServer extends AccountsCommon { selector[serviceIdKey] = serviceData.id; } - let user = this.users.findOne(selector); + let user = this.users.findOne(selector, Accounts._addDefaultFieldSelector()); // When creating a new user we pass through all options. When updating an // existing user, by default we only process/pass through the serviceData diff --git a/packages/accounts-base/accounts_tests.js b/packages/accounts-base/accounts_tests.js index 7a079dca17..0412160afb 100644 --- a/packages/accounts-base/accounts_tests.js +++ b/packages/accounts-base/accounts_tests.js @@ -517,6 +517,40 @@ Tinytest.add( } ); +Tinytest.add( + 'accounts - MeteorUser() obeys options.defaultFieldSelector', + test => { + const ignoreFieldName = "bigArray"; + const userId = Accounts.insertUserDoc({}, { username: Random.id(), [ignoreFieldName]: [1] }); + const stampedToken = Accounts._generateStampedLoginToken(); + Accounts._insertLoginToken(userId, stampedToken); + const options = Accounts._options; + + // stub Meteor.userId() so it works outside methods and returns the correct user: + const origAccountsUserId = Accounts.userId; + Accounts.userId = () => userId; + + Accounts._options = {}; + + // test the field is included by default + let user = Meteor.user(); + test.isNotUndefined(user[ignoreFieldName], 'included by default'); + + // test the field is excluded + Accounts.config({defaultFieldSelector: {[ignoreFieldName]: 0}}); + user = Meteor.user(); + test.isUndefined(user[ignoreFieldName], 'excluded'); + + // test the field can still be retrieved if required + user = Meteor.user({fields: {[ignoreFieldName]: 1}}); + test.isNotUndefined(user[ignoreFieldName], 'field can be retrieved'); + test.isUndefined(user.username, 'field selector works'); + + Accounts._options = options; + Accounts.userId = origAccountsUserId; + } +); + Tinytest.add( 'accounts - verify onExternalLogin hook can update oauth user profiles', test => { @@ -527,16 +561,24 @@ Tinytest.add( 'facebook', { id: facebookId }, { profile: { foo: 1 } }, - ).id; + ).userId; + const ignoreFieldName = "bigArray"; + const c = Meteor.users.update(uid1, {$set: {[ignoreFieldName]: [1]}}); let users = Meteor.users.find({ 'services.facebook.id': facebookId }).fetch(); test.length(users, 1); test.equal(users[0].profile.foo, 1); + test.isNotUndefined(users[0][ignoreFieldName], 'ignoreField - before limit fields'); // Verify user profile data can be modified using the onExternalLogin // hook, for existing users. - Accounts.onExternalLogin((options) => { + // Also verify that the user object is filtered by _options.defaultFieldSelector + const accountsOptions = Accounts._options; + Accounts._options = {}; + Accounts.config({defaultFieldSelector: {[ignoreFieldName]: 0}}); + Accounts.onExternalLogin((options, user) => { options.profile.foo = 2; + test.isUndefined(users[ignoreFieldName], 'ignoreField - after limit fields'); return options; }); Accounts.updateOrCreateUserFromExternalService( @@ -544,9 +586,11 @@ Tinytest.add( { id: facebookId }, { profile: { foo: 1 } }, ); + // test.isUndefined(users[0][ignoreFieldName], 'ignoreField - fields limited'); users = Meteor.users.find({ 'services.facebook.id': facebookId }).fetch(); test.length(users, 1); test.equal(users[0].profile.foo, 2); + test.isNotUndefined(users[0][ignoreFieldName], 'ignoreField - still there'); // Verify user profile data can be modified using the onExternalLogin // hook, for new users. @@ -555,7 +599,7 @@ Tinytest.add( 'facebook', { id: facebookId }, { profile: { foo: 3 } }, - ).id; + ).userId; users = Meteor.users.find({ 'services.facebook.id': facebookId }).fetch(); test.length(users, 1); test.equal(users[0].profile.foo, 2); @@ -564,5 +608,6 @@ Tinytest.add( Meteor.users.remove(uid1); Meteor.users.remove(uid2); Accounts._onExternalLoginHook = null; + Accounts._options = accountsOptions; } ); \ No newline at end of file diff --git a/packages/accounts-password/password_server.js b/packages/accounts-password/password_server.js index dcb8258dc1..d90e64a9fc 100644 --- a/packages/accounts-password/password_server.js +++ b/packages/accounts-password/password_server.js @@ -5,7 +5,7 @@ const bcryptHash = Meteor.wrapAsync(bcrypt.hash); const bcryptCompare = Meteor.wrapAsync(bcrypt.compare); // Utility for grabbing user -const getUserById = id => Meteor.users.findOne(id); +const getUserById = (id, options) => Meteor.users.findOne(id, Accounts._addDefaultFieldSelector(options)); // User records have a 'services.password.bcrypt' field on them to hold // their hashed passwords (unless they have a 'services.password.srp' @@ -73,6 +73,9 @@ const getRoundsFromBcryptHash = hash => { // properties `digest` and `algorithm` (in which case we bcrypt // `password.digest`). // +// The user parameter needs at least user._id and user.services +Accounts._checkPasswordUserFields = {_id: 1, services: 1}, +// Accounts._checkPassword = (user, password) => { const result = { userId: user._id @@ -120,12 +123,14 @@ const handleError = (msg, throwError = true) => { /// LOGIN /// -Accounts._findUserByQuery = query => { +Accounts._findUserByQuery = (query, options) => { let user = null; if (query.id) { - user = getUserById(query.id); + // default field selector is added within getUserById() + user = getUserById(query.id, options); } else { + options = Accounts._addDefaultFieldSelector(options); let fieldName; let fieldValue; if (query.username) { @@ -139,11 +144,11 @@ Accounts._findUserByQuery = query => { } let selector = {}; selector[fieldName] = fieldValue; - user = Meteor.users.findOne(selector); + user = Meteor.users.findOne(selector, options); // If user is not found, try a case insensitive lookup if (!user) { selector = selectorForFastCaseInsensitiveLookup(fieldName, fieldValue); - const candidateUsers = Meteor.users.find(selector).fetch(); + const candidateUsers = Meteor.users.find(selector, options).fetch(); // No match if multiple candidates are found if (candidateUsers.length === 1) { user = candidateUsers[0]; @@ -161,11 +166,12 @@ Accounts._findUserByQuery = query => { * insensitive search, it returns null. * @locus Server * @param {String} username The username to look for + * @param {Object} [options] `options` parameter to be passed to `Meteor.user.findOne(selector, options)`. Can be used to limit the returned fields. * @returns {Object} A user if found, else null * @importFromPackage accounts-base */ Accounts.findUserByUsername = - username => Accounts._findUserByQuery({ username }); + (username, options) => Accounts._findUserByQuery({ username }, options); /** * @summary Finds the user with the specified email. @@ -174,10 +180,12 @@ Accounts.findUserByUsername = * insensitive search, it returns null. * @locus Server * @param {String} email The email address to look for + * @param {Object} [options] `options` parameter to be passed to `Meteor.user.findOne(selector, options)`. Can be used to limit the returned fields. * @returns {Object} A user if found, else null * @importFromPackage accounts-base */ -Accounts.findUserByEmail = email => Accounts._findUserByQuery({ email }); +Accounts.findUserByEmail = + (email, options) => Accounts._findUserByQuery({ email }, options); // Generates a MongoDB selector that can be used to perform a fast case // insensitive lookup for the given fieldName and string. Since MongoDB does @@ -230,7 +238,13 @@ const checkForCaseInsensitiveDuplicates = (fieldName, displayName, fieldValue, o if (fieldValue && !skipCheck) { const matchedUsers = Meteor.users.find( - selectorForFastCaseInsensitiveLookup(fieldName, fieldValue)).fetch(); + selectorForFastCaseInsensitiveLookup(fieldName, fieldValue), + { + fields: {_id: 1}, + // we only need a maximum of 2 users for the logic below to work + limit: 2, + } + ).fetch(); if (matchedUsers.length > 0 && // If we don't have a userId yet, any match we find is a duplicate @@ -289,7 +303,10 @@ Accounts.registerLoginHandler("password", options => { }); - const user = Accounts._findUserByQuery(options.user); + const user = Accounts._findUserByQuery(options.user, {fields: { + services: 1, + ...Accounts._checkPasswordUserFields, + }}); if (!user) { handleError("User not found"); } @@ -358,7 +375,10 @@ Accounts.registerLoginHandler("password", options => { password: passwordValidator }); - const user = Accounts._findUserByQuery(options.user); + const user = Accounts._findUserByQuery(options.user, {fields: { + services: 1, + ...Accounts._checkPasswordUserFields, + }}); if (!user) { handleError("User not found"); } @@ -419,7 +439,9 @@ Accounts.setUsername = (userId, newUsername) => { check(userId, NonEmptyString); check(newUsername, NonEmptyString); - const user = getUserById(userId); + const user = getUserById(userId, {fields: { + username: 1, + }}); if (!user) { handleError("User not found"); } @@ -465,7 +487,10 @@ Meteor.methods({changePassword: function (oldPassword, newPassword) { throw new Meteor.Error(401, "Must be logged in"); } - const user = getUserById(this.userId); + const user = getUserById(this.userId, {fields: { + services: 1, + ...Accounts._checkPasswordUserFields, + }}); if (!user) { handleError("User not found"); } @@ -523,7 +548,7 @@ Meteor.methods({changePassword: function (oldPassword, newPassword) { Accounts.setPassword = (userId, newPlaintextPassword, options) => { options = { logout: true , ...options }; - const user = getUserById(userId); + const user = getUserById(userId, {fields: {_id: 1}}); if (!user) { throw new Meteor.Error(403, "User not found"); } @@ -556,7 +581,7 @@ const pluckAddresses = (emails = []) => emails.map(email => email.address); Meteor.methods({forgotPassword: options => { check(options, {email: String}); - const user = Accounts.findUserByEmail(options.email); + const user = Accounts.findUserByEmail(options.email, {fields: {emails: 1}}); if (!user) { handleError("User not found"); } @@ -581,6 +606,8 @@ Meteor.methods({forgotPassword: options => { */ Accounts.generateResetToken = (userId, email, reason, extraTokenData) => { // Make sure the user exists, and email is one of their addresses. + // Don't limit the fields in the user object since the user is returned + // by the function and some other fields might be used elsewhere. const user = getUserById(userId); if (!user) { handleError("Can't find user"); @@ -638,6 +665,8 @@ Accounts.generateResetToken = (userId, email, reason, extraTokenData) => { */ Accounts.generateVerificationToken = (userId, email, extraTokenData) => { // Make sure the user exists, and email is one of their addresses. + // Don't limit the fields in the user object since the user is returned + // by the function and some other fields might be used elsewhere. const user = getUserById(userId); if (!user) { handleError("Can't find user"); @@ -782,8 +811,13 @@ Meteor.methods({resetPassword: function (...args) { check(token, String); check(newPassword, passwordValidator); - const user = Meteor.users.findOne({ - "services.password.reset.token": token}); + const user = Meteor.users.findOne( + {"services.password.reset.token": token}, + {fields: { + services: 1, + emails: 1, + }} + ); if (!user) { throw new Meteor.Error(403, "Token expired"); } @@ -889,7 +923,12 @@ Meteor.methods({verifyEmail: function (...args) { check(token, String); const user = Meteor.users.findOne( - {'services.email.verificationTokens.token': token}); + {'services.email.verificationTokens.token': token}, + {fields: { + services: 1, + emails: 1, + }} + ); if (!user) throw new Meteor.Error(403, "Verify email link expired"); @@ -948,7 +987,7 @@ Accounts.addEmail = (userId, newEmail, verified) => { verified = false; } - const user = getUserById(userId); + const user = getUserById(userId, {fields: {emails: 1}}); if (!user) throw new Meteor.Error(403, "User not found"); @@ -1030,7 +1069,7 @@ Accounts.removeEmail = (userId, email) => { check(userId, NonEmptyString); check(email, NonEmptyString); - const user = getUserById(userId); + const user = getUserById(userId, {fields: {_id: 1}}); if (!user) throw new Meteor.Error(403, "User not found"); @@ -1153,4 +1192,4 @@ Accounts.createUser = (options, callback) => { Meteor.users._ensureIndex('services.email.verificationTokens.token', {unique: 1, sparse: 1}); Meteor.users._ensureIndex('services.password.reset.token', - {unique: 1, sparse: 1}); + {unique: 1, sparse: 1}); \ No newline at end of file diff --git a/packages/accounts-password/password_tests.js b/packages/accounts-password/password_tests.js index 43ec4dc1a1..479a0f0181 100644 --- a/packages/accounts-password/password_tests.js +++ b/packages/accounts-password/password_tests.js @@ -1700,10 +1700,12 @@ if (Meteor.isServer) (() => { ) // We should be able to change the username - Tinytest.add("passwords - change username", test => { + Tinytest.add("passwords - change username & findUserByUsername", test => { const username = Random.id(); + const ignoreFieldName = "profile"; const userId = Accounts.createUser({ - username: username + username, + [ignoreFieldName]: {name: 'foo'}, }); test.isTrue(userId); @@ -1714,7 +1716,27 @@ if (Meteor.isServer) (() => { test.equal(Accounts._findUserByQuery({id: userId}).username, newUsername); // Test findUserByUsername as well while we're here - test.equal(Accounts.findUserByUsername(newUsername)._id, userId); + let user = Accounts.findUserByUsername(newUsername); + test.equal(user._id, userId, 'userId - ignore'); + test.isNotUndefined(user[ignoreFieldName], 'field - no ignore'); + + // Test default field selector + const options = Accounts._options; + Accounts._options = {defaultFieldSelector: {[ignoreFieldName]: 0}}; + user = Accounts.findUserByUsername(newUsername); + test.equal(user.username, newUsername, 'username - default ignore'); + test.isUndefined(user[ignoreFieldName], 'field - default ignore'); + + // Test default field selector over-ride + user = Accounts.findUserByUsername(newUsername, { + fields: { + [ignoreFieldName]: 1 + } + }); + test.isUndefined(user.username, 'username - override'); + test.isNotUndefined(user[ignoreFieldName], 'field - override'); + + Accounts._options = options; }); Tinytest.add("passwords - change username to a new one only differing " + @@ -1760,10 +1782,14 @@ if (Meteor.isServer) (() => { user2OriginalUsername); }); - Tinytest.add("passwords - add email", test => { + Tinytest.add("passwords - add email & findUserByEmail", test => { const origEmail = `${Random.id()}@turing.com`; + const username = Random.id(); + const ignoreFieldName = "profile"; const userId = Accounts.createUser({ - email: origEmail + email: origEmail, + username, + [ignoreFieldName]: {name: 'foo'}, }); const newEmail = `${Random.id()}@turing.com`; @@ -1779,7 +1805,28 @@ if (Meteor.isServer) (() => { ]); // Test findUserByEmail as well while we're here - test.equal(Accounts.findUserByEmail(origEmail)._id, userId); + let user = Accounts.findUserByEmail(origEmail); + test.equal(user._id, userId); + test.isNotUndefined(user[ignoreFieldName], 'field - no ignore'); + + // Test default field selector + const options = Accounts._options; + Accounts._options = {defaultFieldSelector: {[ignoreFieldName]: 0}}; + user = Accounts.findUserByEmail(origEmail); + test.equal(user.username, username, 'username - default ignore'); + test.isUndefined(user[ignoreFieldName], 'field - default ignore'); + + // Test default field selector over-ride + user = Accounts.findUserByEmail(origEmail, { + fields: { + [ignoreFieldName]: 1 + } + }); + test.equal(user._id, userId, 'userId - override'); + test.isUndefined(user.username, 'username - override'); + test.isNotUndefined(user[ignoreFieldName], 'field - override'); + + Accounts._options = options; }); Tinytest.add("passwords - add email when the user has an existing email " + From 3b9cda3c78ba0acf2b9694f8bcf83b0d6b39516b Mon Sep 17 00:00:00 2001 From: Chris Morison Date: Mon, 16 Dec 2019 19:46:25 +1200 Subject: [PATCH 4/8] improved _addDefaultFieldSelector(); added tests; #10469 --- .../accounts-base/accounts_client_tests.js | 39 ++++++++++++++++++- packages/accounts-base/accounts_common.js | 27 ++++++++++++- packages/accounts-base/accounts_server.js | 4 +- packages/accounts-base/accounts_tests.js | 18 +++++++-- 4 files changed, 80 insertions(+), 8 deletions(-) diff --git a/packages/accounts-base/accounts_client_tests.js b/packages/accounts-base/accounts_client_tests.js index e6b2385df5..207870285d 100644 --- a/packages/accounts-base/accounts_client_tests.js +++ b/packages/accounts-base/accounts_client_tests.js @@ -1,5 +1,13 @@ const username = 'jsmith'; const password = 'password'; +const excludeField = 'excludeField'; +const defaultExcludeField = 'defaultExcludeField'; +const excludeValue = 'foo'; +const profile = { + name: username, + [excludeField]: excludeValue, + [defaultExcludeField]: excludeValue, +} const logoutAndCreateUser = (test, done, nextTests) => { Meteor.logout(() => { @@ -7,7 +15,7 @@ const logoutAndCreateUser = (test, done, nextTests) => { test.isFalse(Meteor.user()); // Setup a new test user - Accounts.createUser({ username, password }, () => { + Accounts.createUser({ username, password, profile }, () => { // Handle next tests nextTests(test, done); }); @@ -100,3 +108,32 @@ Tinytest.addAsync( }); } ); + +Tinytest.addAsync( + 'accounts - Meteor.user obeys explicit and default field selectors', + (test, done) => { + logoutAndCreateUser(test, done, () => { + Meteor.loginWithPassword(username, password, () => { + // by default, all fields should be returned + test.equal(Meteor.user().profile[excludeField], excludeValue); + + // this time we want to exclude the default fields + const options = Accounts._options; + Accounts._options = {}; + Accounts.config({defaultFieldSelector: {['profile.'+defaultExcludeField]: 0}}); + let user = Meteor.user(); + test.isUndefined(user.profile[defaultExcludeField]); + test.equal(user.profile[excludeField], excludeValue); + test.equal(user.profile.name, username); + + // this time we only want certain fields... + user = Meteor.user({fields: {'profile.name': 1}}); + test.isUndefined(user.profile[excludeField]); + test.isUndefined(user.profile[defaultExcludeField]); + test.equal(user.profile.name, username); + Accounts._options = options; + removeTestUser(done); + }); + }); + } +); diff --git a/packages/accounts-base/accounts_common.js b/packages/accounts-base/accounts_common.js index 8a601cbb42..6cb15f7852 100644 --- a/packages/accounts-base/accounts_common.js +++ b/packages/accounts-base/accounts_common.js @@ -79,10 +79,33 @@ export class AccountsCommon { // merge the defaultFieldSelector with an existing options object _addDefaultFieldSelector(options = {}) { - return options.fields || !this._options.defaultFieldSelector ? options : { + // this will be the most common case for most people, so make it quick + if (!this._options.defaultFieldSelector) return options; + + // if no field selector then just use defaultFieldSelector + if (!options.fields) return { ...options, fields: this._options.defaultFieldSelector, }; + + // if empty field selector then the full user object is explicitly requested, so obey + const keys = Object.keys(options.fields); + if (!keys.length) return options; + + // if the requested fields are +ve then ignore defaultFieldSelector + // assume they are all either +ve or -ve because Mongo doesn't like mixed + if (!!options.fields[keys[0]]) return options; + + // The requested fields are -ve. + // If the defaultFieldSelector is +ve then use requested fields, otherwise merge them + const keys2 = Object.keys(this._options.defaultFieldSelector); + return this._options.defaultFieldSelector[keys2[0]] ? options : { + ...options, + fields: { + ...options.fields, + ...this._options.defaultFieldSelector, + } + } } /** @@ -333,4 +356,4 @@ export const EXPIRE_TOKENS_INTERVAL_MS = 600 * 1000; // 10 minutes export const CONNECTION_CLOSE_DELAY_MS = 10 * 1000; // A large number of expiration days (approximately 100 years worth) that is // used when creating unexpiring tokens. -const LOGIN_UNEXPIRING_TOKEN_DAYS = 365 * 100; \ No newline at end of file +const LOGIN_UNEXPIRING_TOKEN_DAYS = 365 * 100; diff --git a/packages/accounts-base/accounts_server.js b/packages/accounts-base/accounts_server.js index aced00f4f2..87b049af5a 100644 --- a/packages/accounts-base/accounts_server.js +++ b/packages/accounts-base/accounts_server.js @@ -1212,7 +1212,7 @@ export class AccountsServer extends AccountsCommon { selector[serviceIdKey] = serviceData.id; } - let user = this.users.findOne(selector, Accounts._addDefaultFieldSelector()); + let user = this.users.findOne(selector, {fields: this._options.defaultFieldSelector}); // When creating a new user we pass through all options. When updating an // existing user, by default we only process/pass through the serviceData @@ -1576,4 +1576,4 @@ const setupUsersCollection = users => { users._ensureIndex("services.resume.loginTokens.when", { sparse: 1 }); // For expiring password tokens users._ensureIndex('services.password.reset.when', { sparse: 1 }); -}; \ No newline at end of file +}; diff --git a/packages/accounts-base/accounts_tests.js b/packages/accounts-base/accounts_tests.js index 0412160afb..0aa92459c9 100644 --- a/packages/accounts-base/accounts_tests.js +++ b/packages/accounts-base/accounts_tests.js @@ -518,7 +518,7 @@ Tinytest.add( ); Tinytest.add( - 'accounts - MeteorUser() obeys options.defaultFieldSelector', + 'accounts - Meteor.user() obeys options.defaultFieldSelector', test => { const ignoreFieldName = "bigArray"; const userId = Accounts.insertUserDoc({}, { username: Random.id(), [ignoreFieldName]: [1] }); @@ -540,11 +540,23 @@ Tinytest.add( Accounts.config({defaultFieldSelector: {[ignoreFieldName]: 0}}); user = Meteor.user(); test.isUndefined(user[ignoreFieldName], 'excluded'); + user = Meteor.user({}); + test.isUndefined(user[ignoreFieldName], 'excluded {}'); // test the field can still be retrieved if required user = Meteor.user({fields: {[ignoreFieldName]: 1}}); test.isNotUndefined(user[ignoreFieldName], 'field can be retrieved'); - test.isUndefined(user.username, 'field selector works'); + test.isUndefined(user.username, 'field can be retrieved username'); + + // test a combined negative field specifier + user = Meteor.user({fields: {username: 0}}); + test.isUndefined(user[ignoreFieldName], 'combined field selector'); + test.isUndefined(user.username, 'combined field selector username'); + + // test an explicit request for the full user object + user = Meteor.user({fields: {}}); + test.isNotUndefined(user[ignoreFieldName], 'full selector'); + test.isNotUndefined(user.username, 'full selector username'); Accounts._options = options; Accounts.userId = origAccountsUserId; @@ -610,4 +622,4 @@ Tinytest.add( Accounts._onExternalLoginHook = null; Accounts._options = accountsOptions; } -); \ No newline at end of file +); From 0b1925404c89213e3912f83036e4966cc2469ced Mon Sep 17 00:00:00 2001 From: Chris Morison Date: Wed, 18 Dec 2019 03:46:36 +1200 Subject: [PATCH 5/8] Make server onLogOut hook only fetch user if required #10469 --- packages/accounts-base/accounts_server.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/accounts-base/accounts_server.js b/packages/accounts-base/accounts_server.js index 87b049af5a..8d584cec7c 100644 --- a/packages/accounts-base/accounts_server.js +++ b/packages/accounts-base/accounts_server.js @@ -188,8 +188,10 @@ export class AccountsServer extends AccountsCommon { }; _successfulLogout(connection, userId) { - const user = userId && this.users.findOne(userId, {fields: this._options.defaultFieldSelector}); + // don't fetch the user object unless there are some callbacks registered + let user; this._onLogoutHook.each(callback => { + if (!user && userId) user = this.users.findOne(userId, {fields: this._options.defaultFieldSelector}); callback({ user, connection }); return true; }); From c7df65006433318d94413ddc3e362f8b14bc0c23 Mon Sep 17 00:00:00 2001 From: wildhart Date: Wed, 18 Dec 2019 20:12:44 +1200 Subject: [PATCH 6/8] Improved docs of new API #10469 --- packages/accounts-base/accounts_common.js | 8 +++++--- packages/accounts-password/password_server.js | 8 +++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/accounts-base/accounts_common.js b/packages/accounts-base/accounts_common.js index 6cb15f7852..d46e40f989 100644 --- a/packages/accounts-base/accounts_common.js +++ b/packages/accounts-base/accounts_common.js @@ -111,7 +111,8 @@ export class AccountsCommon { /** * @summary Get the current user record, or `null` if no user is logged in. A reactive data source. * @locus Anywhere - * @param {Object} [options] `options` parameter to be passed to `Meteor.user.findOne(selector, options)`. Can be used to limit the returned fields. + * @param {Object} [options] + * @param {MongoFieldSpecifier} options.fields Dictionary of fields to return or exclude. */ user(options) { const userId = this.userId(); @@ -164,7 +165,7 @@ export class AccountsCommon { * @param {Number} options.passwordResetTokenExpirationInDays The number of days from when a link to reset password is sent until token expires and user can't reset password with the link anymore. Defaults to 3. * @param {Number} options.passwordEnrollTokenExpirationInDays The number of days from when a link to set inital password is sent until token expires and user can't set password with the link anymore. Defaults to 30. * @param {Boolean} options.ambiguousErrorMessages Return ambiguous error messages from login failures to prevent user enumeration. Defaults to false. - * @param {Object} options.defaultFieldSelector Default Mongo field selector, to exclude large custom fields from `Meteor.user()` & `Meteor.findUserBy...()` functions when called without a field selector and `onLogin`, `onLoginFailure` & `onLogout` callbacks. Example: `Accounts.config({ defaultFieldSelector: { myBigArray: 0 }})`. + * @param {MongoFieldSpecifier} options.defaultFieldSelector To exclude by default large custom fields from `Meteor.user()` and `Meteor.findUserBy...()` functions when called without a field selector, and all `onLogin`, `onLoginFailure` and `onLogout` callbacks. Example: `Accounts.config({ defaultFieldSelector: { myBigArray: 0 }})`. */ config(options) { // We don't want users to accidentally only call Accounts.config on the @@ -335,7 +336,8 @@ Meteor.userId = () => Accounts.userId(); * @summary Get the current user record, or `null` if no user is logged in. A reactive data source. * @locus Anywhere but publish functions * @importFromPackage meteor - * @param {Object} [options] `options` parameter to be passed to `Meteor.user.findOne(selector, options)`. Can be used to limit the returned fields. + * @param {Object} [options] + * @param {MongoFieldSpecifier} options.fields Dictionary of fields to return or exclude. */ Meteor.user = (options) => Accounts.user(options); diff --git a/packages/accounts-password/password_server.js b/packages/accounts-password/password_server.js index d90e64a9fc..f6ec8c9ef2 100644 --- a/packages/accounts-password/password_server.js +++ b/packages/accounts-password/password_server.js @@ -166,7 +166,8 @@ Accounts._findUserByQuery = (query, options) => { * insensitive search, it returns null. * @locus Server * @param {String} username The username to look for - * @param {Object} [options] `options` parameter to be passed to `Meteor.user.findOne(selector, options)`. Can be used to limit the returned fields. + * @param {Object} [options] + * @param {MongoFieldSpecifier} options.fields Dictionary of fields to return or exclude. * @returns {Object} A user if found, else null * @importFromPackage accounts-base */ @@ -180,7 +181,8 @@ Accounts.findUserByUsername = * insensitive search, it returns null. * @locus Server * @param {String} email The email address to look for - * @param {Object} [options] `options` parameter to be passed to `Meteor.user.findOne(selector, options)`. Can be used to limit the returned fields. + * @param {Object} [options] + * @param {MongoFieldSpecifier} options.fields Dictionary of fields to return or exclude. * @returns {Object} A user if found, else null * @importFromPackage accounts-base */ @@ -1192,4 +1194,4 @@ Accounts.createUser = (options, callback) => { Meteor.users._ensureIndex('services.email.verificationTokens.token', {unique: 1, sparse: 1}); Meteor.users._ensureIndex('services.password.reset.token', - {unique: 1, sparse: 1}); \ No newline at end of file + {unique: 1, sparse: 1}); From 97b2ef8b86f8adeb4a33c70dad402cc0499037e2 Mon Sep 17 00:00:00 2001 From: wildhart Date: Thu, 19 Dec 2019 01:46:52 +1300 Subject: [PATCH 7/8] Updated History.md with #10469 improvements including new Meteor.user() options parameter and Accounts.config({defaultFieldSelector...}) --- History.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/History.md b/History.md index c415b5d0ea..9ab2a7e51a 100644 --- a/History.md +++ b/History.md @@ -1,5 +1,20 @@ ## v.NEXT +* `Meteor.user()`, `Meteor.findUserByEmail()` and `Meteor.findUserByUserName()` can take a new + `options` parameter which can be used to limit the returned fields. Useful for minimizing + DB bandwidth on the server and avoiding unnecessary reactive UI updates on the client. + [Issue #10469](https://github.com/meteor/meteor/issues/10469) +* `Accounts.config()` has a new option `defaultFieldSelector` which will apply to all + `Meteor.user()` and `Meteor.findUserBy...()` functions without explicit field selectors, and + also to all `onLogin`, `onLogout` and `onLoginFailure` callbacks. This is useful if you store + large data on the user document (e.g. a growing list of transactions) which do no need to be + retrieved from the DB whenever you or a package author call `Meteor.user()` without limiting the + fields. [Issue #10469](https://github.com/meteor/meteor/issues/10469) +* Lots of internal calls to `Meteor.user()` without field specifiers in `accounts-base` and + `accounts-password` packages have been optimized with explicit field selectors to only fetch + the fields needed by the functions they are in. + [Issue #10469](https://github.com/meteor/meteor/issues/10469) + ## v1.8.1, 2019-04-03 ### Breaking changes From 413565ba6c5a5a4a8e31f66122c4cc20bd026204 Mon Sep 17 00:00:00 2001 From: wildhart Date: Fri, 20 Dec 2019 13:19:52 +1200 Subject: [PATCH 8/8] Small optimization to modified defaultResumeLoginHandler() --- packages/accounts-base/accounts_server.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/accounts-base/accounts_server.js b/packages/accounts-base/accounts_server.js index 45facc7dd9..1d65de1bc1 100644 --- a/packages/accounts-base/accounts_server.js +++ b/packages/accounts-base/accounts_server.js @@ -1320,7 +1320,7 @@ const defaultResumeLoginHandler = (accounts, options) => { // need to. let user = accounts.users.findOne( {"services.resume.loginTokens.hashedToken": hashedToken}, - {fields: {"services.resume.loginTokens": 1}}); + {fields: {"services.resume.loginTokens.$": 1}}); if (! user) { // If we didn't find the hashed login token, try also looking for @@ -1334,6 +1334,7 @@ const defaultResumeLoginHandler = (accounts, options) => { {"services.resume.loginTokens.token": options.resume} ] }, + // Note: Cannot use ...loginTokens.$ positional operator with $or query. {fields: {"services.resume.loginTokens": 1}}); } @@ -1573,4 +1574,4 @@ const setupUsersCollection = users => { users._ensureIndex("services.resume.loginTokens.when", { sparse: 1 }); // For expiring password tokens users._ensureIndex('services.password.reset.when', { sparse: 1 }); -}; +}; \ No newline at end of file