Allow instructors to share files via shared group id

nbgrader/converters/base.py

@@ -46,7 +46,7 @@ class BaseConverter(LoggingConfigurable):
 
     @default("permissions")
     def _permissions_default(self):
-        return 444
+        return 664 if self.coursedir.groupshared else 444
 
     coursedir = Instance(CourseDirectory, allow_none=True)
 
@@ -249,6 +249,29 @@ def set_permissions(self, assignment_id, student_id):
         for dirname, _, filenames in os.walk(dest):
             for filename in filenames:
                 os.chmod(os.path.join(dirname, filename), permissions)
+            # If groupshared, set dir permissions - see comment below.
+            st_mode = os.stat(dirname).st_mode
+            if self.coursedir.groupshared and st_mode & 0o2770 != 0o2770:
+                try:
+                    os.chmod(dirname, (st_mode|0o2770) & 0o2777)
+                except PermissionError:
+                    self.log.warning("Could not update permissions of %s to make it groupshared", dirname)
+        # If groupshared, set write permissions on directories.  Directories
+        # are created within ipython_genutils.path.ensure_dir_exists via
+        # nbconvert.writer, (unless there are supplementary files) with a
+        # default mode of 755 and there is no way to pass the mode arguments
+        # all the way to there!  So we have to walk and fix.
+        if self.coursedir.groupshared:
+            # Root may be created in this step, and is not included above.
+            rootdir = self.coursedir.format_path(self._output_directory, '.', '.')
+            # Add 2770 to existing dir permissions (don't unconditionally override)
+            st_mode = os.stat(rootdir).st_mode
+            if st_mode & 0o2770 != 0o2770:
+                try:
+                    os.chmod(rootdir, (st_mode|0o2770) & 0o2777)
+                except PermissionError:
+                    self.log.warning("Could not update permissions of %s to make it groupshared", rootdir)
+
 
     def convert_single_notebook(self, notebook_filename):
         """Convert a single notebook.

nbgrader/converters/generate_assignment.py

@@ -42,7 +42,7 @@ class GenerateAssignment(BaseConverter):
 
     @default("permissions")
     def _permissions_default(self):
-        return 644
+        return 664 if self.coursedir.groupshared else 644
 
     @property
     def _input_directory(self):

nbgrader/converters/generate_feedback.py

@@ -36,7 +36,7 @@ def _exporter_class_default(self):
 
     @default("permissions")
     def _permissions_default(self):
-        return 644
+        return 664 if self.coursedir.groupshared else 644
 
     def _load_config(self, cfg, **kwargs):
         if 'Feedback' in cfg:

nbgrader/coursedir.py

@@ -4,7 +4,7 @@
 from textwrap import dedent
 
 from traitlets.config import LoggingConfigurable
-from traitlets import Integer, Unicode, List, default, validate, TraitError
+from traitlets import Integer, Bool, Unicode, List, default, validate, TraitError
 
 from .utils import full_split, parse_utc
 
@@ -219,6 +219,21 @@ def _db_url_default(self):
         )
     ).tag(config=True)
 
+    groupshared = Bool(
+        False,
+        help=dedent(
+            """
+            Make all instructor files group writeable (g+ws, default g+r only)
+            and exchange directories group readable/writeable (g+rws, default
+            g=nothing only ) by default.  This should only be used if you
+            carefully set the primary groups of your notebook servers and fully
+            understand the unix permission model.  This changes the default
+            permissions from 444 (unwriteable) to 664 (writeable), so that
+            other instructors are able to delete/overwrite files.
+            """
+        )
+    ).tag(config=True)
+
     @default("root")
     def _root_default(self):
         return os.getcwd()

nbgrader/exchange/exchange.py

@@ -126,12 +126,36 @@ def do_copy(self, src, dest, log=None):
                                                include=self.coursedir.include,
                                                max_file_size=self.coursedir.max_file_size,
                                                log=self.log))
+        # copytree copies access mode too - so we must add go+rw back to it if
+        # we are in groupshared.
+        if self.coursedir.groupshared:
+            for dirname, _, filenames in os.walk(dest):
+                # dirs become ug+rwx
+                st_mode = os.stat(dirname).st_mode
+                if st_mode & 0o2770 != 0o2770:
+                    try:
+                        os.chmod(dirname, (st_mode|0o2770) & 0o2777)
+                    except PermissionError:
+                        self.log.warning("Could not update permissions of %s to make it groupshared", dirname)
+
+                for filename in filenames:
+                    filename = os.path.join(dirname, filename)
+                    st_mode = os.stat(filename).st_mode
+                    if st_mode & 0o660 != 0o660:
+                        try:
+                            os.chmod(filename, (st_mode|0o660) & 0o777)
+                        except PermissionError:
+                            self.log.warning("Could not update permissions of %s to make it groupshared", filename)
 
     def start(self):
         if sys.platform == 'win32':
             self.fail("Sorry, the exchange is not available on Windows.")
 
-        self.ensure_root()
+        if not self.coursedir.groupshared:
+            # This just makes sure that directory is o+rwx.  In group shared
+            # case, it is up to admins to ensure that instructors can write
+            # there.
+            self.ensure_root()
         self.set_timestamp()
 
         self.init_src()
@@ -161,5 +185,5 @@ def ensure_directory(self, path, mode):
             # so we have to create and then chmod.
             os.chmod(path, mode)
         else:
-            if not self_owned(path):
+            if not self.coursedir.groupshared and not self_owned(path):
                 self.fail("You don't own the directory: {}".format(path))

nbgrader/exchange/release_assignment.py

@@ -32,7 +32,7 @@ def _load_config(self, cfg, **kwargs):
         super(ExchangeReleaseAssignment, self)._load_config(cfg, **kwargs)
 
     def ensure_root(self):
-        perms = S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IWGRP|S_IXGRP|S_IROTH|S_IWOTH|S_IXOTH
+        perms = S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IWGRP|S_IXGRP|S_IROTH|S_IWOTH|S_IXOTH|((S_IWGRP|S_ISGID) if self.coursedir.groupshared else 0)
 
         # if root doesn't exist, create it and set permissions
         if not os.path.exists(self.root):
@@ -77,19 +77,22 @@ def init_dest(self):
         self.inbound_path = os.path.join(self.course_path, 'inbound')
         self.dest_path = os.path.join(self.outbound_path, self.coursedir.assignment_id)
         # 0755
+        # groupshared: +2040
         self.ensure_directory(
             self.course_path,
-            S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH
+            S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH|((S_ISGID|S_IWGRP) if self.coursedir.groupshared else 0)
         )
         # 0755
+        # groupshared: +2040
         self.ensure_directory(
             self.outbound_path,
-            S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH
+            S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH|((S_ISGID|S_IWGRP) if self.coursedir.groupshared else 0)
         )
         # 0733 with set GID so student submission will have the instructors group
+        # groupshared: +0040
         self.ensure_directory(
             self.inbound_path,
-            S_ISGID|S_IRUSR|S_IWUSR|S_IXUSR|S_IWGRP|S_IXGRP|S_IWOTH|S_IXOTH
+            S_ISGID|S_IRUSR|S_IWUSR|S_IXUSR|S_IWGRP|S_IXGRP|S_IWOTH|S_IXOTH|(S_IRGRP if self.coursedir.groupshared else 0)
         )
 
     def copy_files(self):
@@ -108,6 +111,6 @@ def copy_files(self):
         self.do_copy(self.src_path, self.dest_path)
         self.set_perms(
             self.dest_path,
-            fileperms=(S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH),
-            dirperms=(S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH))
+            fileperms=(S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH|(S_IWGRP if self.coursedir.groupshared else 0)),
+            dirperms=(S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH|((S_ISGID|S_IWGRP) if self.coursedir.groupshared else 0)))
         self.log.info("Released as: {} {}".format(self.coursedir.course_id, self.coursedir.assignment_id))

nbgrader/exchange/release_feedback.py

@@ -1,7 +1,7 @@
 import os
 import shutil
 import glob
-from stat import S_IRUSR, S_IWUSR, S_IXUSR, S_IXGRP, S_IXOTH
+from stat import S_IRUSR, S_IWUSR, S_IXUSR, S_IRGRP, S_IWGRP, S_IXGRP, S_IXOTH, S_ISGID
 
 from .exchange import Exchange
 from ..utils import notebook_hash
@@ -22,7 +22,7 @@ def init_dest(self):
         # 0755
         self.ensure_directory(
             self.outbound_feedback_path,
-            S_IRUSR | S_IWUSR | S_IXUSR | S_IXGRP | S_IXOTH
+            S_IRUSR | S_IWUSR | S_IXUSR | S_IXGRP | S_IXOTH | ((S_IRGRP|S_IWGRP|S_ISGID) if self.coursedir.groupshared else 0)
         )
 
     def copy_files(self):

nbgrader/tests/apps/base.py

@@ -59,7 +59,11 @@ def _make_file(self, path, contents=""):
             fh.write(contents)
 
     def _get_permissions(self, filename):
-        return oct(os.stat(filename).st_mode)[-3:]
+        st_mode = os.stat(filename).st_mode
+        # If setgid is true, return four bytes.  For testing CourseDirectory.groupshared.
+        if st_mode & 0o2000:
+            return oct(st_mode)[-4:]
+        return oct(st_mode)[-3:]
 
     def _file_contents(self, path):
         with open(path, "r") as fh:

nbgrader/tests/apps/test_nbgrader_autograde.py

@@ -526,11 +526,14 @@ def test_skip_extra_notebooks(self, db, course_dir):
         assert os.path.isfile(join(course_dir, "autograded", "foo", "ps1", "p1.ipynb"))
         assert not os.path.isfile(join(course_dir, "autograded", "foo", "ps1", "p1 copy.ipynb"))
 
-    def test_permissions(self, course_dir):
+    @pytest.mark.parametrize("groupshared", [False, True])
+    def test_permissions(self, course_dir, groupshared):
         """Are permissions properly set?"""
         with open("nbgrader_config.py", "a") as fh:
             fh.write("""c.CourseDirectory.db_assignments = [dict(name='ps1', duedate='2015-02-02 14:58:23.948203 America/Los_Angeles')]\n""")
-            fh.write("""c.CourseDirectory.db_students = [dict(id="foo"), dict(id="bar")]""")
+            fh.write("""c.CourseDirectory.db_students = [dict(id="foo"), dict(id="bar")]\n""")
+            if groupshared:
+                fh.write("""c.CourseDirectory.groupshared = True\n""")
 
         self._empty_notebook(join(course_dir, "source", "ps1", "foo.ipynb"))
         self._make_file(join(course_dir, "source", "ps1", "foo.txt"), "foo")
@@ -540,10 +543,26 @@ def test_permissions(self, course_dir):
         self._make_file(join(course_dir, "source", "foo", "ps1", "foo.txt"), "foo")
         run_nbgrader(["autograde", "ps1"])
 
+        if not groupshared:
+            if sys.platform == 'win32':
+                perms = '444'
+            else:
+                perms = '444'
+        else:
+            if sys.platform == 'win32':
+                perms = '666'
+                dirperms = '777'
+            else:
+                perms = '664'
+                dirperms = '2775'
+
         assert os.path.isfile(join(course_dir, "autograded", "foo", "ps1", "foo.ipynb"))
         assert os.path.isfile(join(course_dir, "autograded", "foo", "ps1", "foo.txt"))
-        assert self._get_permissions(join(course_dir, "autograded", "foo", "ps1", "foo.ipynb")) == "444"
-        assert self._get_permissions(join(course_dir, "autograded", "foo", "ps1", "foo.txt")) == "444"
+        if groupshared:
+            # non-groupshared doesn't make guarantees about directory perms
+            assert self._get_permissions(join(course_dir, "autograded", "foo", "ps1")) == dirperms
+        assert self._get_permissions(join(course_dir, "autograded", "foo", "ps1", "foo.ipynb")) == perms
+        assert self._get_permissions(join(course_dir, "autograded", "foo", "ps1", "foo.txt")) == perms
 
     def test_custom_permissions(self, course_dir):
         """Are custom permissions properly set?"""

nbgrader/tests/apps/test_nbgrader_collect.py

@@ -1,5 +1,8 @@
 import os
 import time
+import pytest
+
+from os.path import join
 
 from .. import run_nbgrader
 from .base import BaseTestApp
@@ -133,3 +136,18 @@ def test_owner_check(self, exchange, course_dir, cache):
         # This warning can be disabled
         out = self._collect("--assignment=ps1", exchange, flags=["--ExchangeCollect.check_owner=False"])
         assert 'WARNING' not in out
+
+    @notwindows
+    @pytest.mark.parametrize("groupshared", [False, True])
+    def test_permissions(self, exchange, course_dir, cache, groupshared):
+        if groupshared:
+            with open("nbgrader_config.py", "a") as fh:
+                fh.write("""c.CourseDirectory.groupshared = True\n""")
+        self._release_and_fetch("ps1", exchange, course_dir)
+        self._submit("ps1", exchange, cache, flags=["--student=foobar_student",])
+
+        # By default, a warning is raised if the student id does not match the directory owner
+        self._collect("--assignment=ps1", exchange)
+        assert self._get_permissions(join(exchange, "abc101", "inbound")) == ("2733" if not groupshared else "2773")
+        assert self._get_permissions(join(course_dir, "submitted", "foobar_student", "ps1")) == ("777" if not groupshared else "2777")
+        assert self._get_permissions(join(course_dir, "submitted", "foobar_student", "ps1", "p1.ipynb")) == ("644" if not groupshared else "664")

nbgrader/tests/apps/test_nbgrader_generate_assignment.py

@@ -169,21 +169,36 @@ def test_force_f(self, course_dir):
         assert not os.path.isfile(join(course_dir, "release", "ps1", "foo.txt"))
         assert not os.path.isfile(join(course_dir, "release", "ps1", "blah.pyc"))
 
-    def test_permissions(self, course_dir):
+    @pytest.mark.parametrize("groupshared", [False, True])
+    def test_permissions(self, course_dir, groupshared):
         """Are permissions properly set?"""
         self._empty_notebook(join(course_dir, 'source', 'ps1', 'foo.ipynb'))
         self._make_file(join(course_dir, 'source', 'ps1', 'foo.txt'), 'foo')
         with open("nbgrader_config.py", "a") as fh:
             fh.write("""c.CourseDirectory.db_assignments = [dict(name="ps1")]\n""")
+            if groupshared:
+                fh.write("""c.CourseDirectory.groupshared = True\n""")
         run_nbgrader(["generate_assignment", "ps1"])
 
-        if sys.platform == 'win32':
-            perms = '666'
+        if not groupshared:
+            if sys.platform == 'win32':
+                perms = '666'
+            else:
+                perms = '644'
         else:
-            perms = '644'
+            if sys.platform == 'win32':
+                perms = '666'
+                dirperms = '777'
+            else:
+                perms = '664'
+                dirperms = '2775'
 
         assert os.path.isfile(join(course_dir, "release", "ps1", "foo.ipynb"))
         assert os.path.isfile(join(course_dir, "release", "ps1", "foo.txt"))
+        if groupshared:
+            # non-groupshared doesn't make guarantees about directory perms
+            assert self._get_permissions(join(course_dir, "release")) == dirperms
+            assert self._get_permissions(join(course_dir, "release", "ps1")) == dirperms
         assert self._get_permissions(join(course_dir, "release", "ps1", "foo.ipynb")) == perms
         assert self._get_permissions(join(course_dir, "release", "ps1", "foo.txt")) == perms
 

nbgrader/tests/apps/test_nbgrader_generate_feedback.py

@@ -1,5 +1,6 @@
 import os
 import sys
+import pytest
 from os.path import join, exists, isfile
 
 from ...utils import remove
@@ -171,24 +172,38 @@ def test_filter_notebook(self, db, course_dir):
         assert isfile(join(course_dir, "feedback", "foo", "ps1", "data", "bar.txt"))
         assert not isfile(join(course_dir, "feedback", "foo", "ps1", "blah.pyc"))
 
-    def test_permissions(self, course_dir):
+    @pytest.mark.parametrize("groupshared", [False, True])
+    def test_permissions(self, course_dir, groupshared):
         """Are permissions properly set?"""
         with open("nbgrader_config.py", "a") as fh:
             fh.write("""c.CourseDirectory.db_assignments = [dict(name="ps1")]\n""")
             fh.write("""c.CourseDirectory.db_students = [dict(id="foo")]\n""")
+            if groupshared:
+                fh.write("""c.CourseDirectory.groupshared = True\n""")
         self._empty_notebook(join(course_dir, "source", "ps1", "foo.ipynb"))
         run_nbgrader(["generate_assignment", "ps1"])
 
         self._empty_notebook(join(course_dir, "submitted", "foo", "ps1", "foo.ipynb"))
         run_nbgrader(["autograde", "ps1"])
         run_nbgrader(["generate_feedback", "ps1"])
 
-        if sys.platform == 'win32':
-            perms = '666'
+        if not groupshared:
+            if sys.platform == 'win32':
+                perms = '666'
+            else:
+                perms = '644'
         else:
-            perms = '644'
+            if sys.platform == 'win32':
+                perms = '666'
+                dirperms = '777'
+            else:
+                perms = '664'
+                dirperms = '2775'
 
         assert isfile(join(course_dir, "feedback", "foo", "ps1", "foo.html"))
+        if groupshared:
+            # non-groupshared doesn't guarantee anything about directory perms
+            assert self._get_permissions(join(course_dir, "feedback", "foo", "ps1")) == dirperms
         assert self._get_permissions(join(course_dir, "feedback", "foo", "ps1", "foo.html")) == perms
 
     def test_custom_permissions(self, course_dir):

nbgrader/tests/apps/test_nbgrader_releaseassignment.py

@@ -1,6 +1,7 @@
 import os
 import shutil
 import stat
+import pytest
 from os.path import join
 
 from .. import run_nbgrader
@@ -92,3 +93,14 @@ def test_exchange_bad_perms(self, exchange, course_dir):
         self._copy_file(join("files", "test.ipynb"), join(course_dir, "release", "ps1", "p1.ipynb"))
         self._release("--assignment=ps1", exchange)
         assert os.path.isfile(join(exchange, "abc101", "outbound", "ps1", "p1.ipynb"))
+
+    @notwindows
+    @pytest.mark.parametrize("groupshared", [False, True])
+    def test_permissions(self, exchange, course_dir, groupshared):
+        if groupshared:
+            with open("nbgrader_config.py", "a") as fh:
+                fh.write("""c.CourseDirectory.groupshared = True""")
+        self._copy_file(join("files", "test.ipynb"), join(course_dir, "release", "ps1", "p1.ipynb"))
+        self._release("--assignment=ps1", exchange)
+        assert self._get_permissions(join(exchange, "abc101", "outbound", "ps1")) == ("755" if not groupshared else "2775")
+        assert self._get_permissions(join(exchange, "abc101", "outbound", "ps1", "p1.ipynb")) == ("644" if not groupshared else "664")