Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

Commit

Permalink
fix(users): GitHub strategy missing email (#1250)
Browse files Browse the repository at this point in the history
Fixes an issue with an empty/missing/null Email coming from GitHub's
OAuth call response.

Also, introduces the `sparse` index option on the User model's Email
field. This will ensure that we can have multiple User documents without
the Email field.

Adds a server-side User model test for the sparse index setting on the
email field.

Confirms that User documents without the email field are not indexed,
illustrating the sparse option on the schema's email field works
properly.

Added the dropdb task to the Gulp test:client & test:server tasks, to
ensure we have a clean database & that any indexes are rebuilt; this
will ensure any Schema changes (in this case the email index is rebuilt using
the sparse index option) are reflected when the database is started again.

Added a UPGRADE.md for tracking important upgrade information for our
user's to be aware of, when we introduce potentially breaking changes.

Included an explanation of the Sparse index being added, and how to apply it
to an existing MEANJS application's database.

Adds a script for dropping the `email` field's index from the User
collection.

Related #1145
  • Loading branch information
mleanos committed Apr 29, 2016
1 parent e3cd65f commit 4906611
Show file tree
Hide file tree
Showing 6 changed files with 106 additions and 4 deletions.
12 changes: 12 additions & 0 deletions UPGRADE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<a name="0.5.0"></a>
## 0.5.0 (TBD)

* The Users collection's Email field index now uses the MongoDB [Sparse](https://docs.mongodb.org/manual/core/index-sparse/) option
* The User model's Email field is not required, and there are scenarios where you will have User documents that don't contain the Email field. In this case, without the Sparse option, you will get a unique constraint index exception thrown when an additional User document is saved without this field.
* If you are upgrading an existing MEANJS application, and have not changed this field, you will already have an existing index on the Email field of your Users collection. If this is the case, you will need to follow these steps for the `sparse` option to be applied.


1. Run the upgrade script from the root of your MEANJS application to drop the index: `node scripts/upgrade-users-sparse-index`
2. Restart your MEANJS application.

When your application starts back up, Mongoose will rebuild your Email field's index; now with the `sparse` option applied to it.
4 changes: 2 additions & 2 deletions gulpfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ gulp.task('test', function (done) {
});

gulp.task('test:server', function (done) {
runSequence('env:test', ['copyLocalEnvConfig', 'makeUploadsDir'], 'lint', 'mocha', done);
runSequence('env:test', ['copyLocalEnvConfig', 'makeUploadsDir', 'dropdb'], 'lint', 'mocha', done);
});

// Watch all server files for changes & run server tests (test:server) task on changes
Expand All @@ -381,7 +381,7 @@ gulp.task('test:server:watch', function (done) {
});

gulp.task('test:client', function (done) {
runSequence('env:test', 'lint', 'karma', done);
runSequence('env:test', 'lint', 'dropdb', 'karma', done);
});

gulp.task('test:e2e', function (done) {
Expand Down
2 changes: 1 addition & 1 deletion modules/users/server/config/strategies/github.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ module.exports = function (config) {
firstName: firstName,
lastName: lastName,
displayName: displayName,
email: profile.emails[0].value,
email: (profile.emails && profile.emails.length) ? profile.emails[0].value : undefined,
username: profile.username,
// jscs:disable requireCamelCaseOrUpperCaseIdentifiers
profileImageURL: (providerData.avatar_url) ? providerData.avatar_url : undefined,
Expand Down
5 changes: 4 additions & 1 deletion modules/users/server/models/user.server.model.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ var UserSchema = new Schema({
},
email: {
type: String,
unique: true,
index: {
unique: true,
sparse: true // For this to work on a previously indexed field, the index must be dropped & the application restarted.
},
lowercase: true,
trim: true,
default: '',
Expand Down
23 changes: 23 additions & 0 deletions modules/users/tests/server/user.server.model.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,29 @@ describe('User Model Unit Tests:', function () {

});

it('should not index missing email field, thus not enforce the model\'s unique index', function (done) {
var _user1 = new User(user1);
_user1.email = undefined;

var _user3 = new User(user3);
_user3.email = undefined;

_user1.save(function (err) {
should.not.exist(err);
_user3.save(function (err) {
should.not.exist(err);
_user3.remove(function (err) {
should.not.exist(err);
_user1.remove(function (err) {
should.not.exist(err);
done();
});
});
});
});

});

it('should not save the password in plain text', function (done) {
var _user1 = new User(user1);
var passwordBeforeSave = _user1.password;
Expand Down
64 changes: 64 additions & 0 deletions scripts/upgrade-users-sparse-index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
'use strict';
// Set the Node ENV
process.env.NODE_ENV = 'development';

var chalk = require('chalk'),
mongoose = require('../config/lib/mongoose');

mongoose.loadModels();

var _indexToRemove = 'email_1',
errors = [],
processedCount = 0;

mongoose.connect(function (db) {
// get a reference to the User collection
var userCollection = db.connections[0].collections.users;

console.log();
console.log(chalk.yellow('Removing index "' +
_indexToRemove + '" from the User collection.'));
console.log();

// Remove the index
userCollection.dropIndex(_indexToRemove, function (err, result) {
var message = 'Successfully removed the index "' + _indexToRemove + '".';

if (err) {
errors.push(err);
message = 'An error occured while removing the index "' +
_indexToRemove + '".';

if (err.message.indexOf('index not found with name') !== -1) {
message = 'Index "' + _indexToRemove + '" could not be found.' +
'\r\nPlease double check the index name in your ' +
'mongodb User collection.';
}

reportAndExit(message);
} else {
reportAndExit(message);
}
});
});

function reportAndExit(message) {
if (errors.length) {
console.log(chalk.red(message));
console.log();

console.log(chalk.yellow('Errors:'));
for (var i = 0; i < errors.length; i++) {
console.log(chalk.red(errors[i]));

if (i === errors.length -1) {
process.exit(0);
}
}
} else {
console.log(chalk.green(message));
console.log(chalk.green('The next time your application starts, ' +
'Mongoose will rebuild the index "' + _indexToRemove + '".'));
process.exit(0);
}
}

0 comments on commit 4906611

Please sign in to comment.