Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.1.2
    • Fix Version/s: 2.2
    • Component/s: Web Services
    • Labels:
      None
    • Testing Instructions:
      Hide

      1- create a web service (enabled and authorised for a specific username)
      2- in the mysql/postgres 'external_services' table, set manually the SHORTNAME
      3- Enter in your browser: http://yourmoodle/login/token.php?service=SHORTNAME&username=USERNAME&password=PASSWORD (do not test with an admin user expect if asked)

      //none admin username who hasn't the moodle/webservice:createtoken capability
      a) you already created a token in the admin with an expired valid date => error message (Moodle will fail to find a token, and try to create a new one)
      b) you already created a token in the admin with a different ip address => error message (Moodle will fail to find a token, and try to create a new one)
      c) you already created a token in the admin but for a different service => error message (Moodle will fail to find a token, and try to create a new one)
      d) you already created a token in the admin (the service is enabled and authorised for everybody, correct valid dates, correct ip restrictions) => the token is returned

      //none admin username who has the moodle/webservice:createtoken capbility (excepted if mentioned)
      e) the user is an admin
      f) the user is not authorised on a restricted service => error message
      g) the user is authorised but has a expired valid date (manage service admin page, click on authorised user then on the user full name link) => error message
      h) the user is authorised but has a different ip address => error message
      i) the user has not the capability to create a token (moodle/webservice:createtoken) => error message
      j) the user never visited his security keys page (i.e. not token was previously generated) and everything is set up correctly => a token is returned
      k) you run j) a new time (so a token has been generated) => the same token is returned.
      l) the user has not the service required capability.

      Now you are going to use the 'moodle_mobile_app' shortname (the mobile service). Enable it first in the administration. Redo i), it should success if the user doesn't have the 'moodle/webservice:createtoken' capability. Mobile service does not require the capability but the 'moodle/webservice:createmobiletoken' capability.

      Show
      1- create a web service (enabled and authorised for a specific username) 2- in the mysql/postgres 'external_services' table, set manually the SHORTNAME 3- Enter in your browser: http://yourmoodle/login/token.php?service=SHORTNAME&username=USERNAME&password=PASSWORD (do not test with an admin user expect if asked) //none admin username who hasn't the moodle/webservice:createtoken capability a) you already created a token in the admin with an expired valid date => error message (Moodle will fail to find a token, and try to create a new one) b) you already created a token in the admin with a different ip address => error message (Moodle will fail to find a token, and try to create a new one) c) you already created a token in the admin but for a different service => error message (Moodle will fail to find a token, and try to create a new one) d) you already created a token in the admin (the service is enabled and authorised for everybody, correct valid dates, correct ip restrictions) => the token is returned //none admin username who has the moodle/webservice:createtoken capbility (excepted if mentioned) e) the user is an admin f) the user is not authorised on a restricted service => error message g) the user is authorised but has a expired valid date (manage service admin page, click on authorised user then on the user full name link) => error message h) the user is authorised but has a different ip address => error message i) the user has not the capability to create a token (moodle/webservice:createtoken) => error message j) the user never visited his security keys page (i.e. not token was previously generated) and everything is set up correctly => a token is returned k) you run j) a new time (so a token has been generated) => the same token is returned. l) the user has not the service required capability. Now you are going to use the 'moodle_mobile_app' shortname (the mobile service). Enable it first in the administration. Redo i), it should success if the user doesn't have the 'moodle/webservice:createtoken' capability. Mobile service does not require the capability but the 'moodle/webservice:createmobiletoken' capability.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
    • Rank:
      19051

      Description

      It would be good if the token.php was more flexible and allow other services to create a token on the fly (if capability 'createtoken' are respected). This would allow any client to retrieve tokens by this script.

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment -

          I pushed the patch on github, it's a big change that need quite lot of testing. Note that the UI modification will need to be resolved in another issue (service shortname should be editable)

          Show
          Jérôme Mouneyrac added a comment - I pushed the patch on github, it's a big change that need quite lot of testing. Note that the UI modification will need to be resolved in another issue (service shortname should be editable)
          Hide
          Jérôme Mouneyrac added a comment - - edited

          This patch fixes two issues:

          • code not respecting the fact that a service could have an ip restriction and valid until date (like token can have too)
          • this script should be called for any service, not only the mobile service.

          This patch does not fix:

          • the fact that we can not edit the shortname in the administration yet (MDL-29807)
          • the authentication part of the code (MDL-29583)
          • the issue related with exceptions that are thrown in clear (MDL-29498)
          Show
          Jérôme Mouneyrac added a comment - - edited This patch fixes two issues: code not respecting the fact that a service could have an ip restriction and valid until date (like token can have too) this script should be called for any service, not only the mobile service. This patch does not fix: the fact that we can not edit the shortname in the administration yet ( MDL-29807 ) the authentication part of the code ( MDL-29583 ) the issue related with exceptions that are thrown in clear ( MDL-29498 )
          Hide
          Jérôme Mouneyrac added a comment -

          Sam, if you have a bit of time to peer review this first issue related to /login/token.php, it would be great

          Show
          Jérôme Mouneyrac added a comment - Sam, if you have a bit of time to peer review this first issue related to /login/token.php, it would be great
          Hide
          Jérôme Mouneyrac added a comment -

          If Sam you are too busy, I put Apu as watcher, you can throw the peer review to him.

          Show
          Jérôme Mouneyrac added a comment - If Sam you are too busy, I put Apu as watcher, you can throw the peer review to him.
          Hide
          Jérôme Mouneyrac added a comment -

          I rebased. I also fixed another issue: we didn't check if the user is authorised. I went through all the testing steps and a bit more. I'm sending for integration peer reviewer are quite really busy.
          I'd like to have it integrated. Other issues about token.php depend of this fix being integrated or not. Thanks.

          Show
          Jérôme Mouneyrac added a comment - I rebased. I also fixed another issue: we didn't check if the user is authorised. I went through all the testing steps and a bit more. I'm sending for integration peer reviewer are quite really busy. I'd like to have it integrated. Other issues about token.php depend of this fix being integrated or not. Thanks.
          Hide
          Jérôme Mouneyrac added a comment -

          Note: this issue will be backported if accepted.

          Show
          Jérôme Mouneyrac added a comment - Note: this issue will be backported if accepted.
          Hide
          Sam Hemelryk added a comment -

          Hi Jerome,

          Certainly the changes are looking very good, much better checking!
          However there are a couple of things that need to be tidied up:

          1. line 87: invalidtimedtoken for an unmet capability check, surely this should be a different message.
          2. The checks for $unsettoken seem incorrect. In four tokens are returned and the very first one is unset then $unsettoken will set be set to true and no further tokens will be checked.
          3. If a external_service_users record has expired should it be deleted like we delete expired tokens?
          4. The checks when first checking a token that follow the restricted users check should be moved above it so that if they fail we don't' make unnecessary db queries and checks.
          5. In a similar way the checks if no tokens are found need to be moved inside the check to make sure the service is the mobile service. Otherwise the exceptions are likely to be out of order (you will get a user not allowed for service that isn't the mobile service).

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Jerome, Certainly the changes are looking very good, much better checking! However there are a couple of things that need to be tidied up: line 87: invalidtimedtoken for an unmet capability check, surely this should be a different message. The checks for $unsettoken seem incorrect. In four tokens are returned and the very first one is unset then $unsettoken will set be set to true and no further tokens will be checked. If a external_service_users record has expired should it be deleted like we delete expired tokens? The checks when first checking a token that follow the restricted users check should be moved above it so that if they fail we don't' make unnecessary db queries and checks. In a similar way the checks if no tokens are found need to be moved inside the check to make sure the service is the mobile service. Otherwise the exceptions are likely to be out of order (you will get a user not allowed for service that isn't the mobile service). Cheers Sam
          Hide
          Jérôme Mouneyrac added a comment -

          1- I fixed the wrong message.
          2- I rewrote the $unsettoken part.
          3- in webservice/lib.php/webservice_base_server::load_function_info(), you can see the user check is in the SQL. The user is not removed from the list of authorised user. It's a bit different from the token being invalid. If you want I can write an issue for that (but not sure it is).
          4- I moved all the blocks related to checking if a user is valid on a service above the other checks. It's now only one block.
          5- I guess it has been fixed with 4-. Note that the only difference between requesting a token for moodle mobile service and any other service is in the capabily check:

          if ( ($serviceshortname == MOODLE_OFFICIAL_MOBILE_SERVICE and has_capability('moodle/webservice:createmobiletoken', get_system_context()))
                          //Note: automatically token generation is not available to admin (they must create a token manually)
                          or (!is_siteadmin($user) && has_capability('moodle/webservice:createtoken', get_system_context()))) {
          

          Retesting and submitting for integration. Thanks for your review Sam.

          Show
          Jérôme Mouneyrac added a comment - 1- I fixed the wrong message. 2- I rewrote the $unsettoken part. 3- in webservice/lib.php/webservice_base_server::load_function_info(), you can see the user check is in the SQL. The user is not removed from the list of authorised user. It's a bit different from the token being invalid. If you want I can write an issue for that (but not sure it is). 4- I moved all the blocks related to checking if a user is valid on a service above the other checks. It's now only one block. 5- I guess it has been fixed with 4-. Note that the only difference between requesting a token for moodle mobile service and any other service is in the capabily check: if ( ($serviceshortname == MOODLE_OFFICIAL_MOBILE_SERVICE and has_capability('moodle/webservice:createmobiletoken', get_system_context())) //Note: automatically token generation is not available to admin (they must create a token manually) or (!is_siteadmin($user) && has_capability('moodle/webservice:createtoken', get_system_context()))) { Retesting and submitting for integration. Thanks for your review Sam.
          Hide
          Sam Hemelryk added a comment -

          Hi Jerome,

          The changes look absolutely spot on thanks!
          I was about to integrate this when I noticed just one more thing. With the changes now it doesn't look like the SQL in $tokensql needs to return any of the fields from external_services, and in fact doesn't need to join to external services as you have already fetched the service above now and have the id which could be directly referenced in the SQL as a param.
          Does that make sense?

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Jerome, The changes look absolutely spot on thanks! I was about to integrate this when I noticed just one more thing. With the changes now it doesn't look like the SQL in $tokensql needs to return any of the fields from external_services, and in fact doesn't need to join to external services as you have already fetched the service above now and have the id which could be directly referenced in the SQL as a param. Does that make sense? Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Hi Jerome,

          I've integrated this now however I will open a new issue to clean up the SQL statement selecting tokens to be done before this is considered for back-porting.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Jerome, I've integrated this now however I will open a new issue to clean up the SQL statement selecting tokens to be done before this is considered for back-porting. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Linking to MDL-29931 to review the SQL statement.

          Show
          Sam Hemelryk added a comment - Linking to MDL-29931 to review the SQL statement.
          Hide
          Aparup Banerjee added a comment -

          love this change! this works fine for me in both DBs.
          Thanks Jerome

          Show
          Aparup Banerjee added a comment - love this change! this works fine for me in both DBs. Thanks Jerome
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Done, your delicious hacks have been sent upstream, many thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Done, your delicious hacks have been sent upstream, many thanks! Closing as fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: