Moodle
  1. Moodle
  2. MDL-27542

Export calendar by userid & problem getting the calendar URL for subscription

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.1.2, 2.2
    • Fix Version/s: 2.0.6, 2.1.3
    • Component/s: Calendar
    • Labels:
    • Environment:
      Mac + Safari + MySQL
    • Testing Instructions:
      Hide

      before applying patch, do the following:
      if you are testing it on 2.1 or 2.2, do the following:

      • on calendar/export.php line 97, add: $now = usergetdate(time());
      • on calendar/lib.php line 1809, change:
        $context = get_context_instance(CONTEXT_USER);
        to
        $context = get_context_instance(CONTEXT_USER, $data->userid);

      The above hacks are existing bug that occurs on 2.1 and 2.2. Without the hack, you won't be able to add event or perform the testing.

      1. add new events for the calendar.
      2. view the calendar by month
      3. select 'export calendar' button
      4. copy get calendar url link.
      5. apply the patch
      if you are on 2.1 or 2.2, make sure you remove the above hacks before applying the patch.
      6. go to view calendar by month
      7. select 'export calendar' button

      make sure the export and get calendar url works. Also make sure old calendar url works after applying the patch.

      Show
      before applying patch, do the following: if you are testing it on 2.1 or 2.2, do the following: on calendar/export.php line 97, add: $now = usergetdate(time()); on calendar/lib.php line 1809, change: $context = get_context_instance(CONTEXT_USER); to $context = get_context_instance(CONTEXT_USER, $data->userid); The above hacks are existing bug that occurs on 2.1 and 2.2. Without the hack, you won't be able to add event or perform the testing. 1. add new events for the calendar. 2. view the calendar by month 3. select 'export calendar' button 4. copy get calendar url link. 5. apply the patch if you are on 2.1 or 2.2, make sure you remove the above hacks before applying the patch. 6. go to view calendar by month 7. select 'export calendar' button make sure the export and get calendar url works. Also make sure old calendar url works after applying the patch.
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      17204

      Description

      This is one follow-up of MDL-27192.

      1) Right now the calendar/export_execute.php script uses to work by username + token, completely ignoring any possible mnet stuff, potentially leading to privacy problems or, at least, wrong behavior because username is not a valid unique identifier at all.

      Solution: Make it work with "userid" parameter instead but keep the old (username based) way working for BC. Marking it as deprecated for, say, 2.3

      2) The export interface doesn't seem to work ok here, so switching from week to 60 days back and forth doesn't cause the URL for subscriptions to be refreshed at all (always "weekly" is shown).

      Ciao

        Issue Links

          Activity

          Hide
          Rossiani Wijaya added a comment -

          Hi Eloy,

          When you have a chance could you take a look the patches and provide some feedback?

          Note: patch for master is slightly different from the stable version. In master, it uses userid instead of username.

          Thanks

          Show
          Rossiani Wijaya added a comment - Hi Eloy, When you have a chance could you take a look the patches and provide some feedback? Note: patch for master is slightly different from the stable version. In master, it uses userid instead of username. Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Rossiani,

          as commented, I think it's ok to start generating the hashes in a new way (using exclusive user->id, instead of user->username). Perfect!

          But we need to maintain compatibility with the already generated hashes, so I think your patch is missing, when "Check authentication token" is performed, to allow also the old sha1 (without $user->id) to be checked. It should have one comment saying why we are allowing that check-fallback, pointing to this issue.

          And that double-check will be there surely for ages, or any current URL will stop working.

          Also, I don't know why you use both the $user->id and the $user->username as part of the hash (in the stables), the id is unique enough, isn't it?

          Finally, I've seen some query also using id and username (as params). Once again id is enough, IMO.

          ==== ==== ====

          Said all this, I guess you don't need any difference in the patches, simply:

          • all them should start generating userid-based hashes, and
          • all them should be able to fallback to username check if the userid check fails.

          Just that, or am I missing anything?

          About the rest: changes to support intervals and YUI changes, I have not reviewed them, at first sight they look ok IMO, testing will reveal it.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Rossiani, as commented, I think it's ok to start generating the hashes in a new way (using exclusive user->id, instead of user->username). Perfect! But we need to maintain compatibility with the already generated hashes, so I think your patch is missing, when "Check authentication token" is performed, to allow also the old sha1 (without $user->id) to be checked. It should have one comment saying why we are allowing that check-fallback, pointing to this issue. And that double-check will be there surely for ages, or any current URL will stop working. Also, I don't know why you use both the $user->id and the $user->username as part of the hash (in the stables), the id is unique enough, isn't it? Finally, I've seen some query also using id and username (as params). Once again id is enough, IMO. ==== ==== ==== Said all this, I guess you don't need any difference in the patches, simply: all them should start generating userid-based hashes, and all them should be able to fallback to username check if the userid check fails. Just that, or am I missing anything? About the rest: changes to support intervals and YUI changes, I have not reviewed them, at first sight they look ok IMO, testing will reveal it. Ciao
          Hide
          Ankit Agarwal added a comment -

          Hi,
          This is looking good.
          Just noted that there is an String "Invalid authentication" which is hard coded.
          Rest looks good.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi, This is looking good. Just noted that there is an String "Invalid authentication" which is hard coded. Rest looks good. Thanks
          Hide
          Rossiani Wijaya added a comment -

          Thanks Ankit for reviewing.

          Most of die messages are hard coded. I think it would be ok to leave it as it.

          Integrator please let me know if I need to create a string for the die message.

          Submitting for integration review.

          Show
          Rossiani Wijaya added a comment - Thanks Ankit for reviewing. Most of die messages are hard coded. I think it would be ok to leave it as it. Integrator please let me know if I need to create a string for the die message. Submitting for integration review.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I think it looks perfect now and testing includes both old-urls and new-urls checks, perfect.

          Regarding the missing string and die() calls, as far as such functionality is used user-session-less (to integrate the cal into other calendars and friends), I think it's ok to leave the messages harcoded, although, being purist, surely we should look to the ical specs to see if there is some sort of official xml reply for this cases. So clients will return proper error code/message.

          So I would leave this as is now and surely create one follow-up improvement issue to make the the export feature to always return proper ical-xml formatted code (for error/auth situations, if the spec allows it).

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I think it looks perfect now and testing includes both old-urls and new-urls checks, perfect. Regarding the missing string and die() calls, as far as such functionality is used user-session-less (to integrate the cal into other calendars and friends), I think it's ok to leave the messages harcoded, although, being purist, surely we should look to the ical specs to see if there is some sort of official xml reply for this cases. So clients will return proper error code/message. So I would leave this as is now and surely create one follow-up improvement issue to make the the export feature to always return proper ical-xml formatted code (for error/auth situations, if the spec allows it). Thanks and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I have created MDL-30200 about to prospect if it's possible to replace current die()/redirect() responses by proper iCal ones.

          Show
          Eloy Lafuente (stronk7) added a comment - I have created MDL-30200 about to prospect if it's possible to replace current die()/redirect() responses by proper iCal ones.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Note: I've added one extra commit in 20_STABLE deleting one print_object() left there.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! Note: I've added one extra commit in 20_STABLE deleting one print_object() left there. Ciao
          Hide
          Glenn Ansley added a comment -

          I could not get this to work with consistency. It is definitely possible that I don't completely understand what's going on here. If that is the case though, I'd suggest some more robust Testing Instructions.

          On 2.1 and master (not integrations) I can confirm that the URL does not seem to work correctly at all. I get a access error when trying to reach the URL.

          On the integration branches, I find that the URL is actually generated - which is good. I can also confirm that in some instances I receive the correct data from the calendar. In other instances, however, I believe I am receiving the wrong information. Here are my steps that led me to fail. These were performed on integrations 2.1 branch. I apologize for the length of this. There is a summary at the bottom.

          1. Log into Moodle and click on current month link from sidebar
          2. Select the 'New Event' button in top-right
          3. Create a new event that is titled "Test User Event". Make it a User type of event. Make the event for later the same day and submit
          4. Select the 'New Event' button in top-right
          5. Create a new event that is titled "Test Site Event". Make it a "Site" type of event. Make the event for later in the current week. Submit event.
          6. Select a course you have admin rights to modify
          7. Click 'New Event..." from sidebar block
          8. Create a new course event titled "Test Course Event". Make it a 'Course' type of event. Make the date later in the same month but not in the current week. Submit.
          9. Select the New Event button from at the top-right
          10. Create a new event. Make the title 'Future Site Event'. Make it a 'Site' type of event. Set it for a date in the next month from the current month and later than all previously created events above. Save changes.
          11. Click home in the Moodle breadcrumb bar and see all events other than the course event on the sidbear calendar (this is correct)
          12. Enter the course associated with the course event
          13. View all of current months events including the course event in sidebar (This is correct)
          14. [Now attempting to follow this ticket's testing instructions]
          15. Click home in the Moodle breadcrumb bar
          16. Click the current month's name in the sidebar to view the full calendar
          17. Click the export calendar button
          18. Note "All events" and "This week" is selected.
          19. Click "Get Calendar URL"
          20. Note that calendar URL is created.
          21. Copy and paste calendar URL into browser and note that URL works.
          22. Save file to disk (don't open with iCal/etc)
          23. Open with a text editor and note that Test User and Test Site events are available. (This seems to work correctly).
          24. Go back to export screen (should still be open in browser)
          25. Select "All Events" and "This Month" and then click "Get Calendar URL"
          26. Repeat steps 20, 21, and 22
          27. Open the file in a file editor and note that "Test User Event" and "Test Site event" is present but that the course event is missing. This seems like a fail of functionality or of managed expectations since I clicked "All Events".
          28. Go back to the Calendar export screen and select "Events related to courses" and "This month"
          29. Repeat steps 20, 21, and 22
          30. Open the file in a text editor and note that the file was created in iCal format but that there are no events in it. It seems that the "Test Course Event" should have been included in this file.
          31. Go back to the Calendar export screen and select "All events" and "Recent and next 60 days"
          32. Repeat steps 20, 21, and 22
          33. Open the file in a text editor
          34. Note that "User event", "Test Site Event", and "Test Future Event" are all present but that course event is missing. This seems like a fail of functionality or of managed expectations since i clicked "All Events"
          35. Go back to the Calendar export screen and select "Events related to courses and "Recent and next 60 days"
          36. Repeat steps 20, 21, and 22
          37. Open the file in a text editor and note that the file was created in iCal format but that there are no events in it. It seems that the "Test Course Event" should have been included in this file.

          Some notes based on above testing:

          • Course events don't appear to be getting added to iCal for All Events or for "Events related to courses". This sounds like an error for both instances - certainly for the second instance.
          • Unauthenticated users with one of these URLs can successfully download the iCal file but the file has no event data inside it. Is this as designed or is the authtoken supposed to give them access.
          Show
          Glenn Ansley added a comment - I could not get this to work with consistency. It is definitely possible that I don't completely understand what's going on here. If that is the case though, I'd suggest some more robust Testing Instructions. On 2.1 and master (not integrations) I can confirm that the URL does not seem to work correctly at all. I get a access error when trying to reach the URL. On the integration branches, I find that the URL is actually generated - which is good. I can also confirm that in some instances I receive the correct data from the calendar. In other instances, however, I believe I am receiving the wrong information. Here are my steps that led me to fail. These were performed on integrations 2.1 branch. I apologize for the length of this. There is a summary at the bottom. 1. Log into Moodle and click on current month link from sidebar 2. Select the 'New Event' button in top-right 3. Create a new event that is titled "Test User Event". Make it a User type of event. Make the event for later the same day and submit 4. Select the 'New Event' button in top-right 5. Create a new event that is titled "Test Site Event". Make it a "Site" type of event. Make the event for later in the current week. Submit event. 6. Select a course you have admin rights to modify 7. Click 'New Event..." from sidebar block 8. Create a new course event titled "Test Course Event". Make it a 'Course' type of event. Make the date later in the same month but not in the current week. Submit. 9. Select the New Event button from at the top-right 10. Create a new event. Make the title 'Future Site Event'. Make it a 'Site' type of event. Set it for a date in the next month from the current month and later than all previously created events above. Save changes. 11. Click home in the Moodle breadcrumb bar and see all events other than the course event on the sidbear calendar (this is correct) 12. Enter the course associated with the course event 13. View all of current months events including the course event in sidebar (This is correct) 14. [Now attempting to follow this ticket's testing instructions] 15. Click home in the Moodle breadcrumb bar 16. Click the current month's name in the sidebar to view the full calendar 17. Click the export calendar button 18. Note "All events" and "This week" is selected. 19. Click "Get Calendar URL" 20. Note that calendar URL is created. 21. Copy and paste calendar URL into browser and note that URL works. 22. Save file to disk (don't open with iCal/etc) 23. Open with a text editor and note that Test User and Test Site events are available. (This seems to work correctly). 24. Go back to export screen (should still be open in browser) 25. Select "All Events" and "This Month" and then click "Get Calendar URL" 26. Repeat steps 20, 21, and 22 27. Open the file in a file editor and note that "Test User Event" and "Test Site event" is present but that the course event is missing. This seems like a fail of functionality or of managed expectations since I clicked "All Events". 28. Go back to the Calendar export screen and select "Events related to courses" and "This month" 29. Repeat steps 20, 21, and 22 30. Open the file in a text editor and note that the file was created in iCal format but that there are no events in it. It seems that the "Test Course Event" should have been included in this file. 31. Go back to the Calendar export screen and select "All events" and "Recent and next 60 days" 32. Repeat steps 20, 21, and 22 33. Open the file in a text editor 34. Note that "User event", "Test Site Event", and "Test Future Event" are all present but that course event is missing. This seems like a fail of functionality or of managed expectations since i clicked "All Events" 35. Go back to the Calendar export screen and select "Events related to courses and "Recent and next 60 days" 36. Repeat steps 20, 21, and 22 37. Open the file in a text editor and note that the file was created in iCal format but that there are no events in it. It seems that the "Test Course Event" should have been included in this file. Some notes based on above testing: Course events don't appear to be getting added to iCal for All Events or for "Events related to courses". This sounds like an error for both instances - certainly for the second instance. Unauthenticated users with one of these URLs can successfully download the iCal file but the file has no event data inside it. Is this as designed or is the authtoken supposed to give them access.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Thanks for the complete test & feedback Glenn.

          • About course events not being added it really looks like a separate bug, nothing introduced by this change. Easily verifiable using current version without the change.
          • About the file being returned empty... if not logged, that is really odd, because clearly it renders the export with shared secret 100% not useful. Once again, I'm not sure if that is related with the changes in this issue or existed previously.

          If I get some time, I'll try to test reproduce those issues here to confirm if they are previous errors. Else HQ/Rossiani will next week. That will allow us to know if the trouble-maker is this issue or no (I think it isn't, but) and decide about to revert it or no.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks for the complete test & feedback Glenn. About course events not being added it really looks like a separate bug, nothing introduced by this change. Easily verifiable using current version without the change. About the file being returned empty... if not logged, that is really odd, because clearly it renders the export with shared secret 100% not useful. Once again, I'm not sure if that is related with the changes in this issue or existed previously. If I get some time, I'll try to test reproduce those issues here to confirm if they are previous errors. Else HQ/Rossiani will next week. That will allow us to know if the trouble-maker is this issue or no (I think it isn't, but) and decide about to revert it or no. Ciao
          Hide
          Rossiani Wijaya added a comment -

          Hi Glen Thanks for testing this.

          I agree with Eloy, the course events issue is not related to this fix. I will create new issue to address that.

          On second point, I tried to access the url without login to the system and able to see the contents of the events. If I tried to export the calendar without logging in to the system, I get in valid authentication error message.

          Could you describe the steps to reproduce the issue for unauthenticated user?

          Thanks

          Show
          Rossiani Wijaya added a comment - Hi Glen Thanks for testing this. I agree with Eloy, the course events issue is not related to this fix. I will create new issue to address that. On second point, I tried to access the url without login to the system and able to see the contents of the events. If I tried to export the calendar without logging in to the system, I get in valid authentication error message. Could you describe the steps to reproduce the issue for unauthenticated user? Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oki,

          Rossiani, Glenn, I'm considering this passed right now. I need to put all the current issues into correct status aiming to release 2.2beta later today.

          Please, don't forget about the pending issues and feel free to continue comenting here and/or in the followup issues (MDL-30246) created by Rossiani.

          Thanks for the hard work!

          Show
          Eloy Lafuente (stronk7) added a comment - Oki, Rossiani, Glenn, I'm considering this passed right now. I need to put all the current issues into correct status aiming to release 2.2beta later today. Please, don't forget about the pending issues and feel free to continue comenting here and/or in the followup issues ( MDL-30246 ) created by Rossiani. Thanks for the hard work!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yes, you got this finally upstream, just in time for Moodle 2.2beta. Congrats and thanks!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yes, you got this finally upstream, just in time for Moodle 2.2beta. Congrats and thanks! Ciao

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: