Moodle
  1. Moodle
  2. MDL-40848

Improve verification when connecting to open badges backpack

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5.1
    • Fix Version/s: 2.5.3
    • Component/s: Badges
    • Labels:
    • Testing Instructions:
      Hide

      Connecting to a backpack

      1. Create an account at http://backpack.openbadges.org/backpack/login.
      2. Login to Moodle
      3. Click My profile settings > Badges > Backpack settings
      4. Click "sign in with your Email"
      5. Enter the email you used in step 1 and sign in.

      What should happen

      The Status should show "Connecting…" then reload and display "Connected". The email address you entered should be displayed under "Email address". Any badge collections in your backpack should be available to import.

      Testing error handling

      • Turn off javascript before visiting the "Backpack settings" page. You should get a message indicating javascript is required.
      • Close the login dialog before completing the login process. You should get an error message.
      • Create a persona account at persona.org but don't link it to a backpack. Then try and connect using that account. You should be able to login but should get an error when it tries to connect.
      • In badges/backpackconnect.php L57 change 'TIMEOUT' to 'TIMEOUT_MS' then try to login. You should get a message indicating your connection attempt failed due to a timeout.
      Show
      Connecting to a backpack Create an account at http://backpack.openbadges.org/backpack/login . Login to Moodle Click My profile settings > Badges > Backpack settings Click "sign in with your Email" Enter the email you used in step 1 and sign in. What should happen The Status should show "Connecting…" then reload and display "Connected". The email address you entered should be displayed under "Email address". Any badge collections in your backpack should be available to import. Testing error handling Turn off javascript before visiting the "Backpack settings" page. You should get a message indicating javascript is required. Close the login dialog before completing the login process. You should get an error message. Create a persona account at persona.org but don't link it to a backpack. Then try and connect using that account. You should be able to login but should get an error when it tries to connect. In badges/backpackconnect.php L57 change 'TIMEOUT' to 'TIMEOUT_MS' then try to login. You should get a message indicating your connection attempt failed due to a timeout.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-40848_master

      Description

      Currently when a user connects to a backpack they only need to provide the email address of a backpack owner and the badges will be pulled in and displayed in their profile. There is no confirmation that the backpack belongs to the user.

      To an extent this is a limitation of the badges infrastructure, and since users may use a different email address in their backpack to their Moodle email there is no easy way to check that the user owns the backpack.

      I posted to the open badges forums about it here:

      https://groups.google.com/forum/#!topic/openbadges-dev/fHFvEQCXyYU

      Francois from Mozilla suggested we could validate them using Persona and I even implemented a proof of concept that does work, but it would require the user to sign up for Persona, and have javascript and 3rd party cookies enabled for it to function.

      On the other hand pushing a badge to a backpack already requires all of those things so perhaps it is acceptable?

        Gliffy Diagrams

        1. master_MDL-40848_v1.patch
          265 kB
          Simon Coggins
        2. master_MDL-40848_v2.patch
          265 kB
          Simon Coggins
        3. MDL-40848_proof_of_concept.patch
          3 kB
          Simon Coggins
        1. persona_sign_in_black.png
          3 kB
        2. persona login UI.png
          102 kB

          Issue Links

            Activity

            Hide
            Dan Poltawski added a comment - - edited

            On the other hand pushing a badge to a backpack already requires all of those things so perhaps it is acceptable?

            Considering that the only backpack we support at the moment is mozilla's one (right?). Then that seems to make complete sense to me. In fact, what are your arguments that we shouldn't do it!

            I think that Fred mentioned to me that you are also thinking of making clearer where badges have come from in the interface too? That I think could help.

            Finally, have you considered what we would do for upgrading already connected backpacks..?

            ps. Off topic: I particularly hate that third party cookies requirement, I have them turned off and it confused me the first time I saw it, was surprised that persona didn't even detect it!

            Show
            Dan Poltawski added a comment - - edited On the other hand pushing a badge to a backpack already requires all of those things so perhaps it is acceptable? Considering that the only backpack we support at the moment is mozilla's one (right?). Then that seems to make complete sense to me. In fact, what are your arguments that we shouldn't do it! I think that Fred mentioned to me that you are also thinking of making clearer where badges have come from in the interface too? That I think could help. Finally, have you considered what we would do for upgrading already connected backpacks..? ps. Off topic: I particularly hate that third party cookies requirement, I have them turned off and it confused me the first time I saw it, was surprised that persona didn't even detect it!
            Hide
            Dan Poltawski added a comment -

            Ah I see your comment about the email address, in the thread:

            Dave, yes I considered displaying the email, but I can imagine people might be using their personal email for their backpack, which they might not necessarily want to share with everyone within the learning management system.

            Hmm, thats an interesting point. But surely we should be able to show some 'identity' of the badge they are displaying, it seems to me like that might be a necessary compromise to show off your badges. Or at least, is there a url we could use?

            Show
            Dan Poltawski added a comment - Ah I see your comment about the email address, in the thread: Dave, yes I considered displaying the email, but I can imagine people might be using their personal email for their backpack, which they might not necessarily want to share with everyone within the learning management system. Hmm, thats an interesting point. But surely we should be able to show some 'identity' of the badge they are displaying, it seems to me like that might be a necessary compromise to show off your badges. Or at least, is there a url we could use?
            Hide
            Simon Coggins added a comment -

            Okay, here is my (very hacky) proof of concept. With this patch applied go to My Profile Settings > Badges > Backpack Settings and open the developer console.

            Press the Login to Persona button (which would actually replace the email field on this page) and you should be verified via Persona.

            You should get a message in the console explaining if you were verified or not which will include the verified email address. Within the backpackconnect.php file we could store the email in the db.

            Would be good to know what people think before we take it further.

            Show
            Simon Coggins added a comment - Okay, here is my (very hacky) proof of concept. With this patch applied go to My Profile Settings > Badges > Backpack Settings and open the developer console. Press the Login to Persona button (which would actually replace the email field on this page) and you should be verified via Persona. You should get a message in the console explaining if you were verified or not which will include the verified email address. Within the backpackconnect.php file we could store the email in the db. Would be good to know what people think before we take it further.
            Hide
            Simon Coggins added a comment -

            Currently JS/3rd party/cookies/Persona are required for pushing to a backpack, but only Persona is required for displaying badges from a backpack. This change would make all three required for both pushing or displaying.

            Personally I think we probably should make this change.

            For existing connections we would have to make a call - let them get away with it or force them to reconnect.

            Simon

            Show
            Simon Coggins added a comment - Currently JS/3rd party/cookies/Persona are required for pushing to a backpack, but only Persona is required for displaying badges from a backpack. This change would make all three required for both pushing or displaying. Personally I think we probably should make this change. For existing connections we would have to make a call - let them get away with it or force them to reconnect. Simon
            Hide
            Dan Poltawski added a comment -

            I think this solution is OK Simon.

            I suppose it would be good to think about how we would handle other backpack providers if/when they come on stream, but i'm all for this pragmatic solution to the current problem.

            Show
            Dan Poltawski added a comment - I think this solution is OK Simon. I suppose it would be good to think about how we would handle other backpack providers if/when they come on stream, but i'm all for this pragmatic solution to the current problem.
            Hide
            Simon Coggins added a comment -

            Cool, in that case here's what's still needed with this patch:

            • Modularize the JS, convert more to YUI, and include properly
            • Move persona login image into site
            • Better error handling
            • Print message if JS is disabled
            • Use Moodle curl library
            • Save email to DB in backpackconnect.php
            • Some work on the UI and add help to make it clear what's going on
            • Decide what to do about existing connections
            Show
            Simon Coggins added a comment - Cool, in that case here's what's still needed with this patch: Modularize the JS, convert more to YUI, and include properly Move persona login image into site Better error handling Print message if JS is disabled Use Moodle curl library Save email to DB in backpackconnect.php Some work on the UI and add help to make it clear what's going on Decide what to do about existing connections
            Hide
            Simon Coggins added a comment -

            Attached some UI mockups of proposed implementation.

            Show
            Simon Coggins added a comment - Attached some UI mockups of proposed implementation.
            Hide
            Simon Coggins added a comment -

            I've tidied up the proof of concept as discussed. To be applied to 30th August weekly release (hash ee788142).

            Show
            Simon Coggins added a comment - I've tidied up the proof of concept as discussed. To be applied to 30th August weekly release (hash ee788142).
            Hide
            Simon Coggins added a comment -

            New patch attached with a few small improvements following advice from Mozilla.

            This new patch (v2) is to be applied to the Sept 5th weekly release (b69ec28).

            Show
            Simon Coggins added a comment - New patch attached with a few small improvements following advice from Mozilla. This new patch (v2) is to be applied to the Sept 5th weekly release (b69ec28).
            Hide
            Simon Coggins added a comment -

            Ping. Would anyone be able to give this a quick review - I'm keen to get it in as the current behaviour is not really ideal.

            Show
            Simon Coggins added a comment - Ping. Would anyone be able to give this a quick review - I'm keen to get it in as the current behaviour is not really ideal.
            Hide
            Dan Poltawski added a comment - - edited

            Hi Simon,

            I think this works really nicely.

            1. Just a hint about using git format-patch to generate the patch, as it makes it easier to integrate, retains the authorship and the current patch doesn't apply cleanly due to the image included and patch format stuff.

            badges/backpackconnect.php
            2. I don't think that PARAM_TEXT is correct for the assert param, that param allows multilang content. Looking on the mozilla site doesn't give any clues, but I think you should just allow everything (PARAM_RAW). I'd also prefer the explicit param name 'assertion'.
            3. Comment: I thought you'd be able to use moodle_url to get those url parts for audience but it seems they are protected. We could make accessors for them, seems useful to me.
            4. You could use CURLE_OPERATION_TIMEOUTED rather than 28 I think.
            5. I think that you shouldbe passing around and verifying the sessionkey along with the request (in line with what it says on the mozilla site about csrf protection https://developer.mozilla.org/en-US/docs/Mozilla/Persona/Security_Considerations )

            badges/backpack.js
            6. I am not keen on the function names we've got in here (they seem like they might colide as they are in the global scope), but I see this is an existing problem. Still, I think it might be worth prefixing with badges_ or something in the short term just so we don't end up with two clashing. (I think the real solution is to use a proper yui module, but its not quite my area)

            lib/backpacklib.php
            7. Just an observation that BADGE_BACKPACKURL isn't the only instance of openbadges we've got hardcoded. (still, I think its a good direction)

            8. I am not certain if we should keep this as a 'hidden' security issue, I wonder what your thoughts are? I mean, will we backport the same fix to 2.6?

            cheers!
            Dan

            Show
            Dan Poltawski added a comment - - edited Hi Simon, I think this works really nicely. 1. Just a hint about using git format-patch to generate the patch, as it makes it easier to integrate, retains the authorship and the current patch doesn't apply cleanly due to the image included and patch format stuff. badges/backpackconnect.php 2. I don't think that PARAM_TEXT is correct for the assert param, that param allows multilang content. Looking on the mozilla site doesn't give any clues, but I think you should just allow everything (PARAM_RAW). I'd also prefer the explicit param name 'assertion'. 3. Comment: I thought you'd be able to use moodle_url to get those url parts for audience but it seems they are protected. We could make accessors for them, seems useful to me. 4. You could use CURLE_OPERATION_TIMEOUTED rather than 28 I think. 5. I think that you shouldbe passing around and verifying the sessionkey along with the request (in line with what it says on the mozilla site about csrf protection https://developer.mozilla.org/en-US/docs/Mozilla/Persona/Security_Considerations ) badges/backpack.js 6. I am not keen on the function names we've got in here (they seem like they might colide as they are in the global scope), but I see this is an existing problem. Still, I think it might be worth prefixing with badges_ or something in the short term just so we don't end up with two clashing. (I think the real solution is to use a proper yui module, but its not quite my area) lib/backpacklib.php 7. Just an observation that BADGE_BACKPACKURL isn't the only instance of openbadges we've got hardcoded. (still, I think its a good direction) 8. I am not certain if we should keep this as a 'hidden' security issue, I wonder what your thoughts are? I mean, will we backport the same fix to 2.6? cheers! Dan
            Hide
            Simon Coggins added a comment -

            Thanks Dan,

            1. Okay thanks. I'll resubmit using format-patch (or just via github.com if we're going to remove hidden status).

            2. Will do.

            3. Yes it was annoying I couldn't use moodle_url. Should I file a separate bug for that, or fix as part of this one so I can use it?

            4. I think you're right, I'll change it.

            5. Hmm, yes that was an oversight, thanks.

            6. I filed a separate bug for moving to a module (MDL-41802), I will add badges_ prefix for now.

            7. Yes, would make sense to use this everywhere. I will file a separate bug for that.

            8. I think we should backport this to 2.5 (although my preference would be to leave any existing connections rather than forcing people to reconnect). I'm happy for you to remove the hidden security status.

            Show
            Simon Coggins added a comment - Thanks Dan, 1. Okay thanks. I'll resubmit using format-patch (or just via github.com if we're going to remove hidden status). 2. Will do. 3. Yes it was annoying I couldn't use moodle_url. Should I file a separate bug for that, or fix as part of this one so I can use it? 4. I think you're right, I'll change it. 5. Hmm, yes that was an oversight, thanks. 6. I filed a separate bug for moving to a module ( MDL-41802 ), I will add badges_ prefix for now. 7. Yes, would make sense to use this everywhere. I will file a separate bug for that. 8. I think we should backport this to 2.5 (although my preference would be to leave any existing connections rather than forcing people to reconnect). I'm happy for you to remove the hidden security status.
            Hide
            Dan Poltawski added a comment -

            3. It'be great if you want to do it, but it'll be more work.. if you are going to do it in this issue please ensure to expand the moodle_url unit tests to cover it. (I think its fine to ignore it for this issue.)

            thanks!

            Show
            Dan Poltawski added a comment - 3. It'be great if you want to do it, but it'll be more work.. if you are going to do it in this issue please ensure to expand the moodle_url unit tests to cover it. (I think its fine to ignore it for this issue.) thanks!
            Hide
            Simon Coggins added a comment -

            I've created a new ticket with the fix (MDL-41806) since it's a separate issue really.

            Show
            Simon Coggins added a comment - I've created a new ticket with the fix ( MDL-41806 ) since it's a separate issue really.
            Hide
            Simon Coggins added a comment - - edited

            For 2. The Mozilla recommended way of filtering the assertion is:

              $assert = filter_input(
                INPUT_POST,
                'assertion',
                FILTER_UNSAFE_RAW,
                FILTER_FLAG_STRIP_LOW|FILTER_FLAG_STRIP_HIGH
              );
            

            Which strips all characters except ascii codes 32-127. I switched to using required_param() as that's the normal Moodle way, but since there isn't an equivalent PARAM_ type maybe I would be better calling filter_input instead. The Mozilla approach is actually a bit stricter in that it requires it to be a POST parameter whereas required_param() also allows GET params.

            Show
            Simon Coggins added a comment - - edited For 2. The Mozilla recommended way of filtering the assertion is: $assert = filter_input( INPUT_POST, 'assertion', FILTER_UNSAFE_RAW, FILTER_FLAG_STRIP_LOW|FILTER_FLAG_STRIP_HIGH ); Which strips all characters except ascii codes 32-127. I switched to using required_param() as that's the normal Moodle way, but since there isn't an equivalent PARAM_ type maybe I would be better calling filter_input instead. The Mozilla approach is actually a bit stricter in that it requires it to be a POST parameter whereas required_param() also allows GET params.
            Hide
            Simon Coggins added a comment -

            1. Now a github branch
            2. Converted to Mozilla recommended approach
            3. Submitted as MDL-41806
            4. Done
            5. Done
            6. Done with prefix for now, see MDL-41802
            7. Opened MDL-41810
            8. No longer a security issue

            Show
            Simon Coggins added a comment - 1. Now a github branch 2. Converted to Mozilla recommended approach 3. Submitted as MDL-41806 4. Done 5. Done 6. Done with prefix for now, see MDL-41802 7. Opened MDL-41810 8. No longer a security issue
            Hide
            Dan Poltawski added a comment -

            Thanks Simon - sending for integration

            Show
            Dan Poltawski added a comment - Thanks Simon - sending for integration
            Hide
            Simon Coggins added a comment -

            Adding patch for 2.5

            Show
            Simon Coggins added a comment - Adding patch for 2.5
            Hide
            Sam Hemelryk added a comment -

            Thanks Simon - this has been integrated now.

            Show
            Sam Hemelryk added a comment - Thanks Simon - this has been integrated now.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Just guessing if this needs some change once MDL-41806 has landed and moodle_url has better accessors (master only), for your consideration.

            Show
            Eloy Lafuente (stronk7) added a comment - Just guessing if this needs some change once MDL-41806 has landed and moodle_url has better accessors (master only), for your consideration.
            Hide
            Simon Coggins added a comment -

            Eloy,

            Yes - if someone on the integration team can fix this up now that the accessors are available that would be great, otherwise I will create a new bug to do it.

            Simon

            Show
            Simon Coggins added a comment - Eloy, Yes - if someone on the integration team can fix this up now that the accessors are available that would be great, otherwise I will create a new bug to do it. Simon
            Hide
            Mark Nelson added a comment -

            Nice work Simon!

            Much better than just typing in any email, which could easily be abused. Great work.

            The only reason I am failing this is because it does not seem to have been integrated into master, only 2.5. I executed "git log --grep='MDL-40848'" in 2.5 and was able to find the patch, but did not find it in master.

            Show
            Mark Nelson added a comment - Nice work Simon! Much better than just typing in any email, which could easily be abused. Great work. The only reason I am failing this is because it does not seem to have been integrated into master, only 2.5. I executed "git log --grep=' MDL-40848 '" in 2.5 and was able to find the patch, but did not find it in master.
            Hide
            Sam Hemelryk added a comment -

            Gah - sorry about that its beeing integrated there now.

            Show
            Sam Hemelryk added a comment - Gah - sorry about that its beeing integrated there now.
            Hide
            Mark Nelson added a comment -

            Don't worry Sam, not everyone is perfect (except Ankit).

            Ok, all works great now on both branches. I can connect to a backpack and the error handling works as described.

            Thanks!

            Show
            Mark Nelson added a comment - Don't worry Sam, not everyone is perfect (except Ankit). Ok, all works great now on both branches. I can connect to a backpack and the error handling works as described. Thanks!
            Hide
            Marina Glancy added a comment -

            And THANK YOU again for making Moodle better every day!

            Another weekly release has been released.

            Show
            Marina Glancy added a comment - And THANK YOU again for making Moodle better every day! Another weekly release has been released.
            Hide
            Mary Cooch added a comment -

            Just doing some docs_required housekeeping and I noticed the label. Not sure what precisely needs documenting for users here so if anyone knows, please let me know or add to the http://docs.moodle.org/26/en/Badges page.

            Show
            Mary Cooch added a comment - Just doing some docs_required housekeeping and I noticed the label. Not sure what precisely needs documenting for users here so if anyone knows, please let me know or add to the http://docs.moodle.org/26/en/Badges page.
            Hide
            Yuliya Bozhko added a comment -

            Hi Mary,

            Verification is now done via Mozilla Persona service. It's better to try yourself to see how it works.

            Basically, how it was working before: users would fill out a form with text field providing their backpack email and it connected to Moodle. It was not very reliable as users could provide anyone's email and display badges that didn't belong to them. What happens right now is that users are prompted to use their Persona account to authenticate with their backpack (simply put they are asked to log in to their backpack). Now, they can't just add their backpack email to Moodle via text field as before.

            If you have any more questions, please let me know.

            Yuliya

            Show
            Yuliya Bozhko added a comment - Hi Mary, Verification is now done via Mozilla Persona service. It's better to try yourself to see how it works. Basically, how it was working before: users would fill out a form with text field providing their backpack email and it connected to Moodle. It was not very reliable as users could provide anyone's email and display badges that didn't belong to them. What happens right now is that users are prompted to use their Persona account to authenticate with their backpack (simply put they are asked to log in to their backpack). Now, they can't just add their backpack email to Moodle via text field as before. If you have any more questions, please let me know. Yuliya
            Hide
            Mary Cooch added a comment -

            Belated thanks Yuliya - added a quick note to http://docs.moodle.org/en/Badges_FAQ

            Show
            Mary Cooch added a comment - Belated thanks Yuliya - added a quick note to http://docs.moodle.org/en/Badges_FAQ

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: