Moodle
  1. Moodle
  2. MDL-32688

Guest access to My Moodle fails on Fatal error and results in a blank page

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.2, 2.3.2, 2.4
    • Fix Version/s: 2.2.6, 2.3.3
    • Component/s: My home
    • Labels:
    • Testing Instructions:
      Hide

      // Test guest access

      With:

      • Security -> Site policies: 'Force users to login' enabled.
      • Users -> Permissions -> User policies: 'Auto-login guests' enabled.
      • Appearance -> Navigation: Default home page for users = site/my moodle/user preference
      1. Logged in as guest go to My Moodle (e.g. by direct url http://your-moodle-domain.com/my).
      2. Expected: My Moodle page shows system default, no editing.
      Show
      // Test guest access With: Security -> Site policies: 'Force users to login' enabled. Users -> Permissions -> User policies: 'Auto-login guests' enabled. Appearance -> Navigation: Default home page for users = site/my moodle/user preference Logged in as guest go to My Moodle (e.g. by direct url http://your-moodle-domain.com/my ). Expected: My Moodle page shows system default, no editing.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      39641

      Description

      Fatal error: Call to a member function add() on a non-object in .../my/index.php on line 88, on guest access to My Moodle.

      Line 88 is:

              $PAGE->settingsnav->get('usercurrentsettings')->add(get_string('makethismyhome'), new moodle_url('/my/', array('setdefaulthome'=>true)), navigation_node::TYPE_SETTING);
      

      Probably the easiest way to reproduce is to try a direct access to my moodle by url (http://<your-moodle-domain>/my) when logged out.

      This bug is perhaps more annoying to logged in users whose session times out when they are on my moodle, to the effect that when they refresh the page they get the blank instead of login page.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Hi, Itamar.

          I just tried to replicate that, but I was redirected to the login page.

          Are there other circumstances/settings that I might need to change to match your setup?

          Show
          Michael de Raadt added a comment - Hi, Itamar. I just tried to replicate that, but I was redirected to the login page. Are there other circumstances/settings that I might need to change to match your setup?
          Hide
          Itamar Tzadok added a comment - - edited

          Yes, I've just checked and it turns out that out of the three options for 'Default home page for users' in Appearance -> Navigation, the error occurs only when set to 'user preference' and indeed this is exactly what line 88 is trying to add. Hope this helps

          Show
          Itamar Tzadok added a comment - - edited Yes, I've just checked and it turns out that out of the three options for 'Default home page for users' in Appearance -> Navigation, the error occurs only when set to 'user preference' and indeed this is exactly what line 88 is trying to add. Hope this helps
          Hide
          Itamar Tzadok added a comment -

          But I'm not sure how allowing guests on my moodle can be useful to begin with and I'd prefer to prevent guests auto access to my moodle, which could be easily done by changing require_login() in line 46 to require_login(null, false).

          Show
          Itamar Tzadok added a comment - But I'm not sure how allowing guests on my moodle can be useful to begin with and I'd prefer to prevent guests auto access to my moodle, which could be easily done by changing require_login() in line 46 to require_login(null, false).
          Hide
          Itamar Tzadok added a comment -

          More input. If I change require_login to require_login(null, false), it will deny guest auto login but only when 'Force users to login' in Security -> Site policies is disabled. If 'Force users to login' is enabled guests pass the require login and fall again on the Fatal error.

          Show
          Itamar Tzadok added a comment - More input. If I change require_login to require_login(null, false), it will deny guest auto login but only when 'Force users to login' in Security -> Site policies is disabled. If 'Force users to login' is enabled guests pass the require login and fall again on the Fatal error.
          Hide
          Michael de Raadt added a comment -

          Thanks for continuing with this.

          I had to explicitly log in as a guest with defaulthomepage set to "User preference" before I could see this error.

          I agree that there is no point allowing guests to see the My Moodle page.

          Technically, a user logged in as a guest is logged in. As well as requiring login, you will need to test if the user is a guest...

          if(is_guest()) {
              redirect(get_login_url());
          }
          

          Please continue working on this. If you have a working solution, we would welcome a patch and testing instructions.

          Show
          Michael de Raadt added a comment - Thanks for continuing with this. I had to explicitly log in as a guest with defaulthomepage set to "User preference" before I could see this error. I agree that there is no point allowing guests to see the My Moodle page. Technically, a user logged in as a guest is logged in. As well as requiring login, you will need to test if the user is a guest... if (is_guest()) { redirect(get_login_url()); } Please continue working on this. If you have a working solution, we would welcome a patch and testing instructions.
          Hide
          Itamar Tzadok added a comment -

          Will do.

          Show
          Itamar Tzadok added a comment - Will do.
          Hide
          Martha added a comment - - edited

          I have the same issue in Moodle 2.1.2 and it's annoying for logged users with time out sessions.

          Show
          Martha added a comment - - edited I have the same issue in Moodle 2.1.2 and it's annoying for logged users with time out sessions.
          Hide
          David Monllaó added a comment -

          Hi Itamar,

          The patch looks great for me, but following the testing steps I've been unable to reproduce the problem, instead of enabling "Force users to login" I've enabled "Auto-login guests" (Users -> Permissions -> User policies) which avoids the require_login() login page redirection and continues the execution until the fatal error is thrown, in order to test the patch the testing instructions should be changed accordingly. I've also been able to reproduce the problem like Michael did, logged as a guest and accessing my moodle, it throws an error that is also solved by your proposed patch

          Regarding the behaviour with auto-login guests and the time out sessions, I wonder why it's not managed in a centralized way by require_login; if a session is expired users probably will be more interested in the login page than in viewing the same page as a guest user. Adding a related issue about it

          Show
          David Monllaó added a comment - Hi Itamar, The patch looks great for me, but following the testing steps I've been unable to reproduce the problem, instead of enabling "Force users to login" I've enabled "Auto-login guests" (Users -> Permissions -> User policies) which avoids the require_login() login page redirection and continues the execution until the fatal error is thrown, in order to test the patch the testing instructions should be changed accordingly. I've also been able to reproduce the problem like Michael did, logged as a guest and accessing my moodle, it throws an error that is also solved by your proposed patch Regarding the behaviour with auto-login guests and the time out sessions, I wonder why it's not managed in a centralized way by require_login; if a session is expired users probably will be more interested in the login page than in viewing the same page as a guest user. Adding a related issue about it
          Hide
          Itamar Tzadok added a comment -

          Thanks David. Note that the error is in the settings nav and may occur whenever a guest get to access My Moodle under the particular default home preference. This does not need to involve session time out and so the proposed adjustment of the require_login may not fix the issue. This issue seems to require either denying My Moodle from guests (as in the submitted fix) or adjusting the settings navigation behavior.

          Show
          Itamar Tzadok added a comment - Thanks David. Note that the error is in the settings nav and may occur whenever a guest get to access My Moodle under the particular default home preference. This does not need to involve session time out and so the proposed adjustment of the require_login may not fix the issue. This issue seems to require either denying My Moodle from guests (as in the submitted fix) or adjusting the settings navigation behavior.
          Hide
          David Monllaó added a comment -

          Hi Itamar,

          Yes, the patch looks great, are different things. Please, remember to update the testing instructions before submitting for integration, thanks

          Show
          David Monllaó added a comment - Hi Itamar, Yes, the patch looks great, are different things. Please, remember to update the testing instructions before submitting for integration, thanks
          Hide
          Itamar Tzadok added a comment -

          Please see updated testing instructions.

          Show
          Itamar Tzadok added a comment - Please see updated testing instructions.
          Hide
          David Monllaó added a comment -

          Thanks Itamar, I've just changed a bit the testing instructions (autologin is a moodle setting) and added an additional scenario to test with logged users. It's all ok for me

          Show
          David Monllaó added a comment - Thanks Itamar, I've just changed a bit the testing instructions (autologin is a moodle setting) and added an additional scenario to test with logged users. It's all ok for me
          Hide
          David Monllaó added a comment -

          Send it to integration when you are ready

          Show
          David Monllaó added a comment - Send it to integration when you are ready
          Hide
          Itamar Tzadok added a comment -

          I don't have the integration-sending privileges. Please send it for me. Thanks!

          Show
          Itamar Tzadok added a comment - I don't have the integration-sending privileges. Please send it for me. Thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Michael de Raadt added a comment -

          Thanks for your efforts, Itamar.

          Show
          Michael de Raadt added a comment - Thanks for your efforts, Itamar.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi, basically I agree and think that it's nosense to show any /my page to guests... so redirecting them always to login seems a valid alternative... but...

          in the other side, it seems that the /my page is prepared to handle those guest accesses here and there, so I'm a bit concerned about being changing some previous behavior out there.

          So, for the sake of confirmation... can you imagine any situation where the solution of redirecting to login could be not ok? I cannot, just guessing...

          I'll halt this for some hours, just in case somebody can realize a good cause for preventing redirect.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, basically I agree and think that it's nosense to show any /my page to guests... so redirecting them always to login seems a valid alternative... but... in the other side, it seems that the /my page is prepared to handle those guest accesses here and there, so I'm a bit concerned about being changing some previous behavior out there. So, for the sake of confirmation... can you imagine any situation where the solution of redirecting to login could be not ok? I cannot, just guessing... I'll halt this for some hours, just in case somebody can realize a good cause for preventing redirect. Ciao
          Hide
          David Monllaó added a comment -

          Hi Eloy,

          For the comments I understand that the biggest problem is with logged users whose session has expired, with the "autologinguest" setting enabled this users are autologged as guests and then the error is thrown. The isguestuser() current behaviour was introduced in MDL-19124 (I didn't find any reference to guest users in the tracker entry) replacing the old behaviour (https://github.com/dmonllao/moodle/commit/03d9401e7d581f2aac097f2ad2f2f7d070481afc#L24L12) where guest users are not allowed to use my moodle.

          Show
          David Monllaó added a comment - Hi Eloy, For the comments I understand that the biggest problem is with logged users whose session has expired, with the "autologinguest" setting enabled this users are autologged as guests and then the error is thrown. The isguestuser() current behaviour was introduced in MDL-19124 (I didn't find any reference to guest users in the tracker entry) replacing the old behaviour ( https://github.com/dmonllao/moodle/commit/03d9401e7d581f2aac097f2ad2f2f7d070481afc#L24L12 ) where guest users are not allowed to use my moodle.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yes David,

          but that means that, since Moodle 2.0, guest users have been able to access to the /my page. With this patch we are negating that access forever.

          So the important bit to know is if preventing that access (that is what the proposed fix implements) can have some impact and sites out there are using that "feature" to present something to guests.

          IMO (personal), guests shouldn't be able to access that page at all, in fact it can disclose some information that shouldn't be shown to them, so I'm all about to enforce the redirect always.

          I just need a justification for the previous behavior, that seems to allow access to guests "on purpose". And that "purpose" is the one I want to know before proceeding with the change.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yes David, but that means that, since Moodle 2.0, guest users have been able to access to the /my page. With this patch we are negating that access forever. So the important bit to know is if preventing that access (that is what the proposed fix implements) can have some impact and sites out there are using that "feature" to present something to guests. IMO (personal), guests shouldn't be able to access that page at all, in fact it can disclose some information that shouldn't be shown to them, so I'm all about to enforce the redirect always. I just need a justification for the previous behavior, that seems to allow access to guests "on purpose". And that "purpose" is the one I want to know before proceeding with the change. Ciao
          Hide
          Mike Churchward added a comment -

          (joining the conversation)...

          Is the proposed change to redirect guest users to the login page if they attempt to access the "My Moodle" page? Would this also be the case if the site was configured to use "My Moodle" as the site page, the site allowed guest users and the user was logged in as a guest?

          If that is the case, I don't think this is the right solution. In the case I described above, the user would be legally logged in as a guest, but then told to login every time he accessed the home page.

          I think the most sensible solution is to allow a guest to see the "My Moodle" page, if that page can be configured appropriately for guest users. If it can't, then they should be redirected to the site main page - not the login page.

          My $0.02.

          mike

          Show
          Mike Churchward added a comment - (joining the conversation)... Is the proposed change to redirect guest users to the login page if they attempt to access the "My Moodle" page? Would this also be the case if the site was configured to use "My Moodle" as the site page, the site allowed guest users and the user was logged in as a guest? If that is the case, I don't think this is the right solution. In the case I described above, the user would be legally logged in as a guest, but then told to login every time he accessed the home page. I think the most sensible solution is to allow a guest to see the "My Moodle" page, if that page can be configured appropriately for guest users. If it can't, then they should be redirected to the site main page - not the login page. My $0.02. mike
          Hide
          Itamar Tzadok added a comment -

          I agree that this is more of a workaround than a solution and not necessarily the best workaround at that.

          I think that guest in my moodle is a special case which should be more adiminsterable. So perhaps adding an admin setting to allow/prevent guest access to my moodle. In addition, the navigation for the particular case that raises the fatal error should be fixed.

          Show
          Itamar Tzadok added a comment - I agree that this is more of a workaround than a solution and not necessarily the best workaround at that. I think that guest in my moodle is a special case which should be more adiminsterable. So perhaps adding an admin setting to allow/prevent guest access to my moodle. In addition, the navigation for the particular case that raises the fatal error should be fixed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm going to reopen this... I think Mike's use case clearly shows one situation where preventing access to my/ to guests may be not a good idea.

          So surely we need to look into another area to prevent that error (navigation stuff itself?) without changing my/ current behaviors.

          Thanks Itamar and Mike, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'm going to reopen this... I think Mike's use case clearly shows one situation where preventing access to my/ to guests may be not a good idea. So surely we need to look into another area to prevent that error (navigation stuff itself?) without changing my/ current behaviors. Thanks Itamar and Mike, ciao
          Hide
          Itamar Tzadok added a comment -

          Eloy please change back to development in progress (or someone grant me workflow privileges) and I will take 2 on the issue with admin setting and nav fix.

          Show
          Itamar Tzadok added a comment - Eloy please change back to development in progress (or someone grant me workflow privileges) and I will take 2 on the issue with admin setting and nav fix.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I've reopened it... you should be able now to move to "development in progress" (I cannot, sorry, crazy perms!).

          Show
          Eloy Lafuente (stronk7) added a comment - I've reopened it... you should be able now to move to "development in progress" (I cannot, sorry, crazy perms!).
          Hide
          Itamar Tzadok added a comment -

          Proposed solution:

          1. Add admin setting for allowguestmy (allow guests on my moodle page) defaults to yes. If set to no guest will be redirected to fronpage.

          2. Avoid the nav fatal error by skipping the defaulthomepage check for guests on grounds that guests don't set preferences. (And so, at least for the time being we don't need to touch the code in the settingsnav)

          Work underway.

          Show
          Itamar Tzadok added a comment - Proposed solution: 1. Add admin setting for allowguestmy (allow guests on my moodle page) defaults to yes. If set to no guest will be redirected to fronpage. 2. Avoid the nav fatal error by skipping the defaulthomepage check for guests on grounds that guests don't set preferences. (And so, at least for the time being we don't need to touch the code in the settingsnav) Work underway.
          Hide
          David Monllaó added a comment -

          Hi,

          Ok, if someone assigns the capability moodle/my:manageblocks (Configure system templates for My Moodle pages) to the guest role, guests can access the default my moodle page, where the admin can set a new distribution of blocks different than the frontpage distribution, this could be a possible use.

          About Mike comment, according to the the defaulthomepage setting description "This determines the home page for logged in users" but get_home_page() always returns HOMEPAGE_SITE as home page for guest users; I also 'grepped' moodle code to find defaulthomepage references and I couldn't find anything guest-problematic, so this setting IMO would not be the problem (which not means that could be other problems of course) but this function made me think that guest users are not supposed to access my moodle in any case, also the only way I've found to access my moodle as a guest is hardcoding the URL.

          If guest users are supposed to access my moodle there are other changes to add to make it consistent, like get_home_page without the !isguestuser() or a link in the navigation tree for example

          Show
          David Monllaó added a comment - Hi, Ok, if someone assigns the capability moodle/my:manageblocks (Configure system templates for My Moodle pages) to the guest role, guests can access the default my moodle page, where the admin can set a new distribution of blocks different than the frontpage distribution, this could be a possible use. About Mike comment, according to the the defaulthomepage setting description "This determines the home page for logged in users" but get_home_page() always returns HOMEPAGE_SITE as home page for guest users; I also 'grepped' moodle code to find defaulthomepage references and I couldn't find anything guest-problematic, so this setting IMO would not be the problem (which not means that could be other problems of course) but this function made me think that guest users are not supposed to access my moodle in any case, also the only way I've found to access my moodle as a guest is hardcoding the URL. If guest users are supposed to access my moodle there are other changes to add to make it consistent, like get_home_page without the !isguestuser() or a link in the navigation tree for example
          Hide
          Itamar Tzadok added a comment -

          A new commit with a different solution. Guests access to my moodle can be configured in admin -> appearance -> navigation, defaults to yes as per current behavior.

          An additional feature is the ability for a user to set default home page back to site.

          Show
          Itamar Tzadok added a comment - A new commit with a different solution. Guests access to my moodle can be configured in admin -> appearance -> navigation, defaults to yes as per current behavior. An additional feature is the ability for a user to set default home page back to site.
          Hide
          Mike Churchward added a comment -

          I think the key thing is that the solution should not force guest users (who are logged in as guests) to the login page, if clicking "home" takes them to "My Moodle".

          Show
          Mike Churchward added a comment - I think the key thing is that the solution should not force guest users (who are logged in as guests) to the login page, if clicking "home" takes them to "My Moodle".
          Hide
          Itamar Tzadok added a comment - - edited

          Indeed, and the solution that is currently waiting for peer review complies with this key behavior. It just adds flexibility in that admin can set the site to prevent guest access to my moodle in which case guests would be redirected back to front page if they try to access my moodle.

          Show
          Itamar Tzadok added a comment - - edited Indeed, and the solution that is currently waiting for peer review complies with this key behavior. It just adds flexibility in that admin can set the site to prevent guest access to my moodle in which case guests would be redirected back to front page if they try to access my moodle.
          Hide
          Mike Churchward added a comment -

          Hi Itamar... I haven't tested your code, but what happens when the site home page is configured to default to "My Moodle", and guest access to "My Moodle" has been configured to not allow it? It looks like it will go into an endless redirect.

          Show
          Mike Churchward added a comment - Hi Itamar... I haven't tested your code, but what happens when the site home page is configured to default to "My Moodle", and guest access to "My Moodle" has been configured to not allow it? It looks like it will go into an endless redirect.
          Hide
          Itamar Tzadok added a comment - - edited

          It doesn't go into endless redirect because you are not redirected from the front page if that's your home page and by core behavior guests' homepage is always SITE. (See also David's last comment above) This also means that by core behavior when guests can access My Moodle, they are not automatically redirected there even if the default home page is set to My Moodle. My fix doesn't change that.

          Show
          Itamar Tzadok added a comment - - edited It doesn't go into endless redirect because you are not redirected from the front page if that's your home page and by core behavior guests' homepage is always SITE. (See also David's last comment above) This also means that by core behavior when guests can access My Moodle, they are not automatically redirected there even if the default home page is set to My Moodle. My fix doesn't change that.
          Hide
          David Monllaó added a comment -

          Hi Itamar,

          Sorry for the delay returning to this issue; I ask Eloy about his opinion, it could be better to separate two things; to solve the current problem (guest user + CFG->defaulthomepage == HOMEPAGE_USER) throws a fatal error, and to add a new feature to allow guest user to my moodle or not.

          IMO the scope of this issue is the first point, the patch for this issue will be added to all the stable versions since it's a current wrong behaviour, you can work on the second point in another issue and it would be added only in 2.4 as it would be adding a new feature.

          Thanks again for working on this Itamar

          Show
          David Monllaó added a comment - Hi Itamar, Sorry for the delay returning to this issue; I ask Eloy about his opinion, it could be better to separate two things; to solve the current problem (guest user + CFG->defaulthomepage == HOMEPAGE_USER) throws a fatal error, and to add a new feature to allow guest user to my moodle or not. IMO the scope of this issue is the first point, the patch for this issue will be added to all the stable versions since it's a current wrong behaviour, you can work on the second point in another issue and it would be added only in 2.4 as it would be adding a new feature. Thanks again for working on this Itamar
          Hide
          David Monllaó added a comment -

          Thanks Itamar, it looks good to me, sending for integration review

          Show
          David Monllaó added a comment - Thanks Itamar, it looks good to me, sending for integration review
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Itamar Tzadok added a comment -

          Any chance this would be integrated soon? Too many installations to hack after upgrade.

          Show
          Itamar Tzadok added a comment - Any chance this would be integrated soon? Too many installations to hack after upgrade.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week because the integration period is over.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (22, 23 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (22, 23 & master), thanks!
          Hide
          Mark Nelson added a comment -

          This fix solves the issue. Thanks.

          Show
          Mark Nelson added a comment - This fix solves the issue. Thanks.
          Hide
          Dan Poltawski added a comment -

          Congratulations, you've done it!

          Thanks, this change is now in the latest weekly release!

          Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!

          Show
          Dan Poltawski added a comment - Congratulations, you've done it! Thanks, this change is now in the latest weekly release! Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: