Skip to content

Commit

Permalink
Merge pull request strongloop#3074 from ariskemper/fix_user_id
Browse files Browse the repository at this point in the history 
Fix User model to handle custom PK name
  • Loading branch information
@bajtos
bajtos authored Jan 23, 2017
2 parents 9a1f86b + efd8237 commit 4c9c623
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 39 deletions.
21 changes: 16 additions & 5 deletions common/models/user.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,7 @@ module.exports = function(User) {
var user = this;
var userModel = this.constructor;
var registry = userModel.registry;
var pkName = userModel.definition.idName() || 'id';
assert(typeof options === 'object', 'options required when calling user.verify()');
assert(options.type, 'You must supply a verification type (options.type)');
assert(options.type === 'email', 'Unsupported verification type');
Expand Down Expand Up @@ -425,7 +426,7 @@ module.exports = function(User) {
displayPort +
urlPath +
'?uid=' +
options.user.id +
options.user[pkName] +
'&redirect=' +
options.redirect;

Expand Down Expand Up @@ -485,7 +486,7 @@ module.exports = function(User) {
if (err) {
fn(err);
} else {
fn(null, {email: email, token: user.verificationToken, uid: user.id});
fn(null, {email: email, token: user.verificationToken, uid: user[pkName]});
}
});
}
Expand Down Expand Up @@ -855,7 +856,13 @@ module.exports = function(User) {
User.observe('before save', function beforeEmailUpdate(ctx, next) {
if (ctx.isNewInstance) return next();
if (!ctx.where && !ctx.instance) return next();
var where = ctx.where || {id: ctx.instance.id};
var pkName = ctx.Model.definition.idName() || 'id';
if (ctx.where) {
var where = ctx.where;
} else {
var where = {};
where[pkName] = ctx.instance[pkName];
}

var isPartialUpdateChangingPassword = ctx.data && 'password' in ctx.data;

Expand All @@ -871,7 +878,10 @@ module.exports = function(User) {
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};
var user = {};
user[pkName] = u[pkName];
user['email'] = u['email'];
return user;
});
if (ctx.instance) {
var emailChanged = ctx.instance.email !== ctx.hookState.originalUserData[0].email;
Expand All @@ -895,6 +905,7 @@ module.exports = function(User) {
if (!ctx.instance && !ctx.data) return next();
if (!ctx.hookState.originalUserData) return next();

var pkName = ctx.Model.definition.idName() || 'id';
var newEmail = (ctx.instance || ctx.data).email;
var isPasswordChange = ctx.hookState.isPasswordChange;

Expand All @@ -903,7 +914,7 @@ module.exports = function(User) {
var userIdsToExpire = ctx.hookState.originalUserData.filter(function(u) {
return (newEmail && u.email !== newEmail) || isPasswordChange;
}).map(function(u) {
return u.id;
return u[pkName];
});
ctx.Model._invalidateAccessTokensOfUsers(userIdsToExpire, ctx.options, next);
});
Expand Down
74 changes: 40 additions & 34 deletions test/user.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,17 @@ describe('User', function() {
app.model(Email, {dataSource: 'email'});

// attach User and related models
// forceId is set to false for the purpose of updating the same affected user within the
// `Email Update` test cases.
User = app.registry.createModel('TestUser', {}, {
User = app.registry.createModel({
name: 'TestUser',
base: 'User',
properties: {
// Use a custom id property to verify that User methods
// are correctly looking up the primary key
pk: {type: 'String', defaultFn: 'guid', id: true},
},
http: {path: 'test-users'},
// forceId is set to false for the purpose of updating the same affected user within the
// `Email Update` test cases.
forceId: false,
// Speed up the password hashing algorithm for tests
saltWorkFactor: 4,
Expand Down Expand Up @@ -92,7 +98,7 @@ describe('User', function() {
it('Create a new user', function(done) {
User.create({email: '[email protected]', password: 'bar'}, function(err, user) {
assert(!err);
assert(user.id);
assert(user.pk);
assert(user.email);

done();
Expand All @@ -104,7 +110,7 @@ describe('User', function() {
User.create({email: '[email protected]', password: 'bar'}, function(err, user) {
if (err) return done(err);

assert(user.id);
assert(user.pk);
assert.equal(user.email, user.email.toLowerCase());

done();
Expand All @@ -115,7 +121,7 @@ describe('User', function() {
User.create({email: '[email protected]', password: 'bar'}, function(err, user) {
if (err) return done(err);

assert(user.id);
assert(user.pk);
assert(user.email);
assert.notEqual(user.email, user.email.toLowerCase());

Expand Down Expand Up @@ -238,7 +244,7 @@ describe('User', function() {
async.series([
function(next) {
User.create({email: '[email protected]', password: 'bar'}, function(err, user) {
usersId = user.id;
usersId = user.pk;
next(err);
});
},
Expand Down Expand Up @@ -281,7 +287,7 @@ describe('User', function() {
{name: 'myname', email: '[email protected]', password: 'bar'},
], function(err, users) {
userIds = users.map(function(u) {
return u.id;
return u.pk;
});
next(err);
});
Expand Down Expand Up @@ -409,7 +415,7 @@ describe('User', function() {
it('accepts passwords that are exactly 72 characters long', function(done) {
User.create({email: '[email protected]', password: pass72Char}, function(err, user) {
if (err) return done(err);
User.findById(user.id, function(err, userFound) {
User.findById(user.pk, function(err, userFound) {
if (err) return done(err);
assert(userFound);
done();
Expand Down Expand Up @@ -1074,7 +1080,7 @@ describe('User', function() {
it('logs in a user by with realm', function(done) {
User.login(credentialWithRealm, function(err, accessToken) {
assertGoodToken(accessToken);
assert.equal(accessToken.userId, user1.id);
assert.equal(accessToken.userId, user1.pk);

done();
});
Expand All @@ -1083,7 +1089,7 @@ describe('User', function() {
it('logs in a user by with realm in username', function(done) {
User.login(credentialRealmInUsername, function(err, accessToken) {
assertGoodToken(accessToken);
assert.equal(accessToken.userId, user1.id);
assert.equal(accessToken.userId, user1.pk);

done();
});
Expand All @@ -1092,7 +1098,7 @@ describe('User', function() {
it('logs in a user by with realm in email', function(done) {
User.login(credentialRealmInEmail, function(err, accessToken) {
assertGoodToken(accessToken);
assert.equal(accessToken.userId, user1.id);
assert.equal(accessToken.userId, user1.pk);

done();
});
Expand All @@ -1110,7 +1116,7 @@ describe('User', function() {
it('logs in a user by with realm', function(done) {
User.login(credentialWithRealm, function(err, accessToken) {
assertGoodToken(accessToken);
assert.equal(accessToken.userId, user1.id);
assert.equal(accessToken.userId, user1.pk);

done();
});
Expand Down Expand Up @@ -1229,7 +1235,7 @@ describe('User', function() {
var u = new User({username: 'a', password: 'b', email: '[email protected]'});

u.save(function(err, user) {
User.findById(user.id, function(err, uu) {
User.findById(user.pk, function(err, uu) {
uu.hasPassword('b', function(err, isMatch) {
assert(isMatch);

Expand All @@ -1241,15 +1247,15 @@ describe('User', function() {

it('should match a password after it is changed', function(done) {
User.create({email: '[email protected]', username: 'bat', password: 'baz'}, function(err, user) {
User.findById(user.id, function(err, foundUser) {
User.findById(user.pk, function(err, foundUser) {
assert(foundUser);
foundUser.hasPassword('baz', function(err, isMatch) {
assert(isMatch);
foundUser.password = 'baz2';
foundUser.save(function(err, updatedUser) {
updatedUser.hasPassword('baz2', function(err, isMatch) {
assert(isMatch);
User.findById(user.id, function(err, uu) {
User.findById(user.pk, function(err, uu) {
uu.hasPassword('baz2', function(err, isMatch) {
assert(isMatch);

Expand Down Expand Up @@ -2048,7 +2054,7 @@ describe('User', function() {

it('invalidates sessions when email is changed using `updateOrCreate`', function(done) {
User.updateOrCreate({
id: user.id,
pk: user.pk,
email: updatedEmailCredentials.email,
}, function(err, userInstance) {
if (err) return done(err);
Expand All @@ -2059,7 +2065,7 @@ describe('User', function() {
it('invalidates sessions after `replaceById`', function(done) {
// The way how the invalidation is implemented now, all sessions
// are invalidated on a full replace
User.replaceById(user.id, currentEmailCredentials, function(err, userInstance) {
User.replaceById(user.pk, currentEmailCredentials, function(err, userInstance) {
if (err) return done(err);
assertNoAccessTokens(done);
});
Expand All @@ -2069,7 +2075,7 @@ describe('User', function() {
// The way how the invalidation is implemented now, all sessions
// are invalidated on a full replace
User.replaceOrCreate({
id: user.id,
pk: user.pk,
email: currentEmailCredentials.email,
password: currentEmailCredentials.password,
}, function(err, userInstance) {
Expand All @@ -2087,7 +2093,7 @@ describe('User', function() {

it('keeps sessions AS IS if firstName is added using `updateOrCreate`', function(done) {
User.updateOrCreate({
id: user.id,
pk: user.pk,
firstName: 'Loay',
email: currentEmailCredentials.email,
}, function(err, userInstance) {
Expand Down Expand Up @@ -2154,15 +2160,15 @@ describe('User', function() {
},
function updatePartialUser(next) {
User.updateAll(
{id: userPartial.id},
{pk: userPartial.pk},
{age: userPartial.age + 1},
function(err, info) {
if (err) return next(err);
next();
});
},
function verifyTokensOfPartialUser(next) {
AccessToken.find({where: {userId: userPartial.id}}, function(err, tokens1) {
AccessToken.find({where: {userId: userPartial.pk}}, function(err, tokens1) {
if (err) return next(err);
expect(tokens1.length).to.equal(1);
next();
Expand Down Expand Up @@ -2214,11 +2220,11 @@ describe('User', function() {
});
},
function(next) {
AccessToken.find({where: {userId: user1.id}}, function(err, tokens1) {
AccessToken.find({where: {userId: user1.pk}}, function(err, tokens1) {
if (err) return next(err);
AccessToken.find({where: {userId: user2.id}}, function(err, tokens2) {
AccessToken.find({where: {userId: user2.pk}}, function(err, tokens2) {
if (err) return next(err);
AccessToken.find({where: {userId: user3.id}}, function(err, tokens3) {
AccessToken.find({where: {userId: user3.pk}}, function(err, tokens3) {
if (err) return next(err);

expect(tokens1.length).to.equal(1);
Expand Down Expand Up @@ -2259,7 +2265,7 @@ describe('User', function() {
});
},
function verifyTokensOfSpecialUser(next) {
AccessToken.find({where: {userId: userSpecial.id}}, function(err, tokens1) {
AccessToken.find({where: {userId: userSpecial.pk}}, function(err, tokens1) {
if (err) return done(err);
expect(tokens1.length, 'tokens - special user tokens').to.equal(0);
next();
Expand All @@ -2280,7 +2286,7 @@ describe('User', function() {
var options = {accessToken: originalUserToken1};
user.updateAttribute('email', '[email protected]', options, function(err) {
if (err) return done(err);
AccessToken.find({where: {userId: user.id}}, function(err, tokens) {
AccessToken.find({where: {userId: user.pk}}, function(err, tokens) {
if (err) return done(err);
var tokenIds = tokens.map(function(t) { return t.id; });
expect(tokenIds).to.eql([originalUserToken1.id]);
Expand Down Expand Up @@ -2321,9 +2327,9 @@ describe('User', function() {
});
},
function(next) {
AccessToken.find({where: {userId: user1.id}}, function(err, tokens1) {
AccessToken.find({where: {userId: user1.pk}}, function(err, tokens1) {
if (err) return next(err);
AccessToken.find({where: {userId: user2.id}}, function(err, tokens2) {
AccessToken.find({where: {userId: user2.pk}}, function(err, tokens2) {
if (err) return next(err);
expect(tokens1.length).to.equal(1);
expect(tokens2.length).to.equal(0);
Expand All @@ -2338,7 +2344,7 @@ describe('User', function() {
});

function assertPreservedTokens(done) {
AccessToken.find({where: {userId: user.id}}, function(err, tokens) {
AccessToken.find({where: {userId: user.pk}}, function(err, tokens) {
if (err) return done(err);
var actualIds = tokens.map(function(t) { return t.id; });
actualIds.sort();
Expand All @@ -2350,7 +2356,7 @@ describe('User', function() {
};

function assertNoAccessTokens(done) {
AccessToken.find({where: {userId: user.id}}, function(err, tokens) {
AccessToken.find({where: {userId: user.pk}}, function(err, tokens) {
if (err) return done(err);
expect(tokens.length).to.equal(0);
done();
Expand All @@ -2376,7 +2382,7 @@ describe('User', function() {
});
},
function findUser(next) {
User.findById(userInstance.id, function(err, info) {
User.findById(userInstance.pk, function(err, info) {
if (err) return next(err);
assert.equal(info.email, NEW_EMAIL);
assert.equal(info.emailVerified, false);
Expand All @@ -2398,7 +2404,7 @@ describe('User', function() {
});
},
function findUser(next) {
User.findById(userInstance.id, function(err, info) {
User.findById(userInstance.pk, function(err, info) {
if (err) return next(err);
assert.equal(info.email, NEW_EMAIL);
assert.equal(info.emailVerified, true);
Expand All @@ -2420,7 +2426,7 @@ describe('User', function() {
});
},
function findUser(next) {
User.findById(userInstance.id, function(err, info) {
User.findById(userInstance.pk, function(err, info) {
if (err) return next(err);
assert.equal(info.realm, 'test');
assert.equal(info.emailVerified, true);
Expand Down

0 comments on commit 4c9c623

Please sign in to comment.