Moodle
  1. Moodle
  2. MDL-30578

CMILongIdentifier and CMIIdentifier are not checked for valid URI (RFC 3986 [6]) and URN syntax

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.1, 2.3
    • Fix Version/s: 2.2.2
    • Component/s: SCORM
    • Testing Instructions:
      Hide

      It can be tested by running the SCORM Test Harness for DMI package - Data Model Implementation Test 1.
      1. Download the DMI Test package.
      2. Run the SCORM Test Harness for DMI as specified here - SCORM Test Harness
      3. Check the Log for the the errors listed in the description.

      NOTE: Many other errors are reported by this test - this patch only addresses the issues mentioned.

      Show
      It can be tested by running the SCORM Test Harness for DMI package - Data Model Implementation Test 1. 1. Download the DMI Test package . 2. Run the SCORM Test Harness for DMI as specified here - SCORM Test Harness 3. Check the Log for the the errors listed in the description. NOTE: Many other errors are reported by this test - this patch only addresses the issues mentioned.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
      master_MDL-30578

      Description

      We need to fix the regex expression for CMIIdentifier and CMILongIdentifier.
      The current allows - empty string - "" as a valid identifier and does not return a 406 erorr - which should not be the case, causing the following errors in Activity 1 DMI -

      ERROR: Evaluating SetValue("cmi.objectives.1.id", "") - Expected: "false" - LMS Returned: "true"
      ERROR: Evaluating GetLastError("") - Expected: "406" - LMS Returned: "0"
      Evaluating GetValue("cmi.objectives.1.id") - Expected: "" - LMS Returned: ""
      ERROR: Evaluating GetLastError("") - Expected: "301" - LMS Returned: "0"
      ERROR: Evaluating SetValue("cmi.objectives.1.id", "MYID01") - Expected: "true" - LMS Returned: "false"
      ERROR: Evaluating GetLastError("") - Expected: "0" - LMS Returned: "351"
      ERROR: Evaluating GetValue("cmi.objectives.1.id") - Expected: "MYID01" - LMS Returned: ""
      Evaluating GetLastError("") - Expected: "0" - LMS Returned: "0"
      ERROR: Evaluating SetValue("cmi.objectives.2.id", "MYID01") - Expected: "false" - LMS Returned: "true"
      ERROR: Evaluating GetLastError("") - Expected: "351" - LMS Returned: "0"
      ERROR: Evaluating GetValue("cmi.objectives.2.id") - Expected: "" - LMS Returned: "MYID01"
      ERROR: Evaluating GetLastError("") - Expected: "301" - LMS Returned: "0"
      ERROR: Evaluating GetValue("cmi.objectives.2.id") - Expected: "" - LMS Returned: "MYID01"
      ERROR: Evaluating GetLastError("") - Expected: "301" - LMS Returned: "0"

      We need to check for the string value when creating cmi.< >.n.id as per the format for a valid URI (RFC 3986 [6]) and URN syntax as per RFC 2141 [3] and return a 406 error code if the value is not in the proper format.
      It says in particular -
      1. The string should not contain all white spaces (and should not be an empty string).
      2. If the string starts with "urn:" it should have the full format like - "urn:something:SomethingOrSomethingElse"
      3. If it does not contain "urn:" at the start of string, then the string can contain any printable characters without any whitespace.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Mayank Gupta added a comment -

            This patch -

            https://github.com/mayankgupta/moodle/commit/11bc1c7c1efea0808f5dac12c0529ecc76710357

            updates the regex expressions of CMILongIdentifier and CMIIdentifier to check the string value for valid URI (RFC 3986 [6]) and URN syntax, thus fixing the issues in Activity 1 - DMI test package.

            After updating -

            Evaluating SetValue("cmi.objectives.1.id", "") - Expected: "false" - LMS Returned: "false"
            Evaluating GetLastError("") - Expected: "406" - LMS Returned: "406"
            Evaluating GetValue("cmi.objectives.1.id") - Expected: "" - LMS Returned: ""
            Evaluating GetLastError("") - Expected: "301" - LMS Returned: "301"
            Evaluating SetValue("cmi.objectives.1.id", "MYID01") - Expected: "true" - LMS Returned: "true"
            Evaluating GetLastError("") - Expected: "0" - LMS Returned: "0"
            Evaluating GetValue("cmi.objectives.1.id") - Expected: "MYID01" - LMS Returned: "MYID01"
            Evaluating GetLastError("") - Expected: "0" - LMS Returned: "0"
            Evaluating SetValue("cmi.objectives.2.id", "MYID01") - Expected: "false" - LMS Returned: "false"
            Evaluating GetLastError("") - Expected: "351" - LMS Returned: "351"
            Evaluating GetValue("cmi.objectives.2.id") - Expected: "" - LMS Returned: ""
            Evaluating GetLastError("") - Expected: "301" - LMS Returned: "301

            Thanks,
            Mayank Gupta.

            Show
            Mayank Gupta added a comment - This patch - https://github.com/mayankgupta/moodle/commit/11bc1c7c1efea0808f5dac12c0529ecc76710357 updates the regex expressions of CMILongIdentifier and CMIIdentifier to check the string value for valid URI (RFC 3986 [6] ) and URN syntax, thus fixing the issues in Activity 1 - DMI test package. After updating - Evaluating SetValue("cmi.objectives.1.id", "") - Expected: "false" - LMS Returned: "false" Evaluating GetLastError("") - Expected: "406" - LMS Returned: "406" Evaluating GetValue("cmi.objectives.1.id") - Expected: "" - LMS Returned: "" Evaluating GetLastError("") - Expected: "301" - LMS Returned: "301" Evaluating SetValue("cmi.objectives.1.id", "MYID01") - Expected: "true" - LMS Returned: "true" Evaluating GetLastError("") - Expected: "0" - LMS Returned: "0" Evaluating GetValue("cmi.objectives.1.id") - Expected: "MYID01" - LMS Returned: "MYID01" Evaluating GetLastError("") - Expected: "0" - LMS Returned: "0" Evaluating SetValue("cmi.objectives.2.id", "MYID01") - Expected: "false" - LMS Returned: "false" Evaluating GetLastError("") - Expected: "351" - LMS Returned: "351" Evaluating GetValue("cmi.objectives.2.id") - Expected: "" - LMS Returned: "" Evaluating GetLastError("") - Expected: "301" - LMS Returned: "301 Thanks, Mayank Gupta.
            Hide
            Dan Marsden added a comment -

            NOTE TO INTEGRATOR: This patch is for Master only - relates to SCORM 2004 which we don't "officially" support...yet...

            We have plans to set up an easy way for people to see the results of the Test harness against qa.moodle.net without having to set it up themselves - the start of this is in MDLSITE-1595

            Show
            Dan Marsden added a comment - NOTE TO INTEGRATOR: This patch is for Master only - relates to SCORM 2004 which we don't "officially" support...yet... We have plans to set up an easy way for people to see the results of the Test harness against qa.moodle.net without having to set it up themselves - the start of this is in MDLSITE-1595
            Hide
            Eloy Lafuente (stronk7) 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
            Eloy Lafuente (stronk7) 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
            Mayank Gupta added a comment -

            Thanks Eloy, I have updated the branch, should not cause any conflict issues now.

            Thanks,
            Mayank Gupta.

            Show
            Mayank Gupta added a comment - Thanks Eloy, I have updated the branch, should not cause any conflict issues now. Thanks, Mayank Gupta.
            Hide
            Aparup Banerjee added a comment - - edited

            Hi Mayank

            I've already mistakenly committed one of your merging commits into master in integration.git (3cf721b)
            We need to try to reduce these in our history ( the developer updating their branch isn't quiet relevant to moodle development history).

            As such what Eloy means in his well known message above is that we need to rebase rather than merge when we want to update our branches from moodle.git.

            http://docs.moodle.org/dev/Git_for_developers#Rebasing_a_branch is a reference i quickly googled

            Please rebase your branch and i can easily integrate this.

            PS: rebasing is annoying but at some level we need to do it. come onto developer chat and we can go thru it if you need any help

            Show
            Aparup Banerjee added a comment - - edited Hi Mayank I've already mistakenly committed one of your merging commits into master in integration.git (3cf721b) We need to try to reduce these in our history ( the developer updating their branch isn't quiet relevant to moodle development history). As such what Eloy means in his well known message above is that we need to rebase rather than merge when we want to update our branches from moodle.git. http://docs.moodle.org/dev/Git_for_developers#Rebasing_a_branch is a reference i quickly googled Please rebase your branch and i can easily integrate this. PS: rebasing is annoying but at some level we need to do it. come onto developer chat and we can go thru it if you need any help
            Hide
            Mayank Gupta added a comment -

            Hi Aparup,

            Apologies for getting back late and for causing the inconvenience.
            I could not infer the reason that the the update history of a local branch needs to be ignored, was just bent on the facts of rebasing after pushing and thus merged it.
            Apologies again!

            I have rebased the branch - https://github.com/mayankgupta/moodle/compare/master...master_MDL-30578

            Thanks and apologies!

            • Mayank.
            Show
            Mayank Gupta added a comment - Hi Aparup, Apologies for getting back late and for causing the inconvenience. I could not infer the reason that the the update history of a local branch needs to be ignored, was just bent on the facts of rebasing after pushing and thus merged it. Apologies again! I have rebased the branch - https://github.com/mayankgupta/moodle/compare/master...master_MDL-30578 Thanks and apologies! Mayank.
            Hide
            Aparup Banerjee added a comment -

            Thanks Mayank , pushing this on for integration

            Show
            Aparup Banerjee added a comment - Thanks Mayank , pushing this on for integration
            Hide
            Aparup Banerjee added a comment -

            Hi Mayank, thanks for rebasing, theres no conflicts now :-D

            that [-a-z-0-9] part of the regex, it seems there might be a typo in there with that range, could you confirm thats the regex you want? i'm guessing you want something like [a-zA-Z0-9] ?

            Show
            Aparup Banerjee added a comment - Hi Mayank, thanks for rebasing, theres no conflicts now :-D that [-a-z-0-9] part of the regex, it seems there might be a typo in there with that range, could you confirm thats the regex you want? i'm guessing you want something like [a-zA-Z0-9] ?
            Hide
            Eloy Lafuente (stronk7) added a comment -

            The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

            This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

            This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

            Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

            Show
            Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
            Hide
            Eloy Lafuente (stronk7) 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
            Eloy Lafuente (stronk7) 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
            Aparup Banerjee added a comment -

            ping Mayank / Dan ,
            Could really use feedback on the regular expression issue in this patch.

            Show
            Aparup Banerjee added a comment - ping Mayank / Dan , Could really use feedback on the regular expression issue in this patch.
            Hide
            Aparup Banerjee added a comment -

            Reopening this for Dan Marsden to have a look at the iffy regex.

            Show
            Aparup Banerjee added a comment - Reopening this for Dan Marsden to have a look at the iffy regex.
            Hide
            Dan Marsden added a comment -

            Thanks Apu.

            Mayank - Apu pointed out that this:
            [a-z0-9][-a-z-0-9]
            could probably be replaced by:
            [a-z0-9]

            unless we're both missing something?

            Show
            Dan Marsden added a comment - Thanks Apu. Mayank - Apu pointed out that this: [a-z0-9] [-a-z-0-9] could probably be replaced by: [a-z0-9] unless we're both missing something?
            Hide
            Mayank Gupta added a comment -

            Hi Dan and Apu,
            Please correct me if I am wrong - I guess we would need [A-Za-z0-9-] instead - to allow upper case, lower case and hyphens.
            A valid urn looks like this - urn:ADL:interaction-id-0001.

            I have not updated my code will do after your suggestions.

            and Thanks Apu for pointing this out!

            Thanks,
            Mayank.

            Show
            Mayank Gupta added a comment - Hi Dan and Apu, Please correct me if I am wrong - I guess we would need [A-Za-z0-9-] instead - to allow upper case, lower case and hyphens. A valid urn looks like this - urn:ADL:interaction-id-0001. I have not updated my code will do after your suggestions. and Thanks Apu for pointing this out! Thanks, Mayank.
            Hide
            Aparup Banerjee added a comment -

            np at all Mayank, really glad to see contributions from you

            ok to allow hyphens in the character class , the hypen has to be immediately after the '[' as in [-a-zA-Z0-9] (ref http://www.regular-expressions.info/reference.html) , which will get you upper case letter, lower case letter and the single hyphen. hm, the

            {1,4000} seems to be a count on '//S' which is the item in the expression.

            i'm not sure what the effects are if the hypens are elsewhere but it may be undesired.

            so after a little mucking around with the regex i end up with
            urn:[a-zA-Z0-9]{1,31}:[-a-zA-Z0-9\S]{1,4000}

            does that make sense?

            Show
            Aparup Banerjee added a comment - np at all Mayank, really glad to see contributions from you ok to allow hyphens in the character class , the hypen has to be immediately after the '[' as in [-a-zA-Z0-9] (ref http://www.regular-expressions.info/reference.html ) , which will get you upper case letter, lower case letter and the single hyphen. hm, the {1,4000} seems to be a count on '//S' which is the item in the expression. i'm not sure what the effects are if the hypens are elsewhere but it may be undesired. so after a little mucking around with the regex i end up with urn: [a-zA-Z0-9] {1,31}: [-a-zA-Z0-9\S] {1,4000} does that make sense?
            Hide
            Mayank Gupta added a comment - - edited

            I believe both are valid, the order where the hyphen is does not matter - [-a-zA-Z0-9] and [A-Za-z0-9-] are both the same.

            From the SCORM 4th edition RTE book -

            All URNs are required to have the following syntax (phrases in quotes are required):
            <URN> ::= “urn:”<NID>“:”<NSS>
            where <NID> is the Namespace Identifier and <NSS> is the Namespace Specific String

            Example: urn:ADL:interaction-id-0001

            But this is just a recommendation. This does not stop the content developers to use - a CMILongIdentifier such as -

            urn:aaaaaAAAA-AAAadaa09*w2:thisthisSomethingSOmethignSomethi2919ng-id-something8199*19

            I am not very sure about the difference between this -

             urn:[a-zA-Z0-9]{1,31}:[-a-zA-Z0-9\S]{1,4000} 

            and

             ^(?:(?!urn:)\\S{1,4000}|urn:[A-Za-z0-9-]{1,31}:\\S{1,4000})$ 

            Thanks,
            Mayank.

            Show
            Mayank Gupta added a comment - - edited I believe both are valid, the order where the hyphen is does not matter - [-a-zA-Z0-9] and [A-Za-z0-9-] are both the same. From the SCORM 4th edition RTE book - All URNs are required to have the following syntax (phrases in quotes are required): <URN> ::= “urn:”<NID>“:”<NSS> where <NID> is the Namespace Identifier and <NSS> is the Namespace Specific String Example: urn:ADL:interaction-id-0001 But this is just a recommendation. This does not stop the content developers to use - a CMILongIdentifier such as - urn:aaaaaAAAA-AAAadaa09*w2:thisthisSomethingSOmethignSomethi2919ng-id-something8199*19 I am not very sure about the difference between this - urn:[a-zA-Z0-9]{1,31}:[-a-zA-Z0-9\S]{1,4000} and ^(?:(?!urn:)\\S{1,4000}|urn:[A-Za-z0-9-]{1,31}:\\S{1,4000})$ Thanks, Mayank.
            Hide
            Aparup Banerjee added a comment -

            ah cool, yep you're right, dash first or last seems fine.

            Mayank, i'm starting to think that the second regex makes perfect sense now actually given the urn.

            ^(?:(?!urn:)\\S{1,4000}|urn:[A-Za-z0-9-]{1,31}:\\S{1,4000})$
            

            Show
            Aparup Banerjee added a comment - ah cool, yep you're right, dash first or last seems fine. Mayank, i'm starting to think that the second regex makes perfect sense now actually given the urn. ^(?:(?!urn:)\\S{1,4000}|urn:[A-Za-z0-9-]{1,31}:\\S{1,4000})$
            Hide
            Mayank Gupta added a comment -

            Dan any suggestions that you might have on this?

            Show
            Mayank Gupta added a comment - Dan any suggestions that you might have on this?
            Hide
            Dan Marsden added a comment -

            +1 for the 2nd one like Apu said - thanks Apu/Mayank!

            Show
            Dan Marsden added a comment - +1 for the 2nd one like Apu said - thanks Apu/Mayank!
            Hide
            Mayank Gupta added a comment -

            Thanks Dan

            I have updated the code.

            Show
            Mayank Gupta added a comment - Thanks Dan I have updated the code.
            Hide
            Aparup Banerjee added a comment -

            Thanks Mayank, submitting this for integration for you.

            Show
            Aparup Banerjee added a comment - Thanks Mayank, submitting this for integration for you.
            Hide
            Aparup Banerjee added a comment -

            Thanks guys, this has been integrated into 2.2.x and master now.

            Show
            Aparup Banerjee added a comment - Thanks guys, this has been integrated into 2.2.x and master now.
            Hide
            Adrian Greeve added a comment -

            I didn't come across any of the errors that this patch is trying to fix.
            Test passed.
            I didn't get through as much of the scorm test when I tested one of the other patches last year. It's nice to see progress being made

            Show
            Adrian Greeve added a comment - I didn't come across any of the errors that this patch is trying to fix. Test passed. I didn't get through as much of the scorm test when I tested one of the other patches last year. It's nice to see progress being made
            Hide
            Eloy Lafuente (stronk7) added a comment -

            This is now available in the git and cvs repositories.

            Consider the responsibility of your fingerprints engraved there for future generations!

            Thanks for the work, closing, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - This is now available in the git and cvs repositories. Consider the responsibility of your fingerprints engraved there for future generations! Thanks for the work, closing, ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: