Moodle
  1. Moodle
  2. MDL-35130

Improper string/spacing in Shibboleth authentication page

    Details

    • Testing Instructions:
      Hide

      This can be tested without an actual Shibboleth setup, but please note that after following these steps you won't be able to log back in to the instance without manually disabling Shibboleth.

      1. (If not already a Shib instance) Turn on Shibboleth authentication. Make sure the alternative login URL is set in the main Authentication page (this should happen automatically and isn't part of this patch).
      2. Purge the site caches.
      3. Logout.
      4. Attempt to log back in. You should be taken to the alternate Shibboleth authentication page.
      5. Verify that the text correctly sets up a link to the Moodle Administrator email account.

      If you need to recover the system afterwards find the alternateloginurl setting in mdl_config and null it out.

      Show
      This can be tested without an actual Shibboleth setup, but please note that after following these steps you won't be able to log back in to the instance without manually disabling Shibboleth. 1. (If not already a Shib instance) Turn on Shibboleth authentication. Make sure the alternative login URL is set in the main Authentication page (this should happen automatically and isn't part of this patch). 2. Purge the site caches. 3. Logout. 4. Attempt to log back in. You should be taken to the alternate Shibboleth authentication page. 5. Verify that the text correctly sets up a link to the Moodle Administrator email account. If you need to recover the system afterwards find the alternateloginurl setting in mdl_config and null it out.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-35130-master
    • Rank:
      43759

      Description

      On the IDP selection page the link to notify the Moodle Administrator is improperly spaced so the white space between "contact the" and "Moodle Administrator" is linked. Additionally, the Moodle Administrator text is hard-coded.

        Activity

        Hide
        Michael de Raadt added a comment -

        Thanks for spotting that and providing a patch.

        Show
        Michael de Raadt added a comment - Thanks for spotting that and providing a patch.
        Hide
        Michael de Raadt added a comment -

        Hi, Charles.

        I was going to suggest using HTML_WRITER::link() as a means of rendering the anchor tag, but as all the rest of the content of the plugin is not using renderers, there is no point insisting on this.

        I was considering why you have shifted the tag to the string. This creates a larger change than the minimal removal of a single space character. Generally we avoid adding tags into strings as the can be confusing to translators (and prone to error). However, this could possibly benefit other languages by allowing the link to be translated into a different position in the sentence. So I'm happy either way there.

        It might be nice to elaborate on how to set up the Shibboleth authentication plugin in the testing instructions. Apart from turning it on, Are there any settings that need to change? How does one get to the authentication page?

        Apart from that, it's a pretty simple change. You could provide the fix for 2.2 and 2.3, although I suspect that little has changed in this code, so it should be safe to cherry-pick to other supported branches.

        I'll watch this issue and can push it up to integration after your response.

        Show
        Michael de Raadt added a comment - Hi, Charles. I was going to suggest using HTML_WRITER::link() as a means of rendering the anchor tag, but as all the rest of the content of the plugin is not using renderers, there is no point insisting on this. I was considering why you have shifted the tag to the string. This creates a larger change than the minimal removal of a single space character. Generally we avoid adding tags into strings as the can be confusing to translators (and prone to error). However, this could possibly benefit other languages by allowing the link to be translated into a different position in the sentence. So I'm happy either way there. It might be nice to elaborate on how to set up the Shibboleth authentication plugin in the testing instructions. Apart from turning it on, Are there any settings that need to change? How does one get to the authentication page? Apart from that, it's a pretty simple change. You could provide the fix for 2.2 and 2.3, although I suspect that little has changed in this code, so it should be safe to cherry-pick to other supported branches. I'll watch this issue and can push it up to integration after your response.
        Hide
        Charles Fulton added a comment -

        Thanks Michael. I considered keeping it simple but I didn't like keeping the untranslated string there. I've updated the testing instructions per your suggestions. This should also be good to cherry-pick to 2.2 and 2.3; the Shib pages are stable.

        Show
        Charles Fulton added a comment - Thanks Michael. I considered keeping it simple but I didn't like keeping the untranslated string there. I've updated the testing instructions per your suggestions. This should also be good to cherry-pick to 2.2 and 2.3; the Shib pages are stable.
        Hide
        Michael de Raadt added a comment -

        Sorry, I was waiting for something to happen, but obviously the thing needing to happen was me pushing this to integration.

        Show
        Michael de Raadt added a comment - Sorry, I was waiting for something to happen, but obviously the thing needing to happen was me pushing this to integration.
        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
        Dan Poltawski added a comment -

        Hi Charles,

        This looks ok, but i'm afraid we can't accept changing the language strings like that on the stable branches.

        The problem is that we need to allow the language packs to work properly in both 2.3.0 and 2.3.3 and if we change the behaviour of the string, part way through that wont be the case. If the french translators change to use the new placeholder, the string will be incorrect when installed on 2.3.0.

        So, the solution is to create a new string for this with the new behaviour.

        Show
        Dan Poltawski added a comment - Hi Charles, This looks ok, but i'm afraid we can't accept changing the language strings like that on the stable branches. The problem is that we need to allow the language packs to work properly in both 2.3.0 and 2.3.3 and if we change the behaviour of the string, part way through that wont be the case. If the french translators change to use the new placeholder, the string will be incorrect when installed on 2.3.0. So, the solution is to create a new string for this with the new behaviour.
        Hide
        CiBoT added a comment -

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

        Show
        CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
        Hide
        Charles Fulton added a comment -

        Thanks Dan; rewrote add a new string instead and rebased.

        Show
        Charles Fulton added a comment - Thanks Dan; rewrote add a new string instead and rebased.
        Hide
        Charles Fulton added a comment -

        Rebased against 2.4/2.5.

        Show
        Charles Fulton added a comment - Rebased against 2.4/2.5.
        Hide
        Dan Poltawski added a comment -

        Submitting for integration for Charles.

        [Sorry charles, feel free to shout louder to get it pushed back for integration!]

        Show
        Dan Poltawski added a comment - Submitting for integration for Charles. [Sorry charles, feel free to shout louder to get it pushed back for integration!]
        Hide
        Dan Poltawski added a comment -

        Thanks Charles, i've added a new commit on master to remove that old string too.

        Show
        Dan Poltawski added a comment - Thanks Charles, i've added a new commit on master to remove that old string too.
        Hide
        Frédéric Massart added a comment -

        Test passed, thanks!

        Show
        Frédéric Massart added a comment - Test passed, thanks!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        And your fantastic code has met core, hope they become good friends for a long period.

        Closing, thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - And your fantastic code has met core, hope they become good friends for a long period. Closing, thanks!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: