[MDL-38837] Check for http or https for include in openbadges backpack support. - Moodle Tracker

Details

Type: Bug
Status: Closed
Priority: Major
Resolution: Fixed
Affects Version/s: 2.5
Fix Version/s: 2.5.2
Component/s: Badges
Labels:
- moodle.org
- triaged

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:

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.
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:
https://github.com/totara/openbadges
Pull Master Branch:
~~MDL-38837~~_dev
Pull Master Diff URL:
https://github.com/totara/openbadges/commit/2c9108611d2afd711d68bd7a951a25f952589a45

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

Attachments

Issue Links

Discovered while testing

MDL-40710 Redesign t/backpack

Closed

has a non-specific relationship to

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

Closed

Testing discovered

MDL-35073 Open Badges Integration

Closed

Activity

Ascending order - Click to sort in descending order

Hide

Permalink

Andrew Nicols added a comment - 12/Jul/13 1:16 PM

Just seen this issue affecting moodle.org

Show

Andrew Nicols added a comment - 12/Jul/13 1:16 PM Just seen this issue affecting moodle.org

Hide

Permalink

Simon Coggins added a comment - 12/Jul/13 8:06 PM

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 - 12/Jul/13 8:06 PM 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

Permalink

Yuliya Bozhko added a comment - 16/Jul/13 4:13 AM

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 - 16/Jul/13 4:13 AM 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

Permalink

Frédéric Massart added a comment - 17/Jul/13 8:56 AM

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

Show

Frédéric Massart added a comment - 17/Jul/13 8:56 AM 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

Permalink

Yuliya Bozhko added a comment - 17/Jul/13 9:54 PM

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 - 17/Jul/13 9:54 PM 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

Permalink

Yuliya Bozhko added a comment - 17/Jul/13 10:44 PM

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

Show

Yuliya Bozhko added a comment - 17/Jul/13 10:44 PM Oh, wait, I totally misunderstood your second point Never mind. Fixing things now

Hide

Permalink

Frédéric Massart added a comment - 18/Jul/13 1:15 AM

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 - 18/Jul/13 1:15 AM 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

Permalink

Dan Poltawski added a comment - 22/Jul/13 6:48 AM

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

Show

Dan Poltawski added a comment - 22/Jul/13 6:48 AM Linking to MDL-28484 , which I haven't fixed but would've helped with this.

Hide

Permalink

Dan Poltawski added a comment - 22/Jul/13 7:03 AM

Integrated to master and 25 - thanks Yuliya!

Show

Dan Poltawski added a comment - 22/Jul/13 7:03 AM Integrated to master and 25 - thanks Yuliya!

Hide

Permalink

Ankit Agarwal added a comment - 24/Jul/13 3:00 AM - 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 - 24/Jul/13 3:00 AM - 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

Permalink

Simon Coggins added a comment - 24/Jul/13 4:25 AM

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 - 24/Jul/13 4:25 AM 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

Permalink

Ankit Agarwal added a comment - 24/Jul/13 5:23 AM

Great, it has one more vote now

Show

Ankit Agarwal added a comment - 24/Jul/13 5:23 AM Great, it has one more vote now

Hide

Permalink

Damyon Wiese added a comment - 26/Jul/13 5:02 AM

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 - 26/Jul/13 5:02 AM Thanks again for another week of fixes, improvements and testing. These changes have been released to the world. Cheers, Damyon

People

Assignee:

Yuliya Bozhko

Reporter:

Damyon Wiese

Peer reviewer:

Frédéric Massart

Integrator:

Dan Poltawski

Tester:

Ankit Agarwal

Participants:

Andrew Nicols, Ankit Agarwal, Damyon Wiese, Dan Poltawski, Frédéric Massart, Simon Coggins, Yuliya Bozhko

Votes:

2 Vote for this issue

Watchers:

6 Start watching this issue

Dates

Created:

02/Apr/13 8:04 AM

Updated:

26/Jul/13 5:02 AM

Resolved:

26/Jul/13 5:02 AM

Fix Release Date:

9/Sep/13