fix(users): GitHub strategy missing email #1250

Merged
merged 1 commit into from Apr 29, 2016

Projects

None yet

7 participants

@mleanos
Member
mleanos commented Mar 6, 2016

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.

Related #1145

@mleanos
Member
mleanos commented Mar 6, 2016

I'll add some tests to this PR, sometime tomorrow.

@mleanos
Member
mleanos commented Mar 6, 2016

Added a test to ensure the sparse option is working properly.

@lirantal WDYT?

@lirantal
Member
lirantal commented Mar 6, 2016

I think it's overall ok, although I'm wondering if there's really a case where GitHub will return no email for a user?

@mleanos
Member
mleanos commented Mar 6, 2016

If a user chooses the "Don't show my email address" option in their Profile settings for Public email. Then GitHub won't return the email address. Also, Twitter doesn't send user's email at all.

@lirantal
Member
lirantal commented Mar 6, 2016

Which only make you wonder how twitter ever worked, right? :)
Ok, let's see if anyone else has anything else to contribute

@mleanos
Member
mleanos commented Mar 6, 2016

Twitter worked, because we allow empty/missing email fields in our User model. The issue will only show itself if there's an attempt to save a new User without the email field. Thus, mongodb treats both of those as the same, and won't allow it since we have the unique index setting on that field; there's a violation of the unique constraint in that case.

We don't run into this issue with local testing, since most of us just use one User for any particular Social Account. I guess User's of this framework haven't ran into this issue, or reported it. Until @farajfarook did.

@lirantal
Member
lirantal commented Mar 8, 2016

That's what I was referring to - that probably users of MEAN.JS who used GitHub did get this problem for such cases where the user opted out from sharing the email detail.

Anyway, it looks like a correct way of handling it.
LGTM.

@codydaig maybe another pair of eyes?

@lirantal
Member
lirantal commented Mar 8, 2016

@mleanos BTW you are definitely able to run some manual tests with local users signup, GitHub as well as another strategy, right? just to make sure we're not breaking anything larger here.

@mleanos mleanos was assigned by lirantal Mar 8, 2016
@lirantal lirantal added this to the 0.5.0 milestone Mar 8, 2016
@lirantal lirantal added the bug label Mar 8, 2016
@mleanos
Member
mleanos commented Mar 9, 2016

@lirantal I've been going through the Social Accounts implementation & testing. I've noticed some issues with the other providers as well. This fix is part of a bigger initiative to get the Social Providers working 100%. I have a list of issues I've come across, but haven't organized it yet.

I have tested this with the other provider's and there aren't any issues, other than the other aspects of the Social Provider's implementation that are broken. However, this does also take care of the issue raised in #1145.

I did extensive testing with the sparse option, and it won't have any adverse effects. The only nuance is that when this is integrated into an existing application, the index on the email field in the MongoDB instance would need to be dropped. The next time the application is run, Mongoose will rebuild the index using the sparse option. I added a comment in the Schema for that.

Also, the test I added will ensure that the sparse index works; regardless of which Provider is being used.

@lirantal
Member

@mleanos ok, that's clear.
Do users have to manually drop the index or will mongodb rebuild it automatically for them?
If it's manually, I'm not sure users will understand that straight away - is it possible to catch the error from mongodb on that and show a message to guide them?

@mleanos
Member
mleanos commented Mar 10, 2016

User's do have to manually drop the index. However, we can probably add a Gulp task that is executed with one of our other tasks, that start the server; default, prod, test. Do you think that would work?

Another option might be to add something to our mongoose server config, that ensures the index is dropped if the sparse option is not set for it. Or we simply display a message in this case. I haven't looked into that, but I think it will be possible. One caveat to this option is that this check will be performed every time the database is started; I'm not sure how I feel about that.

I'd like to make it as easy as possible for our users. However, it might be more trouble for us than it's worth. As it stands, it's technically not a breaking change. Users will only run into any issue when a duplicate user is saved that violates the indexes unique constraint; in this case, a missing/empty email field.

I'm happy to do more work on this to make it easier on our users. But I just wanted to throw those options out there. And the inline comment might suffice for now.

@lirantal
Member

We would like to be unobtrusive but in the same time not break things horribly.
This has been a recurrent issue for us with other updates we had before like fixing the users table's salt which was accidentally binary and that messed things up and was a breaking change too when we fixed it.

I would like to suggest that we have an UPGRADE.md (and possibly accompanying wiki page or whatever else helps as well) that states the big changes that require user involvement when the version changes.

@ilanbiala WDYT?

@ilanbiala
Member

If we do like Angular does and put in our Changelog the breaking changes and the fixes, then I think that is sufficient.

@lirantal
Member

