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

We do not have a function to detect if we are being served on https

    Details

    • Testing Instructions:
      Hide
      1. Check that behat tests passed on nightly

      For each of these scenarios

      1. http site with no loginhttps
      2. https site with no loginhttps
      3. http site with loginhttps

      Complete these steps

      1. Use a captcha in mod/feedback
      2. Attempt to login
      3. Logout
      4. Change user settings
      5. Upload a file
      6. Insert a file that has already been uploaded
      7. Ensure images and previews work as expected
      Show
      Check that behat tests passed on nightly For each of these scenarios http site with no loginhttps https site with no loginhttps http site with loginhttps Complete these steps Use a captcha in mod/feedback Attempt to login Logout Change user settings Upload a file Insert a file that has already been uploaded Ensure images and previews work as expected
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_28_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-28484-master

      Description

      As we don't have a function, different places around Moodle are all doing a string comparison themselves.

        Gliffy Diagrams

          Issue Links

            Activity

            poltawski Dan Poltawski created issue -
            poltawski Dan Poltawski made changes -
            Field Original Value New Value
            Assignee moodle.com [ moodle.com ] Dan Poltawski [ poltawski ]
            poltawski Dan Poltawski made changes -
            Status Open [ 1 ] Development in progress [ 3 ]
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Petr,

            You are are the resident expert on these https issues, I was wondering what you thought about this?

            (I don't like the name of my moodlelib function and I also don't know if moodlelib is the right place for this).

            The use case is that in a plugin I want to embed some content from an external site - and convert the (external) url to https:// if moodle is served on https - to prevent 'insecure content' warnings in IE

            Show
            poltawski Dan Poltawski added a comment - Hi Petr, You are are the resident expert on these https issues, I was wondering what you thought about this? (I don't like the name of my moodlelib function and I also don't know if moodlelib is the right place for this). The use case is that in a plugin I want to embed some content from an external site - and convert the (external) url to https:// if moodle is served on https - to prevent 'insecure content' warnings in IE
            poltawski Dan Poltawski made changes -
            Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
            Peer reviewer skodak
            Hide
            poltawski Dan Poltawski added a comment -

            An example of a use for this is in MDL-28486

            Show
            poltawski Dan Poltawski added a comment - An example of a use for this is in MDL-28486
            poltawski Dan Poltawski made changes -
            Link This issue blocks MDL-28486 [ MDL-28486 ]
            Hide
            skodak Petr Skoda added a comment -

            Thinking aloud - hmm, there are two alternative locations:
            1/ setuplib.php - it verifies the HTTP_HOST and sets up the FULLME, ME and friends
            2/ $PAGE->https_required() does something related, the problem is that the PAGE should not be used everywhere (I am not really sure about filelib.php now)

            The linked MDL->28486 should probably use something like PAGE->is_https() because it should probably deal with loginhttps too in case somebody uses youtube on the login/profile page.

            Show
            skodak Petr Skoda added a comment - Thinking aloud - hmm, there are two alternative locations: 1/ setuplib.php - it verifies the HTTP_HOST and sets up the FULLME, ME and friends 2/ $PAGE->https_required() does something related, the problem is that the PAGE should not be used everywhere (I am not really sure about filelib.php now) The linked MDL->28486 should probably use something like PAGE->is_https() because it should probably deal with loginhttps too in case somebody uses youtube on the login/profile page.
            Hide
            skodak Petr Skoda added a comment -

            Hmm, we should really get rid of the text caching we have in format_text and pass the PAGE as filter parameter instead. The plugins that need access to db could cache only the necessary information more effectively. There is a bug for this somewhere, I can not find it now, sorry...

            Show
            skodak Petr Skoda added a comment - Hmm, we should really get rid of the text caching we have in format_text and pass the PAGE as filter parameter instead. The plugins that need access to db could cache only the necessary information more effectively. There is a bug for this somewhere, I can not find it now, sorry...
            salvetore Michael de Raadt made changes -
            Fix Version/s STABLE backlog [ 10463 ]
            Labels triaged
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Hello Dan,

            You might want to update all instances of https detection with added function.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Hello Dan, You might want to update all instances of https detection with added function.
            rajeshtaneja Rajesh Taneja made changes -
            Link This issue has a non-specific relationship to MDL-27364 [ MDL-27364 ]
            poltawski Dan Poltawski made changes -
            Status Waiting for peer review [ 10012 ] Development in progress [ 3 ]
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Petr:

            I'm looking at this again in combination with an updated patch for MDL-28486 (youtube/vimeo).

            I was pondering between:

            • lib/pagelib.php
            • lib/weblib.php
            • lib/setuplib.php

            With pagelib, I agree that it should be on a per-page basis. The difficulty here comes in making sure that we're not duplicating code or forgetting scenarios (e.g. checking that login, profile, and admin pages return the correct value based on loginhttps).

            I also considered using setuplib to determine the correct return value and adding a function to weblib – the checks for sslproxy and friends are all located in the initialise_fullme() function. It would be pretty simple to check the value of $rurl['scheme'] but it feels a touch wrong. Additionally, I'm not sure of the best place to store the value - $CFG feels wrong too.

            Have you got any thoughts on this?

            TIA,

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Petr: I'm looking at this again in combination with an updated patch for MDL-28486 (youtube/vimeo). I was pondering between: lib/pagelib.php lib/weblib.php lib/setuplib.php With pagelib, I agree that it should be on a per-page basis. The difficulty here comes in making sure that we're not duplicating code or forgetting scenarios (e.g. checking that login, profile, and admin pages return the correct value based on loginhttps). I also considered using setuplib to determine the correct return value and adding a function to weblib – the checks for sslproxy and friends are all located in the initialise_fullme() function. It would be pretty simple to check the value of $rurl ['scheme'] but it feels a touch wrong. Additionally, I'm not sure of the best place to store the value - $CFG feels wrong too. Have you got any thoughts on this? TIA, Andrew
            Hide
            skodak Petr Skoda added a comment -

            hi

            1/ it should go to weblib.php
            2/ I guess it should return true on loginhttps pages too

            Show
            skodak Petr Skoda added a comment - hi 1/ it should go to weblib.php 2/ I guess it should return true on loginhttps pages too
            poltawski Dan Poltawski made changes -
            Link This issue has been marked as being related by MDL-38837 [ MDL-38837 ]
            poltawski Dan Poltawski made changes -
            Fix Version/s BACKEND [ 12582 ]
            poltawski Dan Poltawski made changes -
            Affects Version/s 2.5.2 [ 12664 ]
            Hide
            tonybutler Tony Butler added a comment - - edited

            Are there any plans to implement this soon?

            We just fell foul of an instance of (!isset($_SERVER['HTTPS']) || $_SERVER['HTTPS'] != "on") ? 'http' : 'https'; in mod_lti, which of course failed to detect that our reverse proxies serve HTTPS, because our web servers themselves don't. Took quite a while to figure out why our external tool wasn't working properly.

            Ruslan has just found another 22 instances of $_SERVER['HTTPS'].

            Cheers,
            Tony

            Show
            tonybutler Tony Butler added a comment - - edited Are there any plans to implement this soon? We just fell foul of an instance of (!isset($_SERVER ['HTTPS'] ) || $_SERVER ['HTTPS'] != "on") ? 'http' : 'https'; in mod_lti, which of course failed to detect that our reverse proxies serve HTTPS, because our web servers themselves don't. Took quite a while to figure out why our external tool wasn't working properly. Ruslan has just found another 22 instances of $_SERVER ['HTTPS'] . Cheers, Tony
            tonybutler Tony Butler made changes -
            Affects Version/s 2.7.1 [ 13550 ]
            Affects Version/s 2.6.4 [ 13551 ]
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Tony,

            I'm not working on this t the moment - if you guys fancy taking this on it'd be great.

            Show
            poltawski Dan Poltawski added a comment - Hi Tony, I'm not working on this t the moment - if you guys fancy taking this on it'd be great.
            poltawski Dan Poltawski made changes -
            Assignee Dan Poltawski [ poltawski ]
            poltawski Dan Poltawski made changes -
            Status Development in progress [ 3 ] Open [ 1 ]
            Hide
            tonybutler Tony Butler added a comment - - edited

            I had a feeling that was coming Dan Poltawski. Yeah, I don't see any reason why not.

            So what's the best way forward? Is the method used in your patch still the HQ-recommended way to do it, or have things changed since then (or are they expected to)?

            Cheers,
            Tony

            Show
            tonybutler Tony Butler added a comment - - edited I had a feeling that was coming Dan Poltawski . Yeah, I don't see any reason why not. So what's the best way forward? Is the method used in your patch still the HQ-recommended way to do it, or have things changed since then (or are they expected to)? Cheers, Tony
            tonybutler Tony Butler made changes -
            Assignee Tony Butler [ tonybutler ]
            Hide
            poltawski Dan Poltawski added a comment -

            Actually, lets add John Okely here - as hes doing some related work in MDL-46269 - maybe this collides.

            Show
            poltawski Dan Poltawski added a comment - Actually, lets add John Okely here - as hes doing some related work in MDL-46269 - maybe this collides.
            poltawski Dan Poltawski made changes -
            Link This issue has a non-specific relationship to MDL-46269 [ MDL-46269 ]
            Hide
            tonybutler Tony Butler added a comment -

            I've made a start, hopefully along the right track. I've replaced all the instances of individual detection that I could find apart from those in external libraries, installlib.php and setuplib.php (which presumably can't access $CFG->wwwroot because it hasn't necessarily been set up yet?).

            Is there anything we can do for external libraries? Ruslan mentioned that it was possible to override $_SERVER['HTTPS'].

            Show
            tonybutler Tony Butler added a comment - I've made a start, hopefully along the right track. I've replaced all the instances of individual detection that I could find apart from those in external libraries, installlib.php and setuplib.php (which presumably can't access $CFG->wwwroot because it hasn't necessarily been set up yet?). Is there anything we can do for external libraries? Ruslan mentioned that it was possible to override $_SERVER ['HTTPS'] .
            Hide
            kabalin Ruslan Kabalin added a comment - - edited

            Overriding $_SERVER['HTTPS'] at the init stages should do the trick. I spotted some people are doing this to solve a problem when web server, located behind loadbalancer and ssl terminator, thinks it is http-only, while in fact the content is served using https. The common approach is to set "X-Forwarded-Proto" on loadbalancer to "https", and then track the header down in the application using the code similar to:

            if (isset($_SERVER['HTTP_X_FORWARDED_PROTO']) && $_SERVER['HTTP_X_FORWARDED_PROTO'] == 'https') {
              $_SERVER['HTTPS'] = 'On';
            }
            

            In Moodle case, however, we know beforehand whether site is served using http or https, so there is no need to use "X-Forwarded-Proto", we can just rely on $CFG->wwwroot content and override $_SERVER['HTTPS'] to inform contrib libraries.

            Show
            kabalin Ruslan Kabalin added a comment - - edited Overriding $_SERVER ['HTTPS'] at the init stages should do the trick. I spotted some people are doing this to solve a problem when web server, located behind loadbalancer and ssl terminator, thinks it is http-only, while in fact the content is served using https. The common approach is to set "X-Forwarded-Proto" on loadbalancer to "https", and then track the header down in the application using the code similar to: if (isset($_SERVER['HTTP_X_FORWARDED_PROTO']) && $_SERVER['HTTP_X_FORWARDED_PROTO'] == 'https') { $_SERVER['HTTPS'] = 'On'; } In Moodle case, however, we know beforehand whether site is served using http or https, so there is no need to use "X-Forwarded-Proto", we can just rely on $CFG->wwwroot content and override $_SERVER ['HTTPS'] to inform contrib libraries.
            skodak Petr Skoda made changes -
            Peer reviewer Petr Skoda [ skodak ]
            Hide
            johno John Okely added a comment -

            Thanks for the heads up Dan I suspect this will conflict with MDL-42834 a lot once it's done so good to know about it.

            When working on MDL-42834 I was quite surprised to find that https was always checked with a string comparison great to hear this will get cleaned up!

            Show
            johno John Okely added a comment - Thanks for the heads up Dan I suspect this will conflict with MDL-42834 a lot once it's done so good to know about it. When working on MDL-42834 I was quite surprised to find that https was always checked with a string comparison great to hear this will get cleaned up!
            Hide
            tonybutler Tony Butler added a comment -

            Requesting a peer review just to get some feedback on my approach to this.

            Show
            tonybutler Tony Butler added a comment - Requesting a peer review just to get some feedback on my approach to this.
            tonybutler Tony Butler made changes -
            Status Open [ 1 ] Waiting for peer review [ 10012 ]
            cibot CiBoT made changes -
            Labels patch triaged ci patch triaged
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-28484 using repository: https://github.com/lucisgit/moodle

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-28484 using repository: https://github.com/lucisgit/moodle master (branch: MDL-28484-master | CI Job ) Coding style problems found Testing instructions are missing. More information about this report
            johno John Okely made changes -
            Original Estimate 0 minutes [ 0 ]
            Remaining Estimate 0 minutes [ 0 ]
            Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
            Peer reviewer John Okely [ johno ]
            Hide
            johno John Okely added a comment - - edited

            Thanks Tony, this is great!

            Do we consider

            (!isset($_SERVER['HTTPS']) || $_SERVER['HTTPS'] != "on")

            to be identical to

            strpos($CFG->wwwroot, 'https:')

            in all cases. If not, is this functionality we are prepared to change?

            Believe it or not, httpswwwroot will always be the same as wwwroot except for a login page on a http site when loginhttps is enabled. In which case httpswwwroot will start with https:// and wwwroot will start with http://. So you can make this change:

            -return (strpos($CFG->wwwroot, 'https://') === 0);
            +return (strpos($CFG->httpswwwroot, 'https://') === 0);
            

            And replace all these calls, too. (and any other you can find)

            filter/mathjaxloader/filter.php:            if (strpos($CFG->httpswwwroot, 'https:') === 0) {
            lib/outputrequirementslib.php:        if (strpos($CFG->httpswwwroot, 'https:') === 0) {
            lib/outputcomponents.php:            if (strpos($CFG->httpswwwroot, 'https:') === 0) {
            lib/form/recaptcha.php:        if (!empty($attributes['https']) or strpos($CFG->httpswwwroot, 'https:') === 0) {
            user/profile.php:    if (strpos($CFG->httpswwwroot, 'https:') === 0) {
            

            Then when we get rid of login https the function can be put back to $CFG->wwwroot. As a side benefit, this will make the total code change for deprecating loginhttps simpler, thanks

            Show
            johno John Okely added a comment - - edited Thanks Tony, this is great! Do we consider (!isset($_SERVER['HTTPS']) || $_SERVER['HTTPS'] != "on") to be identical to strpos($CFG->wwwroot, 'https:') in all cases. If not, is this functionality we are prepared to change? Believe it or not, httpswwwroot will always be the same as wwwroot except for a login page on a http site when loginhttps is enabled. In which case httpswwwroot will start with https:// and wwwroot will start with http:// . So you can make this change: -return (strpos($CFG->wwwroot, 'https://') === 0); +return (strpos($CFG->httpswwwroot, 'https://') === 0); And replace all these calls, too. (and any other you can find) filter/mathjaxloader/filter.php: if (strpos($CFG->httpswwwroot, 'https:') === 0) { lib/outputrequirementslib.php: if (strpos($CFG->httpswwwroot, 'https:') === 0) { lib/outputcomponents.php: if (strpos($CFG->httpswwwroot, 'https:') === 0) { lib/form/recaptcha.php: if (!empty($attributes['https']) or strpos($CFG->httpswwwroot, 'https:') === 0) { user/profile.php: if (strpos($CFG->httpswwwroot, 'https:') === 0) { Then when we get rid of login https the function can be put back to $CFG->wwwroot. As a side benefit, this will make the total code change for deprecating loginhttps simpler, thanks
            johno John Okely made changes -
            Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
            johno John Okely made changes -
            Testing Instructions # Check that behat tests passed on nightly
            h4.For each of these scenarios
            # http site with no loginhttps
            # https site with no loginhttps
            # http site with loginhttps
            h4.Complete these steps
            # Use a captcha in mod/feedback
            # Attempt to login
            # Logout
            # Change user settings
            # Upload a file
            # Insert a file that has already been uploaded
            # Ensure images and previews work as expected
            cibot CiBoT made changes -
            Labels ci patch triaged patch triaged
            johno John Okely made changes -
            Testing Instructions # Check that behat tests passed on nightly
            h4.For each of these scenarios
            # http site with no loginhttps
            # https site with no loginhttps
            # http site with loginhttps
            h4.Complete these steps
            # Use a captcha in mod/feedback
            # Attempt to login
            # Logout
            # Change user settings
            # Upload a file
            # Insert a file that has already been uploaded
            # Ensure images and previews work as expected
            # Check that behat tests passed on nightly

            h4.For each of these scenarios
            # http site with no loginhttps
            # https site with no loginhttps
            # http site with loginhttps

            h4.Complete these steps
            # Use a captcha in mod/feedback
            # Attempt to login
            # Logout
            # Change user settings
            # Upload a file
            # Insert a file that has already been uploaded
            # Ensure images and previews work as expected
            Hide
            kabalin Ruslan Kabalin added a comment -

            John, I think we may consider (!isset($_SERVER['HTTPS']) || $_SERVER['HTTPS'] != "on") to be identical to strpos($CFG->wwwroot, 'https:')
            in all cases. But there are some external libraries that are using $_SERVER['HTTPS'], thus, in addition to the change in the patch I suggest updating $_SERVER['HTTPS'] early based on is_https function output.

            Show
            kabalin Ruslan Kabalin added a comment - John, I think we may consider (!isset($_SERVER ['HTTPS'] ) || $_SERVER ['HTTPS'] != "on") to be identical to strpos($CFG->wwwroot, 'https:') in all cases. But there are some external libraries that are using $_SERVER ['HTTPS'] , thus, in addition to the change in the patch I suggest updating $_SERVER ['HTTPS'] early based on is_https function output.
            Hide
            johno John Okely added a comment - - edited

            Ah great.

            Since $_SERVER is supposed to be populated by the web server I think changing it directly might be solving the wrong problem, although I'm no expert in this area. Also, I'm pretty sure that $_SERVER will reset itself upon each request.

            Show
            johno John Okely added a comment - - edited Ah great. Since $_SERVER is supposed to be populated by the web server I think changing it directly might be solving the wrong problem, although I'm no expert in this area. Also, I'm pretty sure that $_SERVER will reset itself upon each request.
            Hide
            tonybutler Tony Butler added a comment -

            Thanks for reviewing John, and for adding the testing instructions.

            I've updated the function as per your advice, and included the extra calls. I've also added Ruslan's $_SERVER['HTTPS'] override at the end of lib/setup.php (and it does seem to work). Thanks for that Ruslan.

            Cheers,
            Tony

            Show
            tonybutler Tony Butler added a comment - Thanks for reviewing John, and for adding the testing instructions. I've updated the function as per your advice, and included the extra calls. I've also added Ruslan's $_SERVER ['HTTPS'] override at the end of lib/setup.php (and it does seem to work). Thanks for that Ruslan. Cheers, Tony
            tonybutler Tony Butler made changes -
            Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
            cibot CiBoT made changes -
            Labels patch triaged ci patch triaged
            Hide
            cibot CiBoT added a comment -
            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-28484 using repository: https://github.com/lucisgit/moodle master (branch: MDL-28484-master | CI Job ) Coding style problems found More information about this report
            johno John Okely made changes -
            Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
            Hide
            johno John Okely added a comment -

            Thanks Tony. This looks good. If you could add anything to the testing instructions that needs testing that would be great, then it will be ready for integration review.

            Show
            johno John Okely added a comment - Thanks Tony. This looks good. If you could add anything to the testing instructions that needs testing that would be great, then it will be ready for integration review.
            johno John Okely made changes -
            Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
            cibot CiBoT made changes -
            Labels ci patch triaged patch triaged
            Hide
            tonybutler Tony Butler added a comment -

            Thanks John. It would be nice to add some way of testing the $_SERVER['HTTPS'] override, but it requires a fairly specific server environment (i.e. HTTP behind an HTTPS reverse proxy).

            Show
            tonybutler Tony Butler added a comment - Thanks John. It would be nice to add some way of testing the $_SERVER ['HTTPS'] override, but it requires a fairly specific server environment (i.e. HTTP behind an HTTPS reverse proxy).
            tonybutler Tony Butler made changes -
            Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
            cibot CiBoT made changes -
            Labels patch triaged ci patch triaged
            Hide
            cibot CiBoT added a comment -
            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-28484 using repository: https://github.com/lucisgit/moodle master (branch: MDL-28484-master | CI Job ) Coding style problems found More information about this report
            johno John Okely made changes -
            Status Waiting for peer review [ 10012 ] Waiting for integration review [ 10010 ]
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            cibot CiBoT made changes -
            Status Waiting for integration review [ 10010 ] Waiting for integration review [ 10010 ]
            Currently in integration Yes [ 10041 ]
            cibot CiBoT made changes -
            Labels ci patch triaged patch triaged
            cibot CiBoT made changes -
            Labels patch triaged ci patch triaged
            Hide
            tonybutler Tony Butler added a comment -

            Hi John,

            I've now tested the $_SERVER['HTTPS'] override in our production environment with the LTI tool that prompted me to take on this issue in the first place, and I can confirm that everything now works as expected.

            I've also rebased my diff branch to the latest master.

            Cheers,
            Tony

            Show
            tonybutler Tony Butler added a comment - Hi John, I've now tested the $_SERVER ['HTTPS'] override in our production environment with the LTI tool that prompted me to take on this issue in the first place, and I can confirm that everything now works as expected. I've also rebased my diff branch to the latest master. Cheers, Tony
            Hide
            skodak Petr Skoda added a comment -

            Hi, I believe the setup.php modification is incorrect - the _SERVER should be hacked only if sslproxy is enabled because without sslproxy it would not work anyway.

            Show
            skodak Petr Skoda added a comment - Hi, I believe the setup.php modification is incorrect - the _SERVER should be hacked only if sslproxy is enabled because without sslproxy it would not work anyway.
            Hide
            skodak Petr Skoda added a comment -

            Also the server global should not be probably modified in cli scripts.

            Show
            skodak Petr Skoda added a comment - Also the server global should not be probably modified in cli scripts.
            samhemelryk Sam Hemelryk made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator Sam Hemelryk [ samhemelryk ]
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Tony,

            First up thank you very much for working on this.

            I have just been speaking to Petr now about the two points that he noted and I am in agreement with both.
            _SERVER should only be hacked if sslproxy is enabled and its not a CLI script.
            On top of that while discussing it with him we identified that the _SERVER hack would perhaps be better located within initialise_fullme as it already has the required sslproxy handling and consolidating the ssl handling hacks would aid in us finding them in the future.

            Sending it back this week sorry so that these things can be looked into.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Tony, First up thank you very much for working on this. I have just been speaking to Petr now about the two points that he noted and I am in agreement with both. _SERVER should only be hacked if sslproxy is enabled and its not a CLI script. On top of that while discussing it with him we identified that the _SERVER hack would perhaps be better located within initialise_fullme as it already has the required sslproxy handling and consolidating the ssl handling hacks would aid in us finding them in the future. Sending it back this week sorry so that these things can be looked into. Cheers Sam
            samhemelryk Sam Hemelryk made changes -
            Status Integration review in progress [ 10004 ] Reopened [ 4 ]
            cibot CiBoT made changes -
            Labels ci patch triaged patch triaged
            cibot CiBoT made changes -
            Status Reopened [ 4 ] Reopened [ 4 ]
            Currently in integration Yes [ 10041 ]
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            tonybutler Tony Butler added a comment - - edited

            Thanks Sam, Petr. That makes a lot more sense.

            Change committed and rebased to latest master.

            Show
            tonybutler Tony Butler added a comment - - edited Thanks Sam, Petr. That makes a lot more sense. Change committed and rebased to latest master.
            tonybutler Tony Butler made changes -
            Labels patch triaged patch ready_for_integration triaged
            poltawski Dan Poltawski made changes -
            Status Reopened [ 4 ] Waiting for peer review [ 10012 ]
            poltawski Dan Poltawski made changes -
            Labels patch ready_for_integration triaged patch triaged
            cibot CiBoT made changes -
            Labels patch triaged ci patch triaged
            Hide
            cibot CiBoT added a comment -
            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-28484 using repository: https://github.com/lucisgit/moodle master (branch: MDL-28484-master | CI Job ) Coding style problems found More information about this report
            johno John Okely made changes -
            Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
            Hide
            johno John Okely added a comment -

            Looks good from what I can see, and the points from integration review have been addressed. Submitting for integration review.

            Show
            johno John Okely added a comment - Looks good from what I can see, and the points from integration review have been addressed. Submitting for integration review.
            johno John Okely made changes -
            Status Peer review in progress [ 10013 ] Waiting for integration review [ 10010 ]
            cibot CiBoT made changes -
            Status Waiting for integration review [ 10010 ] Waiting for integration review [ 10010 ]
            Integrator Sam Hemelryk [ samhemelryk ]
            Currently in integration Yes [ 10041 ]
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            cibot CiBoT made changes -
            Labels ci patch triaged patch triaged
            cibot CiBoT made changes -
            Labels patch triaged ci patch triaged
            damyon Damyon Wiese made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator Damyon Wiese [ damyon ]
            Hide
            damyon Damyon Wiese added a comment -

            Thanks for finishing this Tony,

            Integrated to master. I added one more commit to update some comments that were suggesting the old strpos method of https detection.

            Show
            damyon Damyon Wiese added a comment - Thanks for finishing this Tony, Integrated to master. I added one more commit to update some comments that were suggesting the old strpos method of https detection.
            damyon Damyon Wiese made changes -
            Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
            Fix Version/s 2.8 [ 13150 ]
            Fix Version/s STABLE backlog [ 10463 ]
            Fix Version/s BACKEND [ 12582 ]
            damyon Damyon Wiese made changes -
            Labels ci patch triaged api_change ci dev_docs_required patch triaged
            stronk7 Eloy Lafuente (stronk7) made changes -
            Tester CiBoT [ cibot ]
            damyon Damyon Wiese made changes -
            Tester CiBoT [ cibot ] Andrew Nicols [ dobedobedoh ]
            dobedobedoh Andrew Nicols made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Everything seems to be A-OK to me

            Thanks all,

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Everything seems to be A-OK to me Thanks all, Andrew
            dobedobedoh Andrew Nicols made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks for your hard work, this change is now part of Moodle 2.8-beta.

            Why do we never have time to do it right, but always have time to do it over?
            --anonymous

            Show
            poltawski Dan Poltawski added a comment - Thanks for your hard work, this change is now part of Moodle 2.8-beta. Why do we never have time to do it right, but always have time to do it over? --anonymous
            poltawski Dan Poltawski made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Currently in integration Yes [ 10041 ]
            Integration date 10/Oct/14
            Hide
            danielneis Daniel Neis added a comment -

            Hello,

            the change introduced here is causing problems to CAS authentication.
            Here at Universidade Federal de Santa Catarina, Brazil, we are using CFG->sslproxy with a HTTP server serving Moodle, both use Nginx and php-fpm.
            The happens at auth/cas/CAS/CAS/Client.php, when it tests the following:

            3503             if ( ($this->_isHttps() && $server_port!=443)
            3504                 || (!$this->_isHttps() && $server_port!=80)
            3505             ) {
            3506                 $server_url .= ':';
            3507                 $server_url .= $server_port;
            3508             }
            

            The "_isHttps()" returns true but the server_port is 80. Then it rewrites the URL adding the server_port value and it messes things. The final URL will be something like https://m28.moodle.ufsc.br:80/login

            Kind regards,
            Daniel

            Show
            danielneis Daniel Neis added a comment - Hello, the change introduced here is causing problems to CAS authentication. Here at Universidade Federal de Santa Catarina, Brazil, we are using CFG->sslproxy with a HTTP server serving Moodle, both use Nginx and php-fpm. The happens at auth/cas/CAS/CAS/Client.php, when it tests the following: 3503 if ( ($this->_isHttps() && $server_port!=443) 3504 || (!$this->_isHttps() && $server_port!=80) 3505 ) { 3506 $server_url .= ':'; 3507 $server_url .= $server_port; 3508 } The "_isHttps()" returns true but the server_port is 80. Then it rewrites the URL adding the server_port value and it messes things. The final URL will be something like https://m28.moodle.ufsc.br:80/login Kind regards, Daniel
            damyon Damyon Wiese made changes -
            Link This issue caused a regression MDL-48050 [ MDL-48050 ]
            Hide
            damyon Damyon Wiese added a comment -

            Thanks for reporting this Daniel,

            I created MDL-48050 as a regression from this issue.

            Show
            damyon Damyon Wiese added a comment - Thanks for reporting this Daniel, I created MDL-48050 as a regression from this issue.
            Subversion JIRA

            Links Hierarchy

             Documentation

            Invalid license: EXPIRED

              People

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

                Dates

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