Invalidate sessions after email change #2693

loay · 2016-08-30T19:10:15Z

connect to https://github.com/strongloop-internal/scrum-loopback/issues/924

Delete all tokens of the current user after an email was changed.
~~The currently active token should not be deleted.~~

bajtos · 2016-09-01T14:10:52Z

common/models/user.js

+      if (ctx.instance) {
+        var oldEmail = ctx.instance.email;
+      }
+      UserModel.observe('after save', function(ctx, next) {


This will add a new observer after whenever a save operation is called. That won't work.

The correct solution is to store oldEmail in ctx.hookState instead of creating a local closure.

// in before save ctx.hookState.oldEmail = ctx.instance.email // in after save var oldEmail = ctx.hookState.oldEmail

Having said that, I am afraid we don't have any easy way how to detect when a field was changed.

The only reliable solution is to query the database for the old version and compare it with the data being persisted.

bajtos · 2016-09-01T14:13:15Z

Please test also that sessions are not invalidated when email was not changed.

Both test cases should be run for all operations changing data (create, updateAttributes, updateOrCreate, replaceAttributes, replaceOrCreate).

bajtos · 2016-09-01T14:15:13Z

Most of the concerns from #2665 (comment) apply here too:

Please add a unit-test to verify that access tokens of other users are not changed

Token removal should be configurable, i.e. the user should be able to decide whether they want to log out all other sessions or not

The currently active token should not be deleted.

loay · 2016-09-05T04:43:29Z

@bajtos PTAL. Thanks.

bajtos · 2016-09-05T10:55:24Z

common/models/user.js

@@ -668,6 +668,45 @@ module.exports = function(User) {
      next();
    });

+    // Delete old sessions once email is updated
+    var oldEmail, newEmail, oldId, newId;


These variables are shared among all invocations of your hook. What will happen when there are two "save" operations in progress at the same time?

bajtos · 2016-09-05T11:35:01Z

@loay the implementation looks better now. I think your tests are not covering two important cases: User.create and User.updateOrCreate when a new user is created.

Also the part The currently active token should not be deleted. is not implemented yet, but let's leave that as the second step in this pull request, do it only after the rest is already done and reviewed.

bajtos · 2016-09-14T07:56:14Z

test/user.test.js

+          function secondLoginWithOriginalEmail(next) {
+            User.login({
+              email: currentEmailCredentials.email,
+              password: currentEmailCredentials.password },


coding style: one arg per line. see https://github.com/strongloop/loopback-contributor-docs/blob/master/style-guide.md#one-arg-per-line

loay · 2016-09-14T14:42:10Z

@bajtos @rmg
About keeping the currently active token, I am not sure if I agree about that. Some apps usually give the option for passwords change, but for email, which is the username as well, I think all sessions including the current active one should be deleted. It is not too annoying to be done, and the user will psychologically feel safer when they get to log back in.

loay · 2016-09-15T02:14:00Z

@bajtos PTAL. Thanks
Actually I removed extend function refactoring, as it was failing on several modules, so removed it but did other refactoring.

loay · 2016-09-15T18:45:31Z

@slnode test please

bajtos · 2016-09-16T11:07:31Z

About keeping the currently active token, I am not sure if I agree about that. Some apps usually give the option for passwords change, but for email, which is the username as well, I think all sessions including the current active one should be deleted. It is not too annoying to be done, and the user will psychologically feel safer when they get to log back in.

Fair enough. We can always implement that later, if/when there is user demand.

bajtos · 2016-09-16T11:09:45Z

common/models/user.js

+        });
+      } else if (ctx.instance) {
+        ctx.hookState.id = ctx.instance.id;
+        var oldId = ctx.hookState.id;


AFAICT, ctx.hookState.id is not used anywhere else, please remove.

Also the name oldId is a bit misleading. The id is (almost) never changed, therefore there is no such thing as "old" and "new" id.

I am proposing to use id or userId instead.

bajtos · 2016-09-16T11:12:23Z

common/models/user.js

+          return { id: u.id, email: u.email };
+        });
+      }
+      next();


Once again. This next is misplaced, you need to call it from inside Model.find callback.

Considering that both branches use the same callback, I am suggesting the following implementation:

var where = ctx.where || { id: ctx.instance.id }; // feel free to add more safety checks ctx.Model.find({ where: where }, function(err, userInstances) { if (err) return next(err); ctx.hookState.originalUserData = userInstances.map(function(u) { return { id: u.id, email: u.email }; }); next(); });

bajtos · 2016-09-16T11:12:41Z

common/models/user.js

+    UserModel.observe('after save', function afterEmailUpdate(ctx, next) {
+      if (!ctx.Model.relations.accessTokens) return next();
+      var AccessToken = ctx.Model.relations.accessTokens.modelTo;
+      var  newEmail = (ctx.instance || ctx.data).email;


two spaces after var, use one only.

bajtos · 2016-09-16T11:15:30Z

common/models/user.js

+      var  newEmail = (ctx.instance || ctx.data).email;
+      if (!ctx.hookState.originalUserData) return next();
+      var idsToExpire = ctx.hookState.originalUserData.filter(function(u) {
+        return u.email !== newEmail;


What if we are making a partial update and email is not changed at all?

user.updateAttributes({ age: user.age+1 });

newEmail is undefined, u.email is the existing email, which means you will incorrectly invalidate all access tokens. What I find surprising is that you already have a test covering this path (here). How comes it's passing? What am I missing?

Implementation wise, I think we should detect this situation early in "before save" hook to avoid unnecessary query.

The 4 unit tests of keeps sessions AS IS are testing this case with different ways of update. newEmail is not undefined. It is the current/existing email or ([email protected]). A previous implementation was generating an undefined value but that was fixed.

bajtos · 2016-09-16T11:15:55Z

common/models/user.js

+      });
+      if (!idsToExpire.length) return next();
+      AccessToken.deleteAll({ userId: { inq: idsToExpire }}, function(err, info) {
+        if (err) return next(err);


No need to be so verbose, just pass next as the callback of deleteAll.

bajtos · 2016-09-16T11:16:14Z

test/user.test.js

@@ -48,6 +48,7 @@ describe('User', function() {
    User = app.registry.createModel('TestUser', {}, {
      base: 'User',
      http: { path: 'test-users' },
+      forceId: false,


Please add a comment explaining why we need forceId:false.

bajtos · 2016-09-16T11:16:38Z

test/user.test.js

+        });
+      });
+
+      afterEach('Deleting original user', function deleteOriginalAccounts(done) {


delete current user

TBH, I am not sure if it's necessary to delete these test user instances. If you think it is important, then please add another hook to delete any remaining access tokens too.

bajtos · 2016-09-16T11:20:48Z

test/user.test.js

+            });
+          },
+        ], function(err) {
+          done();


Unhandled err, you need done(err). Even better, simply pass done as the callback argument of async.series.

bajtos

Reviewed, PTAL

bajtos · 2016-09-16T11:22:33Z

test/user.test.js

+            });
+          },
+        ], function(err) {
+          done();


Unhandled err, pass done as the callback of async.series.

bajtos · 2016-09-16T11:22:51Z

test/user.test.js

+            });
+          },
+        ], function(err) {
+          done();


Unhandled err, pass done as the callback of async.series.

bajtos · 2016-09-16T11:24:46Z

test/user.test.js

+            User.login(newUserCred, function(err, newAccessToken) {
+              if (err) return done(err);
+              assert(newAccessToken.id);
+              assertPreservedToken(next);


This check does not make sense to me. Of course the new access token is preserved, we didn't make any changes in User data.

I think you need to verify that access tokens of the first (original) user were not changed by User.create.

Create a new User

Verify that existing access tokens were not deleted

bajtos · 2016-09-16T11:25:11Z

test/user.test.js

+            User.login(newUserCred, function(err, newAccessToken2) {
+              if (err) return done(err);
+              assert(newAccessToken2.id);
+              assertPreservedToken(next);


ditto, this check does not make sense to me.

bajtos · 2016-09-16T11:25:42Z

test/user.test.js

+          function(next) {
+            User.login(
+              { email: '[email protected]', password: 'u1pass' },
+              function(err, accessToken1) {


unhandled err

bajtos · 2016-09-16T11:25:48Z

test/user.test.js

+              function(err, accessToken1) {
+                User.login(
+                  { email: '[email protected]', password: 'u2pass' },
+                  function(err, accessToken2) {


bajtos · 2016-09-16T11:28:18Z

test/user.test.js

+      });
+    });
+
+    describe('when where object contains only the name', function() {


Why is name so special? I think this describe block is extraneous, just use it('invalidates sessions after using updateAll') without nesting it it another describe block.

bajtos · 2016-09-16T11:28:41Z

test/user.test.js

+                  next();
+                });
+          },
+          function(next) {


please give a name to this function, e.g. verifyTokensOfSpecialUser.

bajtos · 2016-09-16T11:28:44Z

test/user.test.js

+              next();
+            });
+          },
+          function(next) {


bajtos

Almost there :)

bajtos · 2016-09-19T11:53:32Z

common/models/user.js

@@ -709,7 +698,7 @@ module.exports = function(User) {
      });
      if (!idsToExpire.length) return next();
      AccessToken.deleteAll({ userId: { inq: idsToExpire }}, function(err, info) {
-        if (err) return next(err);
+        if (err) return next();


This is dropping (ignoring) any deleteAll errors. Is that intentional?

I would personally use the following implementation:

AccessToken.deleteAll({ userId: { inq: idsToExpire }}, next);

bajtos · 2016-09-19T11:54:27Z

test/user.test.js

-        User.deleteById(user.id, function(err, user) {
-          if (err) return done (err);
-          done();
+          done(err);


I think there is no need to create a dummy function forwarding err to done. You can write:

], done);

Unless I am missing something?

This applies to many other places below too.

bajtos · 2016-09-19T11:59:09Z

test/user.test.js

@@ -1807,9 +1809,10 @@ describe('User', function() {

  describe('Email Update', function() {
    describe('User changing email property', function() {
-      var user;
+      var user, originalUserToken1, originalUserToken2, newUserCreated, userSpecial, userNormal;


AFAICT, userSpecial and userNormal are used only in a single test only. Could you please move the declaration of these two variables down to the test case?

bajtos · 2016-09-19T12:00:57Z

test/user.test.js

+        AccessToken.find({ where: { userId: user.id }}, function(err, tokens) {
+          if (err) return done(err);
+          expect(tokens.length).to.equal(2);
+          assert(originalUserToken1);


What is the intention of this assert? The contents of originalUserToken1 should not change between the time it was initialized and the time you run the assert here. Did you perhaps want to assert that tokens contains both originalUserToken1 and originalUserToken2?

Yes absolutely. I didn't just want to make sure that the 2 tokens are still there, but also the exact same tokens.

I am tweaking the code a bit to ensure it has both tokens.

bajtos · 2016-09-19T12:02:44Z

common/models/user.js

+        next();
+      });
+    });
+    UserModel.observe('after save', function afterEmailUpdate(ctx, next) {


Please add an empty line between UserModel.observe blocks to visually delimit them.

UserModel.observe('before save', . }); UserModel.observe('after save', ... });

bajtos · 2016-09-19T12:03:00Z

common/models/user.js

+      });
+    });
+
+


Too many empty lines, keep one empty line only.

bajtos

LGTM, please squash the commits before landing.

doublemarked · 2016-12-15T09:43:08Z

We've discovered this new behavior after bumping Loopback versions. Unfortunately it was not caught by our automated tests (though in the future it will be).

I realize this was in response to a security issue, but I feel like somehow there could be a better process to introducing breaking changes. Maybe at least clear announcements/notifications when things like this are landing? How do we as developers guard against changes like this breaking our software?

doublemarked · 2016-12-15T10:55:05Z

I guess one more thing... @bajtos you wrote,

Also the part The currently active token should not be deleted. is not implemented yet, but let's leave that as the second step in this pull request, do it only after the rest is already done and reviewed.

This is precisely the part that is breaking existing code, because it requires us to rework our frontend to accommodate tokens that suddenly get invalidated. Will such a fix be anticipated in the future? We're trying to figure out whether we should be reworking our frontend to anticipate/explain why the user suddenly needs to reauthenticate after changing their email address, or whether we can hold out on 2.34.1 until such a fix lands.

bajtos · 2016-12-16T14:40:20Z

Hi @doublemarked, I apologise for the troubles we have caused.

I realize this was in response to a security issue, but I feel like somehow there could be a better process to introducing breaking changes. Maybe at least clear announcements/notifications when things like this are landing? How do we as developers guard against changes like this breaking our software?

I believe we are announcing security-related fixes, @loay @rmg @gunjpan could one of you please provide more information on the channels we are using? Did we announce this patch?

@doublemarked do you have any suggestion/advise on what channel would work best for developers like you? I think it may be best to discuss this with broader community, would you mind starting the discussion in a new github issue?

This is precisely the part that is breaking existing code, because it requires us to rework our frontend to accommodate tokens that suddenly get invalidated. Will such a fix be anticipated in the future? We're trying to figure out whether we should be reworking our frontend to anticipate/explain why the user suddenly needs to reauthenticate after changing their email address, or whether we can hold out on 2.34.1 until such a fix lands.

I agree the current situation, where a change of email or password (see #3021) logs out the user, creates a rather poor experience and we should fix this soon. One of the reasons why this was not made in the original pull requests, is that we don't have a good solution for getting the current access token. The easiest way forward is to wait for #3023 to land, after which we can use ctx.options.accessToken to exclude the current access token from the delete query.

Let's create a new issue so that this is not forgotten: #3034

doublemarked · 2016-12-20T13:28:58Z

Thank you for the reply, @bajtos. I do see/understand how #3023 is a barrier for the correct behavior. I need to catch up on the communication there, but I'm happy to see you prioritizing it. We will probably hang on at v2.34.1 for now and monitor how #3023 and #3034 progress.

Regarding communication channels - sure, great idea. I've opened #3042 on this topic.

Thank you!

loay added the #review label Aug 30, 2016

bajtos reviewed Sep 1, 2016
View reviewed changes

loay force-pushed the sessEmail branch from ebe5a9d to a3bed55 Compare September 3, 2016 06:59

bajtos reviewed Sep 5, 2016
View reviewed changes

bajtos reviewed Sep 14, 2016
View reviewed changes

bajtos reviewed Sep 16, 2016

View reviewed changes

bajtos suggested changes Sep 16, 2016

View reviewed changes

bajtos suggested changes Sep 19, 2016

View reviewed changes

bajtos approved these changes Sep 19, 2016

View reviewed changes

Invalidate sessions after email change

bcc2d99

loay force-pushed the sessEmail branch from e2e91dc to bcc2d99 Compare September 19, 2016 14:26

loay merged commit 8061d12 into master Sep 19, 2016

loay deleted the sessEmail branch September 19, 2016 17:54

loay removed the #review label Sep 19, 2016

loay mentioned this pull request Sep 19, 2016

Backport/invalidate email sessions #2759

Merged

bajtos mentioned this pull request Sep 23, 2016

Require verification after email change #2765

Merged

loay mentioned this pull request Sep 27, 2016

Add partial update test case #2795

Merged

bajtos mentioned this pull request Dec 16, 2016

Email/password change should not invalidate current session #3034

Closed

doublemarked mentioned this pull request Dec 20, 2016

Discussion: Developer/Strongloop communication #3042

Closed

3 tasks

Invalidate sessions after email change #2693

Invalidate sessions after email change #2693

Conversation

loay commented Aug 30, 2016 • edited Loading

Choose a reason for hiding this comment

bajtos commented Sep 1, 2016

bajtos commented Sep 1, 2016

loay commented Sep 5, 2016

Choose a reason for hiding this comment

bajtos commented Sep 5, 2016

Choose a reason for hiding this comment

loay commented Sep 14, 2016

loay commented Sep 15, 2016

loay commented Sep 15, 2016

bajtos commented Sep 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

doublemarked commented Dec 15, 2016

doublemarked commented Dec 15, 2016 • edited Loading

bajtos commented Dec 16, 2016

doublemarked commented Dec 20, 2016

loay commented Aug 30, 2016 •

edited

Loading

bajtos Sep 16, 2016 •

edited

Loading

bajtos Sep 16, 2016 •

edited

Loading

bajtos Sep 19, 2016 •

edited

Loading

doublemarked commented Dec 15, 2016 •

edited

Loading