jupyter / nbgrader

A system for assigning and grading notebooks
https://nbgrader.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
1.28k stars 316 forks source link

Why should the root of the exchange be writable by anyone? #1225

Open nthiery opened 4 years ago

nthiery commented 4 years ago

If the root directory of the exchange is not writable by all, nbgrader submit fails with an error Unwritable directory, please contact your instructor:. It seems to be a security hazard to actually have this directory writable by anyone (a malicious user could e.g. rename a course directory, making it unavailble to other users). And for the submission itself, the user only need write permissions on the inbound subdirectory.

Is there a use case? Shouldn't this be turned off by default?

perllaghu commented 4 years ago

Are you still musing on this? Would you still like an answer?

If you consider a class running notebooks as pythonic scripts through jupyterhub (none of this docker malarkie at all) - then each user has their own UID on the server (probably part of the same group, but definitely different users).... so for my user notebook (which is running as me) to write into the exchange outbound folder, I need to have write permission there (it's the exchange code in my system that's doing the writing, not some central thing). Of course, if 'outbounddoesn't exist, the code needs to create it, hence the write atexchange` level.

Likewise, if I'm an instructor, it's my notebook that needs to write [and create, if it's missing] to inbound (and feedback)

Hope that helps.

nthiery commented 4 years ago

If you consider a class running notebooks as pythonic scripts through jupyterhub (none of this docker malarkie at all) - then each user has their own UID on the server (probably part of the same group, but definitely different users).... so for my user notebook (which is running as me) to write into the exchange outbound folder, I need to have write permission there (it's the exchange code in my system that's doing the writing, not some central thing). Of course, if 'outbounddoesn't exist, the code needs to create it, hence the write atexchange` level.

Likewise, if I'm an instructor, it's my notebook that needs to write [and create, if it's missing] to inbound (and feedback)

Yes, the inbound/ and outbound/ definitely need special permissions. But why the exchange/ directory itself would need to be writable by anyone? This enables anyone to wreak havoc; e.g. rename exchange/MyCourse to exchange/OldCourse, create a new MyCourse directory and thereby get control over all future submissions.

rkdarst commented 4 years ago

Yeah, I've been operating with that check disabled (via the groupshared option) and things work fine. I think the reason it insists is an assumption that the sysadmin does not set things up or there aren't different uids for each user that are managed carefully, so wants everything open to make it "easy".

perllaghu commented 4 years ago

... because the directory structure (as given in exchange/submit.py) is

self.inbound_path = os.path.join(self.root, self.coursedir.course_id, 'inbound')

so it's exchange/{course_id}/inbound - and the student's notebook-server creates it if it doesn't exist..... which it needs to do, as release_assignment only makes the exchange/{course_id}/outbound, and not a matching inbound

nthiery commented 4 years ago

... because the directory structure (as given in exchange/submit.py) is

self.inbound_path = os.path.join(self.root, self.coursedir.course_id, 'inbound')

so it's exchange/{course_id}/inbound - and the student's notebook-server creates it if it doesn't exist..... which it needs to do, as release_assignment only makes the exchange/{course_id}/outbound, and not a matching inbound

Ok, thanks for the explanation!

In the current state:

What about:

(1) Not imposing write permissions for anyone on `exchange/`` (2) Have release_assignment create the inbound directory (if needed) at the same time it creates the outbound directory

perllaghu commented 4 years ago

Worth a try... fork the code & test it :)

jhamrick commented 4 years ago

I think the reason it insists is an assumption that the sysadmin does not set things up or there aren't different uids for each user that are managed carefully, so wants everything open to make it "easy".

Yes, that's the reason the root is writeable (this was something that @ellisonbg needed at CalPoly).

(1) Not imposing write permissions for anyone on `exchange/``

I think in principle this should be ok, as long as for example we can give group write access for instructors. Though this does still add a step that a sysadmin would have to take care of (adding instructors to the relevant group).

(2) Have release_assignment create the inbound directory (if needed) at the same time it creates the outbound directory

The danger with this is that if you add a student after the assignment has been released, they then wouldn't have an inbound folder. You could check for this when adding students but it seems like it could easily get a bit complicated to implement this.

rkdarst commented 4 years ago

(2) Have release_assignment create the inbound directory (if needed) at the same time it creates the outbound directory

The danger with this is that if you add a student after the assignment has been released, they then wouldn't have an inbound folder. You could check for this when adding students but it seems like it could easily get a bit complicated to implement this.

I interpreted this to mean "instructor makes inbound/ and sets it writeable but not listable by everyone", then students can keep making their own returned-assignment subdirs like they do now. It already does this and works pretty well.

... though maybe it only works if instructor tries to submit once first. This logic can be moved up to "create inbound/ when releasing the first assignment". (when checking before I think I realized this flaw in our setup, but don't remember exactly now.)