Moodle
  1. Moodle
  2. MDL-38655

Admin cannot view assignment submission file/image/comments when team submissions is enabled

    Details

    • Rank:
      48702

      Description

      When an assignment has team submissions enabled, the admin user can not view images in submitted as part of online text or download a file submitted.

      Reproduction steps:

      1. Create an assignment and enable team submissions with online text.
      2. As a student make a submission to the assignment and include an image in the online text (via the TinyMCE editor).
      3. View the submission as an admin.

      Expected results:

      • Admin can view the image submitted as part of the online submission.

      Actual results:

      • Admin can not view the image submitted as part of the online submission.

      This can also be reproduced using file submissions, which is how I originally found the issue. And while I haven't tested it is likely to affect submission comments as well because it uses the method where the issue is found.

      I believe the issue caused by the is_enrolled() check made in the assign class can_view_group_submission() method. The is_enrolled() call returns false for the admin, which in turn causes can_view_group_submission() to return false as well. can_view_group_submission() is used by assignsubmission_onlinetext_pluginfile() and assignsubmission_file_pluginfile().

      Possible solutions:

      • Move the has_capability() check for 'mod/assign:grade' above the is_enrolled() check.
      • Add a check for admins in the conditional with is_enrolled() (e.g. if (!$isadmin && !is_enrolled())

      If either of these sound good (or if even you'd like to go another route), I'd be glad to supply a patch or pull request. Just let me know.

        Issue Links

          Activity

          Hide
          Damyon Wiese added a comment -

          Note: Whatever change is done here must be applied equally to all permission checks in mod_assign - not just team assignment.

          Show
          Damyon Wiese added a comment - Note: Whatever change is done here must be applied equally to all permission checks in mod_assign - not just team assignment.
          Hide
          Damyon Wiese added a comment -

          Removing the is_enrolled check would allow suspended users to see the assignment. Will think about it and probably recommend the is_admin check.

          Show
          Damyon Wiese added a comment - Removing the is_enrolled check would allow suspended users to see the assignment. Will think about it and probably recommend the is_admin check.
          Hide
          Adam Olley added a comment - - edited

          is_admin() would be wrong. This would still fail for users that have a role in the course with mod/assign:grade at the course or higher. The correct way should be to return true if they have that capability. If you can grade the assignment, you should be allowed to view the submission.

          master:
          https://github.com/aolley/moodle/tree/MDL-38655
          https://github.com/aolley/moodle/commit/cb05904caeafc66bfe3371a9d6f22e3583d6ec6c

          24_STABLE
          https://github.com/aolley/moodle/tree/MDL-38655_24

          23_STABLE
          https://github.com/aolley/moodle/tree/MDL-38655_23

          Test case:
          Create a user that has a role with mod/assign:grade at the course or higher, but is /not/ enrolled in the course. The user should be able to download submissions.

          Show
          Adam Olley added a comment - - edited is_admin() would be wrong. This would still fail for users that have a role in the course with mod/assign:grade at the course or higher. The correct way should be to return true if they have that capability. If you can grade the assignment, you should be allowed to view the submission. master: https://github.com/aolley/moodle/tree/MDL-38655 https://github.com/aolley/moodle/commit/cb05904caeafc66bfe3371a9d6f22e3583d6ec6c 24_STABLE https://github.com/aolley/moodle/tree/MDL-38655_24 23_STABLE https://github.com/aolley/moodle/tree/MDL-38655_23 Test case: Create a user that has a role with mod/assign:grade at the course or higher, but is /not/ enrolled in the course. The user should be able to download submissions.
          Hide
          Damyon Wiese added a comment -

          This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

          For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

          Show
          Damyon Wiese added a comment - This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment
          Hide
          Kris Stokking added a comment -

          I'm escalating this to major - bugs such as this have a big impact on the supportability of Moodle as the admin role is typically used by an institution's support staff.

          Show
          Kris Stokking added a comment - I'm escalating this to major - bugs such as this have a big impact on the supportability of Moodle as the admin role is typically used by an institution's support staff.
          Hide
          Adam Olley added a comment -

          Rebased my fix to relevant branches.

          Show
          Adam Olley added a comment - Rebased my fix to relevant branches.
          Hide
          Damyon Wiese added a comment -

          Thanks for the patch Adam,

          It's not quite right though - as the failed unit tests with this patch show - this allows teachers to see suspended users regardless of 'moodle/course:viewsuspendedusers'.

          The is_active_user check needs to go before the mod/assign:grade check.

          Can you please add a fix for this?

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Thanks for the patch Adam, It's not quite right though - as the failed unit tests with this patch show - this allows teachers to see suspended users regardless of 'moodle/course:viewsuspendedusers'. The is_active_user check needs to go before the mod/assign:grade check. Can you please add a fix for this? Thanks, Damyon
          Hide
          Adam Olley added a comment -

          Updated commit.

          Show
          Adam Olley added a comment - Updated commit.
          Hide
          Damyon Wiese added a comment -

          Thanks for the update Adam.

          All looks good now.

          Notes for integrators:

          There are existing unit tests for this to catch regressions (25 and master).

          The suspended user check was added in MDL-40218 which is master only (because it would have changed behaviour on stables).

          Show
          Damyon Wiese added a comment - Thanks for the update Adam. All looks good now. Notes for integrators: There are existing unit tests for this to catch regressions (25 and master). The suspended user check was added in MDL-40218 which is master only (because it would have changed behaviour on stables).
          Hide
          Damyon Wiese added a comment -

          Sending for integration.

          Show
          Damyon Wiese added a comment - Sending for integration.
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          Integrated to master, 25 and 24 - thanks Adam

          Show
          Dan Poltawski added a comment - Integrated to master, 25 and 24 - thanks Adam
          Hide
          David Monllaó added a comment -

          It passes, working as expected. Tested in 24, 25 and master.

          Show
          David Monllaó added a comment - It passes, working as expected. Tested in 24, 25 and master.
          Hide
          David Monllaó added a comment -

          For the record, I've seen this debugging message when submitting an online text as student, using group submissions and online text enabled

          Before calling format_text(), the content must be processed with file_rewrite_pluginfile_urls()
          line 1255 of /lib/weblib.php: call to debugging()
          line 145 of /mod/assign/submission/onlinetext/locallib.php: call to format_text()
          line 5270 of /mod/assign/locallib.php: call to assign_submission_onlinetext->save()
          line 5364 of /mod/assign/locallib.php: call to assign->save_submission()
          line 384 of /mod/assign/locallib.php: call to assign->process_save_submission()
          line 53 of /mod/assign/view.php: call to assign->view()
          
          Show
          David Monllaó added a comment - For the record, I've seen this debugging message when submitting an online text as student, using group submissions and online text enabled Before calling format_text(), the content must be processed with file_rewrite_pluginfile_urls() line 1255 of /lib/weblib.php: call to debugging() line 145 of /mod/assign/submission/onlinetext/locallib.php: call to format_text() line 5270 of /mod/assign/locallib.php: call to assign_submission_onlinetext->save() line 5364 of /mod/assign/locallib.php: call to assign->save_submission() line 384 of /mod/assign/locallib.php: call to assign->process_save_submission() line 53 of /mod/assign/view.php: call to assign->view()
          Hide
          David Monllaó added a comment -

          I've reproduced it in last week's master so it is not related: MDL-42242

          Show
          David Monllaó added a comment - I've reproduced it in last week's master so it is not related: MDL-42242
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels.

          Or, if you prefer, yes, you fixed that boring issue.

          Thanks anyway! Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels. Or, if you prefer, yes, you fixed that boring issue. Thanks anyway! Ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: