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

Feedback activity does not display CAPTCHA if site uses HTTPS

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.7, 2.5.3, 2.6
    • Fix Version/s: 2.5.5, 2.6.2
    • Component/s: Feedback
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      Prerequisites:

      Test:

      • Create a course and add an instance of the Feedback activity
      • Click the 'Edit questions' tab and add a 'Captcha'
      • Go back to the 'Overview' tab and click 'Answer the questions...'
      • Check that the CAPTCHA is displayed
      • Ideally, turn off HTTPS for the site and check that the CAPTCHA still displays over HTTP
      Show
      Prerequisites: A Moodle site that is configured to always use HTTPS A reCAPTCHA public and private key pair for the site's domain, created at https://www.google.com/recaptcha/admin/create Moodle configured to use the reCAPTCHA keys at https://your.moodle.site/admin/settings.php?section=manageauths Test: Create a course and add an instance of the Feedback activity Click the 'Edit questions' tab and add a 'Captcha' Go back to the 'Overview' tab and click 'Answer the questions...' Check that the CAPTCHA is displayed Ideally, turn off HTTPS for the site and check that the CAPTCHA still displays over HTTP
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-43632-master

      Description

      The CAPTCHA question type in the Feedback module does not display anything if the Moodle site is using HTTPS.

      The activity uses an implementation of reCAPTCHA, which requires the API request for the challenge to be made via HTTPS if the site is using HTTPS. However it is currently coded to always make the request via HTTP.

        Gliffy Diagrams

          Activity

          Hide
          tonybutler Tony Butler added a comment -

          Should I be using strpos($CFG->httpswwwroot, 'https:') === 0 instead of isset($_SERVER['HTTPS']) && !empty($_SERVER['HTTPS']) && $_SERVER['HTTPS'] == 'on'?

          Anyone have any views on this?

          Cheers,
          Tony

          Show
          tonybutler Tony Butler added a comment - Should I be using strpos($CFG->httpswwwroot, 'https:') === 0 instead of isset($_SERVER ['HTTPS'] ) && !empty($_SERVER ['HTTPS'] ) && $_SERVER ['HTTPS'] == 'on'? Anyone have any views on this? Cheers, Tony
          Hide
          abgreeve Adrian Greeve added a comment -

          Hello Tony,

          You should be using (strpos($CFG->wwwroot, 'https://') === 0
          $CFG->httpswwwroot will be disappearing in the future (to try and remove confusion.) There is an issue about creating a function for this sort of thing (MDL-28484) but this has yet to be integrated.

          Could you also please mention that you need to configure the reCAPTCHA public and private key in the pre-requisites section of your testing instructions?

          Besides that everything else looks good.

          Thanks for the fix.

          Show
          abgreeve Adrian Greeve added a comment - Hello Tony, You should be using (strpos($CFG->wwwroot, 'https://') === 0 $CFG->httpswwwroot will be disappearing in the future (to try and remove confusion.) There is an issue about creating a function for this sort of thing ( MDL-28484 ) but this has yet to be integrated. Could you also please mention that you need to configure the reCAPTCHA public and private key in the pre-requisites section of your testing instructions? Besides that everything else looks good. Thanks for the fix.
          Hide
          tonybutler Tony Butler added a comment -

          Thanks Adrian - I've updated the code to reflect this.

          I did make a mental note to include the reCAPTCHA key configuration and then promptly forgot to mention it.

          Show
          tonybutler Tony Butler added a comment - Thanks Adrian - I've updated the code to reflect this. I did make a mental note to include the reCAPTCHA key configuration and then promptly forgot to mention it.
          Hide
          grabs Andreas Grabs added a comment -

          Hi Tony,
          thank you very much for your work .
          Best regards
          Andreas

          Show
          grabs Andreas Grabs added a comment - Hi Tony, thank you very much for your work . Best regards Andreas
          Hide
          abgreeve Adrian Greeve added a comment -

          Thanks Tony,

          Submitting for integration.

          Show
          abgreeve Adrian Greeve added a comment - Thanks Tony, Submitting for integration.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          I'm assigning this to Damyon coz it seems he already has integrated it. Can you confirm, plz, that the fix has landed also to supported stables? Fill the versions appropriately. TIA!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - I'm assigning this to Damyon coz it seems he already has integrated it. Can you confirm, plz, that the fix has landed also to supported stables? Fill the versions appropriately. TIA!
          Hide
          damyon Damyon Wiese added a comment -

          Yes - (my bad). This looks correct and has been integrated to 25, 26 and master. (25 and 26 were cherry-picks from the master branch).

          It is better if we have separate git branches for the stables, because it implies the developer has tested the fix on stables before sending it to integration.

          Show
          damyon Damyon Wiese added a comment - Yes - (my bad). This looks correct and has been integrated to 25, 26 and master. (25 and 26 were cherry-picks from the master branch). It is better if we have separate git branches for the stables, because it implies the developer has tested the fix on stables before sending it to integration.
          Hide
          tonybutler Tony Butler added a comment -

          I've added diff URLs for the 2.5 and 2.6 backports and also rebased master.

          Show
          tonybutler Tony Butler added a comment - I've added diff URLs for the 2.5 and 2.6 backports and also rebased master.
          Hide
          dmonllao David Monllaó added a comment -

          Passes. Tested in 25, 26 and master

          Show
          dmonllao David Monllaó added a comment - Passes. Tested in 25, 26 and master
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          I won't be saying "Thanks!" this week. I'm tired of it.

          For the good (and the bad), your code is now part of Moodle, the best LMS in the world. Hope you are contributing for that to continue being a fact (and not the opposite), sincerely.

          Just closing this as fixed, ciao

          PS: Just a bit of black/cruel humor, sorry, LOL!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - I won't be saying "Thanks!" this week. I'm tired of it. For the good (and the bad), your code is now part of Moodle, the best LMS in the world. Hope you are contributing for that to continue being a fact (and not the opposite), sincerely. Just closing this as fixed, ciao PS: Just a bit of black/cruel humor, sorry, LOL!

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                10/Mar/14