Moodle
  1. Moodle
  2. MDL-34695

Individual wiki displays edit/view permission error for student users

    Details

    • Database:
      PostgreSQL
    • Testing Instructions:
      Hide

      #. Create a wiki with mode=individual.
      #. Optional: Reduced the session timeout to 5 mins. (site admin > Server > Session handling > sessiontimeout
      #. Log in as a student and enter the course.
      #. Allow yourself to be distracted until the session timeout period has passed.
      #. Click on the wiki activity. You should be redirected to the login page, and after logging in, you should be redirected to the wiki page.
      #. Click the Create page button.

      Make sure you don't get "You can not edit this page" error.

      Show
      #. Create a wiki with mode=individual. #. Optional: Reduced the session timeout to 5 mins. (site admin > Server > Session handling > sessiontimeout #. Log in as a student and enter the course. #. Allow yourself to be distracted until the session timeout period has passed. #. Click on the wiki activity. You should be redirected to the login page, and after logging in, you should be redirected to the wiki page. #. Click the Create page button. Make sure you don't get "You can not edit this page" error.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      43154

      Description

      A problem exists where if by some means a 'group 0, user 0' subwiki (ie. the subwiki normally created for a collaborative non-grouped wiki) is created in a wiki configured for individual mode, any student users who have not yet had their subwiki created will receive a "You can not edit this page" or "You can not view this page" error message if they try to use the wiki activity. This is because the 'group 0, user 0' subwiki is being retrieved for the student instead of a new subwiki being created. Any student who had their subwiki created before the anomalous 'group 0, user 0' subwiki was created is unaffected.

      I'm unable to reproduce this situation through the UI under laboratory conditions, but it has definitely caused breakage in our Production environment and I have attached a demonstration course backup with test users and data that represents the same scenario. Student Alpha and Student Beta have had their subwiki created before the anomalous subwiki was created. Students Gamma and Delta will be unable to create their wiki. Logging in as each student and visiting the wiki in the course will demonstrate the relevant behaviour.

      My patch fixes the problem of the 'group 0, user 0' wiki being fetched and also includes a trap to hopefully help identify the pattern of usage that leads to this anomaly. Only master and MOODLE_22_STABLE have been tested and both exhibit the problem.

        Activity

        Hide
        Michael de Raadt added a comment -

        Thanks for working on that, Jono.

        And thanks for providing a patch.

        Show
        Michael de Raadt added a comment - Thanks for working on that, Jono. And thanks for providing a patch.
        Hide
        Huy Hoang added a comment - - edited

        We ran into the same situation at UMN, and was lucky enough to combine tracing the logs and walking through the codes to find out the cause.

        What happened is that the Wiki mod passes the uid around in the URL. When a student' session timed out but s/he still have the page loaded in browser, clicking on the link to visit the wiki will go through a series of redirection for authentication, and when the student finally lands on the "create" page, the uid has turned into 0.

        I think in general, most of the codes in Moodle use the global $USER to determine the current user, the server side should never trust submitted content without checking. I'm not sure if there is a specific reason why the Wiki is passing uid around in the URL, but I was able to create new subwiki records under any uid simply by changing the URL in the browser. It probably should be classified as a security flaw.

        Show
        Huy Hoang added a comment - - edited We ran into the same situation at UMN, and was lucky enough to combine tracing the logs and walking through the codes to find out the cause. What happened is that the Wiki mod passes the uid around in the URL. When a student' session timed out but s/he still have the page loaded in browser, clicking on the link to visit the wiki will go through a series of redirection for authentication, and when the student finally lands on the "create" page, the uid has turned into 0. I think in general, most of the codes in Moodle use the global $USER to determine the current user, the server side should never trust submitted content without checking. I'm not sure if there is a specific reason why the Wiki is passing uid around in the URL, but I was able to create new subwiki records under any uid simply by changing the URL in the browser. It probably should be classified as a security flaw.
        Hide
        Huy Hoang added a comment -

        attached is the apache log sequence where the uid became 0. IP address and user-agent removed for privacy reason, all log entries came from the same IP.

        Show
        Huy Hoang added a comment - attached is the apache log sequence where the uid became 0. IP address and user-agent removed for privacy reason, all log entries came from the same IP.
        Hide
        Ron Houtman added a comment -

        We're having the same issue. A student can create the initial individual wiki page (but can't change the default name from 'First Page Name' (field is not editable)). Once they create the individual wiki, they can't edit nor view it.

        This is on a new install of 2.3.2 (not an upgraded version)

        Show
        Ron Houtman added a comment - We're having the same issue. A student can create the initial individual wiki page (but can't change the default name from 'First Page Name' (field is not editable)). Once they create the individual wiki, they can't edit nor view it. This is on a new install of 2.3.2 (not an upgraded version)
        Hide
        Huy Hoang added a comment -

        Reading through mod/wiki/view.php, it's obvious that require_login() is called at the end of the four IF branches, but case 0 and case 2 both access $USER before require_login(), resulted in uid = 0.

        My fix was to move require_login() into all 3 branches (case 0-2) as soon as $course and $cm is initialized. It might be ugly, but I think it should be correct that way.

        Show
        Huy Hoang added a comment - Reading through mod/wiki/view.php, it's obvious that require_login() is called at the end of the four IF branches, but case 0 and case 2 both access $USER before require_login(), resulted in uid = 0. My fix was to move require_login() into all 3 branches (case 0-2) as soon as $course and $cm is initialized. It might be ugly, but I think it should be correct that way.
        Hide
        Rossiani Wijaya added a comment -

        Hi everyone,

        I'm unable to reproduce the issue from the UI. I'm only manage to replicate this if I change the wiki setting manually through the DB.

        Could someone provide the steps to replicate this issue please?

        Thanks
        Rosie

        Show
        Rossiani Wijaya added a comment - Hi everyone, I'm unable to reproduce the issue from the UI. I'm only manage to replicate this if I change the wiki setting manually through the DB. Could someone provide the steps to replicate this issue please? Thanks Rosie
        Hide
        Rossiani Wijaya added a comment -

        I'm closing this issue as 'Cannot reproduce'.

        Please create a new issue with steps to replicate if this issue still occurs.

        Thanks
        Rosie

        Show
        Rossiani Wijaya added a comment - I'm closing this issue as 'Cannot reproduce'. Please create a new issue with steps to replicate if this issue still occurs. Thanks Rosie
        Hide
        Jonathon Fowler added a comment -

        Just for the record: since implementing my patch I've had no reports of the problem re-occurring, but I can guarantee that if it was removed I'd be dealing again with the same failure again.

        I've no idea what specific pattern of usage our users managed to find that causes the breakage to occur in the first place, but based on the data in the database and me adding a bunch of tracing into the code to arrive at this patch, the modification logically makes sense (to me at least). I don't have a spare week or more to spend trying all possible things I can think of to identify how exactly the users broke it. Worst case is my fix has no effect at all and is entirely academic, but best case it does fix a problem that is ridiculously obscure but certainly real.

        I hope someone one day can figure out how to reliably reproduce the error.

        Cheers

        Jonathon

        Show
        Jonathon Fowler added a comment - Just for the record: since implementing my patch I've had no reports of the problem re-occurring, but I can guarantee that if it was removed I'd be dealing again with the same failure again. I've no idea what specific pattern of usage our users managed to find that causes the breakage to occur in the first place, but based on the data in the database and me adding a bunch of tracing into the code to arrive at this patch, the modification logically makes sense (to me at least). I don't have a spare week or more to spend trying all possible things I can think of to identify how exactly the users broke it. Worst case is my fix has no effect at all and is entirely academic, but best case it does fix a problem that is ridiculously obscure but certainly real. I hope someone one day can figure out how to reliably reproduce the error. Cheers Jonathon
        Hide
        Ann Adamcik added a comment -

        We ran into this just yesterday, and I've been able to reproduce the problem using the information given by Huy Hoang above. We are running 2.3.2+ (Build: 20120927)

        You might want to set the session timeout to 5 minutes for testing.

        To reproduce:
        1. Create a wiki with mode=individual.
        2. Log in as a student and enter the course.
        3. Allow yourself to be distracted until the session timeout period has passed.
        4. Click on the wiki activity. You should be redirected to the login page, and after logging in, you should be redirected to the wiki page. Notice the uid=0 in the url.
        5. Click the Create page button.

        At this point, the user is presented with the "You can not edit this page" error, and there is an entry created in mdl_subwiki with userid=0.

        Show
        Ann Adamcik added a comment - We ran into this just yesterday, and I've been able to reproduce the problem using the information given by Huy Hoang above. We are running 2.3.2+ (Build: 20120927) You might want to set the session timeout to 5 minutes for testing. To reproduce: 1. Create a wiki with mode=individual. 2. Log in as a student and enter the course. 3. Allow yourself to be distracted until the session timeout period has passed. 4. Click on the wiki activity. You should be redirected to the login page, and after logging in, you should be redirected to the wiki page. Notice the uid=0 in the url. 5. Click the Create page button. At this point, the user is presented with the "You can not edit this page" error, and there is an entry created in mdl_subwiki with userid=0.
        Hide
        Huy Hoang added a comment -

        Can somebody please re-open this ticket?

        The steps to reproduce has been posted, and the bug can be verified just by inspecting the codes ($USER is accessed before require_login() get called).

        Show
        Huy Hoang added a comment - Can somebody please re-open this ticket? The steps to reproduce has been posted, and the bug can be verified just by inspecting the codes ($USER is accessed before require_login() get called).
        Hide
        Rossiani Wijaya added a comment -

        Re-open.

        Thanks Ann for providing the replication steps.

        Show
        Rossiani Wijaya added a comment - Re-open. Thanks Ann for providing the replication steps.
        Hide
        Rossiani Wijaya added a comment -

        Confirming Jonathan's patch does fixed the issue.

        Thanks Jonathan for providing the patch.

        Sending for peer review.

        Show
        Rossiani Wijaya added a comment - Confirming Jonathan's patch does fixed the issue. Thanks Jonathan for providing the patch. Sending for peer review.
        Hide
        Huy Hoang added a comment -

        Please note that Jonathan's patch doesn't prevent the dirty record (userid = 0) from being created in the first place. Also, the code portion to trap anomalous parameters are not needed as we have identified the cause.

        The root cause is because $USER variable is accessed before require_login() is called. Please see the attached wiki_userid_0.patch for mod/wiki/view.php that would prevent the problem in the first place.

        Show
        Huy Hoang added a comment - Please note that Jonathan's patch doesn't prevent the dirty record (userid = 0) from being created in the first place. Also, the code portion to trap anomalous parameters are not needed as we have identified the cause. The root cause is because $USER variable is accessed before require_login() is called. Please see the attached wiki_userid_0.patch for mod/wiki/view.php that would prevent the problem in the first place.
        Hide
        Huy Hoang added a comment -

        call require_login() before accessing $USER

        Show
        Huy Hoang added a comment - call require_login() before accessing $USER
        Hide
        Jonathon Fowler added a comment -

        Huy's right. My patch catches things after they've gone wrong. Combining the two would be the complete solution and would indeed mean the trap I laid is unnecessary.

        Show
        Jonathon Fowler added a comment - Huy's right. My patch catches things after they've gone wrong. Combining the two would be the complete solution and would indeed mean the trap I laid is unnecessary.
        Hide
        Rossiani Wijaya added a comment -

        Hi Huy and Jonathon,

        Thank you for your feedback.

        I updated the patch by combining both patches.

        Show
        Rossiani Wijaya added a comment - Hi Huy and Jonathon, Thank you for your feedback. I updated the patch by combining both patches.
        Hide
        Adrian Greeve added a comment -

        [Y] Syntax
        [-] Output
        [Y] Whitespace
        [-] Language
        [Y] Databases
        [Y] Testing
        [Y] Security
        [-] Documentation
        [Y] Git
        [Y] Sanity check

        Hi Rossie. The patch makes sense and even students that were previously locked out will be able to edit the first page.

        Feel free to submit for integration.

        Show
        Adrian Greeve added a comment - [Y] Syntax [-] Output [Y] Whitespace [-] Language [Y] Databases [Y] Testing [Y] Security [-] Documentation [Y] Git [Y] Sanity check Hi Rossie. The patch makes sense and even students that were previously locked out will be able to edit the first page. Feel free to submit for integration.
        Hide
        Rossiani Wijaya added a comment -

        Thanks Adrian for reviewing.

        Submitting for integration review.

        Show
        Rossiani Wijaya added a comment - Thanks Adrian for reviewing. Submitting for integration review.
        Hide
        mikehas added a comment -

        Jonathon and Huy, Do you know if anyone is using the 2.3 patch in production yet?

        Show
        mikehas added a comment - Jonathon and Huy, Do you know if anyone is using the 2.3 patch in production yet?
        Hide
        Huy Hoang added a comment -

        @mikehas: I don't know any 2.3 in production. We're using the patch for 2.2 production at UMN with no further problem, perhaps we'll skip 2.3 and go straight to 2.4 in summer as there're too much to do for every major upgrade.

        Show
        Huy Hoang added a comment - @mikehas: I don't know any 2.3 in production. We're using the patch for 2.2 production at UMN with no further problem, perhaps we'll skip 2.3 and go straight to 2.4 in summer as there're too much to do for every major upgrade.
        Hide
        mikehas added a comment - - edited

        Thanks Huy. I'm considering whether or not to delete "anomalous" sub_wikis or wait for the integration.

        I think this query identifies wiki's with problems:

        select * from mdl_wiki_subwikis s
        join mdl_wiki w on (w.id = s.wikiid)
        where wikimode = 'individual'
        and groupid = 0
        and userid = 0

        Right now I'm testing if deleting the problematic sub_wikis will alleviate the problem for the 9 inflicted wikis and not cause any other repercussions. I'll verify on our test environment, but in your opinion would this be safe?

        Show
        mikehas added a comment - - edited Thanks Huy. I'm considering whether or not to delete "anomalous" sub_wikis or wait for the integration. I think this query identifies wiki's with problems: select * from mdl_wiki_subwikis s join mdl_wiki w on (w.id = s.wikiid) where wikimode = 'individual' and groupid = 0 and userid = 0 Right now I'm testing if deleting the problematic sub_wikis will alleviate the problem for the 9 inflicted wikis and not cause any other repercussions. I'll verify on our test environment, but in your opinion would this be safe?
        Hide
        Huy Hoang added a comment -

        @mikehas: I had to clean up the data before having the patch too. To be on the safe side, select the related wiki_pages records (mdl_wiki_pages.subwikiid = mdl_wiki_subwikis.id), inspect to make sure they are empty pages. Then when you delete those problematic subwikis, be sure to delete the related wiki_pages too (or you could leave them as orphan).

        Show
        Huy Hoang added a comment - @mikehas: I had to clean up the data before having the patch too. To be on the safe side, select the related wiki_pages records (mdl_wiki_pages.subwikiid = mdl_wiki_subwikis.id), inspect to make sure they are empty pages. Then when you delete those problematic subwikis, be sure to delete the related wiki_pages too (or you could leave them as orphan).
        Hide
        Dan Poltawski added a comment -

        Thanks everyone, i've integrated this now (22, 23 and master).

        Show
        Dan Poltawski added a comment - Thanks everyone, i've integrated this now (22, 23 and master).
        Hide
        Andrew Davis added a comment -

        Works as described. Passing.

        Show
        Andrew Davis added a comment - Works as described. Passing.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Y E S !

        Closing as fixed, many thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Y E S ! Closing as fixed, many thanks!

          People

          • Votes:
            5 Vote for this issue
            Watchers:
            9 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: