Moodle
  1. Moodle
  2. MDL-15122

GSOC 2008: Secure RSS feeds. Making the RSS feeds published by Moodle secure so that only desired people can access the feeds.

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Deferred
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: RSS
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      15711

      Description

      Secure RSS feeds is a project about making the RSS feeds published by Moodle secure and adding them to other areas of Moodle.

      Spec is available here:
      http://docs.moodle.org/en/Student_projects/Secure_RSS_feeds

      Project discussion thread
      http://moodle.org/mod/forum/discuss.php?d=96026

      1. mdl20-rss-tokens.patch
        3 kB
        Ashley Holman
      2. secrss_complete.patch
        158 kB
        Nicolas Connault

        Issue Links

          Activity

          Hide
          Askar Salimbaev added a comment - - edited

          Development in progress...

          Implement core functions ?
          Secure existing RSS feeds in Moodle ?
          Add option to force HTTPS for RSS feeds ?
          RSS for Calendar(Upcoming events) ?
          RSS for Recent Activity ?
          MDL-12563 fixed ?

          Show
          Askar Salimbaev added a comment - - edited Development in progress... Implement core functions ? Secure existing RSS feeds in Moodle ? Add option to force HTTPS for RSS feeds ? RSS for Calendar(Upcoming events) ? RSS for Recent Activity ? MDL-12563 fixed ?
          Hide
          Petr Škoda added a comment -

          thanks, going to test/review it later today

          Show
          Petr Škoda added a comment - thanks, going to test/review it later today
          Hide
          Petr Škoda added a comment -

          1/ I do not like the use of $CFG->forcehttpsforrss in rss/file.php - any session starting should be done by $SESSION global instance
          we can not detect the https reliably in core now - major problems is the offloading of encryption to another server - my +1 to leave this out for now and fix the https handling in core first

          2/ the rss should use contextid as module or plugin identifier now - we will be moving to this for all module/block files next week; the first implementation of new API should be finished in about 2 weeks from now

          3/ rss_auth() does not respect the $CFG->guestloginbutton which also disables any guest login if disabled

          4/ $CFG->forcelogin setting is not respected too I think

          5/ calendar/lib.php - following if does not seem to be correct:
          if (isset($CFG->enablerssfeeds) && isset($CFG->calendar_enablerssfeeds)...

          6/ why the changes in require_user_key_login()?

          7/ please watch your whitespace, especially the trailing spaces and indentation

          8/ do not initialise objects with NULL value

          9/ use proper PHPdoc inline syntax to annotate all new functions/methods

          10/ course_setup() must be used if you assign $COURSE global object directly

          11/ I guess <br /> is not recommended if you want to separate the rss icons in footers, it might be hard to style with css

          Today I was thinking about possible RSS feed upgrade path in 2.0, the current feeds are going to break anyway - we could start using a new file /rssfile.php and let the old one rss/file.php just display feeds with nice error messages including info how to subscribe to new feeds.

          Is there any ETAG support planned? This could help with perf a bit, loading of user caps is not cheap.

          petr

          Show
          Petr Škoda added a comment - 1/ I do not like the use of $CFG->forcehttpsforrss in rss/file.php - any session starting should be done by $SESSION global instance we can not detect the https reliably in core now - major problems is the offloading of encryption to another server - my +1 to leave this out for now and fix the https handling in core first 2/ the rss should use contextid as module or plugin identifier now - we will be moving to this for all module/block files next week; the first implementation of new API should be finished in about 2 weeks from now 3/ rss_auth() does not respect the $CFG->guestloginbutton which also disables any guest login if disabled 4/ $CFG->forcelogin setting is not respected too I think 5/ calendar/lib.php - following if does not seem to be correct: if (isset($CFG->enablerssfeeds) && isset($CFG->calendar_enablerssfeeds)... 6/ why the changes in require_user_key_login()? 7/ please watch your whitespace, especially the trailing spaces and indentation 8/ do not initialise objects with NULL value 9/ use proper PHPdoc inline syntax to annotate all new functions/methods 10/ course_setup() must be used if you assign $COURSE global object directly 11/ I guess <br /> is not recommended if you want to separate the rss icons in footers, it might be hard to style with css Today I was thinking about possible RSS feed upgrade path in 2.0, the current feeds are going to break anyway - we could start using a new file /rssfile.php and let the old one rss/file.php just display feeds with nice error messages including info how to subscribe to new feeds. Is there any ETAG support planned? This could help with perf a bit, loading of user caps is not cheap. petr
          Hide
          Askar Salimbaev added a comment - - edited

          Thanks for the review Petr.

          1) I didn't like it either, so I will be happy to remove this check However, I want to leave the code, which changes RSS links to 'https://', ok?

          2) Can you tell me where I can find more info about this changes?

          3) 5) 11) My mistake. I will fix it.

          4) You are right. But I can't imagine how it works in context of our 'magic' urls. I think the use of this URL's already assumes 'logging in'.

          6) I just wanted to pass user_key to this function directly.

          7) I need to get rid of a habit of sleeping on a spacebar

          10) Ok. By the way, I there is a theme_setup() call in course_setup(), can it cause any problems?

          You mean HTTP ETAG ? Yes, I was thinking about this. And IF-MODIFIED-SINCE too.

          Show
          Askar Salimbaev added a comment - - edited Thanks for the review Petr. 1) I didn't like it either, so I will be happy to remove this check However, I want to leave the code, which changes RSS links to 'https://', ok? 2) Can you tell me where I can find more info about this changes? 3) 5) 11) My mistake. I will fix it. 4) You are right. But I can't imagine how it works in context of our 'magic' urls. I think the use of this URL's already assumes 'logging in'. 6) I just wanted to pass user_key to this function directly. 7) I need to get rid of a habit of sleeping on a spacebar 10) Ok. By the way, I there is a theme_setup() call in course_setup(), can it cause any problems? You mean HTTP ETAG ? Yes, I was thinking about this. And IF-MODIFIED-SINCE too.
          Hide
          Petr Škoda added a comment - - edited

          1) ok
          2) http://tracker.moodle.org/browse/MDL-14589
          4) forcelogin should imho mean that rss feed can not be accessed without key
          10) I guess we will have to tweak require_login() somehow

          I have to work on getting 1.9.2 ready this week, next week I should be working on file related stuff again.

          Show
          Petr Škoda added a comment - - edited 1) ok 2) http://tracker.moodle.org/browse/MDL-14589 4) forcelogin should imho mean that rss feed can not be accessed without key 10) I guess we will have to tweak require_login() somehow I have to work on getting 1.9.2 ready this week, next week I should be working on file related stuff again.
          Hide
          Askar Salimbaev added a comment -

          Here goes update.
          1)Tried to fix most of problems mentioned by Petr
          2)Reformed RSS subsystem(Petr's idea about ETags).

          • No more Cron jobs for RSS feeds.
          • All feeds are generated on the fly (i.e. no cached .xml files)
          • In order to generate RSS feed, each module must implement this two functions in its rsslib.php
            1/ rss_newstuff($instance, $time,&$cache, $info) - checks if there is new stuff. Like looking into forum post table and seeing if something changed there since last feed fetching. If this returns false, second function is called.
            2/ rss_generate_feed($instance,&$cache,$info) - generates and returns XML rss contents

          Most of the times nothing changes in the feed - we do not have to send the actual feed content, we can just send HTTP 304 Not Modified header. And because no actual content is sent, this allows us to skip loading all capabilities, identifying users etc - improve performance.

          It may be convenient to prefetch some data in rss_newstuff(), that's why $cache is used.
          However, it duplicates rcache functionality a bit, so I'm thinking about removing it.

          But there is a problem, when rss_new_stuff() result depends on what capabilities user has. In this situation we could:
          1) Identify user, load all capabilities. This is slow, but reliable way.
          2) Assume that user has all the necessary capabilites. If there are no changes since last feed fetching, then send 304 Not Modified. Otherwise, do the real check during feed content generation. This is compromise solution.

          Show
          Askar Salimbaev added a comment - Here goes update. 1)Tried to fix most of problems mentioned by Petr 2)Reformed RSS subsystem(Petr's idea about ETags). No more Cron jobs for RSS feeds. All feeds are generated on the fly (i.e. no cached .xml files) In order to generate RSS feed, each module must implement this two functions in its rsslib.php 1/ rss_newstuff($instance, $time,&$cache, $info) - checks if there is new stuff. Like looking into forum post table and seeing if something changed there since last feed fetching. If this returns false, second function is called. 2/ rss_generate_feed($instance,&$cache,$info) - generates and returns XML rss contents Most of the times nothing changes in the feed - we do not have to send the actual feed content, we can just send HTTP 304 Not Modified header. And because no actual content is sent, this allows us to skip loading all capabilities, identifying users etc - improve performance. It may be convenient to prefetch some data in rss_newstuff(), that's why $cache is used. However, it duplicates rcache functionality a bit, so I'm thinking about removing it. But there is a problem, when rss_new_stuff() result depends on what capabilities user has. In this situation we could: 1) Identify user, load all capabilities. This is slow, but reliable way. 2) Assume that user has all the necessary capabilites. If there are no changes since last feed fetching, then send 304 Not Modified. Otherwise, do the real check during feed content generation. This is compromise solution.
          Hide
          Askar Salimbaev added a comment -

          Minor update.

          Show
          Askar Salimbaev added a comment - Minor update.
          Hide
          Askar Salimbaev added a comment -

          Another minor update

          Show
          Askar Salimbaev added a comment - Another minor update
          Hide
          Askar Salimbaev added a comment - - edited

          +Unread message RSS feed

          Show
          Askar Salimbaev added a comment - - edited +Unread message RSS feed
          Hide
          Askar Salimbaev added a comment -

          Rsstype fields in DB for assigments and courses.

          Show
          Askar Salimbaev added a comment - Rsstype fields in DB for assigments and courses.
          Hide
          Askar Salimbaev added a comment -

          Latest version. I'll update specs page soon.

          Btw, File API Docs page says something about old file.php used for compatibility. What exactly does it mean?

          Show
          Askar Salimbaev added a comment - Latest version. I'll update specs page soon. Btw, File API Docs page says something about old file.php used for compatibility. What exactly does it mean?
          Hide
          Askar Salimbaev added a comment -

          Another update:
          +file.php just displays error message

          How about merging it with HEAD? To get feedback and bug reports?

          Show
          Askar Salimbaev added a comment - Another update: +file.php just displays error message How about merging it with HEAD? To get feedback and bug reports?
          Hide
          Askar Salimbaev added a comment -

          Update to comply with HEAD.

          Show
          Askar Salimbaev added a comment - Update to comply with HEAD.
          Hide
          Nicolas Connault added a comment -

          Added the new files to the patch (no more need for zip file), and recreated the patch against latest HEAD (2008082602)

          Show
          Nicolas Connault added a comment - Added the new files to the patch (no more need for zip file), and recreated the patch against latest HEAD (2008082602)
          Hide
          Martin Dougiamas added a comment -

          Petr, can you please put this on your list to merge into 2.0?

          Show
          Martin Dougiamas added a comment - Petr, can you please put this on your list to merge into 2.0?
          Hide
          Ashley Holman added a comment -

          I'm attaching a nice and simple patch which implements a token in the RSS feed URL.

          The current feed URL's contain the username, ie. /rss/file.php/<courseid>/<userid>/<module>/<instance>/

          My patch replaces the <userid> part with a token instead.

          The tokens need to be implemented (I believe Jerome is working on this - which is already being done as part of the Web Services code). The two functions to be implemented are:

          lib/rsslib: rss_get_userid_from_token($token)
          lib/rsslib: rss_get_token($userid)

          I've tested this and it works for forums and calendar feeds. Should work for other components as well.

          Currently hard coded a single token, "faketoken" for user ID 3. So you must give userid 3 access to the feed for it to work.

          Show
          Ashley Holman added a comment - I'm attaching a nice and simple patch which implements a token in the RSS feed URL. The current feed URL's contain the username, ie. /rss/file.php/<courseid>/<userid>/<module>/<instance>/ My patch replaces the <userid> part with a token instead. The tokens need to be implemented (I believe Jerome is working on this - which is already being done as part of the Web Services code). The two functions to be implemented are: lib/rsslib: rss_get_userid_from_token($token) lib/rsslib: rss_get_token($userid) I've tested this and it works for forums and calendar feeds. Should work for other components as well. Currently hard coded a single token, "faketoken" for user ID 3. So you must give userid 3 access to the feed for it to work.
          Hide
          Martin Dougiamas added a comment -

          Very elegant, Ashley!

          Giving this to Andrew to review more deeply and get into HEAD this week.

          Show
          Martin Dougiamas added a comment - Very elegant, Ashley! Giving this to Andrew to review more deeply and get into HEAD this week.
          Hide
          Martin Dougiamas added a comment -

          This bug is obsolete and is being moved to MDL-22204 for a new implementation.

          Show
          Martin Dougiamas added a comment - This bug is obsolete and is being moved to MDL-22204 for a new implementation.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: