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
    • Rank:
      33360

      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.

        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: