Repository: illinois/queue
Pull request #233 (2019-03-14 01:53:30)

Improve security for queue settings
The queue settings page can now only be viewed by course staff or queue admins, and will 403 otherwise. 

I also noticed `admissionControlUrl` is not being filtered out on queue GET requests - is this something we want to hide from regular users?

now[bot]: 
This pull request is [automatically deployed](https://zeit.co/docs/features/now-for-github?utm_source=automated&utm_medium=github&utm_campaign=now_bot) with [Now](https://zeit.co/now?utm_source=automated&utm_medium=github&utm_campaign=now_bot).
To access deployments, click Details below or on the icon next to each push.


	

nwalters512: Thinking about how you chose to exclude the queue props, I think it'd be better to exclude it from the [default Sequelize scope](http://docs.sequelizejs.com/manual/scopes.html) and then only explicitly include it for admins/course staff. That'll give us a sane default for whenever we have to read a queue out of the DB.
james9909: How would this work when we need the `courseId` for the queue to check for their permissions? Is there an easy way to add an attribute back to the model after it's already been queried? I don't think we should have to make two separate queries to accomplish this.
Also, would this also require us to query each queue with the correct scope in the "Get all open queues" endpoint?
nwalters512: By this I mean we'd only exclude the private config (admission control) by default; everything else would still be in there. Scopes apply to queries of whole collections too, so nothing would change for the "get 1" vs "get N" cases.
james9909: Regarding my "get N" question, my confusion was how we'd inject the private config for only the correct queues with one single query. Although I realize now that the queue home page doesn't need that kind of information anyways.

The main thing I'm confused about is how we're supposed to apply the correct "admin" scope when making the queue query without already having queried the queue to see if the user is on course staff, which determines whether they're allowed to see the config or not.
nwalters512: Ah, I see what you're getting at now. Yeah, I would say for a list, it's fine if we just unconditionally omit them for now. Regarding checking for course staff, that would indeed be not ideal, but DB queries are also super cheap - most requests are already doing several.

I do like the idea of a settings endpoint - if we start adding more settings in the future, that'd be nice to only ever deliver them on the settings page. When I was first doing this, I actually thought about having a separate table for these sorts of settings, but ultimately decided that a 1-1 relationship like that felt weird. We could revisit that too.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index be4f78c..fbd742d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,7 @@ When a new version is deployed, the changes since the last deploy should be labe
 with the current date and the next changes should go under a **[Next]** header.
 
 ## [Next]
+* Improve queue settings security. ([@james9909](https://github.com/james9909) in [#233](https://github.com/illinois/queue/pull/233))
 
 * Fix confidential queue not being shown correctly to course staff by using `isUserCourseStaffForQueue` instead of `isUserCourseStaff`. ([@jackieo5023](https://github.com/jackieo5023) in [#234](https://github.com/illinois/queue/pull/234))
 
diff --git a/package-lock.json b/package-lock.json
index ce40c15..84fdb74 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -991,11 +991,6 @@
       "resolved": "https://registry.npmjs.org/@illinois/react-use-local-storage/-/react-use-local-storage-1.0.1.tgz",
       "integrity": "sha512-LxlgFaICWd6csoWNzbUrhVZGtEFmRk1B+tODwe5sm/eP/HmaNdPM2c7YpRnoFSfQs3SKHa+90B/BSN5eiEdHEg=="
     },
-    "@types/geojson": {
-      "version": "1.0.6",
-      "resolved": "https://registry.npmjs.org/@types/geojson/-/geojson-1.0.6.tgz",
-      "integrity": "sha512-Xqg/lIZMrUd0VRmSRbCAewtwGZiAk3mEUDvV4op1tGl+LvyPcb/MIOSxTl9z+9+J+R4/vpjiCAT4xeKzH9ji1w=="
-    },
     "@types/node": {
       "version": "11.9.0",
       "resolved": "https://registry.npmjs.org/@types/node/-/node-11.9.0.tgz",
@@ -6210,11 +6205,6 @@
         "is-property": "^1.0.2"
       }
     },
-    "generic-pool": {
-      "version": "3.6.1",
-      "resolved": "https://registry.npmjs.org/generic-pool/-/generic-pool-3.6.1.tgz",
-      "integrity": "sha512-iMmD/pY4q0+V+f8o4twE9JPeqfNuX+gJAaIPB3B0W1lFkBOtTxBo6B0HxHPgGhzQA8jego7EWopcYq/UDJO2KA=="
-    },
     "get-caller-file": {
       "version": "1.0.3",
       "resolved": "https://registry.npmjs.org/get-caller-file/-/get-caller-file-1.0.3.tgz",
@@ -11945,22 +11935,11 @@
       "integrity": "sha512-TTlYpa+OL+vMMNG24xSlQGEJ3B/RzEfUlLct7b5G/ytav+wPrplCpVMFuwzXbkecJrb6IYo1iFb0S9v37754mg=="
     },
     "retry-as-promised": {
-      "version": "2.3.2",
-      "resolved": "https://registry.npmjs.org/retry-as-promised/-/retry-as-promised-2.3.2.tgz",
-      "integrity": "sha1-zZdO5P2bX+A8vzGHHuSCIcB3N7c=",
+      "version": "3.2.0",
+      "resolved": "https://registry.npmjs.org/retry-as-promised/-/retry-as-promised-3.2.0.tgz",
+      "integrity": "sha512-CybGs60B7oYU/qSQ6kuaFmRd9sTZ6oXSc0toqePvV74Ac6/IFZSI1ReFQmtCN+uvW1Mtqdwpvt/LGOiCBAY2Mg==",
       "requires": {
-        "bluebird": "^3.4.6",
-        "debug": "^2.6.9"
-      },
-      "dependencies": {
-        "debug": {
-          "version": "2.6.9",
-          "resolved": "https://registry.npmjs.org/debug/-/debug-2.6.9.tgz",
-          "integrity": "sha512-bC7ElrdJaJnPbAP+1EotYvqZsb3ecl5wi6Bfi6BJTUcNowp6cvspg0jXznRTKDjm/E7AdgFBVeAPVMNcKGsHMA==",
-          "requires": {
-            "ms": "2.0.0"
-          }
-        }
+        "any-promise": "^1.3.0"
       }
     },
     "rimraf": {
@@ -12165,27 +12144,54 @@
       "integrity": "sha1-1WgS4cAXpuTnw+Ojeh2m143TyT4="
     },
     "sequelize": {
-      "version": "4.42.0",
-      "resolved": "https://registry.npmjs.org/sequelize/-/sequelize-4.42.0.tgz",
-      "integrity": "sha512-qxAYnX4rcv7PbNtEidb56REpxNJCdbN0qyk1jb3+e6sE7OrmS0nYMU+MFVbNTJtZfSpOEEL1TX0TkMw+wzZBxg==",
+      "version": "5.1.0",
+      "resolved": "https://registry.npmjs.org/sequelize/-/sequelize-5.1.0.tgz",
+      "integrity": "sha512-LmjEAedMTItkIx0mcBfXVmdkkIQOc+1reuv+UpqSADGvQofZ4Sn9ElUBE8egLgCK4oWjy1Ybsju+YDAJpCv1ww==",
       "requires": {
         "bluebird": "^3.5.0",
         "cls-bluebird": "^2.1.0",
-        "debug": "^3.1.0",
-        "depd": "^1.1.0",
+        "debug": "^4.1.1",
+        "depd": "^2.0.0",
         "dottie": "^2.0.0",
-        "generic-pool": "^3.4.0",
         "inflection": "1.12.0",
-        "lodash": "^4.17.1",
-        "moment": "^2.20.0",
-        "moment-timezone": "^0.5.14",
-        "retry-as-promised": "^2.3.2",
-        "semver": "^5.5.0",
-        "terraformer-wkt-parser": "^1.1.2",
+        "lodash": "^4.17.11",
+        "moment": "^2.24.0",
+        "moment-timezone": "^0.5.21",
+        "retry-as-promised": "^3.1.0",
+        "semver": "^5.6.0",
+        "sequelize-pool": "^1.0.2",
         "toposort-class": "^1.0.1",
         "uuid": "^3.2.1",
-        "validator": "^10.4.0",
-        "wkx": "^0.4.1"
+        "validator": "^10.11.0",
+        "wkx": "^0.4.6"
+      },
+      "dependencies": {
+        "debug": {
+          "version": "4.1.1",
+          "resolved": "https://registry.npmjs.org/debug/-/debug-4.1.1.tgz",
+          "integrity": "sha512-pYAIzeRo8J6KPEaJ0VWOh5Pzkbw/RetuzehGM7QRRX5he4fPHx2rdKMB256ehJCkX+XRQm16eZLqLNS8RSZXZw==",
+          "requires": {
+            "ms": "^2.1.1"
+          }
+        },
+        "depd": {
+          "version": "2.0.0",
+          "resolved": "https://registry.npmjs.org/depd/-/depd-2.0.0.tgz",
+          "integrity": "sha512-g7nH6P6dyDioJogAAGprGpCtVImJhpPk/roCzdb3fIh61/s/nPsfR6onyMwkCAR/OlC3yBC0lESvUoQEAssIrw=="
+        },
+        "ms": {
+          "version": "2.1.1",
+          "resolved": "https://registry.npmjs.org/ms/-/ms-2.1.1.tgz",
+          "integrity": "sha512-tgp+dl5cGk28utYktBsrFqA7HKgrhgPsg6Z/EfhWI4gl1Hwq8B/GmY/0oXZ6nF8hDVesS/FpnYaD/kOWhYQvyg=="
+        }
+      }
+    },
+    "sequelize-pool": {
+      "version": "1.0.2",
+      "resolved": "https://registry.npmjs.org/sequelize-pool/-/sequelize-pool-1.0.2.tgz",
+      "integrity": "sha512-VMKl/gCCdIvB1gFZ7p+oqLFEyZEz3oMMYjkKvfEC7GoO9bBcxmfOOU9RdkoltfXGgBZFigSChihRly2gKtsh2w==",
+      "requires": {
+        "bluebird": "^3.5.3"
       }
     },
     "sequelize-stream": {
@@ -13047,23 +13053,6 @@
         }
       }
     },
-    "terraformer": {
-      "version": "1.0.9",
-      "resolved": "https://registry.npmjs.org/terraformer/-/terraformer-1.0.9.tgz",
-      "integrity": "sha512-YlmQ1fsMWTkKGDGibCRWgmLzrpDRUr63Q025LJ/taYQ6j1Yb8q9McKF7NBi6ACAyUXO6F/bl9w6v4MY307y5Ag==",
-      "requires": {
-        "@types/geojson": "^1.0.0"
-      }
-    },
-    "terraformer-wkt-parser": {
-      "version": "1.2.0",
-      "resolved": "https://registry.npmjs.org/terraformer-wkt-parser/-/terraformer-wkt-parser-1.2.0.tgz",
-      "integrity": "sha512-QU3iA54St5lF8Za1jg1oj4NYc8sn5tCZ08aNSWDeGzrsaV48eZk1iAVWasxhNspYBoCqdHuoot1pUTUrE1AJ4w==",
-      "requires": {
-        "@types/geojson": "^1.0.0",
-        "terraformer": "~1.0.5"
-      }
-    },
     "terser": {
       "version": "3.16.1",
       "resolved": "https://registry.npmjs.org/terser/-/terser-3.16.1.tgz",
diff --git a/package.json b/package.json
index f7c7156..f0b7331 100644
--- a/package.json
+++ b/package.json
@@ -64,7 +64,7 @@
     "redux": "^4.0.1",
     "redux-thunk": "^2.3.0",
     "reselect": "^4.0.0",
-    "sequelize": "^4.42.0",
+    "sequelize": "^5.1.0",
     "sequelize-stream": "^1.0.2",
     "socket.io": "^2.2.0",
     "socket.io-client": "^2.2.0",
diff --git a/src/api/questions.js b/src/api/questions.js
index 53a83dd..7b94ed0 100644
--- a/src/api/questions.js
+++ b/src/api/questions.js
@@ -13,7 +13,7 @@ const {
   requireQueueForQuestion,
   requireQuestion,
   failIfErrors,
-  canUserSeeQuestionDetailsForConfidentialQueue,
+  isUserStudent,
   filterConfidentialQueueQuestionsForUser,
   ApiError,
 } = require('./util')
@@ -137,17 +137,21 @@ router.post(
       }
     }
 
-    const question = Question.build({
-      name: data.name,
-      // Questions in fixed-location queues should never have a location
-      location: res.locals.queue.fixedLocation ? '' : data.location,
-      topic: data.topic,
-      enqueueTime: new Date(),
-      queueId,
-      askedById: askerId,
-    })
+    const question = await Question.create(
+      {
+        name: data.name,
+        // Questions in fixed-location queues should never have a location
+        location: res.locals.queue.fixedLocation ? '' : data.location,
+        topic: data.topic,
+        enqueueTime: new Date(),
+        queueId,
+        askedById: askerId,
+      },
+      {
+        include: [{ model: User, as: 'askedBy' }],
+      }
+    )
 
-    await question.save()
     await question.reload()
     res.status(201).send(question)
   })
@@ -173,7 +177,7 @@ router.get(
     )
     if (isConfidential) {
       const { userAuthz } = res.locals
-      if (!canUserSeeQuestionDetailsForConfidentialQueue(userAuthz, courseId)) {
+      if (isUserStudent(userAuthz, courseId)) {
         const { id: userId } = res.locals.userAuthn
         questions = filterConfidentialQueueQuestionsForUser(userId, questions)
       }
@@ -191,7 +195,7 @@ router.get(
     if (isConfidential) {
       const { id: userId } = res.locals.userAuthn
       const { userAuthz } = res.locals
-      if (!canUserSeeQuestionDetailsForConfidentialQueue(userAuthz, courseId)) {
+      if (isUserStudent(userAuthz, courseId)) {
         if (res.locals.question.askedById !== userId) {
           res.status(403).send('You are not authorized to access that question')
           return
diff --git a/src/api/queues.js b/src/api/queues.js
index 8d6f41a..9e1e3cb 100644
--- a/src/api/queues.js
+++ b/src/api/queues.js
@@ -13,7 +13,7 @@ const {
   requireQueue,
   requireUser,
   failIfErrors,
-  canUserSeeQuestionDetailsForConfidentialQueue,
+  isUserStudent,
   filterConfidentialQueueQuestionsForUser,
 } = require('./util')
 
@@ -33,7 +33,11 @@ function validateLocation(req, res, next) {
 router.get(
   '/',
   safeAsync(async (req, res, _next) => {
-    const queues = await Queue.scope('questionCount').findAll()
+    const queuesResult = await Queue.scope(
+      'defaultScope',
+      'questionCount'
+    ).findAll()
+    const queues = queuesResult.map(queue => queue.get({ plain: true }))
     res.json(queues)
   })
 )
@@ -82,8 +86,12 @@ router.get(
   '/:queueId',
   [requireQueue, failIfErrors],
   safeAsync(async (req, res, _next) => {
-    const { id: queueId } = res.locals.queue
-    const queueResults = await Queue.findOne({
+    const { id: queueId, courseId } = res.locals.queue
+    const { userAuthz } = res.locals
+    const isStudent = isUserStudent(userAuthz, courseId)
+
+    const scope = isStudent ? 'defaultScope' : 'courseStaff'
+    const queueResult = await Queue.scope(scope).findOne({
       where: {
         id: queueId,
       },
@@ -96,6 +104,7 @@ router.get(
           },
           required: false,
           include: [User],
+          attributes: ['startTime', 'endTime'],
         },
         {
           model: Question,
@@ -103,25 +112,29 @@ router.get(
           where: {
             dequeueTime: null,
           },
+          attributes: [
+            'id',
+            'name',
+            'topic',
+            'beingAnswered',
+            'answerStartTime',
+            'enqueueTime',
+            'dequeueTime',
+            'askedById',
+          ],
+          include: [{ model: User, as: 'askedBy' }],
         },
       ],
       order: [[Question, 'id', 'ASC']],
     })
-
     // Convert to plain object that we can manipulate/filter/etc. before
     // sending back to the client
-    const queue = queueResults.get({ plain: true })
+    const queue = queueResult.get({ plain: true })
 
     // If this is a confidential queue, don't send any actual question data
     // back to the client, besides IDs
     if (queue.isConfidential) {
-      const { userAuthz } = res.locals
-      if (
-        !canUserSeeQuestionDetailsForConfidentialQueue(
-          userAuthz,
-          queue.courseId
-        )
-      ) {
+      if (isStudent) {
         const { id: userId } = res.locals.userAuthn
         const filtered = filterConfidentialQueueQuestionsForUser(
           userId,
diff --git a/src/api/util.js b/src/api/util.js
index 9a307fa..4b60a8b 100644
--- a/src/api/util.js
+++ b/src/api/util.js
@@ -65,10 +65,10 @@ const requireModelForModel = (
   next()
 }
 
-const canUserSeeQuestionDetailsForConfidentialQueue = (userAuthz, courseId) => {
+const isUserStudent = (userAuthz, courseId) => {
   const { isAdmin, staffedCourseIds } = userAuthz
   const staffsQueue = staffedCourseIds.findIndex(id => id === courseId) !== -1
-  return isAdmin || staffsQueue
+  return !isAdmin && !staffsQueue
 }
 
 /**
@@ -114,6 +114,6 @@ module.exports = {
   requireModel,
 
   // Stuff for confidential queues
-  canUserSeeQuestionDetailsForConfidentialQueue,
+  isUserStudent,
   filterConfidentialQueueQuestionsForUser,
 }
diff --git a/src/models/Queue.js b/src/models/Queue.js
index c48ef60..b519ba5 100644
--- a/src/models/Queue.js
+++ b/src/models/Queue.js
@@ -34,6 +34,14 @@ module.exports = (sequelize, DataTypes) => {
       defaultScope: {
         attributes: {
           include: ['courseId', 'createdByUserId'],
+          exclude: ['admissionControlEnabled', 'admissionControlUrl'],
+        },
+      },
+      scopes: {
+        courseStaff: {
+          attributes: {
+            include: ['courseId', 'createdByUserId'],
+          },
         },
       },
     }
diff --git a/src/models/index.js b/src/models/index.js
index 5382eb4..1a83745 100644
--- a/src/models/index.js
+++ b/src/models/index.js
@@ -38,7 +38,6 @@ module.exports.initSequelize = sequelize => {
 }
 
 const sequelizeConfig = {
-  operatorsAliases: Sequelize.Op,
   dialectOptions: {
     multipleStatements: true,
   },
diff --git a/src/pages/queueSettings.js b/src/pages/queueSettings.js
index 75d2b26..f562e29 100644
--- a/src/pages/queueSettings.js
+++ b/src/pages/queueSettings.js
@@ -101,7 +101,8 @@ const mapDispatchToProps = dispatch => ({
   dispatch,
 })
 
+const permissions = { requireCourseStaff: true }
 export default connect(
   mapStateToProps,
   mapDispatchToProps
-)(PageWithUser(QueueSettings))
+)(PageWithUser(QueueSettings, permissions))
diff --git a/src/socket/server.js b/src/socket/server.js
index 65dc0cf..ef57e96 100644
--- a/src/socket/server.js
+++ b/src/socket/server.js
@@ -5,7 +5,7 @@ const logger = require('../util/logger')
 const { sequelize, Question, User, ActiveStaff, Queue } = require('../models')
 const { getUserFromJwt, getAuthzForUser } = require('../auth/util')
 const {
-  canUserSeeQuestionDetailsForConfidentialQueue,
+  isUserStudent,
   filterConfidentialQueueQuestionsForUser,
 } = require('../api/util')
 
@@ -201,12 +201,9 @@ module.exports = newIo => {
           userAuthzPromise,
         ])
         const { courseId, isConfidential } = queue
-        const canSeeQuestionDetails = canUserSeeQuestionDetailsForConfidentialQueue(
-          userAuthz,
-          courseId
-        )
+        const isStudent = !isUserStudent(userAuthz, courseId)
         let sendCompleteQuestionData = true
-        if (isConfidential && !canSeeQuestionDetails) {
+        if (isConfidential && isStudent) {
           // All users that shouldn't see confidential information are added
           // to a "public" version of the room that receives the minimum
           // possible set of information


Test patch:
diff --git a/src/api/queues.test.js b/src/api/queues.test.js
index 925946c..0250e71 100644
--- a/src/api/queues.test.js
+++ b/src/api/queues.test.js
@@ -15,6 +15,16 @@ const expectErrorMessage = res => {
   expect(Object.keys(res.body)).toEqual(['message'])
 }
 
+const includesPrivateAttributes = queue => {
+  expect(queue.admissionControlEnabled).toBeDefined()
+  expect(queue.admissionControlUrl).toBeDefined()
+}
+
+const excludesPrivateAttributes = queue => {
+  expect(queue.admissionControlEnabled).toBeUndefined()
+  expect(queue.admissionControlUrl).toBeUndefined()
+}
+
 describe('Queues API', () => {
   describe('GET /api/queues', () => {
     const doGetTest = async user => {
@@ -28,6 +38,11 @@ describe('Queues API', () => {
       expect(res.body[1].location).toBe('There')
       expect(res.body[0].id).toBe(1)
       expect(res.body[1].id).toBe(2)
+
+      res.body.forEach(queue => {
+        // This endpoint shouldn't include private attributes
+        excludesPrivateAttributes(queue)
+      })
     }
     test('succeeds for admin', async () => {
       await doGetTest('admin')
@@ -51,6 +66,7 @@ describe('Queues API', () => {
       expect(res.body.questions).toHaveLength(0)
       expect(res.body).toHaveProperty('activeStaff')
       expect(res.body.activeStaff).toHaveLength(0)
+      includesPrivateAttributes(res.body)
 
       const question = { name: 'a', location: 'b', topic: 'c' }
       const res2 = await request.post('/api/queues/2/questions').send(question)
@@ -61,6 +77,7 @@ describe('Queues API', () => {
       expect(res3.body.questions).toHaveLength(1)
       expect(res3.body.questions[0]).toHaveProperty('askedBy')
       expect(res3.body.questions[0].askedBy.netid).toBe('admin')
+      includesPrivateAttributes(res3.body)
     })
 
     test('succeeds for non-admin', async () => {
@@ -76,6 +93,7 @@ describe('Queues API', () => {
       expect(res.body.questions).toHaveLength(0)
       expect(res.body).toHaveProperty('activeStaff')
       expect(res.body.activeStaff).toHaveLength(0)
+      excludesPrivateAttributes(res.body)
 
       const question = { name: 'a', location: 'b', topic: 'c' }
       const request2 = await requestAsUser(app, 'admin')
@@ -87,6 +105,7 @@ describe('Queues API', () => {
       expect(res3.body.questions).toHaveLength(1)
       expect(res3.body.questions[0]).toHaveProperty('askedBy')
       expect(res3.body.questions[0].askedBy.netid).toBe('admin')
+      excludesPrivateAttributes(res3.body)
     })
   })
 
@@ -102,6 +121,7 @@ describe('Queues API', () => {
       expect(question1).toHaveProperty('askedBy.netid', 'student')
       expect(question2).toHaveProperty('askedById', 6)
       expect(question2).toHaveProperty('askedBy.netid', 'otherstudent')
+      includesPrivateAttributes(res.body)
     }
 
     test('includes all question data for admin', async () => {
@@ -127,6 +147,7 @@ describe('Queues API', () => {
           expect(Object.keys(question)).toEqual(['id'])
         }
       })
+      excludesPrivateAttributes(res.body)
     }
 
     test('excludes question data for other course staff on confidential queue', async () => {
@@ -161,6 +182,8 @@ describe('Queues API', () => {
       expect(res.body.location).toBe('Where')
       expect(res.body.questionCount).toBe(0)
       expect(res.body.isConfidential).toBe(false)
+      expect(res.body.admissionControlEnabled).toBe(false)
+      expect(res.body.admissionControlUrl).toBe(null)
     })
 
     test('succeeds for course staff', async () => {
@@ -176,6 +199,8 @@ describe('Queues API', () => {
       expect(res.body.location).toBe('Where')
       expect(res.body.questionCount).toBe(0)
       expect(res.body.isConfidential).toBe(false)
+      expect(res.body.admissionControlEnabled).toBe(false)
+      expect(res.body.admissionControlUrl).toBe(null)
     })
 
     test('succeeds for confidential queue', async () => {
@@ -191,6 +216,8 @@ describe('Queues API', () => {
       expect(res.body.location).toBe('Where')
       expect(res.body.questionCount).toBe(0)
       expect(res.body.isConfidential).toBe(true)
+      expect(res.body.admissionControlEnabled).toBe(false)
+      expect(res.body.admissionControlUrl).toBe(null)
     })
 
     test('fails if name is missing', async () => {
@@ -312,6 +339,8 @@ describe('Queues API', () => {
       expect(res.body.name).toBe('CS 225 Queue 1 Alter')
       expect(res.body.id).toBe(1)
       expect(res.body.location).toBe('')
+      expect(res.body.admissionControlEnabled).toBe(false)
+      expect(res.body.admissionControlUrl).toBe(null)
     })
 
     test('succeeds for course staff with well-formed request with location', async () => {
@@ -322,6 +351,8 @@ describe('Queues API', () => {
       expect(res.body.name).toBe('CS 225 Queue 1 Alter')
       expect(res.body.id).toBe(1)
       expect(res.body.location).toBe('Where')
+      expect(res.body.admissionControlEnabled).toBe(false)
+      expect(res.body.admissionControlUrl).toBe(null)
     })
 
     test('succeeds for admin with well-formed request no location', async () => {
@@ -332,6 +363,8 @@ describe('Queues API', () => {
       expect(res.body.name).toBe('CS 225 Queue 1 Alter')
       expect(res.body.id).toBe(1)
       expect(res.body.location).toBe('')
+      expect(res.body.admissionControlEnabled).toBe(false)
+      expect(res.body.admissionControlUrl).toBe(null)
     })
 
     test('succeeds for admin with well-formed request with location', async () => {
@@ -342,6 +375,32 @@ describe('Queues API', () => {
       expect(res.body.name).toBe('CS 225 Queue 1 Alter')
       expect(res.body.id).toBe(1)
       expect(res.body.location).toBe('Where')
+      expect(res.body.admissionControlEnabled).toBe(false)
+      expect(res.body.admissionControlUrl).toBe(null)
+    })
+
+    test('succeeds for course staff with well-formed request with admissionControlUrl', async () => {
+      const attributes = { admissionControlUrl: 'http://localhost:3000' }
+      const request = await requestAsUser(app, '225staff')
+      const res = await request.patch('/api/queues/1').send(attributes)
+      expect(res.statusCode).toBe(201)
+      expect(res.body.name).toBe('CS225 Queue')
+      expect(res.body.id).toBe(1)
+      expect(res.body.location).toBe('Here')
+      expect(res.body.admissionControlEnabled).toBe(false)
+      expect(res.body.admissionControlUrl).toBe('http://localhost:3000')
+    })
+
+    test('succeeds for course staff with well-formed request with admissionControlEnabled', async () => {
+      const attributes = { admissionControlEnabled: true }
+      const request = await requestAsUser(app, '225staff')
+      const res = await request.patch('/api/queues/1').send(attributes)
+      expect(res.statusCode).toBe(201)
+      expect(res.body.name).toBe('CS225 Queue')
+      expect(res.body.id).toBe(1)
+      expect(res.body.location).toBe('Here')
+      expect(res.body.admissionControlEnabled).toBe(true)
+      expect(res.body.admissionControlUrl).toBe(null)
     })
 
     test('fails for course staff with ill-formed request no location', async () => {
diff --git a/src/api/util.test.js b/src/api/util.test.js
index 627386e..b9cb876 100644
--- a/src/api/util.test.js
+++ b/src/api/util.test.js
@@ -106,41 +106,32 @@ describe('Testing Utils', () => {
     })
   })
 
-  describe('canUserSeeQuestionDetailsForConfidentialQueue', () => {
-    test('returns true for an admin', () => {
+  describe('isUserStudent', () => {
+    test('returns false for an admin', () => {
       const userAuthz = {
         isAdmin: true,
         staffedCourseIds: [],
       }
-      const value = util.canUserSeeQuestionDetailsForConfidentialQueue(
-        userAuthz,
-        123
-      )
-      expect(value).toBeTruthy()
+      const value = util.isUserStudent(userAuthz, 123)
+      expect(value).toBeFalsy()
     })
 
-    test('returns true for course staff', () => {
+    test('returns false for course staff', () => {
       const userAuthz = {
         isAdmin: false,
         staffedCourseIds: [123],
       }
-      const value = util.canUserSeeQuestionDetailsForConfidentialQueue(
-        userAuthz,
-        123
-      )
-      expect(value).toBeTruthy()
+      const value = util.isUserStudent(userAuthz, 123)
+      expect(value).toBeFalsy()
     })
 
-    test('returns false for non-admin, non-course staff', () => {
+    test('returns true for non-admin, non-course staff', () => {
       const userAuthz = {
         isAdmin: false,
         staffedCourseIds: [],
       }
-      const value = util.canUserSeeQuestionDetailsForConfidentialQueue(
-        userAuthz,
-        123
-      )
-      expect(value).toBeFalsy()
+      const value = util.isUserStudent(userAuthz, 123)
+      expect(value).toBeTruthy()
     })
   })