Moodle
  1. Moodle
  2. MDL-31488

When going direct to a configuration page for authentication module when not logged in an error occurs

    Details

    • Testing Instructions:
      Hide

      Make sure your site has autologin guest turned on
      log out
      go to /admin/auth_config.php?auth=ldap
      you should get an access denied error

      Turn off autologin guest
      log out
      go to /admin/auth_config.php?auth=ldap
      You should be redirected to the login page

      Show
      Make sure your site has autologin guest turned on log out go to /admin/auth_config.php?auth=ldap you should get an access denied error Turn off autologin guest log out go to /admin/auth_config.php?auth=ldap You should be redirected to the login page
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-31488_auth
    • Rank:
      38020

      Description

      For example, if you go to http://demo.moodle.net/admin/auth_config.php?auth=ldap while not logged in, you get a section error.

      An example stack trace from one of my test sites is:
      Section error!
      More information about this error
      Stack trace:
      line 429 of /lib/setuplib.php: moodle_exception thrown
      line 5796 of /lib/adminlib.php: call to print_error()
      line 12 of /admin/auth_config.php: call to admin_externalpage_setup()

      This could happen with other admin pages.

      Expected results is either a message saying not enough permissions or a redirect to login page (most likely the latter)

        Activity

        Hide
        Hugh Davenport added a comment -

        This also shows when a user with limited rights logs in.

        Cheers,

        Hugh

        Show
        Hugh Davenport added a comment - This also shows when a user with limited rights logs in. Cheers, Hugh
        Hide
        Michael de Raadt added a comment -

        Thanks for reporting that.

        You said "This could happen with other admin pages." Can you identify any?

        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. You said "This could happen with other admin pages." Can you identify any? 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
        Andrew Davis added a comment -

        The problem is being caused by admin_externalpage_setup() calling require_login() with no arguments. If the site has auto login guest turned on the user gets logged in as guest and guest has no ability to see the admin page in question (or any admin page most likely). Its not just a matter of prevent auto-login of guest although that would resolve the issue for most people. As Hugh say, any user with limited capabilities will see this error.

        I will do more investigating but I suspect that admin_externalpage_setup() needs to be requiring a capability rather than just requiring a login.

        Show
        Andrew Davis added a comment - The problem is being caused by admin_externalpage_setup() calling require_login() with no arguments. If the site has auto login guest turned on the user gets logged in as guest and guest has no ability to see the admin page in question (or any admin page most likely). Its not just a matter of prevent auto-login of guest although that would resolve the issue for most people. As Hugh say, any user with limited capabilities will see this error. I will do more investigating but I suspect that admin_externalpage_setup() needs to be requiring a capability rather than just requiring a login.
        Hide
        Andrew Davis added a comment -

        I've done some more testing and investigating. The section error is generated whenever the requested "section" doesn't exist in the loaded admin tree. It can not exist because the requested section simply doesn't exist or because the user lacks the capability required to see it. Different sections can depend on different capabilities so it is difficult to know what the root cause is as the time when we realize we can't find the requested section.

        Given that this is an error that is, as far as I know, only encountered by hand crafting a URL I'm not sure its worth investing lots of time writing special handling.

        https://github.com/andyjdavis/moodle/compare/master...MDL-31488_auth
        This is one possible fix. It makes an educated guess as to the reason why the user can't see the section. It will either show an access denied error or a section error depending on whether the user has "moodle/site:config" That is our default capability for admin stuff.

        https://github.com/andyjdavis/moodle/compare/master...MDL-31488_admin2
        Here's an alternative that simply adds an explanatory comment so that anyone getting the error and grepping the code will be able to see why they're getting the error.

        I don't think we should redirect to the login page. If there is a bug introduced into the admin tree causing this section problem to occur when it shouldn't we'd be better off with the user seeing an error rather than being mysteriously redirected.

        Show
        Andrew Davis added a comment - I've done some more testing and investigating. The section error is generated whenever the requested "section" doesn't exist in the loaded admin tree. It can not exist because the requested section simply doesn't exist or because the user lacks the capability required to see it. Different sections can depend on different capabilities so it is difficult to know what the root cause is as the time when we realize we can't find the requested section. Given that this is an error that is, as far as I know, only encountered by hand crafting a URL I'm not sure its worth investing lots of time writing special handling. https://github.com/andyjdavis/moodle/compare/master...MDL-31488_auth This is one possible fix. It makes an educated guess as to the reason why the user can't see the section. It will either show an access denied error or a section error depending on whether the user has "moodle/site:config" That is our default capability for admin stuff. https://github.com/andyjdavis/moodle/compare/master...MDL-31488_admin2 Here's an alternative that simply adds an explanatory comment so that anyone getting the error and grepping the code will be able to see why they're getting the error. I don't think we should redirect to the login page. If there is a bug introduced into the admin tree causing this section problem to occur when it shouldn't we'd be better off with the user seeing an error rather than being mysteriously redirected.
        Hide
        Sam Hemelryk added a comment -

        I've just been looking into this as Andrew asked me to.
        Theres a couple of things to note:

        1. This is happening because plugin settings pages don't get added to the admin tree if the user doesn't have site:config. As such it will have for nearly all plugins.
        2. There is a require login call that gets executed. In the case of demo.moodle.net autologinguest is set up so you arrive at that page when you are not logged in, you get logged in as guest, and then you fail the site:config check leading to the missing section error. If your site doesn't auto login guests then they get redirected to the login page. If they then log in but do not have site:config they see section error.

        While I'm not the expert on this area (Petr is by the way) I would wonder whether this handling is intentional.
        All requests for a page that isn't in the admin tree lead to the user seeing login, or the section missing error depending upon the auto login setting. At the same time plugin pages don't get added to the admin menu unless the user has site:config.
        This means I suppose that a user without site:config would not be able to establish which plugins were installed in a system by hitting the pages and looking at the responses. Perhaps a little more secure, but then not really as you could just ping for known pages.

        I suppose the real question is whether its worth doing anything about this and trying to show a more descriptive error message.
        Thats where your patching come in Andrew, I think if we decide to try to handle it better then the first patch is the way to go.
        The second patch is us deciding that people will work it out, after all only admins should be arriving at those pages. I don't think the second patch makes any sense. It's pretty easy to work back from there, and only developers will ever look at that code. At the moment it is misleading as well, the page won't be found if it doesn't exist OR if the page is a plugin and the user does not have the site:config capability. Pages that require specific capabilities are still added to the admin tree and a proper error message is displayed.

        I think probably it is worth checking with Petr before acting upon it, and if we are going to make a change then patch 1 gets my vote.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - I've just been looking into this as Andrew asked me to. Theres a couple of things to note: This is happening because plugin settings pages don't get added to the admin tree if the user doesn't have site:config. As such it will have for nearly all plugins. There is a require login call that gets executed. In the case of demo.moodle.net autologinguest is set up so you arrive at that page when you are not logged in, you get logged in as guest, and then you fail the site:config check leading to the missing section error. If your site doesn't auto login guests then they get redirected to the login page. If they then log in but do not have site:config they see section error. While I'm not the expert on this area (Petr is by the way) I would wonder whether this handling is intentional. All requests for a page that isn't in the admin tree lead to the user seeing login, or the section missing error depending upon the auto login setting. At the same time plugin pages don't get added to the admin menu unless the user has site:config. This means I suppose that a user without site:config would not be able to establish which plugins were installed in a system by hitting the pages and looking at the responses. Perhaps a little more secure, but then not really as you could just ping for known pages. I suppose the real question is whether its worth doing anything about this and trying to show a more descriptive error message. Thats where your patching come in Andrew, I think if we decide to try to handle it better then the first patch is the way to go. The second patch is us deciding that people will work it out, after all only admins should be arriving at those pages. I don't think the second patch makes any sense. It's pretty easy to work back from there, and only developers will ever look at that code. At the moment it is misleading as well, the page won't be found if it doesn't exist OR if the page is a plugin and the user does not have the site:config capability. Pages that require specific capabilities are still added to the admin tree and a proper error message is displayed. I think probably it is worth checking with Petr before acting upon it, and if we are going to make a change then patch 1 gets my vote. Cheers Sam
        Hide
        Andrew Davis added a comment -

        I'm going for the patch that attempts to figure out why the user is being denied access. That should prompt a user who hasn't noticed that they've automatically been logged in as guest. Any additional input is welcome.

        Show
        Andrew Davis added a comment - I'm going for the patch that attempts to figure out why the user is being denied access. That should prompt a user who hasn't noticed that they've automatically been logged in as guest. Any additional input is welcome.
        Hide
        Sam Hemelryk added a comment -

        Changes look good thanks Andrew, I'm putting this up for integration for you now.
        Just noting I have also tested a fresh install with this patch applied.

        Show
        Sam Hemelryk added a comment - Changes look good thanks Andrew, I'm putting this up for integration for you now. Just noting I have also tested a fresh install with this patch applied.
        Hide
        Dan Poltawski added a comment -

        Hi,

        Tiny comments:
        1/ Do we want this on the stable branches too?
        2/ On 2.2/master get_system_context() is deprecated and we should use context_system::instance() instead.
        3/ The die; is unnecessary since print_error throws an exception, it might make it clearer if we remove the die from the second statement so that people aren't tricked into thinking only the second case 'dies'

        Note these are tiny things, but it'd also be great if they could be fixed, so i'm delaying integrating so you could fix them up.

        Show
        Dan Poltawski added a comment - Hi, Tiny comments: 1/ Do we want this on the stable branches too? 2/ On 2.2/master get_system_context() is deprecated and we should use context_system::instance() instead. 3/ The die; is unnecessary since print_error throws an exception, it might make it clearer if we remove the die from the second statement so that people aren't tricked into thinking only the second case 'dies' Note these are tiny things, but it'd also be great if they could be fixed, so i'm delaying integrating so you could fix them up.
        Hide
        Andrew Davis added a comment -

        I've fixed those things up and produced versions for 2.1 and 2.2.

        Show
        Andrew Davis added a comment - I've fixed those things up and produced versions for 2.1 and 2.2.
        Hide
        Dan Poltawski added a comment -

        Integrated thanks. I squashed a change fixing trailing whitespace intot he commit too

        Show
        Dan Poltawski added a comment - Integrated thanks. I squashed a change fixing trailing whitespace intot he commit too
        Hide
        Rajesh Taneja added a comment -

        Works Great
        Thanks for fixing this, Andrew.

        Show
        Rajesh Taneja added a comment - Works Great Thanks for fixing this, Andrew.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome?

        Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U

        Closing, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome? Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U Closing, ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: