Skip to content

Commit 87a8573

Browse files
david-roccadavid-rocca
authored
Merge pull request #1743 from CVEProject/dr_1731
Resolves issue #1731, #1733 Fixes Mongoose document replacement failures during user updates and appropriately routes inactive users as unauthenticated.
2 parents 5fcafb8 + ddfedd4 commit 87a8573

7 files changed

Lines changed: 188 additions & 12 deletions

File tree

‎src/controller/registry-user.controller/registry-user.controller.js‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -253,12 +253,24 @@ async function updateUser (req, res, next) {
253253
: await userRepo.findOneByUsernameAndOrgShortname(userToEditParameters.username, userToEditParameters.org, { session })
254254

255255
const org = await orgRepo.findOneByShortName(userToEditParameters.org)
256+
if (!org) {
257+
logger.info({ uuid: req.ctx.uuid, message: `Target organization ${userToEditParameters.org} does not exist.` })
258+
return res.status(404).json(error.orgDnePathParam(userToEditParameters.org))
259+
}
256260

257261
if (body.org_short_name && !isSecretariat) {
258262
logger.info({ uuid: req.ctx.uuid, message: 'Only Secretariat can reassign user organization.' })
259263
return res.status(403).json(error.notAllowedToChangeOrganization())
260264
}
261265

266+
if (body.org_short_name) {
267+
const targetOrg = await orgRepo.findOneByShortName(body.org_short_name)
268+
if (!targetOrg) {
269+
logger.info({ uuid: req.ctx.uuid, message: `Target organization ${body.org_short_name} does not exist.` })
270+
return res.status(404).json(error.orgDnePathParam(body.org_short_name))
271+
}
272+
}
273+
262274
if (body.org_short_name && isSecretariat && userToEditParameters.org === org.short_name && body.org_short_name === org.short_name) {
263275
logger.info({ uuid: req.ctx.uuid, message: `User ${userToEditParameters.username} is already in organization ${userToEditParameters.org}.` })
264276
return res.status(403).json(error.alreadyInOrg(org.short_name, userToEditParameters.username))

‎src/middleware/middleware.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ async function optionallyValidateUser (req, res, next) {
6161
authenticated = false
6262
} else {
6363
result = await userRepo.findOneByUserNameAndOrgUUID(user, orgUUID)
64-
if (!result || !result.active) {
64+
if (!result || result.status === 'inactive') {
6565
authenticated = false
6666
} else {
6767
const isPwd = await argon2.verify(result.secret, key)
@@ -133,7 +133,7 @@ async function validateUser (req, res, next) {
133133
return res.status(401).json(error.unauthorized())
134134
}
135135

136-
if (result.active === false || result.status === 'inactive') {
136+
if (result.status === 'inactive') {
137137
logger.warn(JSON.stringify({ uuid: req.ctx.uuid, message: 'User deactivated. Authentication failed for ' + user }))
138138
return res.status(401).json(error.unauthorized())
139139
}

‎src/repositories/baseUserRepository.js‎

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,15 @@ const BaseOrgModel = require('../model/baseorg')
77
const RegistryUser = require('../model/registryuser')
88
const cryptoRandomString = require('crypto-random-string')
99
const UserRepository = require('./userRepository')
10-
const _ = require('lodash')
1110
const getConstants = require('../constants').getConstants
11+
const _ = require('lodash')
12+
13+
const skipNulls = (objValue, srcValue) => {
14+
if (_.isArray(objValue)) {
15+
return srcValue
16+
}
17+
return undefined
18+
}
1219

1320
/**
1421
* @function setAggregateUserObj
@@ -514,19 +521,23 @@ class BaseUserRepository extends BaseRepository {
514521
throw new Error('Legacy user not found')
515522
}
516523

524+
const { ...incomingUserBody } = incomingUser
517525
let legacyObjectRaw
518526
let registryObjectRaw
519527

520528
if (!isRegistryObject) {
521-
legacyObjectRaw = incomingUser
522-
registryObjectRaw = this.convertLegacyToRegistry(incomingUser)
529+
legacyObjectRaw = incomingUserBody
530+
registryObjectRaw = this.convertLegacyToRegistry(incomingUserBody)
523531
} else {
524-
registryObjectRaw = incomingUser
525-
legacyObjectRaw = this.convertRegistryToLegacy(incomingUser)
532+
registryObjectRaw = incomingUserBody
533+
legacyObjectRaw = this.convertRegistryToLegacy(incomingUserBody)
526534
}
527535

528-
const updatedLegacyUser = _.merge(legacyUser, legacyObjectRaw)
529-
const updatedRegistryUser = _.merge(registryUser, registryObjectRaw)
536+
const protectedFieldsRegistry = ['_id', 'UUID', '__v', 'secret', 'created', 'last_updated']
537+
const protectedFieldsLegacy = ['_id', 'UUID', '__v', 'secret', 'time', 'org_UUID']
538+
539+
const updatedRegistryUser = registryUser.overwrite(_.mergeWith(_.pick(registryUser.toObject(), protectedFieldsRegistry), registryObjectRaw, skipNulls))
540+
const updatedLegacyUser = legacyUser.overwrite(_.mergeWith(_.pick(legacyUser.toObject(), protectedFieldsLegacy), legacyObjectRaw, skipNulls))
530541

531542
try {
532543
if (incomingUser.org_short_name) {

‎test/integration-tests/cve-id/getCveIdTest.js‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,8 +188,92 @@ describe('Testing Get CVE-ID endpoint', () => {
188188
expect(cveIdObject.requested_by.user).to.equal(constants.nonSecretariatUserHeaders['CVE-API-USER'])
189189
})
190190
})
191+
it('For Secretariat users, should return full information when getting a single RESERVED CVE ID', async () => {
192+
const cveId = await helpers.cveIdReserveHelper(1, '2023', constants.nonSecretariatUserHeaders['CVE-API-ORG'], 'non-sequential')
193+
194+
await chai.request(app)
195+
.get(`/api/cve-id/${cveId}`)
196+
.set(constants.headers)
197+
.then(async (res, err) => {
198+
expect(err).to.be.undefined
199+
expect(res).to.have.status(200)
200+
expect(res.body.cve_id).to.equal(cveId)
201+
expect(res.body.state).to.equal('RESERVED')
202+
// Secretariat user should see owning org details
203+
expect(res.body.owning_cna).to.equal(constants.nonSecretariatUserHeaders['CVE-API-ORG'])
204+
})
205+
})
206+
207+
it('For owning CNA users, should return full information when getting a single RESERVED CVE ID', async () => {
208+
const cveId = await helpers.cveIdReserveHelper(1, '2023', constants.nonSecretariatUserHeaders['CVE-API-ORG'], 'non-sequential')
209+
210+
await chai.request(app)
211+
.get(`/api/cve-id/${cveId}`)
212+
.set(constants.nonSecretariatUserHeaders)
213+
.then(async (res, err) => {
214+
expect(err).to.be.undefined
215+
expect(res).to.have.status(200)
216+
expect(res.body.cve_id).to.equal(cveId)
217+
expect(res.body.state).to.equal('RESERVED')
218+
// Non-secretariat user from owning org should see owning org details
219+
expect(res.body.owning_cna).to.equal(constants.nonSecretariatUserHeaders['CVE-API-ORG'])
220+
})
221+
})
222+
223+
it('For non-owning CNA users, should return partial information and redacted owning_cna when getting a single RESERVED CVE ID', async function () {
224+
const cveId = await helpers.cveIdReserveHelper(1, '2023', constants.nonSecretariatUserHeaders['CVE-API-ORG'], 'non-sequential')
225+
226+
await chai.request(app)
227+
.get(`/api/cve-id/${cveId}`)
228+
.set(constants.nonSecretariatUserHeaders3) // evidence_15
229+
.then(async (res, err) => {
230+
expect(err).to.be.undefined
231+
expect(res).to.have.status(200)
232+
expect(res.body.cve_id).to.equal(cveId)
233+
expect(res.body.state).to.equal('RESERVED')
234+
expect(res.body.owning_cna).to.equal('[REDACTED]')
235+
expect(res.body).to.not.have.property('requested_by')
236+
})
237+
})
191238
})
192239
context('negative tests', () => {
240+
it('An inactive user should be treated as unauthenticated for optionallyValidateUser endpoints (GET /api/cve-id/:id)', async function () {
241+
const cveId = await helpers.cveIdReserveHelper(1, '2023', constants.nonSecretariatUserHeaders['CVE-API-ORG'], 'non-sequential')
242+
243+
// Deactivate user
244+
await helpers.userDeactivateAsSecHelper(constants.nonSecretariatUserHeaders['CVE-API-USER'], constants.nonSecretariatUserHeaders['CVE-API-ORG'])
245+
246+
await chai.request(app)
247+
.get(`/api/cve-id/${cveId}`)
248+
.set(constants.nonSecretariatUserHeaders)
249+
.then(async (res, err) => {
250+
expect(err).to.be.undefined
251+
expect(res).to.have.status(200)
252+
expect(res.body.cve_id).to.equal(cveId)
253+
expect(res.body.owning_cna).to.equal('[REDACTED]') // Should be redacted because user is treated as unauthenticated
254+
255+
// Reactivate user for other tests
256+
await helpers.userReactivateAsSecHelper(constants.nonSecretariatUserHeaders['CVE-API-USER'], constants.nonSecretariatUserHeaders['CVE-API-ORG'])
257+
})
258+
})
259+
260+
it('An inactive user should be denied access for validateUser endpoints (GET /api/cve-id)', async function () {
261+
// Deactivate user
262+
await helpers.userDeactivateAsSecHelper(constants.nonSecretariatUserHeaders['CVE-API-USER'], constants.nonSecretariatUserHeaders['CVE-API-ORG'])
263+
264+
await chai.request(app)
265+
.get('/api/cve-id')
266+
.set(constants.nonSecretariatUserHeaders)
267+
.then(async (res, err) => {
268+
expect(err).to.be.undefined
269+
expect(res).to.have.status(401)
270+
expect(res.body.error).to.equal('UNAUTHORIZED')
271+
272+
// Reactivate user for other tests
273+
await helpers.userReactivateAsSecHelper(constants.nonSecretariatUserHeaders['CVE-API-USER'], constants.nonSecretariatUserHeaders['CVE-API-ORG'])
274+
})
275+
})
276+
193277
it('Feb 29 2100 should not be valid', async () => {
194278
await chai.request(app)
195279
.get('/api/cve-id?time_modified.gt=2100-02-29T00:00:00Z')

‎test/integration-tests/helpers.js‎

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,40 @@ async function updateOwningOrgAsSecHelper (cveId, newOrgShortName) {
116116
})
117117
}
118118

119+
async function userDeactivateAsSecHelper (userName, orgShortName) {
120+
const user = await chai.request(app)
121+
.get(`/api/registry/org/${orgShortName}/user/${userName}`)
122+
.set(constants.headers)
123+
.then(res => res.body)
124+
125+
await chai.request(app)
126+
.put(`/api/registry/org/${orgShortName}/user/${userName}`)
127+
.set(constants.headers)
128+
.send({ ...user, status: 'inactive' })
129+
.then((res, err) => {
130+
// Safety Expect
131+
expect(res).to.have.status(200)
132+
})
133+
}
134+
135+
async function userReactivateAsSecHelper (userName, orgShortName) {
136+
const user = await chai.request(app)
137+
.get(`/api/registry/org/${orgShortName}/user/${userName}`)
138+
.set(constants.headers)
139+
.then(res => res.body)
140+
141+
user.status = 'active'
142+
143+
await chai.request(app)
144+
.put(`/api/registry/org/${orgShortName}/user/${userName}`)
145+
.set(constants.headers)
146+
.send(user)
147+
.then((res, err) => {
148+
// Safety Expect
149+
expect(res).to.have.status(200)
150+
})
151+
}
152+
119153
module.exports = {
120154
cveIdReserveHelper,
121155
cveIdBulkReserveHelper,
@@ -126,5 +160,7 @@ module.exports = {
126160
cveUpdateAsSecHelper,
127161
cveUpdateAsCnaHelperWithAdpContainer,
128162
userOrgUpdateAsSecHelper,
129-
updateOwningOrgAsSecHelper
163+
updateOwningOrgAsSecHelper,
164+
userDeactivateAsSecHelper,
165+
userReactivateAsSecHelper
130166
}

‎test/integration-tests/user/updateUserTest.js‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,5 +205,36 @@ describe('Testing Edit user endpoint', () => {
205205
expect(res.body.error).to.equal('SECRET_UPDATE_NOT_ALLOWED')
206206
})
207207
})
208+
it('Should return 404 when target organization in path does not exist', async () => {
209+
const user = constants.headers['CVE-API-USER']
210+
await chai.request(app)
211+
.put(`/api/registry/org/non_existent_org/user/${user}`)
212+
.set(constants.headers)
213+
.send({
214+
name: {
215+
first: 'NewFirst',
216+
last: 'NewLast'
217+
}
218+
})
219+
.then((res) => {
220+
expect(res).to.have.status(404)
221+
expect(res.body.error).to.contain('ORG_DNE_PARAM')
222+
})
223+
})
224+
225+
it('Should return 404 when target organization in body does not exist', async () => {
226+
const user = constants.headers['CVE-API-USER']
227+
const org = constants.headers['CVE-API-ORG']
228+
await chai.request(app)
229+
.put(`/api/registry/org/${org}/user/${user}`)
230+
.set(constants.headers)
231+
.send({
232+
org_short_name: 'non_existent_org'
233+
})
234+
.then((res) => {
235+
expect(res).to.have.status(404)
236+
expect(res.body.error).to.contain('ORG_DNE_PARAM')
237+
})
238+
})
208239
})
209240
})

‎test/unit-tests/middleware/mockObjects.middleware.js‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,8 @@ const existentUser = {
3434
},
3535
secret: '$argon2i$v=19$m=4096,t=3,p=1$+qGHEfH5h4/tk404iWBxFw$xV96/b4NvQVvlZIq57wTS8s7gfKzsfMXRiOyf3ffgcw',
3636
username: 'cpadro',
37-
active: true
37+
active: true,
38+
status: 'active'
3839
}
3940

4041
const deactivatedUser = {
@@ -49,7 +50,8 @@ const deactivatedUser = {
4950
},
5051
secret: '$argon2i$v=19$m=4096,t=3,p=1$+qGHEfH5h4/tk404iWBxFw$xV96/b4NvQVvlZIq57wTS8s7gfKzsfMXRiOyf3ffgcw',
5152
username: 'flast',
52-
active: false
53+
active: false,
54+
status: 'inactive'
5355
}
5456

5557
module.exports = {

0 commit comments

Comments
 (0)