search

illinois / queue

A microservice queue for holding open office hours
University of Illinois/NCSA Open Source License
82 stars 36 forks source link

Improve security for queue settings #233

Closed james9909 closed 5 years ago

james9909 commented 5 years ago

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[bot] commented 5 years ago

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

nwalters512 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.