Skip to content

Commit 5e7e7ca

Browse files
ariskemperbajtos
ariskemper
authored and
bajtos
committed
Fix User methods to use correct Primary Key
Do not use hard-coded "id" property name, call `idName()` to get the name of the PK property.
1 parent 6fcb7db commit 5e7e7ca

File tree

2 files changed

+59
-40
lines changed

2 files changed

+59
-40
lines changed

‎common/models/user.js‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -384,6 +384,7 @@ module.exports = function(User) {
384384
var user = this;
385385
var userModel = this.constructor;
386386
var registry = userModel.registry;
387+
var pkName = userModel.definition.idName() || 'id';
387388
assert(typeof options === 'object', 'options required when calling user.verify()');
388389
assert(options.type, 'You must supply a verification type (options.type)');
389390
assert(options.type === 'email', 'Unsupported verification type');
@@ -418,7 +419,7 @@ module.exports = function(User) {
418419
displayPort +
419420
urlPath +
420421
'?uid=' +
421-
options.user.id +
422+
options.user[pkName] +
422423
'&redirect=' +
423424
options.redirect;
424425

@@ -477,7 +478,7 @@ module.exports = function(User) {
477478
if (err) {
478479
fn(err);
479480
} else {
480-
fn(null, { email: email, token: user.verificationToken, uid: user.id });
481+
fn(null, {email: email, token: user.verificationToken, uid: user[pkName]});
481482
}
482483
});
483484
}
@@ -862,6 +863,7 @@ module.exports = function(User) {
862863
var emailChanged;
863864
if (ctx.isNewInstance) return next();
864865
if (!ctx.where && !ctx.instance) return next();
866+
var pkName = ctx.Model.definition.idName() || 'id';
865867

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

@@ -874,11 +876,21 @@ module.exports = function(User) {
874876
ctx.hookState.isPasswordChange = isPartialUpdateChangingPassword ||
875877
isFullReplaceChangingPassword;
876878

877-
var where = ctx.where || {id: ctx.instance.id};
879+
var where;
880+
if (ctx.where) {
881+
where = ctx.where;
882+
} else {
883+
where = {};
884+
where[pkName] = ctx.instance[pkName];
885+
}
886+
878887
ctx.Model.find({where: where}, function(err, userInstances) {
879888
if (err) return next(err);
880889
ctx.hookState.originalUserData = userInstances.map(function(u) {
881-
return { id: u.id, email: u.email };
890+
var user = {};
891+
user[pkName] = u[pkName];
892+
user['email'] = u['email'];
893+
return user;
882894
});
883895
if (ctx.instance) {
884896
emailChanged = ctx.instance.email !== ctx.hookState.originalUserData[0].email;
@@ -904,6 +916,7 @@ module.exports = function(User) {
904916
if (!ctx.instance && !ctx.data) return next();
905917
if (!ctx.hookState.originalUserData) return next();
906918

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

@@ -912,7 +925,7 @@ module.exports = function(User) {
912925
var userIdsToExpire = ctx.hookState.originalUserData.filter(function(u) {
913926
return (newEmail && u.email !== newEmail) || isPasswordChange;
914927
}).map(function(u) {
915-
return u.id;
928+
return u[pkName];
916929
});
917930
ctx.Model._invalidateAccessTokensOfUsers(userIdsToExpire, ctx.options, next);
918931
});

‎test/user.test.js‎

Lines changed: 41 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -43,11 +43,17 @@ describe('User', function() {
4343
app.model(Email, { dataSource: 'email' });
4444

4545
// attach User and related models
46-
// forceId is set to false for the purpose of updating the same affected user within the
47-
// `Email Update` test cases.
48-
User = app.registry.createModel('TestUser', {}, {
46+
User = app.registry.createModel({
47+
name: 'TestUser',
4948
base: 'User',
50-
http: { path: 'test-users' },
49+
properties: {
50+
// Use a custom id property to verify that User methods
51+
// are correctly looking up the primary key
52+
pk: {type: 'String', defaultFn: 'guid', id: true},
53+
},
54+
http: {path: 'test-users'},
55+
// forceId is set to false for the purpose of updating the same affected user within the
56+
// `Email Update` test cases.
5157
forceId: false,
5258
});
5359
app.model(User, { dataSource: 'db' });
@@ -83,7 +89,7 @@ describe('User', function() {
8389
it('Create a new user', function(done) {
8490
User.create({email: '[email protected]', password: 'bar'}, function(err, user) {
8591
assert(!err);
86-
assert(user.id);
92+
assert(user.pk);
8793
assert(user.email);
8894

8995
done();
@@ -95,7 +101,7 @@ describe('User', function() {
95101
User.create({email: '[email protected]', password: 'bar'}, function(err, user) {
96102
if (err) return done(err);
97103

98-
assert(user.id);
104+
assert(user.pk);
99105
assert.equal(user.email, user.email.toLowerCase());
100106

101107
done();
@@ -106,7 +112,7 @@ describe('User', function() {
106112
User.create({email: '[email protected]', password: 'bar'}, function(err, user) {
107113
if (err) return done(err);
108114

109-
assert(user.id);
115+
assert(user.pk);
110116
assert(user.email);
111117
assert.notEqual(user.email, user.email.toLowerCase());
112118

@@ -246,7 +252,7 @@ describe('User', function() {
246252
async.series([
247253
function(next) {
248254
User.create({ email: '[email protected]', password: 'bar' }, function(err, user) {
249-
usersId = user.id;
255+
usersId = user.pk;
250256
next(err);
251257
});
252258
},
@@ -289,7 +295,7 @@ describe('User', function() {
289295
{ name: 'myname', email: '[email protected]', password: 'bar' },
290296
], function(err, users) {
291297
userIds = users.map(function(u) {
292-
return u.id;
298+
return u.pk;
293299
});
294300
next(err);
295301
});
@@ -418,7 +424,7 @@ describe('User', function() {
418424
it('accepts passwords that are exactly 72 characters long', function(done) {
419425
User.create({ email: '[email protected]', password: pass72Char }, function(err, user) {
420426
if (err) return done(err);
421-
User.findById(user.id, function(err, userFound) {
427+
User.findById(user.pk, function(err, userFound) {
422428
if (err) return done(err);
423429
assert(userFound);
424430
done();
@@ -1068,7 +1074,7 @@ describe('User', function() {
10681074
it('logs in a user by with realm', function(done) {
10691075
User.login(credentialWithRealm, function(err, accessToken) {
10701076
assertGoodToken(accessToken);
1071-
assert.equal(accessToken.userId, user1.id);
1077+
assert.equal(accessToken.userId, user1.pk);
10721078

10731079
done();
10741080
});
@@ -1077,7 +1083,7 @@ describe('User', function() {
10771083
it('logs in a user by with realm in username', function(done) {
10781084
User.login(credentialRealmInUsername, function(err, accessToken) {
10791085
assertGoodToken(accessToken);
1080-
assert.equal(accessToken.userId, user1.id);
1086+
assert.equal(accessToken.userId, user1.pk);
10811087

10821088
done();
10831089
});
@@ -1086,7 +1092,7 @@ describe('User', function() {
10861092
it('logs in a user by with realm in email', function(done) {
10871093
User.login(credentialRealmInEmail, function(err, accessToken) {
10881094
assertGoodToken(accessToken);
1089-
assert.equal(accessToken.userId, user1.id);
1095+
assert.equal(accessToken.userId, user1.pk);
10901096

10911097
done();
10921098
});
@@ -1104,7 +1110,7 @@ describe('User', function() {
11041110
it('logs in a user by with realm', function(done) {
11051111
User.login(credentialWithRealm, function(err, accessToken) {
11061112
assertGoodToken(accessToken);
1107-
assert.equal(accessToken.userId, user1.id);
1113+
assert.equal(accessToken.userId, user1.pk);
11081114

11091115
done();
11101116
});
@@ -1222,7 +1228,7 @@ describe('User', function() {
12221228
var u = new User({username: 'a', password: 'b', email: '[email protected]'});
12231229

12241230
u.save(function(err, user) {
1225-
User.findById(user.id, function(err, uu) {
1231+
User.findById(user.pk, function(err, uu) {
12261232
uu.hasPassword('b', function(err, isMatch) {
12271233
assert(isMatch);
12281234

@@ -1234,15 +1240,15 @@ describe('User', function() {
12341240

12351241
it('should match a password after it is changed', function(done) {
12361242
User.create({email: '[email protected]', username: 'bat', password: 'baz'}, function(err, user) {
1237-
User.findById(user.id, function(err, foundUser) {
1243+
User.findById(user.pk, function(err, foundUser) {
12381244
assert(foundUser);
12391245
foundUser.hasPassword('baz', function(err, isMatch) {
12401246
assert(isMatch);
12411247
foundUser.password = 'baz2';
12421248
foundUser.save(function(err, updatedUser) {
12431249
updatedUser.hasPassword('baz2', function(err, isMatch) {
12441250
assert(isMatch);
1245-
User.findById(user.id, function(err, uu) {
1251+
User.findById(user.pk, function(err, uu) {
12461252
uu.hasPassword('baz2', function(err, isMatch) {
12471253
assert(isMatch);
12481254

@@ -2038,7 +2044,7 @@ describe('User', function() {
20382044

20392045
it('invalidates sessions when email is changed using `updateOrCreate`', function(done) {
20402046
User.updateOrCreate({
2041-
id: user.id,
2047+
pk: user.pk,
20422048
email: updatedEmailCredentials.email,
20432049
}, function(err, userInstance) {
20442050
if (err) return done(err);
@@ -2049,7 +2055,7 @@ describe('User', function() {
20492055
it('invalidates sessions after `replaceById`', function(done) {
20502056
// The way how the invalidation is implemented now, all sessions
20512057
// are invalidated on a full replace
2052-
User.replaceById(user.id, currentEmailCredentials, function(err, userInstance) {
2058+
User.replaceById(user.pk, currentEmailCredentials, function(err, userInstance) {
20532059
if (err) return done(err);
20542060
assertNoAccessTokens(done);
20552061
});
@@ -2059,7 +2065,7 @@ describe('User', function() {
20592065
// The way how the invalidation is implemented now, all sessions
20602066
// are invalidated on a full replace
20612067
User.replaceOrCreate({
2062-
id: user.id,
2068+
pk: user.pk,
20632069
email: currentEmailCredentials.email,
20642070
password: currentEmailCredentials.password,
20652071
}, function(err, userInstance) {
@@ -2077,7 +2083,7 @@ describe('User', function() {
20772083

20782084
it('keeps sessions AS IS if firstName is added using `updateOrCreate`', function(done) {
20792085
User.updateOrCreate({
2080-
id: user.id,
2086+
pk: user.pk,
20812087
firstName: 'Loay',
20822088
email: currentEmailCredentials.email,
20832089
}, function(err, userInstance) {
@@ -2144,15 +2150,15 @@ describe('User', function() {
21442150
},
21452151
function updatePartialUser(next) {
21462152
User.updateAll(
2147-
{id: userPartial.id},
2153+
{pk: userPartial.pk},
21482154
{age: userPartial.age + 1},
21492155
function(err, info) {
21502156
if (err) return next(err);
21512157
next();
21522158
});
21532159
},
21542160
function verifyTokensOfPartialUser(next) {
2155-
AccessToken.find({where: {userId: userPartial.id}}, function(err, tokens1) {
2161+
AccessToken.find({where: {userId: userPartial.pk}}, function(err, tokens1) {
21562162
if (err) return next(err);
21572163
expect(tokens1.length).to.equal(1);
21582164
next();
@@ -2204,11 +2210,11 @@ describe('User', function() {
22042210
});
22052211
},
22062212
function(next) {
2207-
AccessToken.find({where: {userId: user1.id}}, function(err, tokens1) {
2213+
AccessToken.find({where: {userId: user1.pk}}, function(err, tokens1) {
22082214
if (err) return next(err);
2209-
AccessToken.find({where: {userId: user2.id}}, function(err, tokens2) {
2215+
AccessToken.find({where: {userId: user2.pk}}, function(err, tokens2) {
22102216
if (err) return next(err);
2211-
AccessToken.find({where: {userId: user3.id}}, function(err, tokens3) {
2217+
AccessToken.find({where: {userId: user3.pk}}, function(err, tokens3) {
22122218
if (err) return next(err);
22132219

22142220
expect(tokens1.length).to.equal(1);
@@ -2249,7 +2255,7 @@ describe('User', function() {
22492255
});
22502256
},
22512257
function verifyTokensOfSpecialUser(next) {
2252-
AccessToken.find({where: {userId: userSpecial.id}}, function(err, tokens1) {
2258+
AccessToken.find({where: {userId: userSpecial.pk}}, function(err, tokens1) {
22532259
if (err) return done(err);
22542260
expect(tokens1.length, 'tokens - special user tokens').to.equal(0);
22552261
next();
@@ -2270,7 +2276,7 @@ describe('User', function() {
22702276
var options = {accessToken: originalUserToken1};
22712277
user.updateAttribute('email', '[email protected]', options, function(err) {
22722278
if (err) return done(err);
2273-
AccessToken.find({where: {userId: user.id}}, function(err, tokens) {
2279+
AccessToken.find({where: {userId: user.pk}}, function(err, tokens) {
22742280
if (err) return done(err);
22752281
var tokenIds = tokens.map(function(t) { return t.id; });
22762282
expect(tokenIds).to.eql([originalUserToken1.id]);
@@ -2311,9 +2317,9 @@ describe('User', function() {
23112317
});
23122318
},
23132319
function(next) {
2314-
AccessToken.find({where: {userId: user1.id}}, function(err, tokens1) {
2320+
AccessToken.find({where: {userId: user1.pk}}, function(err, tokens1) {
23152321
if (err) return next(err);
2316-
AccessToken.find({where: {userId: user2.id}}, function(err, tokens2) {
2322+
AccessToken.find({where: {userId: user2.pk}}, function(err, tokens2) {
23172323
if (err) return next(err);
23182324
expect(tokens1.length).to.equal(1);
23192325
expect(tokens2.length).to.equal(0);
@@ -2339,7 +2345,7 @@ describe('User', function() {
23392345
});
23402346

23412347
function assertPreservedTokens(done) {
2342-
AccessToken.find({where: {userId: user.id}}, function(err, tokens) {
2348+
AccessToken.find({where: {userId: user.pk}}, function(err, tokens) {
23432349
if (err) return done(err);
23442350
var actualIds = tokens.map(function(t) { return t.id; });
23452351
actualIds.sort();
@@ -2351,7 +2357,7 @@ describe('User', function() {
23512357
}
23522358

23532359
function assertNoAccessTokens(done) {
2354-
AccessToken.find({where: {userId: user.id}}, function(err, tokens) {
2360+
AccessToken.find({where: {userId: user.pk}}, function(err, tokens) {
23552361
if (err) return done(err);
23562362
expect(tokens.length).to.equal(0);
23572363
done();
@@ -2377,7 +2383,7 @@ describe('User', function() {
23772383
});
23782384
},
23792385
function findUser(next) {
2380-
User.findById(userInstance.id, function(err, info) {
2386+
User.findById(userInstance.pk, function(err, info) {
23812387
if (err) return next(err);
23822388
assert.equal(info.email, NEW_EMAIL);
23832389
assert.equal(info.emailVerified, false);
@@ -2399,7 +2405,7 @@ describe('User', function() {
23992405
});
24002406
},
24012407
function findUser(next) {
2402-
User.findById(userInstance.id, function(err, info) {
2408+
User.findById(userInstance.pk, function(err, info) {
24032409
if (err) return next(err);
24042410
assert.equal(info.email, NEW_EMAIL);
24052411
assert.equal(info.emailVerified, true);
@@ -2421,7 +2427,7 @@ describe('User', function() {
24212427
});
24222428
},
24232429
function findUser(next) {
2424-
User.findById(userInstance.id, function(err, info) {
2430+
User.findById(userInstance.pk, function(err, info) {
24252431
if (err) return next(err);
24262432
assert.equal(info.realm, 'test');
24272433
assert.equal(info.emailVerified, true);

0 commit comments

Comments
 (0)