Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-38655

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

    Details

    • Testing Instructions:
      Hide
      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.
      Show
      Create an assignment and enable team submissions with online text. As a student make a submission to the assignment and include an image in the online text (via the TinyMCE editor). View the submission as an admin. Expected results: Admin can view the image submitted as part of the online submission.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            damyon 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 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 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 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
            aolley 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
            aolley 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 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 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
            kstokking 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
            kstokking 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
            aolley Adam Olley added a comment -

            Rebased my fix to relevant branches.

            Show
            aolley Adam Olley added a comment - Rebased my fix to relevant branches.
            Hide
            damyon 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 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
            aolley Adam Olley added a comment -

            Updated commit.

            Show
            aolley Adam Olley added a comment - Updated commit.
            Hide
            damyon 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 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 Damyon Wiese added a comment -

            Sending for integration.

            Show
            damyon Damyon Wiese added a comment - Sending for integration.
            Hide
            poltawski 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
            poltawski 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
            poltawski Dan Poltawski added a comment -

            Integrated to master, 25 and 24 - thanks Adam

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

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

            Show
            dmonllao David Monllaó added a comment - It passes, working as expected. Tested in 24, 25 and master.
            Hide
            dmonllao 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
            dmonllao 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
            dmonllao David Monllaó added a comment -

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

            Show
            dmonllao David Monllaó added a comment - I've reproduced it in last week's master so it is not related: MDL-42242
            Hide
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  11/Nov/13