Skip to content

Commit f027eab

Browse files
jujagajujaga
authored
Merge pull request #195 from bcgov/bug/duplicate-users
Bug/duplicate users
2 parents f20ddb7 + 03030b1 commit f027eab

File tree

3 files changed

+122
-57
lines changed

3 files changed

+122
-57
lines changed

‎app/src/db/migrations/20230810000000_009-user-identity-constraint.js

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
exports.up = function (knex) {
2+
return Promise.resolve()
3+
// remove duplicate users - deletes all but oldest of users with matching IdentityId
4+
// duplicates users may have existed in rare case where authentication of a new user was triggered concurrently
5+
.then(() => knex.raw(`DELETE FROM "public"."user" AS u
6+
WHERE u."userId" IN (
7+
SELECT DISTINCT ON ("public"."user"."identityId") "public"."user"."userId"
8+
FROM "public"."user"
9+
WHERE (
10+
SELECT count(*)
11+
FROM "public"."user" AS user2
12+
WHERE user2."identityId" = "public"."user"."identityId") > 1
13+
ORDER BY "public"."user"."identityId", "public"."user"."createdAt" DESC
14+
)`))
15+
16+
// prevent further duplicate users - add unique index over columns identityId and idp in user table
17+
.then(() => knex.schema.alterTable('user', table => {
18+
table.unique(['identityId', 'idp']);
19+
}));
20+
};
21+
22+
exports.down = function (knex) {
23+
return Promise.resolve()
24+
// remove unique index over columns identityId and idp in user table
25+
.then(() => knex.schema.alterTable('user', table => {
26+
table.dropUnique(['identityId', 'idp']);
27+
}));
28+
};

‎app/src/services/user.js

Lines changed: 56 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ const { v4: uuidv4, NIL: SYSTEM_USER } = require('uuid');
33
const { parseIdentityKeyClaims } = require('../components/utils');
44

55
const { IdentityProvider, User } = require('../db/models');
6+
const utils = require('../db/models/utils');
67

78
/**
89
* The User DB Service
@@ -69,26 +70,36 @@ const service = {
6970
createUser: async (data, etrx = undefined) => {
7071
let trx;
7172
try {
73+
let response;
7274
trx = etrx ? etrx : await User.startTransaction();
7375

74-
if (data.idp) {
75-
const identityProvider = await service.readIdp(data.idp);
76-
if (!identityProvider) await service.createIdp(data.idp, trx);
77-
}
76+
const exists = await User.query(trx)
77+
.where({ 'identityId': data.identityId, idp: data.idp })
78+
.first();
7879

79-
const obj = {
80-
userId: uuidv4(),
81-
identityId: data.identityId,
82-
username: data.username,
83-
fullName: data.fullName,
84-
email: data.email,
85-
firstName: data.firstName,
86-
lastName: data.lastName,
87-
idp: data.idp,
88-
createdBy: data.userId
89-
};
80+
if (exists) {
81+
response = exists;
82+
} else { // else add new user
83+
if (data.idp) { // add idp if not in db
84+
const identityProvider = await service.readIdp(data.idp, trx);
85+
if (!identityProvider) await service.createIdp(data.idp, trx);
86+
}
87+
88+
response = await User.query(trx)
89+
.insert({
90+
userId: uuidv4(),
91+
identityId: data.identityId,
92+
username: data.username,
93+
fullName: data.fullName,
94+
email: data.email,
95+
firstName: data.firstName,
96+
lastName: data.lastName,
97+
idp: data.idp,
98+
createdBy: data.userId
99+
})
100+
.returning('*');
101+
}
90102

91-
const response = await User.query(trx).insertAndFetch(obj);
92103
if (!etrx) await trx.commit();
93104
return response;
94105
} catch (err) {
@@ -135,27 +146,43 @@ const service = {
135146
*/
136147
login: async (token) => {
137148
const newUser = service._tokenToUser(token);
138-
const oldUser = await User.query()
139-
.where('identityId', newUser.identityId)
140-
.first();
149+
// wrap with db transaction
150+
return await utils.trxWrapper(async (trx) => {
151+
// check if user exists in db
152+
const oldUser = await User.query(trx)
153+
.where({ 'identityId': newUser.identityId, idp: newUser.idp })
154+
.first();
141155

142-
if (!oldUser) {
143-
// Add user to system
144-
return service.createUser(newUser);
145-
} else {
146-
// Update user data if necessary
147-
return service.updateUser(oldUser.userId, newUser);
148-
}
156+
if (!oldUser) {
157+
// Add user to system
158+
return await service.createUser(newUser, trx);
159+
} else {
160+
// Update user data if necessary
161+
return await service.updateUser(oldUser.userId, newUser, trx);
162+
}
163+
});
149164
},
150165

151166
/**
152167
* @function readIdp
153168
* Gets an identity provider record
154169
* @param {string} code The identity provider code
155170
* @returns {Promise<object>} The result of running the find operation
171+
* @throws The error encountered upon db transaction failure
156172
*/
157-
readIdp: (code) => {
158-
return IdentityProvider.query().findById(code);
173+
readIdp: async (code, etrx = undefined) => {
174+
let trx;
175+
try {
176+
trx = etrx ? etrx : await IdentityProvider.startTransaction();
177+
178+
const response = await IdentityProvider.query(trx).findById(code);
179+
180+
if (!etrx) await trx.commit();
181+
return response;
182+
} catch (err) {
183+
if (!etrx && trx) await trx.rollback();
184+
throw err;
185+
}
159186
},
160187

161188
/**
@@ -222,7 +249,7 @@ const service = {
222249
trx = etrx ? etrx : await User.startTransaction();
223250

224251
if (data.idp) {
225-
const identityProvider = await service.readIdp(data.idp);
252+
const identityProvider = await service.readIdp(data.idp, trx);
226253
if (!identityProvider) await service.createIdp(data.idp, trx);
227254
}
228255

‎app/tests/unit/services/user.spec.js

Lines changed: 38 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
const { NIL: SYSTEM_USER } = require('uuid');
22

33
const { resetModel, trxBuilder } = require('../../common/helper');
4+
const utils = require('../../../src/db/models/utils');
45
const IdentityProvider = require('../../../src/db/models/tables/identityProvider');
56
const User = require('../../../src/db/models/tables/user');
67

@@ -23,10 +24,12 @@ jest.mock('../../../src/db/models/tables/user', () => ({
2324

2425
first: jest.fn(),
2526
findById: jest.fn(),
27+
insert: jest.fn(),
2628
insertAndFetch: jest.fn(),
2729
modify: jest.fn(),
2830
patchAndFetchById: jest.fn(),
2931
query: jest.fn(),
32+
returning: jest.fn(),
3033
select: jest.fn(),
3134
throwIfNotFound: jest.fn(),
3235
where: jest.fn(),
@@ -106,20 +109,21 @@ describe('createUser', () => {
106109
});
107110

108111
it('Creates an idp if no matching idp exists in database', async () => {
112+
User.first.mockResolvedValue(undefined);
109113
readIdpSpy.mockResolvedValue(false);
110114

111115
await service.createUser(user);
112116

113117
expect(readIdpSpy).toHaveBeenCalledTimes(1);
114-
expect(readIdpSpy).toHaveBeenCalledWith('idir');
118+
expect(readIdpSpy).toHaveBeenCalledWith('idir', userTrx);
115119
expect(createIdpSpy).toHaveBeenCalledTimes(1);
116-
expect(createIdpSpy).toHaveBeenCalledWith('idir', expect.anything());
120+
expect(createIdpSpy).toHaveBeenCalledWith('idir', userTrx);
117121

118122
expect(User.startTransaction).toHaveBeenCalledTimes(1);
119-
expect(User.query).toHaveBeenCalledTimes(1);
123+
expect(User.query).toHaveBeenCalledTimes(2);
120124
expect(User.query).toHaveBeenCalledWith(expect.anything());
121-
expect(User.insertAndFetch).toHaveBeenCalledTimes(1);
122-
expect(User.insertAndFetch).toBeCalledWith(
125+
expect(User.insert).toHaveBeenCalledTimes(1);
126+
expect(User.insert).toBeCalledWith(
123127
expect.objectContaining({
124128
...user,
125129
userId: expect.any(String)
@@ -129,18 +133,19 @@ describe('createUser', () => {
129133
});
130134

131135
it('Skips creating an idp if matching idp already exists in database', async () => {
136+
User.first.mockResolvedValue(false);
132137
readIdpSpy.mockReturnValue(true);
138+
133139
await service.createUser(user);
134140

135141
expect(readIdpSpy).toHaveBeenCalledTimes(1);
136-
expect(readIdpSpy).toHaveBeenCalledWith('idir');
137142
expect(createIdpSpy).toHaveBeenCalledTimes(0);
138143

139144
expect(User.startTransaction).toHaveBeenCalledTimes(1);
140-
expect(User.query).toHaveBeenCalledTimes(1);
145+
expect(User.query).toHaveBeenCalledTimes(2);
141146
expect(User.query).toHaveBeenCalledWith(expect.anything());
142-
expect(User.insertAndFetch).toHaveBeenCalledTimes(1);
143-
expect(User.insertAndFetch).toBeCalledWith(
147+
expect(User.insert).toHaveBeenCalledTimes(1);
148+
expect(User.insert).toBeCalledWith(
144149
expect.objectContaining({
145150
...user,
146151
userId: expect.any(String)
@@ -159,13 +164,7 @@ describe('createUser', () => {
159164
expect(User.startTransaction).toHaveBeenCalledTimes(1);
160165
expect(User.query).toHaveBeenCalledTimes(1);
161166
expect(User.query).toHaveBeenCalledWith(expect.anything());
162-
expect(User.insertAndFetch).toHaveBeenCalledTimes(1);
163-
expect(User.insertAndFetch).toBeCalledWith(
164-
expect.objectContaining({
165-
...systemUser,
166-
userId: expect.any(String)
167-
})
168-
);
167+
expect(User.insert).toHaveBeenCalledTimes(0);
169168
expect(userTrx.commit).toHaveBeenCalledTimes(1);
170169
});
171170
});
@@ -202,64 +201,75 @@ describe('listIdps', () => {
202201

203202
describe('login', () => {
204203
const createUserSpy = jest.spyOn(service, 'createUser');
205-
const updateUserSpy = jest.spyOn(service, 'updateUser');
206204
const tokenToUserSpy = jest.spyOn(service, '_tokenToUser');
205+
const trxWrapperSpy = jest.spyOn(utils, 'trxWrapper');
206+
const updateUserSpy = jest.spyOn(service, 'updateUser');
207207

208208
beforeEach(() => {
209-
tokenToUserSpy.mockReset();
210209
createUserSpy.mockReset();
210+
tokenToUserSpy.mockReset();
211+
trxWrapperSpy.mockReset();
211212
updateUserSpy.mockReset();
212213

213214
service._tokenToUser = jest.fn().mockReturnValue(user);
214215
});
215216

216217
afterAll(() => {
217-
tokenToUserSpy.mockRestore();
218218
createUserSpy.mockRestore();
219+
tokenToUserSpy.mockRestore();
220+
trxWrapperSpy.mockReset();
219221
updateUserSpy.mockRestore();
220222
});
221223

222224
it('Adds a new user record', async () => {
223225
User.first.mockResolvedValue(undefined);
226+
createUserSpy.mockResolvedValue(user);
227+
trxWrapperSpy.mockImplementation(callback => callback({}));
228+
224229
await service.login(token);
225230

226231
expect(User.query).toHaveBeenCalledTimes(1);
227-
expect(User.query).toHaveBeenCalledWith();
232+
expect(User.query).toHaveBeenCalledWith(expect.any(Object));
228233
expect(User.where).toHaveBeenCalledTimes(1);
229-
expect(User.where).toHaveBeenCalledWith('identityId', user.identityId);
234+
expect(User.where).toHaveBeenCalledWith({ identityId: user.identityId, idp: user.idp });
230235
expect(User.first).toHaveBeenCalledTimes(1);
231236
expect(User.first).toHaveBeenCalledWith();
232237

233238
expect(createUserSpy).toHaveBeenCalledTimes(1);
234-
expect(createUserSpy).toHaveBeenCalledWith(user);
239+
expect(createUserSpy).toHaveBeenCalledWith(user, expect.any(Object));
235240
expect(updateUserSpy).toHaveBeenCalledTimes(0);
236241
});
237242

238243
it('Updates an existing user record', async () => {
244+
trxWrapperSpy.mockImplementation(callback => callback({}));
239245
User.first.mockResolvedValue({ ...user, userId: 'a96f2809-d6f4-4cef-a02a-3f72edff06d7' });
246+
240247
await service.login(token);
241248

242249
expect(User.query).toHaveBeenCalledTimes(1);
243-
expect(User.query).toHaveBeenCalledWith();
250+
expect(User.query).toHaveBeenCalledWith(expect.any(Object));
244251
expect(User.where).toHaveBeenCalledTimes(1);
245-
expect(User.where).toHaveBeenCalledWith('identityId', user.identityId);
252+
expect(User.where).toHaveBeenCalledWith({ identityId: user.identityId, idp: user.idp });
246253
expect(User.first).toHaveBeenCalledTimes(1);
247254
expect(User.first).toHaveBeenCalledWith();
248255

249256
expect(createUserSpy).toHaveBeenCalledTimes(0);
250257
expect(updateUserSpy).toHaveBeenCalledTimes(1);
251-
expect(updateUserSpy).toHaveBeenCalledWith('a96f2809-d6f4-4cef-a02a-3f72edff06d7', expect.objectContaining(user));
258+
expect(updateUserSpy).toHaveBeenCalledWith('a96f2809-d6f4-4cef-a02a-3f72edff06d7', expect.objectContaining(user), expect.any(Object));
252259
});
253260
});
254261

255262
describe('readIdp', () => {
256-
it('Query identityProvider by code', () => {
257-
service.readIdp('idir');
263+
it('Query identityProvider by code', async () => {
264+
await service.readIdp('idir');
258265

266+
expect(IdentityProvider.startTransaction).toHaveBeenCalledTimes(1);
267+
expect(IdentityProvider.startTransaction).toHaveBeenCalledWith();
259268
expect(IdentityProvider.query).toHaveBeenCalledTimes(1);
260-
expect(IdentityProvider.query).toHaveBeenCalledWith();
269+
expect(IdentityProvider.query).toHaveBeenCalledWith(identityProviderTrx);
261270
expect(IdentityProvider.findById).toHaveBeenCalledTimes(1);
262271
expect(IdentityProvider.findById).toHaveBeenCalledWith('idir');
272+
expect(identityProviderTrx.commit).toHaveBeenCalledTimes(1);
263273
});
264274
});
265275

0 commit comments

Comments
 (0)