Moodle
  1. Moodle
  2. MDL-30388

Site Admin > Blocks > Manage Blocks (Click on Block to get list of) > Show all/Next -- Fails and doesn't do pagination

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1.2
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Administration
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide
      1. Log in as admin
      2. On Site Admin panel go to Plugins > Blocks > Manage Blocks
      3. Click on link in "Instances" column > on blocks (with more then 30 instances)
      4. On top there are links called Show all or Next.
      5. Click on "Next" and you should see next page of results
      6. Click on "Show all" and you should see all the records. In addition you should see Page view link to view pages.
      Show
      Log in as admin On Site Admin panel go to Plugins > Blocks > Manage Blocks Click on link in "Instances" column > on blocks (with more then 30 instances) On top there are links called Show all or Next. Click on "Next" and you should see next page of results Click on "Show all" and you should see all the records. In addition you should see Page view link to view pages.
    • Workaround:
      Hide

      Need to manually edit the URL to add the sesskey. Example:

      https://<URL>/course/search.php?search=&perpage=30&page=1

      should be:
      https://<URL>/course/search.php?search=&perpage=30&page=1&blocklist=7&sesskey=qLwJ2tcQYm

      Show
      Need to manually edit the URL to add the sesskey. Example: https://<URL>/course/search.php?search=&perpage=30&page=1 should be: https://<URL>/course/search.php?search=&perpage=30&page=1&blocklist=7&sesskey=qLwJ2tcQYm
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      wip-mdl-30388
    • Rank:
      33008

      Description

      When looking at the instances of blocks in either M1.9 or M2.x via the "Manage Blocks" screen you cannot get past the first page of results.

      There is a potential security issue according to one our developer's reports. The page that displays the list of instances of blocks doesn't seem to check if the user is logged in before doing a huge query for the block instances. Report is as follows:


      I'm able to view that page without logging in. It seems like a security flaw since i'm not authorized and I can view the content, or refresh the page over and over putting a heavy load on the server while it tries to fetch 6,500 block records. I think I'm able to do this because the sesskey is in the URL.

      https://<URL>/course/search.php?search=&perpage=99999&blocklist=7&sesskey=qLwJ2tcQYm

      is the correct URL for showing all.

      Steps to reproduce
      M1.9:

      1. On Site Admin panel go to Modules > Manage Blocks
      2. Click on link in "Instances" column > on blocks with any instances (about 40+) there are links called Show all or Next.
      3. Click on either the "Show all" or "Next"
      4. Blank page with a search box

      M2.x:

      1. On Site Admin panel go to Plugins > Blocks > Manage Blocks
      2. Click on link in "Instances" column > on blocks with any instances (about 40+) there are links called Show all or Next.
      3. Click on either the "Show all" or "Next"
      4. Blank page with a search box

        Issue Links

          Activity

          Rex Lorenzo created issue -
          Hide
          Michael de Raadt added a comment -

          Thanks for reporting that.

          I've put that on the backlog.

          In the meantime feel free to help us work on this issue. If you are able to provide a patch, please add a patch label so we will spot it.

          Show
          Michael de Raadt added a comment - Thanks for reporting that. I've put that on the backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch, please add a patch label so we will spot it.
          Michael de Raadt made changes -
          Field Original Value New Value
          Security Could be a security issue [ 10030 ] Minor security issue [ 10001 ]
          Fix Version/s STABLE backlog [ 10463 ]
          Priority Minor [ 4 ] Major [ 3 ]
          Labels triaged
          Rajesh Taneja made changes -
          Fix Version/s STABLE Sprint 17 [ 11550 ]
          Fix Version/s STABLE backlog [ 10463 ]
          Rajesh Taneja made changes -
          Status Open [ 1 ] Development in progress [ 3 ]
          Hide
          Rajesh Taneja added a comment -

          This is not a security issue, hence removed.
          In addition to links, few more issues found:

          1. All records were visible (Pagination was not happening)
          2. After clicking "show all ", page navigation bar was still visible and showing two pages.
          Show
          Rajesh Taneja added a comment - This is not a security issue, hence removed. In addition to links, few more issues found: All records were visible (Pagination was not happening) After clicking "show all ", page navigation bar was still visible and showing two pages.
          Rajesh Taneja made changes -
          Summary Site Admin > Blocks > Manage Blocks (Click on Block to get list of) > Show all/Next -- Fails Site Admin > Blocks > Manage Blocks (Click on Block to get list of) > Show all/Next -- Fails and doesn't do pagination
          Security Minor security issue [ 10001 ]
          Rajesh Taneja made changes -
          Pull Master Diff URL https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-30388
          Pull Master Branch wip-mdl-30388
          Testing Instructions As site admin.

          M1.9:
          # On Site Admin panel go to Modules > Manage Blocks
          # Click on link in "Instances" column > on blocks with any instances (about 40+) there are links called Show all or Next.
          # Click on either the "Show all" or "Next"
          # Expected result is to see is either all the instances or the next page of results
          # Actual result is a blank page with a search box

          M2.x:
          # On Site Admin panel go to Plugins > Blocks > Manage Blocks
          # Click on link in "Instances" column > on blocks with any instances (about 40+) there are links called Show all or Next.
          # Click on either the "Show all" or "Next"
          # Expected result is to see is either all the instances or the next page of results
          # Actual result is a blank page with a search box
          # Log in as admin
          # On Site Admin panel go to Plugins > Blocks > Manage Blocks
          # Click on link in "Instances" column > on blocks (with more then 30 instances)
          # On top there are links called Show all or Next.
          # Click on "Next" and you should see next page of results
          # Click on "Show all" and you should see all the records. In addition you should see *Page view* link to view pages.

          Description When looking at the instances of blocks in either M1.9 or M2.x via the "Manage Blocks" screen you cannot get past the first page of results.

          There is a potential security issue according to one our developer's reports. The page that displays the list of instances of blocks doesn't seem to check if the user is logged in before doing a huge query for the block instances. Report is as follows:

          ---
          I'm able to view that page without logging in. It seems like a security flaw since i'm not authorized and I can view the content, or refresh the page over and over putting a heavy load on the server while it tries to fetch 6,500 block records. I think I'm able to do this because the sesskey is in the URL.

          https://&lt;URL&gt;/course/search.php?search=&perpage=99999&blocklist=7&sesskey=qLwJ2tcQYm

          is the correct URL for showing all.
          ---
          When looking at the instances of blocks in either M1.9 or M2.x via the "Manage Blocks" screen you cannot get past the first page of results.

          There is a potential security issue according to one our developer's reports. The page that displays the list of instances of blocks doesn't seem to check if the user is logged in before doing a huge query for the block instances. Report is as follows:

          ---
          I'm able to view that page without logging in. It seems like a security flaw since i'm not authorized and I can view the content, or refresh the page over and over putting a heavy load on the server while it tries to fetch 6,500 block records. I think I'm able to do this because the sesskey is in the URL.

          https://&lt;URL&gt;/course/search.php?search=&perpage=99999&blocklist=7&sesskey=qLwJ2tcQYm

          is the correct URL for showing all.
          ---
          Steps to reproduce
          M1.9:
          # On Site Admin panel go to Modules > Manage Blocks
          # Click on link in "Instances" column > on blocks with any instances (about 40+) there are links called Show all or Next.
          # Click on either the "Show all" or "Next"
          # Blank page with a search box

          M2.x:
          # On Site Admin panel go to Plugins > Blocks > Manage Blocks
          # Click on link in "Instances" column > on blocks with any instances (about 40+) there are links called Show all or Next.
          # Click on either the "Show all" or "Next"
          # Blank page with a search box
          Pull 2.2 Diff URL https://github.com/rajeshtaneja/moodle/compare/MOODLE_22_STABLE...wip-mdl-30388-m22
          Pull 2.1 Branch wip-mdl-30388-m21
          Pull 2.2 Branch wip-mdl-30388-m22
          Pull 2.1 Diff URL https://github.com/rajeshtaneja/moodle/compare/MOODLE_21_STABLE...wip-mdl-30388-m21
          Pull from Repository git://github.com/rajeshtaneja/moodle.git
          Rajesh Taneja made changes -
          Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
          Hide
          Rajesh Taneja added a comment -

          I have added one more commit to show navigation block. Leaving it for integrator to decide, if it's fine to show.

          Show
          Rajesh Taneja added a comment - I have added one more commit to show navigation block. Leaving it for integrator to decide, if it's fine to show.
          Rajesh Taneja made changes -
          Original Estimate 0 minutes [ 0 ]
          Remaining Estimate 0 minutes [ 0 ]
          Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
          Peer reviewer abgreeve
          Hide
          Adrian Greeve added a comment -

          I had a look through the code and tested this out on my machine. The code looks good.
          I'm +1 for putting the navigation block on the side.
          Regarding the security issue. I agree with Rajesh. When not normally logged in it's still possible to navigate to the search page.
          Thanks Raj.

          Show
          Adrian Greeve added a comment - I had a look through the code and tested this out on my machine. The code looks good. I'm +1 for putting the navigation block on the side. Regarding the security issue. I agree with Rajesh. When not normally logged in it's still possible to navigate to the search page. Thanks Raj.
          Adrian Greeve made changes -
          Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
          Rajesh Taneja made changes -
          Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
          Eloy Lafuente (stronk7) made changes -
          Currently in integration Yes [ 10041 ]
          Sam Hemelryk made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Integrator samhemelryk
          Hide
          Sam Hemelryk added a comment -

          Hi Raj,

          Code changes here look perfect, however before this gets integrated I think we need to look at the new pageview string.
          I don't think `Page view` accurately describes what the link is doing.
          How about using the showperpage string instead and feeding in the default perpage value?

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Raj, Code changes here look perfect, however before this gets integrated I think we need to look at the new pageview string. I don't think `Page view` accurately describes what the link is doing. How about using the showperpage string instead and feeding in the default perpage value? Cheers Sam
          Sam Hemelryk made changes -
          Status Integration review in progress [ 10004 ] Reopened [ 4 ]
          Hide
          Rajesh Taneja added a comment -

          Thanks for the feedback Sam.
          I have added Helen (as watcher), for string feedback.

          Show
          Rajesh Taneja added a comment - Thanks for the feedback Sam. I have added Helen (as watcher), for string feedback.
          Rajesh Taneja made changes -
          Status Reopened [ 4 ] Development in progress [ 3 ]
          Hide
          Rajesh Taneja added a comment -

          Thanks for pointing that Sam,

          I have updated the branch with string changed. Also added a define for defult number of records.
          Hope this is fine now.

          Show
          Rajesh Taneja added a comment - Thanks for pointing that Sam, I have updated the branch with string changed. Also added a define for defult number of records. Hope this is fine now.
          Rajesh Taneja made changes -
          Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
          Sam Hemelryk made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Hide
          Sam Hemelryk added a comment -

          Hi Raj,

          I've just been looking at this again now and I'm sending back once more sorry.

          I don't think the define is a good idea.
          It's not really needed given this is only on one page and we have been trying to avoid introducing more defines unless really needed.
          There is one other thing I think you have missed, the perpage defaults to 30 only if the user has the moodle/course:create or moodle/category:manage capabilities, otherwise (for the normal user) it defaults to 10.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Raj, I've just been looking at this again now and I'm sending back once more sorry. I don't think the define is a good idea. It's not really needed given this is only on one page and we have been trying to avoid introducing more defines unless really needed. There is one other thing I think you have missed, the perpage defaults to 30 only if the user has the moodle/course:create or moodle/category:manage capabilities, otherwise (for the normal user) it defaults to 10. Cheers Sam
          Sam Hemelryk made changes -
          Status Integration review in progress [ 10004 ] Reopened [ 4 ]
          Hide
          Rajesh Taneja added a comment -

          grrrr
          Thanks for pointing that Sam.
          Integrated your suggestion. Please review this again.

          Show
          Rajesh Taneja added a comment - grrrr Thanks for pointing that Sam. Integrated your suggestion. Please review this again.
          Rajesh Taneja made changes -
          Status Reopened [ 4 ] Waiting for integration review [ 10010 ]
          Sam Hemelryk made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Hide
          Sam Hemelryk added a comment -

          Thanks Raj, this has been integrated now.
          I did make a commit on top of your work to clean up a typo in the $defultprepage variable

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Raj, this has been integrated now. I did make a commit on top of your work to clean up a typo in the $defultprepage variable Cheers Sam
          Sam Hemelryk made changes -
          Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
          Fix Version/s 2.1.5 [ 11553 ]
          Fix Version/s 2.2.2 [ 11552 ]
          Rossiani Wijaya made changes -
          Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
          Tester rwijaya
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam.

          Show
          Rajesh Taneja added a comment - Thanks Sam.
          Rossiani Wijaya made changes -
          Link This issue testing discovered MDL-31640 [ MDL-31640 ]
          Hide
          Rossiani Wijaya added a comment -

          Hi Raj,

          The pagination is working great. However, I noticed a glitched on the search page. The listed courses can't be move to different category. It just jump to course/search.php page without performing any action.

          I'm passing this issue and creating MDL-30688 to fix the above comment.

          Test passed.

          Show
          Rossiani Wijaya added a comment - Hi Raj, The pagination is working great. However, I noticed a glitched on the search page. The listed courses can't be move to different category. It just jump to course/search.php page without performing any action. I'm passing this issue and creating MDL-30688 to fix the above comment. Test passed.
          Rossiani Wijaya made changes -
          Status Testing in progress [ 10011 ] Tested [ 10006 ]
          Hide
          Rajesh Taneja added a comment -

          Thanks Rossie.
          Issue you create and linked is MDL-31640, and is a different issue then reported. Hence all Good.

          Show
          Rajesh Taneja added a comment - Thanks Rossie. Issue you create and linked is MDL-31640 , and is a different issue then reported. Hence all Good.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks!

          Closing as fixed, heading to zzzZZZzzz, niao

          Show
          Eloy Lafuente (stronk7) added a comment - It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks! Closing as fixed, heading to zzzZZZzzz, niao
          Eloy Lafuente (stronk7) made changes -
          Status Tested [ 10006 ] Closed [ 6 ]
          Resolution Fixed [ 1 ]
          Currently in integration Yes [ 10041 ]
          Eloy Lafuente (stronk7) made changes -
          Integration date 17/Feb/12
          Dan Poltawski made changes -
          Link This issue is duplicated by MDL-18180 [ MDL-18180 ]
          Eloy Lafuente (stronk7) made changes -
          Fix Version/s STABLE Sprint 17 [ 11550 ]

            People

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

              Dates

              • Created:
                Updated:
                Resolved: