Moodle
  1. Moodle
  2. MDL-38837

Check for http or https for include in openbadges backpack support.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5.2
    • Component/s: Badges
    • Testing Instructions:
      Hide

      This should be properly tested as I did not have required set up to do this myself. Testing requires web site to be accessible from the internet.

      When issuer.js from Mozilla Backpack is included, it should be now included through http or https, depending on your install. This can be checked through page source in the browser.

      If Mozilla backpack service is down, it should return a message advising user to try again later. This can be simulated by:

      1. Visiting the my badges page
      2. Opening the developer console and entering "OpenBadges = undefined;" without the quotes
      3. Clicking the "Download to backpack" icon

      For other testing:

      1. Make sure that you have backpack connection set up beforehand.
      2. Export of badges directly to the backpack can be done from two locations (mybadges.php page and issued badge page - badge.php). Try clicking "Add to backpack" from any of those two locations.
      3. If there are no problems with the Mozilla Backpack and user web site is accessible from the Internet, badge should be successfully added to the backpack.
      Show
      This should be properly tested as I did not have required set up to do this myself. Testing requires web site to be accessible from the internet. When issuer.js from Mozilla Backpack is included, it should be now included through http or https, depending on your install. This can be checked through page source in the browser. If Mozilla backpack service is down, it should return a message advising user to try again later. This can be simulated by: Visiting the my badges page Opening the developer console and entering "OpenBadges = undefined;" without the quotes Clicking the "Download to backpack" icon For other testing: Make sure that you have backpack connection set up beforehand. Export of badges directly to the backpack can be done from two locations (mybadges.php page and issued badge page - badge.php). Try clicking "Add to backpack" from any of those two locations. If there are no problems with the Mozilla Backpack and user web site is accessible from the Internet, badge should be successfully added to the backpack.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-38837_dev

      Description

      badges/badge.php

      contains js include for http://backpack.openbadges.org/issuer.js.

      This should be checking for http(s) and including the appropriate js to prevent mixed secure/insecure warnings.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Andrew Nicols added a comment -

            Just seen this issue affecting moodle.org

            Show
            Andrew Nicols added a comment - Just seen this issue affecting moodle.org
            Hide
            Simon Coggins added a comment -

            Yes, for me too. It is preventing directly pushing badges to your backpack, because issuer.js is being blocked as insecure content.

            Loading the page gives:

            [blocked] The page at https://moodle.org/badges/badge.php?hash=3281c2f5e7eba2c0b08264ce523047363c96509c ran insecure content from http://backpack.openbadges.org/issuer.js.

            Then clicking the "Download to backpack" button gives a JS error:

            Uncaught ReferenceError: OpenBadges is not defined

            Show
            Simon Coggins added a comment - Yes, for me too. It is preventing directly pushing badges to your backpack, because issuer.js is being blocked as insecure content. Loading the page gives: [blocked] The page at https://moodle.org/badges/badge.php?hash=3281c2f5e7eba2c0b08264ce523047363c96509c ran insecure content from http://backpack.openbadges.org/issuer.js . Then clicking the "Download to backpack" button gives a JS error: Uncaught ReferenceError: OpenBadges is not defined
            Hide
            Yuliya Bozhko added a comment -

            Submitted both 25 and master branch patches. Please let me know if there are any issues with this patch as I couldn't test it completely myself.

            Show
            Yuliya Bozhko added a comment - Submitted both 25 and master branch patches. Please let me know if there are any issues with this patch as I couldn't test it completely myself.
            Hide
            Frédéric Massart added a comment -

            Hi Yuliya,

            thanks for fixing this. Here are a few minor comments after peer reviewing your patch.

            1. Could you add some testing instructions to confirm that it is still possible to export to your Backpack (from all the possible locations)
            2. When using a YUI module, we often place the ID/Classes/selectors in a constant, so that they are easy to spot, update and re-use through the methods. Could you consider doing that for issued-badge-table and friends?
            3. As it seems that #backpack-error is specific to addtobackpack(), perhaps the ID should reflect that instead of being that generic. But that's not really a big deal. My only concern is that we are often just adding some IDs for JS but end up polluting the HTML, and re-using them in our CSS files, which creates dependencies, and the whole thing becomes a mess.
            4. Really minor, but strpos($CFG->wwwroot, 'https://') !== false could test against === 0, which is bulletproof.

            Feel free to push for integration whenever ready!

            Many thanks!
            Fred

            Show
            Frédéric Massart added a comment - Hi Yuliya, thanks for fixing this. Here are a few minor comments after peer reviewing your patch. Could you add some testing instructions to confirm that it is still possible to export to your Backpack (from all the possible locations) When using a YUI module, we often place the ID/Classes/selectors in a constant, so that they are easy to spot, update and re-use through the methods. Could you consider doing that for issued-badge-table and friends? As it seems that #backpack-error is specific to addtobackpack() , perhaps the ID should reflect that instead of being that generic. But that's not really a big deal. My only concern is that we are often just adding some IDs for JS but end up polluting the HTML, and re-using them in our CSS files, which creates dependencies, and the whole thing becomes a mess. Really minor, but strpos($CFG->wwwroot, 'https://') !== false could test against === 0, which is bulletproof. Feel free to push for integration whenever ready! Many thanks! Fred
            Hide
            Yuliya Bozhko added a comment -

            Hi Fred,

            1. Will do.
            2. I was actually going to create a new task to rewrite JS stuff as a module because it is kind of a separate issue.
            3. Will do.
            4. Make sense. I will correct that.

            Thanks! Please let me know what you think about #2 and I will get everything fixed today.

            Yuliya

            Show
            Yuliya Bozhko added a comment - Hi Fred, 1. Will do. 2. I was actually going to create a new task to rewrite JS stuff as a module because it is kind of a separate issue. 3. Will do. 4. Make sense. I will correct that. Thanks! Please let me know what you think about #2 and I will get everything fixed today. Yuliya
            Hide
            Yuliya Bozhko added a comment -

            Oh, wait, I totally misunderstood your second point Never mind. Fixing things now

            Show
            Yuliya Bozhko added a comment - Oh, wait, I totally misunderstood your second point Never mind. Fixing things now
            Hide
            Frédéric Massart added a comment -

            Hi Yuliya, moving this to a YUI module is certainly a good idea. I personally wouldn't mind the IDs to be places in constants at that moment. But if you found a way to emphasise them in the current patch that is even better!

            Show
            Frédéric Massart added a comment - Hi Yuliya, moving this to a YUI module is certainly a good idea. I personally wouldn't mind the IDs to be places in constants at that moment. But if you found a way to emphasise them in the current patch that is even better!
            Hide
            Dan Poltawski added a comment -

            Linking to MDL-28484, which I haven't fixed but would've helped with this.

            Show
            Dan Poltawski added a comment - Linking to MDL-28484 , which I haven't fixed but would've helped with this.
            Hide
            Dan Poltawski added a comment -

            Integrated to master and 25 - thanks Yuliya!

            Show
            Dan Poltawski added a comment - Integrated to master and 25 - thanks Yuliya!
            Hide
            Ankit Agarwal added a comment - - edited

            Everything works as described. I did see the message "There was a problem connecting to your backpack service provider. Please try again later.", following the instructions. However I found it a bit of surprise, when I connected my backpack no verification whatsoever was required. But since that is not a part of this issue. Am passing this.

            Also I find ngrok (https://ngrok.com/) very useful to test features that need callback. Posting it here, so it might be useful to someone else.

            Thanks

            Show
            Ankit Agarwal added a comment - - edited Everything works as described. I did see the message "There was a problem connecting to your backpack service provider. Please try again later.", following the instructions. However I found it a bit of surprise, when I connected my backpack no verification whatsoever was required. But since that is not a part of this issue. Am passing this. Also I find ngrok ( https://ngrok.com/ ) very useful to test features that need callback. Posting it here, so it might be useful to someone else. Thanks
            Hide
            Simon Coggins added a comment -

            Hi Ankit,

            Regarding verification, this is something that we are aware of and have been considering a solution for. I have filed a ticket with some further information here:

            https://tracker.moodle.org/browse/MDL-40848

            Simon

            Show
            Simon Coggins added a comment - Hi Ankit, Regarding verification, this is something that we are aware of and have been considering a solution for. I have filed a ticket with some further information here: https://tracker.moodle.org/browse/MDL-40848 Simon
            Hide
            Ankit Agarwal added a comment -

            Great, it has one more vote now

            Show
            Ankit Agarwal added a comment - Great, it has one more vote now
            Hide
            Damyon Wiese added a comment -

            Thanks again for another week of fixes, improvements and testing. These changes have been released to the world.

            Cheers, Damyon

            Show
            Damyon Wiese added a comment - Thanks again for another week of fixes, improvements and testing. These changes have been released to the world. Cheers, Damyon

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: