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

          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.
          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.
          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.
          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.
          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
          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.
          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.
          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
          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.
          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
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam.

          Show
          Rajesh Taneja added a comment - Thanks Sam.
          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.
          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

            People

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

              Dates

              • Created:
                Updated:
                Resolved: