Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-22549 Assignment important issues
  3. MDL-22609

PROBLEM G - wrong id used in db in file upload assignment.

    Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Assignment (2.2)
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      Create one 1-file upload assignment.
      As student upload one file.

      Seems ok, but the "itemid" in the files table is the submission->userid. It should be the submission->id instead.

        Gliffy Diagrams

        1. 20100630-MDL-22609.patch
          6 kB
          Aparup Banerjee
        2. 20100701-MDL-22609.patch
          12 kB
          Aparup Banerjee
        3. MDL-22609.patch
          2 kB
          Aparup Banerjee

          Issue Links

            Activity

            Hide
            davosmith Davo Smith added a comment -

            I came across this bug whilst updating my assignment type uploadpdf to Moodle 2.0 (once again, using the built-in 'advanced uploading of files' assignment type as my model).

            I'm trying to understand why using the 'user id' rather than the 'submission id' is incorrect, so that I can make sure I get it right in my plugin.

            As far as I can tell, given the context id, either the 'user id' or the 'submission id' would uniquely identify the file storage area. Additionally, the submission can be retrieved by either id. What difference does this choice make?

            Show
            davosmith Davo Smith added a comment - I came across this bug whilst updating my assignment type uploadpdf to Moodle 2.0 (once again, using the built-in 'advanced uploading of files' assignment type as my model). I'm trying to understand why using the 'user id' rather than the 'submission id' is incorrect, so that I can make sure I get it right in my plugin. As far as I can tell, given the context id, either the 'user id' or the 'submission id' would uniquely identify the file storage area. Additionally, the submission can be retrieved by either id. What difference does this choice make?
            Hide
            nebgor Aparup Banerjee added a comment -

            i'm not sure why userid was used instead of the submission id.
            i think..
            It seems it was a little easier to use userid. Using submission id would seem to me to be more logically separating the file submissions. using userid allows for a clash when looking up the unique file submissions.. however i'm not sure if the clash will actually happen as lookups are giving back sufficiently unique responses for now.

            anyway i've created a patch that uses submission id instead. i wonder about regression issues with already existing file submission though. existing file submissions being referenced with userid may somehow need to be updated to use the submission id in mdl_files.

            Show
            nebgor Aparup Banerjee added a comment - i'm not sure why userid was used instead of the submission id. i think.. It seems it was a little easier to use userid. Using submission id would seem to me to be more logically separating the file submissions. using userid allows for a clash when looking up the unique file submissions.. however i'm not sure if the clash will actually happen as lookups are giving back sufficiently unique responses for now. anyway i've created a patch that uses submission id instead. i wonder about regression issues with already existing file submission though. existing file submissions being referenced with userid may somehow need to be updated to use the submission id in mdl_files.
            Hide
            dougiamas Martin Dougiamas added a comment -

            Submission is better because it's the "real" item in this module. It's possible an assignment type could allow multiple submissions per user, for example.

            Show
            dougiamas Martin Dougiamas added a comment - Submission is better because it's the "real" item in this module. It's possible an assignment type could allow multiple submissions per user, for example.
            Hide
            nebgor Aparup Banerjee added a comment -

            this patch addresses the itemid for files being submitted in assignment submissions.

            it also modifies the upgrade code to move moodle 1.9 assignment submissions files to moodle 2.0 with refereence to submissionid instead of userid as the itemid.

            i have however come upon the situation that a submission being graded/responded/fedback to where the feedback file is uploaded to the course_content file area. that file link does break after upgrading to 2.0 and trying to view the feedback file.

            Show
            nebgor Aparup Banerjee added a comment - this patch addresses the itemid for files being submitted in assignment submissions. it also modifies the upgrade code to move moodle 1.9 assignment submissions files to moodle 2.0 with refereence to submissionid instead of userid as the itemid. i have however come upon the situation that a submission being graded/responded/fedback to where the feedback file is uploaded to the course_content file area. that file link does break after upgrading to 2.0 and trying to view the feedback file.
            Hide
            nebgor Aparup Banerjee added a comment -

            patch now addresses this bug's issue in single upload as well as advanced (multi) upload assignment types. (as well as migrating the old 1.9 files)

            Show
            nebgor Aparup Banerjee added a comment - patch now addresses this bug's issue in single upload as well as advanced (multi) upload assignment types. (as well as migrating the old 1.9 files)
            Hide
            nebgor Aparup Banerjee added a comment -

            patch created for commit into HEAD.

            Show
            nebgor Aparup Banerjee added a comment - patch created for commit into HEAD.
            Hide
            skodak Petr Skoda added a comment -

            reopening
            1/ we should not crate dirs in assignment upgrade if they are not there - we can just "continue" with next submission
            2/ what is the point of random changing of names of variables that contain the path?

            Show
            skodak Petr Skoda added a comment - reopening 1/ we should not crate dirs in assignment upgrade if they are not there - we can just "continue" with next submission 2/ what is the point of random changing of names of variables that contain the path?
            Hide
            skodak Petr Skoda added a comment -

            oh, the directory creation there is completely bogus, reverting everything and changing just the userid-->submission id

            Show
            skodak Petr Skoda added a comment - oh, the directory creation there is completely bogus, reverting everything and changing just the userid-->submission id
            Hide
            skodak Petr Skoda added a comment -

            reclosing

            Show
            skodak Petr Skoda added a comment - reclosing
            Hide
            skodak Petr Skoda added a comment -

            oh, the userid-->submissionid was not changed everywhere, reopening once more and fixing

            Show
            skodak Petr Skoda added a comment - oh, the userid-->submissionid was not changed everywhere, reopening once more and fixing
            Hide
            skodak Petr Skoda added a comment -

            found a security issue in deleting of submission files in upload type, it allowed to delete files from any submission due to the fact that the newly introduced submissionid parameter was not verified at all

            Show
            skodak Petr Skoda added a comment - found a security issue in deleting of submission files in upload type, it allowed to delete files from any submission due to the fact that the newly introduced submissionid parameter was not verified at all
            Hide
            skodak Petr Skoda added a comment -

            oh, the submission record may not exist when we want to upload the response files, so it would make better sense to keep using the userid there, this will require a lot more work...

            Show
            skodak Petr Skoda added a comment - oh, the submission record may not exist when we want to upload the response files, so it would make better sense to keep using the userid there, this will require a lot more work...
            Hide
            nebgor Aparup Banerjee added a comment -

            hmm if the response is supposed to be related to an empty submission then can't we create an empty submission at that time?

            Show
            nebgor Aparup Banerjee added a comment - hmm if the response is supposed to be related to an empty submission then can't we create an empty submission at that time?
            Hide
            nebgor Aparup Banerjee added a comment -

            oh just noticed the deleting code was reverted so its broken now. will verify the submission id this time.

            Show
            nebgor Aparup Banerjee added a comment - oh just noticed the deleting code was reverted so its broken now. will verify the submission id this time.
            Hide
            nebgor Aparup Banerjee added a comment -

            MDL-22893 will affect the management of files here too... linking

            Show
            nebgor Aparup Banerjee added a comment - MDL-22893 will affect the management of files here too... linking
            Hide
            nebgor Aparup Banerjee added a comment -

            MDL-22893 fixed.
            using the filemanager and filepicker lements now to manager files.
            please test and update/resolve this bug as necessary.

            Show
            nebgor Aparup Banerjee added a comment - MDL-22893 fixed. using the filemanager and filepicker lements now to manager files. please test and update/resolve this bug as necessary.
            Hide
            davosmith Davo Smith added a comment -

            Following on from this change, there seem to be a number of calls to $this->count_user_files(), in mod/assignment/type/upload/assignment.class.php which pass in the parameter '$USER->id', rather than '$submission->id'. This means that this regularly gives the wrong answer (potentially allowing more/less files than the assignment limit, stopping the 'submit for marking' button from appearing, showing incorrect data in the navigation block).

            The function 'user_complete' (in mod/assignment/lib.php) is also broken, as it counts files based on the user id, rather than the submission id.

            Show
            davosmith Davo Smith added a comment - Following on from this change, there seem to be a number of calls to $this->count_user_files(), in mod/assignment/type/upload/assignment.class.php which pass in the parameter '$USER->id', rather than '$submission->id'. This means that this regularly gives the wrong answer (potentially allowing more/less files than the assignment limit, stopping the 'submit for marking' button from appearing, showing incorrect data in the navigation block). The function 'user_complete' (in mod/assignment/lib.php) is also broken, as it counts files based on the user id, rather than the submission id.
            Hide
            nebgor Aparup Banerjee added a comment -

            Thank you Davo, thats been fixed in head.

            Show
            nebgor Aparup Banerjee added a comment - Thank you Davo, thats been fixed in head.
            Hide
            davosmith Davo Smith added a comment -

            Hopefully the last one - 'view_upload_form' calls 'can_upload_file', which calls 'count_user_files' on '$submission->id', but nowhere in this chain is there a check to see if '$submission' is null, which results in a debugging error message at the point when a student views an assignment for which they have not submitted any work yet.

            Fix - change the line 904 of mod/assignment/type/upload/assignment.class.php to:

            and (empty($submission) or $this->count_user_files($submission->id) < $this->assignment->var1) // file limit not reached

            (adding the 'empty' check at the start of the line).

            Show
            davosmith Davo Smith added a comment - Hopefully the last one - 'view_upload_form' calls 'can_upload_file', which calls 'count_user_files' on '$submission->id', but nowhere in this chain is there a check to see if '$submission' is null, which results in a debugging error message at the point when a student views an assignment for which they have not submitted any work yet. Fix - change the line 904 of mod/assignment/type/upload/assignment.class.php to: and (empty($submission) or $this->count_user_files($submission->id) < $this->assignment->var1) // file limit not reached (adding the 'empty' check at the start of the line).
            Hide
            nebgor Aparup Banerjee added a comment -

            thanks, its fixed in head.

            Show
            nebgor Aparup Banerjee added a comment - thanks, its fixed in head.
            Hide
            nebgor Aparup Banerjee added a comment -

            the itemid change from userid to submission id has been done in head.

            Show
            nebgor Aparup Banerjee added a comment - the itemid change from userid to submission id has been done in head.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  24/Nov/10