sounds good to me.
@mleanos can you include this kind of update with this PR about the index change and instructions of what should be done?

@mleanos
Member
mleanos commented Mar 13, 2016

I added a UPGRADE.md to this PR. However, I may need help with the markup. I don't have a way of testing the markup locally, since I'm on Windows. And AFAIK Jenkins isn't supported on Windows.

@lirantal lirantal and 1 other commented on an outdated diff Mar 13, 2016
@@ -0,0 +1,12 @@
+<a name="0.4.2"></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. Manually drop the Email field's index in your Users collection, on your applications MongoDB instance.
@lirantal
lirantal Mar 13, 2016 Member

@mleanos
Can we provide actual copy&paste instructions on how to do it to ease the users?

@mleanos
mleanos Mar 13, 2016 Member

Sure. I can add the MongoDB commands to drop the index, and then the commands to start the application.

@lirantal
Member

@mleanos added my comment and also you can see how the markup renders through GitHub, check this URL for your file: https://github.com/mleanos/mean/blob/fix/users-github-strategy-email/UPGRADE.md

@mleanos
Member
mleanos commented Mar 13, 2016

@lirantal I know how to test it once it's in a remote GitHub repo. I just can't view the rendered markup locally. It's not a problem. I can just push to my fork, and test there.

@mleanos
Member
mleanos commented Apr 6, 2016

@lirantal @ilanbiala @codydaig I updated this PR to include a companion upgrade script, for dropping the index using Mongoose. I also updated the UPGRADE readme to include directions on how to use the script.

How does the UPGRADE.md file look to y'all?

@coveralls

Coverage Status

Coverage remained the same at 70.615% when pulling 07b56f6 on mleanos:fix/users-github-strategy-email into bc0b4a6 on meanjs:master.

@coveralls

Coverage Status

Coverage remained the same at 70.615% when pulling 07b56f6 on mleanos:fix/users-github-strategy-email into bc0b4a6 on meanjs:master.

@codydaig codydaig and 2 others commented on an outdated diff Apr 6, 2016
scripts/upgrade-users-sparse-index.js
@@ -0,0 +1,59 @@
+'use strict';
+// Set the Node ENV
+process.env.NODE_ENV = 'development';
+
+var mongoose = require('mongoose'),
+ chalk = require('chalk'),
+ mg = require('../config/lib/mongoose');
@codydaig
codydaig Apr 6, 2016 Member

I think this should be mongoose instead of mg to be consistent with the rest of the project.

@ilanbiala
ilanbiala Apr 6, 2016 Member

mongoose is defined above

@simison
simison Apr 6, 2016 Contributor

mongooseConfig, then? mg is a bit fuzzy.

@codydaig
Member
codydaig commented Apr 6, 2016

Upgrade.md looks fine to me. In the upgrade script, I don't want to start abbreviating package names. I'd rather keep it as mongoose.

@mleanos
Member
mleanos commented Apr 9, 2016

@codydaig I addressed your comment. For clarification, mg was referencing our mongoose configuration & mongoose was already used by the reference to the Mongoose library; the latter was not used in this script so I removed it and renamed the other reference :)

@lirantal How does this look to you now, in regards to your comments about the upgrade instructions?

@coveralls

Coverage Status

Coverage remained the same at 70.615% when pulling 570b69b on mleanos:fix/users-github-strategy-email into bc0b4a6 on meanjs:master.

@codydaig
Member
codydaig commented Apr 9, 2016

LGTM

@lirantal
Member

Great, let's add.

@ilanbiala ilanbiala and 1 other commented on an outdated diff Apr 10, 2016
scripts/upgrade-users-sparse-index.js
+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(chalk.yellow('*************** Removing index "' + _indexToRemove + '" from the User collection ***************'));
@ilanbiala
ilanbiala Apr 10, 2016 Member

Is this line not over 80 characters?

@mleanos
mleanos Apr 10, 2016 Member

@ilanbiala Yes. I don't really like what I did with the messages. I'll clean them up.

Just to clarify, are you concerned with the length of the line of the code, or the output of the console log?

@ilanbiala
ilanbiala Apr 11, 2016 Member

Mostly length, I think the content is pretty good.

@mleanos
mleanos Apr 11, 2016 Member

Alright. I can break these up into multiple lines of code.

@ilanbiala ilanbiala commented on an outdated diff Apr 10, 2016
scripts/upgrade-users-sparse-index.js
+ // get a reference to the User collection
+ var userCollection = db.connections[0].collections.users;
+
+ 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 = 'An index with the name "' + _indexToRemove + '" could not be found. Please double check the index name in your mongodb User collection.';
@ilanbiala
ilanbiala Apr 10, 2016 Member

over 80 characters?

@ilanbiala ilanbiala commented on an outdated diff Apr 10, 2016
scripts/upgrade-users-sparse-index.js
+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 perform ensureIndex() which will rebuild the index "' + _indexToRemove + '".'));
@ilanbiala
ilanbiala Apr 10, 2016 Member

over 80 characters?

@coveralls

Coverage Status

Coverage remained the same at 70.615% when pulling 9c57a79 on mleanos:fix/users-github-strategy-email into f2a6bf9 on meanjs:master.

@coveralls

Coverage Status

Coverage remained the same at 70.615% when pulling 9c57a79 on mleanos:fix/users-github-strategy-email into f2a6bf9 on meanjs:master.

@mleanos
Member
mleanos commented Apr 13, 2016

@ilanbiala I've cleaned up those long lines of code. LGTY?

@ilanbiala ilanbiala and 1 other commented on an outdated diff Apr 13, 2016
scripts/upgrade-users-sparse-index.js
+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 + '".'));
@ilanbiala
ilanbiala Apr 13, 2016 Member

This line is too long still.

@mleanos
mleanos Apr 13, 2016 Member

Forgot that one :|

@trendzetter
Contributor

👍

@ilanbiala ilanbiala commented on an outdated diff Apr 20, 2016
modules/users/tests/server/user.server.model.tests.js
@@ -193,6 +193,29 @@ describe('User Model Unit Tests:', function () {
});
+ it('should not index missing email field, and not enforce unique index constraint (sparse index)', function (done) {
@ilanbiala
ilanbiala Apr 20, 2016 Member

Also slightly long, this is 120 chars

@ilanbiala ilanbiala commented on an outdated diff Apr 20, 2016
scripts/upgrade-users-sparse-index.js
+
+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.'));
@ilanbiala
ilanbiala Apr 20, 2016 Member

97 chars

@ilanbiala
Member

@mleanos strangely we don't have a linting rule for line length. Adding that would be nice for the future so we can catch extra long lines more easily.

@mleanos mleanos fix(users): GitHub strategy missing email
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
bd64737
@coveralls
coveralls commented Apr 28, 2016 edited

Coverage Status

Coverage remained the same at 70.615% when pulling bd64737 on mleanos:fix/users-github-strategy-email into e3cd65f on meanjs:master.

@mleanos
Member
mleanos commented Apr 28, 2016

@ilanbiala I shortened the lines in the upgrade script to be less than 80 chars. As for the test line you mentioned, I shortened it as much as possible but it's still about 105 chars long. I didn't see any other way of shortening that line. Quite a bit of our describe() lines are longer than 80 chars.

If this looks good to you now, I'll merge.

@ilanbiala
Member

LGTm.

@mleanos mleanos merged commit 4906611 into meanjs:master Apr 29, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 70.615%
Details
@Wuntenn Wuntenn pushed a commit to Wuntenn/mean that referenced this pull request Sep 11, 2016
Daron Jones fix(users) patch OAuth default email issue
Avoids issue re:sparse for email and default value used for the email. See comment at change

Related to issue #1250
0caa71b
@Wuntenn Wuntenn pushed a commit to Wuntenn/mean that referenced this pull request Sep 11, 2016
Daron Jones fix(users) patch OAuth default email issue
Intentionally omits setting email in constructor to trigger defaults
when creating user.

Done to hand cases where email is not authorized/given by provider.

Related to issue #1250
3677f39
@Wuntenn Wuntenn pushed a commit to Wuntenn/mean that referenced this pull request Sep 11, 2016
Daron Jones fix(users) patch OAuth default email issue
Intentionally omits setting email in constructor to trigger defaults
when creating user.

Done to hand cases where email is not authorized/given by provider.

Related to issue #1250
8888ba2
@Wuntenn Wuntenn pushed a commit to Wuntenn/mean that referenced this pull request Sep 11, 2016
Daron Jones fix(users) patch OAuth default email issue
Intentionally omits setting email in constructor to trigger defaults
when creating user.

Done to hand cases where email is not authorized/given by provider.

Related to issue #1250
5c64945
@Wuntenn Wuntenn pushed a commit to Wuntenn/mean that referenced this pull request Sep 20, 2016
Daron Jones fix(users) patch OAuth default email issue
- Intentionally omits setting email in constructor to trigger defaults when
creating user. Handles cases where email is not authorized/given by provider.

Related to issue #1250
340ddb1
@mleanos mleanos added a commit that referenced this pull request Sep 21, 2016
@Wuntenn @mleanos Wuntenn + mleanos fix(users) Handle missing email - OAuth (#1501)
* fix(users) patch OAuth default email issue

- Intentionally omits setting email in constructor to trigger defaults when
creating user. Handles cases where email is not authorized/given by provider.

Related to issue #1250
e3eafa6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment