Repository: CVEProject/cve-services
Pull request #628 (2022-04-18 13:52:45)

Keep Org Admins from changing user's org.
Identify the Issue
[#575](https://github.com/CVEProject/cve-services/issues/575)

Description of the Change
Pulls in commit from master that addresses [CVE-2021-46561](https://github.com/advisories/GHSA-w6p2-hpr9-hmj8).



Patch:
diff --git a/src/controller/org.controller/error.js b/src/controller/org.controller/error.js
index 822a4208..10538daf 100644
--- a/src/controller/org.controller/error.js
+++ b/src/controller/org.controller/error.js
@@ -22,6 +22,13 @@ class OrgControllerError extends idrErr.IDRError {
     return err
   }
 
+  notAllowedToChangeOrganization () {
+    const err = {}
+    err.error = 'NOT_ALLOWED_TO_CHANGE_ORGANIZATION'
+    err.message = 'Only the Secretariat can change the organization for a user.'
+    return err
+  }
+
   orgExists (shortname) { // org
     const err = {}
     err.error = 'ORG_EXISTS'
diff --git a/src/controller/org.controller/org.controller.js b/src/controller/org.controller/org.controller.js
index 516ba5e3..30c345a0 100644
--- a/src/controller/org.controller/org.controller.js
+++ b/src/controller/org.controller/org.controller.js
@@ -514,6 +514,10 @@ async function updateUser (req, res, next) {
       } else if (key === 'org_shortname') {
         newOrgShortName = req.ctx.query.org_shortname
         changesRequirePrivilegedRole = true
+        if (!isSecretariat) {
+          logger.info({ uuid: req.ctx.uuid, message: 'The user could not be updated because ' + requesterUsername + ' is an Org Admin and tried to reassign the organization.' })
+          return res.status(403).json(error.notAllowedToChangeOrganization())
+        }
       } else if (key === 'name.first') {
         newUser.name.first = req.ctx.query['name.first']
       } else if (key === 'name.last') {


Test patch:
diff --git a/test-http/src/test/org_user_tests/org_as_org_admin.py b/test-http/src/test/org_user_tests/org_as_org_admin.py
index ac2d43a0..0bf22fa0 100644
--- a/test-http/src/test/org_user_tests/org_as_org_admin.py
+++ b/test-http/src/test/org_user_tests/org_as_org_admin.py
@@ -268,31 +268,31 @@ def test_org_admin_cannot_update_user_for_another_org(org_admin_headers):
     assert res.status_code == 403
     response_contains_json(res, 'error', 'NOT_SAME_ORG_OR_SECRETARIAT')
 
-
-def test_org_admin_cannot_update_user_new_shortname_dne(org_admin_headers):
-    """ services api prevents org admins from updating a user's org that doesn't exist """
-    org = org_admin_headers['CVE-API-ORG']
-    user = org_admin_headers['CVE-API-USER']
-    org_shortname = str(uuid.uuid4())
-    res = requests.put(
-        f'{env.AWG_BASE_URL}{ORG_URL}/{org}/user/{user}?org_shortname={org_shortname}',
-        headers=org_admin_headers
-    )
-    assert res.status_code == 404
-    response_contains_json(res, 'error', 'ORG_DNE')
-
-
-def test_org_admin_cannot_update_duplicate_user_with_new_shortname_and_username(org_admin_headers):
-    """ services api prevents org admins from updating a user's org and username if that user already exist """
-    org1 = org_admin_headers['CVE-API-ORG']
-    user1 = org_admin_headers['CVE-API-USER']
-    org2, user2 = create_new_user_with_new_org_by_uuid()
-    res = requests.put(
-        f'{env.AWG_BASE_URL}{ORG_URL}/{org1}/user/{user1}?org_shortname={org2}&new_username={user2}',
-        headers=org_admin_headers
-    )
-    assert res.status_code == 403
-    response_contains_json(res, 'error', 'DUPLICATE_USERNAME')
+# Admins can't change user's org
+# def test_org_admin_cannot_update_user_new_shortname_dne(org_admin_headers):
+#     """ services api prevents org admins from updating a user's org that doesn't exist """
+#     org = org_admin_headers['CVE-API-ORG']
+#     user = org_admin_headers['CVE-API-USER']
+#     org_shortname = str(uuid.uuid4())
+#     res = requests.put(
+#         f'{env.AWG_BASE_URL}{ORG_URL}/{org}/user/{user}?org_shortname={org_shortname}',
+#         headers=org_admin_headers
+#     )
+#     assert res.status_code == 404
+#     response_contains_json(res, 'error', 'ORG_DNE')
+
+# Admins can't change user's org
+# def test_org_admin_cannot_update_duplicate_user_with_new_shortname_and_username(org_admin_headers):
+#     """ services api prevents org admins from updating a user's org and username if that user already exist """
+#     org1 = org_admin_headers['CVE-API-ORG']
+#     user1 = org_admin_headers['CVE-API-USER']
+#     org2, user2 = create_new_user_with_new_org_by_uuid()
+#     res = requests.put(
+#         f'{env.AWG_BASE_URL}{ORG_URL}/{org1}/user/{user1}?org_shortname={org2}&new_username={user2}',
+#         headers=org_admin_headers
+#     )
+#     assert res.status_code == 403
+#     response_contains_json(res, 'error', 'DUPLICATE_USERNAME')
 
 
 def test_org_admin_cannot_update_duplicate_user_with_new_username(org_admin_headers):
@@ -309,23 +309,23 @@ def test_org_admin_cannot_update_duplicate_user_with_new_username(org_admin_head
     assert res.status_code == 403
     response_contains_json(res, 'error', 'DUPLICATE_USERNAME')
 
-
-def test_org_admin_cannot_update_duplicate_user_with_new_shortname(org_admin_headers):
-    """ services api prevents org admins from updating a user's org if that user already exist """
-    user = org_admin_headers['CVE-API-USER']
-    org1 = org_admin_headers['CVE-API-ORG']
-    org2 = str(uuid.uuid4())
-    res = create_new_user_with_new_org_by_shortname(org2, user) # creating a user with same username as org admin user's username
-    res = requests.put(
-        f'{env.AWG_BASE_URL}{ORG_URL}/{org1}/user/{user}?org_shortname={org2}',
-        headers=org_admin_headers
-    )
-    assert res.status_code == 403
-    response_contains_json(res, 'error', 'DUPLICATE_USERNAME')
+# Admin users aren't able to update a users org
+# def test_org_admin_cannot_update_duplicate_user_with_new_shortname(org_admin_headers):
+#     """ services api prevents org admins from updating a user's org if that user already exist """
+#     user = org_admin_headers['CVE-API-USER']
+#     org1 = org_admin_headers['CVE-API-ORG']
+#     org2 = str(uuid.uuid4())
+#     res = create_new_user_with_new_org_by_shortname(org2, user) # creating a user with same username as org admin user's username
+#     res = requests.put(
+#         f'{env.AWG_BASE_URL}{ORG_URL}/{org1}/user/{user}?org_shortname={org2}',
+#         headers=org_admin_headers
+#     )
+#     assert res.status_code == 403
+#     response_contains_json(res, 'error', 'NOT_ALLOWED_TO_CHANGE_ORGANIZATION')
 
 
 def test_org_admin_update_same_org_user_state_sn_un(org_admin_headers):
-    """  allows admin users to update a user's active state, org shortname, and user username """
+    """  allows admin users to update a user's active state and user username """
     org = org_admin_headers['CVE-API-ORG']
     user = str(uuid.uuid4())
     res = post_new_org_user(org, user) # creating a user with same org as admin org user
@@ -335,7 +335,7 @@ def test_org_admin_update_same_org_user_state_sn_un(org_admin_headers):
     res = post_new_org(new_shortname, new_shortname)  # create new org
     assert res.status_code == 200
     res = requests.put(
-        f'{env.AWG_BASE_URL}{ORG_URL}/{org}/user/{user}?org_shortname={new_shortname}&new_username={new_username}&active=false',
+        f'{env.AWG_BASE_URL}{ORG_URL}/{org}/user/{user}?new_username={new_username}&active=false',
         headers=org_admin_headers
     )
     assert res.status_code == 200
@@ -372,23 +372,23 @@ def test_org_admin_update_same_org_user_roles_name(org_admin_headers):
     assert json.loads(res.content.decode())['updated']['name']['middle'] == 's'
     assert json.loads(res.content.decode())['updated']['name']['suffix'] == 't'
 
-
-def test_org_admin_update_own_user_state_sn_un(org_admin_headers):
-    """  allows admin users to update its own active state, org shortname, and user username """
-    org = org_admin_headers['CVE-API-ORG']
-    user = org_admin_headers['CVE-API-USER']
-    new_shortname = str(uuid.uuid4())   # used in query
-    new_username = str(uuid.uuid4())    # used in query
-    res = post_new_org(new_shortname, new_shortname)  # create new org
-    assert res.status_code == 200
-    res = requests.put(
-        f'{env.AWG_BASE_URL}{ORG_URL}/{org}/user/{user}?org_shortname={new_shortname}&new_username={new_username}&active=false',
-        headers=org_admin_headers
-    )
-    assert res.status_code == 200
-    assert json.loads(res.content.decode())['updated']['active'] == False
-    assert json.loads(res.content.decode())['updated']['username'] == new_username
-    assert json.loads(res.content.decode())['updated']['username'] is not None
+# Admin users can't change org?
+# def test_org_admin_update_own_user_state_sn_un(org_admin_headers):
+#     """  allows admin users to update its own active state, org shortname, and user username """
+#     org = org_admin_headers['CVE-API-ORG']
+#     user = org_admin_headers['CVE-API-USER']
+#     new_shortname = str(uuid.uuid4())   # used in query
+#     new_username = str(uuid.uuid4())    # used in query
+#     res = post_new_org(new_shortname, new_shortname)  # create new org
+#     assert res.status_code == 200
+#     res = requests.put(
+#         f'{env.AWG_BASE_URL}{ORG_URL}/{org}/user/{user}?org_shortname={new_shortname}&new_username={new_username}&active=false',
+#         headers=org_admin_headers
+#     )
+#     assert res.status_code == 200
+#     assert json.loads(res.content.decode())['updated']['active'] == False
+#     assert json.loads(res.content.decode())['updated']['username'] == new_username
+#     assert json.loads(res.content.decode())['updated']['username'] is not None
 
 
 def test_org_admin_update_own_user_roles_name(org_admin_headers):
diff --git a/test-http/src/test/org_user_tests/regular_users.py b/test-http/src/test/org_user_tests/regular_users.py
index b7ed4e67..6b1566af 100644
--- a/test-http/src/test/org_user_tests/regular_users.py
+++ b/test-http/src/test/org_user_tests/regular_users.py
@@ -78,7 +78,7 @@ def test_regular_user_cannot_update_organization_with_new_shortname(reg_user_hea
         headers=reg_user_headers
     )
     assert res.status_code == 403
-    response_contains_json(res, 'error', 'NOT_ORG_ADMIN_OR_SECRETARIAT')
+    response_contains_json(res, 'error', 'NOT_ALLOWED_TO_CHANGE_ORGANIZATION')
 
 
 def test_regular_user_cannot_update_active_state(reg_user_headers):
diff --git a/test/unit-tests/user/userUpdateTest.js b/test/unit-tests/user/userUpdateTest.js
index 8718b162..ef23125b 100644
--- a/test/unit-tests/user/userUpdateTest.js
+++ b/test/unit-tests/user/userUpdateTest.js
@@ -277,6 +277,54 @@ describe('Testing the PUT /org/:shortname/user/:username endpoint in Org Control
         })
     })
 
+    it('User is not updated because Org Admin is trying to change organization', (done) => {
+      class Org {
+        async getOrgUUID () {
+          return userFixtures.existentOrg.UUID
+        }
+
+        async isSecretariat () {
+          return false
+        }
+      }
+
+      class User {
+        async findOneByUserNameAndOrgUUID () {
+          return userFixtures.existentUser
+        }
+
+        async isAdmin () {
+          return true
+        }
+      }
+
+      app.route('/user-not-updated-admin-changing-org/:shortname/:username')
+        .put((req, res, next) => {
+          const factory = {
+            getOrgRepository: () => { return new Org() },
+            getUserRepository: () => { return new User() }
+          }
+          req.ctx.repositories = factory
+          next()
+        }, orgParams.parsePostParams, orgController.USER_UPDATE_SINGLE)
+
+      chai.request(app)
+        .put(`/user-not-updated-admin-changing-org/${userFixtures.existentOrgDummy.short_name}/${userFixtures.userA.username}?org_shortname=${userFixtures.existentOrgDummy.short_name}`)
+        .set(userFixtures.userDHeader)
+        .end((err, res) => {
+          if (err) {
+            done(err)
+          }
+
+          expect(res).to.have.status(403)
+          expect(res).to.have.property('body').and.to.be.a('object')
+          const errObj = error.notAllowedToChangeOrganization()
+          expect(res.body.error).to.equal(errObj.error)
+          expect(res.body.message).to.equal(errObj.message)
+          done()
+        })
+    })
+
     it('User is not updated because requestor is Org Admin of different organization', (done) => {
       class Org {
         async getOrgUUID () {