Moodle
  1. Moodle
  2. MDL-36674

Cannot edit my profile when HTTPS is turn on

    Details

    • Rank:
      46173

      Description

      Using $version = 2012062502.10 with "Use HTTPS for logins" checked;

      Well, actually it work only if I am in the front page and I go edit my profile, page is displayed correctly.
      If I'm in any course and I go to My Profile Setting >Edit Profile, I got the following error:

      [16-Nov-2012 22:20:45 UTC] Default exception handler: Coding error detected, it must be fixed by a programmer: out_as_local_url called on a non-local URL Debug: 
      Error code: codingerror
      * line 752 of \lib\weblib.php: coding_exception thrown
      * line 3564 of \lib\navigationlib.php: call to moodle_url->out_as_local_url()
      * line 3290 of \lib\navigationlib.php: call to settings_navigation->load_course_settings()
      * line 716 of \lib\pagelib.php: call to settings_navigation->initialise()
      * line 732 of \lib\pagelib.php: call to moodle_page->magic_get_settingsnav()
      * line 133 of \blocks\settings\block_settings.php: call to moodle_page->__get()
      * line 281 of \blocks\moodleblock.class.php: call to block_settings->get_content()
      * line 232 of \blocks\moodleblock.class.php: call to block_base->formatted_contents()
      * line 937 of \lib\blocklib.php: call to block_base->get_content_for_output()
      * line 989 of \lib\blocklib.php: call to block_manager->create_block_contents()
      * line 352 of \lib\blocklib.php: call to block_manager->ensure_content_created()
      * line 6 of \theme\uqat\layout\general.php: call to block_manager->region_has_content()
      * line 768 of \lib\outputrenderers.php: call to include()
      * line 715 of \lib\outputrenderers.php: call to core_renderer->render_page_layout()
      * line 299 of \user\editadvanced.php: call to core_renderer->header()
      

      in moodle_url->out_as_local_url, problematic lines are :

      #751       if (strpos($url, $CFG->wwwroot) !== 0) {
      #752           throw new coding_exception('out_as_local_url called on a non-local URL');
      

      I figures it have to do with my $CFG->wwwroot being http://moodle.uqat.ca while $url=https://moodle.uqat.ca/user/editadvanced.php?course=5&id=3387.

        Issue Links

          Activity

          Hide
          Rajesh Taneja added a comment -

          Thanks for reporting this.

          I've put that on the backlog and adding Petr as watcher for this issue.

          In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.

          Show
          Rajesh Taneja added a comment - Thanks for reporting this. I've put that on the backlog and adding Petr as watcher for this issue. In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.
          Hide
          Frédéric Massart added a comment - - edited

          Thanks Raj, your patch looks good and should prevent errors when HTTPS is enabled. I am just wondering if for readability it wouldn't be easier to have if/elseif/else. First checking the match of wwwroot, then httpswwwroot, then failing. But that's just a thought.

          On the other hand, that would be a good time to add some Unit Tests about this function in lib/tests/weblib_test.php.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - - edited Thanks Raj, your patch looks good and should prevent errors when HTTPS is enabled. I am just wondering if for readability it wouldn't be easier to have if/elseif/else. First checking the match of wwwroot, then httpswwwroot, then failing. But that's just a thought. On the other hand, that would be a good time to add some Unit Tests about this function in lib/tests/weblib_test.php. Cheers, Fred
          Hide
          Rajesh Taneja added a comment -

          Thanks for pointing this Fred,

          I have modified code to integrate your suggestions. Back for your review.

          Show
          Rajesh Taneja added a comment - Thanks for pointing this Fred, I have modified code to integrate your suggestions. Back for your review.
          Hide
          Frédéric Massart added a comment -

          Thanks Raj, could you justify in this, I am not sure myself, but if there is a reason for that then fine with me :

          • Is that really necessary to check the status of $CFG->loginhttps in out_as_local_url()? I wonder if that would create issues in case HTTPS is enabled globally, not just on the login page.
          • In the test that throw exceptions, why aren't you setting the httpswwwroot and loginhttps? I assume the first of those needs to know which is the local domain.

          Thanks, sorry for the back and forth! If this is irrelevant, please push for integration!

          Show
          Frédéric Massart added a comment - Thanks Raj, could you justify in this, I am not sure myself, but if there is a reason for that then fine with me : Is that really necessary to check the status of $CFG->loginhttps in out_as_local_url()? I wonder if that would create issues in case HTTPS is enabled globally, not just on the login page. In the test that throw exceptions, why aren't you setting the httpswwwroot and loginhttps? I assume the first of those needs to know which is the local domain. Thanks, sorry for the back and forth! If this is irrelevant, please push for integration!
          Hide
          Rajesh Taneja added a comment -

          Thanks Fred,

          $CFG->loginhttps ensures that string search for httpswwwroot should only be done if $CFG->loginhttps is set. Else it should not compare it. We change httpswwwroot protocol when $CFG->loginhttps is set.

          This was discussed with Dan and he suggested to use this. Hence, keeping it for now.

          Show
          Rajesh Taneja added a comment - Thanks Fred, $CFG->loginhttps ensures that string search for httpswwwroot should only be done if $CFG->loginhttps is set. Else it should not compare it. We change httpswwwroot protocol when $CFG->loginhttps is set. This was discussed with Dan and he suggested to use this. Hence, keeping it for now.
          Hide
          Dan Poltawski added a comment -

          Hi Raj,

          1. Could you add comments to the new if branches of out_as_local_url() as its not immediately obvious what is going on there if you are not familiar with the subtelties of loginhttps and httpswwwroot.
          2. You shouldn't need to 'reverse' your changes in the unit tests, phpunit will do it for you call '$this->resetAfterTest();' (and should detect wheny you haven't done it). From a code POV, I prefer doing this, although I know that a phpunit reset is kind of heavy. Please could you consult with Petr to see what he thinks would be the best approach.
          3. I think it could be preferable to do a search and replace for http > https for the $CFG>httpswwwroot change, the way you've written the test means that if anyone ever changes the unit test wwwroot, it'll break. Again, Petr might have an idea how we can simulate this loginhttps environment better.
          4. Regarding freds comment - My comment regarding loginhttps check was relating to us not really supporting two urls. Though now I look at fresh face of the code I don't see why we should be so strict, I wonder if we have a similar problem with $CFG->sslproxy = true?
          Show
          Dan Poltawski added a comment - Hi Raj, Could you add comments to the new if branches of out_as_local_url() as its not immediately obvious what is going on there if you are not familiar with the subtelties of loginhttps and httpswwwroot. You shouldn't need to 'reverse' your changes in the unit tests, phpunit will do it for you call '$this->resetAfterTest();' (and should detect wheny you haven't done it). From a code POV, I prefer doing this, although I know that a phpunit reset is kind of heavy. Please could you consult with Petr to see what he thinks would be the best approach. I think it could be preferable to do a search and replace for http > https for the $CFG >httpswwwroot change, the way you've written the test means that if anyone ever changes the unit test wwwroot, it'll break. Again, Petr might have an idea how we can simulate this loginhttps environment better. Regarding freds comment - My comment regarding loginhttps check was relating to us not really supporting two urls. Though now I look at fresh face of the code I don't see why we should be so strict, I wonder if we have a similar problem with $CFG->sslproxy = true?
          Hide
          Rajesh Taneja added a comment -

          Thanks Dan,

          Will wait for Petr's thought on unit test and will do the needful.

          Show
          Rajesh Taneja added a comment - Thanks Dan, Will wait for Petr's thought on unit test and will do the needful.
          Hide
          Petr Škoda added a comment - - edited

          hi, the patch is going to fail:
          1/ imagine you have http://example.com/1 and http://example.com/11 on the same server, you should

          if (strpos($url, $CFG->wwwroot.'/') === 0 or $url === $CFG->wwwroot) 

          2/ do not use string replace, what if there is the url itself in parameter - use substr() instead
          3/ you can not use httpswwwroot like that because it is different only on some pages, it should be easier to just replace the ^http: with https:, do not change the $CFG->httpswwwroot; in unit test

          Show
          Petr Škoda added a comment - - edited hi, the patch is going to fail: 1/ imagine you have http://example.com/1 and http://example.com/11 on the same server, you should if (strpos($url, $CFG->wwwroot.'/') === 0 or $url === $CFG->wwwroot) 2/ do not use string replace, what if there is the url itself in parameter - use substr() instead 3/ you can not use httpswwwroot like that because it is different only on some pages, it should be easier to just replace the ^http: with https:, do not change the $CFG->httpswwwroot; in unit test
          Hide
          Petr Škoda added a comment -

          It does not matter much if you undo the CFG changes or if you request reset in unit test.

          Show
          Petr Škoda added a comment - It does not matter much if you undo the CFG changes or if you request reset in unit test.
          Hide
          Rajesh Taneja added a comment -

          Thanks for your input Petr,

          I have updated the branch with your inputs.
          Pushing it for peer-review.

          Show
          Rajesh Taneja added a comment - Thanks for your input Petr, I have updated the branch with your inputs. Pushing it for peer-review.
          Hide
          Frédéric Massart added a comment -

          Thanks Raj, I noticed that substr() would return false when the URL equals wwwroot, it should probably be an empty string. I'd personally go with a preg_replace('^' . preg_quote($wwwroot) .... Same function could be used instead of str_replace(), but the probability of having 2 'http://' is null, so...

          Show
          Frédéric Massart added a comment - Thanks Raj, I noticed that substr() would return false when the URL equals wwwroot, it should probably be an empty string. I'd personally go with a preg_replace('^' . preg_quote($wwwroot) .... Same function could be used instead of str_replace(), but the probability of having 2 'http://' is null, so...
          Hide
          Rajesh Taneja added a comment -

          grrrrr...

          Thanks Fred, I have fixed this, and added related test case.
          I am sticking to substr as it's faster then preg_replace and for preg we have to add slashes to escape delimiter.
          Can you please review this again.

          Show
          Rajesh Taneja added a comment - grrrrr... Thanks Fred, I have fixed this, and added related test case. I am sticking to substr as it's faster then preg_replace and for preg we have to add slashes to escape delimiter. Can you please review this again.
          Hide
          Frédéric Massart added a comment -

          Looks good to me, pushing forward. Thanks Raj!

          Show
          Frédéric Massart added a comment - Looks good to me, pushing forward. Thanks Raj!
          Hide
          Petr Škoda added a comment -

          The logic looks ok now, thanks a lot.

          Show
          Petr Škoda added a comment - The logic looks ok now, thanks a lot.
          Hide
          Paul Nijbakker added a comment -

          We tested the patch and it appears to work as intended.

          Show
          Paul Nijbakker added a comment - We tested the patch and it appears to work as intended.
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Pavel Krejci added a comment -

          Hello, thanks for the fix. Tested Raj's patch on 2.4.1, looks ok, but TinyMCE javascript is linked with plain http which is problem for default display behavior in Chrome as it ignores non-https links in ssl secured pages and only shows small shield icon in url bar and that can be easily overlooked.

          Show
          Pavel Krejci added a comment - Hello, thanks for the fix. Tested Raj's patch on 2.4.1, looks ok, but TinyMCE javascript is linked with plain http which is problem for default display behavior in Chrome as it ignores non-https links in ssl secured pages and only shows small shield icon in url bar and that can be easily overlooked.
          Hide
          Rajesh Taneja added a comment -

          Thanks Pavel,

          AFAIK that is a separate issue, please feel free to open another bug for it.

          Show
          Rajesh Taneja added a comment - Thanks Pavel, AFAIK that is a separate issue, please feel free to open another bug for it.
          Hide
          Dan Poltawski added a comment -

          Integrated, to master, 24 and 23 thanks Raj!

          Show
          Dan Poltawski added a comment - Integrated, to master, 24 and 23 thanks Raj!
          Hide
          Jason Fowler added a comment -

          All working fine Raj

          Show
          Jason Fowler added a comment - All working fine Raj
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks! Closing as fixed, ciao

            People

            • Votes:
              13 Vote for this issue
              Watchers:
              22 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: