Improve security for queue settings #233

james9909 · 2019-03-14T01:53:30Z

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?

vercel · 2019-03-14T01:53:32Z

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

nwalters512

Looks good, can you add a changelog entry?

nwalters512 · 2019-03-14T21:47:53Z

Thinking about how you chose to exclude the queue props, I think it'd be better to exclude it from the default Sequelize scope 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 · 2019-03-14T22:07:33Z

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 · 2019-03-14T22:47:33Z

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 · 2019-03-14T23:11:11Z

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 so it's not particularly important in the "Get all open queues" endpoint

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. Is there another way to determine if a user is on course staff without querying the queue itself?

Something that I was considering to get around this while still implementing the scoping method is to have a separate "settings" endpoint that we'd only query in the settings page that will contain all that private information.

nwalters512 · 2019-03-14T23:22:52Z

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.

Sequelize v5 fixes an issue with scopes not properly merging attribute excludes when multiple scopes are in use. See sequelize/sequelize#9087

src/api/queues.js

nwalters512 · 2019-03-18T04:07:42Z

src/api/queues.js

-      return queue
-    })
+    const queuesResult = await Queue.scope(
+      'defaultScope',


Is specifying defaultScope explicitly required?

We need defaultScope so that the private attribute filtering gets applied, otherwise only questionCount will be used.

nwalters512 · 2019-03-18T04:09:34Z

src/api/questions.js

-      queueId,
-      askedById: askerId,
-    })
+    const question = await Question.create(


Can you clarify why there changes were necessary?

We expect that the object returned from Question.build contains the appropriate askedBy model through association, but it doesn't do that anymore.
The docs suggest to use .create to create with association.

Looking through the docs some more, it's possible to use .build() to achieve the same effect, but since we're not doing anything special between .build() and .save(), I don't see a reason to use it over .create()

nwalters512 · 2019-03-18T04:09:56Z

src/api/queues.js

@@ -113,13 +113,25 @@ router.get(
          },
          required: false,
          include: [User],
+          attributes: ['startTime', 'endTime'],


Can you clarify why the changes in this file were necessary?

I can't find the exact change from v5, but without attributes, sequelize will throw an error stating include.attributes.map is not a function.

nwalters512 · 2019-03-18T04:10:25Z

src/models/index.js

@@ -38,8 +38,6 @@ module.exports.initSequelize = sequelize => {
 }

 const sequelizeConfig = {
-  // See http://docs.sequelizejs.com/manual/querying.html#operators-security


Can you clarify why this change was necessary?

For security purposes, string-based operators ($or, $and, etc) have been deprecated and sequelize will now show deprecation warnings, even though we're not using string operators anywhere.
I was reading through sequelize/sequelize#8417 and it seems like the general consensus was to set operatorsAliases to false to completely disable string-based operators, but that will also trigger a warning as seen here.
Removing the option altogether has the same effect as setting it to false, but without the warning.

nwalters512

LGTM!

Only allow course staff to view the settings page

229dcd4

james9909 requested a review from nwalters512 March 14, 2019 01:53

nwalters512 reviewed Mar 14, 2019

View reviewed changes

james9909 added 2 commits March 14, 2019 14:39

Filter admission control properties for students

7fd261e

Update CHANGELOG

0e85f72

vercel bot deployed to staging March 14, 2019 19:41 View deployment

Merge branch 'master' into queue-settings-security

0e8dbbc

vercel bot deployed to staging March 14, 2019 21:45 View deployment

james9909 changed the title ~~Only allow course staff to view the settings page~~ Improve security for queue settings Mar 14, 2019

WIP: Use scopes

b46f45f

vercel bot deployed to staging March 15, 2019 20:57 View deployment

Upgrade sequelize to v5.1.0

638b5b1

Sequelize v5 fixes an issue with scopes not properly merging attribute excludes when multiple scopes are in use. See sequelize/sequelize#9087

vercel bot deployed to staging March 16, 2019 02:10 View deployment

Remove operatorsAliases

b4a9cd0

vercel bot deployed to staging March 16, 2019 02:15 View deployment

nwalters512 reviewed Mar 18, 2019

View reviewed changes

Remove unnecessary query

fc91573

vercel bot deployed to staging March 18, 2019 14:34 View deployment

nwalters512 approved these changes Mar 24, 2019

View reviewed changes

nwalters512 merged commit 8ba96a3 into master Mar 24, 2019

nwalters512 deleted the queue-settings-security branch March 24, 2019 21:49

This was referenced Mar 25, 2019

Course staff cannot access queue settings #237

Closed

Course staff page no longer shows staff name [regression] #238

Closed

james9909 mentioned this pull request Apr 2, 2019

Fix logic for filtering confidential questions #257

Merged

nwalters512 mentioned this pull request Apr 9, 2019

Rewrite queue page with hooks #263

Merged

james9909 mentioned this pull request Apr 14, 2019

Fix missing question attributes #268

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve security for queue settings #233

Improve security for queue settings #233

james9909 commented Mar 14, 2019

vercel bot commented Mar 14, 2019

nwalters512 left a comment

nwalters512 commented Mar 14, 2019

james9909 commented Mar 14, 2019

nwalters512 commented Mar 14, 2019

james9909 commented Mar 14, 2019 •

edited

Loading

nwalters512 commented Mar 14, 2019

nwalters512 Mar 18, 2019

james9909 Mar 18, 2019

nwalters512 Mar 18, 2019

james9909 Mar 18, 2019 •

edited

Loading

james9909 Mar 18, 2019

nwalters512 Mar 18, 2019

james9909 Mar 18, 2019

nwalters512 Mar 18, 2019

james9909 Mar 18, 2019 •

edited

Loading

nwalters512 left a comment

Improve security for queue settings #233

Improve security for queue settings #233

Conversation

james9909 commented Mar 14, 2019

vercel bot commented Mar 14, 2019

nwalters512 left a comment

Choose a reason for hiding this comment

nwalters512 commented Mar 14, 2019

james9909 commented Mar 14, 2019

nwalters512 commented Mar 14, 2019

james9909 commented Mar 14, 2019 • edited Loading

nwalters512 commented Mar 14, 2019

nwalters512 Mar 18, 2019

Choose a reason for hiding this comment

james9909 Mar 18, 2019

Choose a reason for hiding this comment

nwalters512 Mar 18, 2019

Choose a reason for hiding this comment

james9909 Mar 18, 2019 • edited Loading

Choose a reason for hiding this comment

james9909 Mar 18, 2019

Choose a reason for hiding this comment

nwalters512 Mar 18, 2019

Choose a reason for hiding this comment

james9909 Mar 18, 2019

Choose a reason for hiding this comment

nwalters512 Mar 18, 2019

Choose a reason for hiding this comment

james9909 Mar 18, 2019 • edited Loading

Choose a reason for hiding this comment

nwalters512 left a comment

Choose a reason for hiding this comment

james9909 commented Mar 14, 2019 •

edited

Loading

james9909 Mar 18, 2019 •

edited

Loading

james9909 Mar 18, 2019 •

edited

Loading