Moodle
  1. Moodle
  2. MDL-38289

Coding error with conditional access based on user profile field: Requested user profile field does not exist

    Details

    • Testing Instructions:
      Hide

      Test pre-requisites

      1. Set $CFG->cachetext = 0;
      2. Enable enableavailability
      3. Add all the possible items to frontpage (list of courses, list of categories, etc...)
      4. Add any activity on the front page with Restrict access set to User field AIM ID is not empty
      5. Add an assignment to any course with Restrict access set to User field AIM ID is not empty
        • Copy its URL for future reference

      Test steps

      1. Go on the front page, and logout entirely (not even as guest)
      2. Make sure you don't experience any error on the front page
      3. Wait 2 minutes, and make sure there still is no error
      4. Login as admin, and delete the activity on the front page
      5. Logout and visit the front page again
      6. Make sure you don't experience any error on the front page
      7. Wait 2 minutes, and make sure there still is no error
      8. Directly access the assigmnent using its URL
      9. Make sure you're redirected to the login page
      Show
      Test pre-requisites Set $CFG->cachetext = 0; Enable enableavailability Add all the possible items to frontpage (list of courses, list of categories, etc...) Add any activity on the front page with Restrict access set to User field AIM ID is not empty Add an assignment to any course with Restrict access set to User field AIM ID is not empty Copy its URL for future reference Test steps Go on the front page, and logout entirely (not even as guest) Make sure you don't experience any error on the front page Wait 2 minutes, and make sure there still is no error Login as admin, and delete the activity on the front page Logout and visit the front page again Make sure you don't experience any error on the front page Wait 2 minutes, and make sure there still is no error Directly access the assigmnent using its URL Make sure you're redirected to the login page
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
    • Pull Master Branch:
      MDL-38289-master-int
    • Rank:
      48150

      Description

      I have bumped into this in live course when trying to follow assignment links from e-mail notifications when not logged in Moodle.

      I can reproduce this error on qa.moodle.net.

      Steps to reproduce:

      1. Set intro section 0 to conditional access: "Not available if your aim is empty"
      2. Add assignment to course section 1.
      3. Log off and click on direct link to assignment created in step 2. (ie. http://qa.moodle.net/mod/assign/view.php?id=281)

      I get this error:

      Requested user profile field does not exist
      
      Debug info:
      Error code: codingerror
      Stack trace:
      
          line 1345 of /lib/conditionlib.php: coding_exception thrown
          line 1039 of /lib/conditionlib.php: call to condition_info_base->get_cached_user_profile_field()
          line 280 of /lib/conditionlib.php: call to condition_info_base->is_available()
          line 1744 of /lib/modinfolib.php: call to condition_info_section->is_available()
          line 337 of /lib/modinfolib.php: call to section_info->__construct()
          line 1393 of /lib/modinfolib.php: call to course_modinfo->__construct()
          line 2838 of /lib/moodlelib.php: call to get_fast_modinfo()
          line 39 of /mod/assign/view.php: call to require_login()
      

      What I expect: login form with redirection to assignment after successful login

        Issue Links

          Activity

          Hide
          Pavel Krejci added a comment -

          Found similar but AFAIK distinct issue when searching if it has been already reported

          Show
          Pavel Krejci added a comment - Found similar but AFAIK distinct issue when searching if it has been already reported
          Hide
          Dan Poltawski added a comment -

          Ah thanks a lot Pavel - I was just going to link the two issues.

          Show
          Dan Poltawski added a comment - Ah thanks a lot Pavel - I was just going to link the two issues.
          Hide
          Ray Lawrence added a comment -

          Ouch. I've just been caught by this too. This is occurring with standard profile fields. So conditional access based on user profile fields is completely broken. I was thrown out to the site completely: Coding error detected, it must be fixed by a programmer: Requested user profile field does not exist

          Show
          Ray Lawrence added a comment - Ouch. I've just been caught by this too. This is occurring with standard profile fields. So conditional access based on user profile fields is completely broken. I was thrown out to the site completely: Coding error detected, it must be fixed by a programmer: Requested user profile field does not exist
          Hide
          Jody Steele added a comment -

          This is caused by the standard profile fields being treated differently than the custom profile fields, in conjunction with an unauthenticated user (which doesn't have ANY profile fields). If we replace the throw below with a return false, the problem goes away.

          /lib/conditionlib.php 1294-1321
                  if ($iscurrentuser) {
                      if (!$iscustomprofilefield) {
                          if (property_exists($USER, $field)) {
                              return $USER->{$field};
                          } else {
                              // Unknown user field. This should not happen.
                              throw new coding_exception('Requested user profile field does not exist');
                          }
                      }
                      // Checking if the custom profile fields are already available.
                      if (!isset($USER->profile)) {
                          // Drat! they're not. We need to use a temp object and load them.
                          // We don't use $USER as the profile fields are loaded into the object.
                          $user = new stdClass;
                          $user->id = $USER->id;
                          // This should ALWAYS be set, but just in case we check.
                          require_once($CFG->dirroot.'/user/profile/lib.php');
                          profile_load_custom_fields($user);
                          if (array_key_exists($field, $user->profile)) {
                              return $user->profile[$field];
                          }
                      } else if (array_key_exists($field, $USER->profile)) {
                          // Hurrah they're available, this is easy.
                          return $USER->profile[$field];
                      }
                      // The profile field doesn't exist.
                      return false;
                  } else {
          

          Replace

                              throw new coding_exception('Requested user profile field does not exist');
          

          with

                              return false;
          
          Show
          Jody Steele added a comment - This is caused by the standard profile fields being treated differently than the custom profile fields, in conjunction with an unauthenticated user (which doesn't have ANY profile fields). If we replace the throw below with a return false, the problem goes away. /lib/conditionlib.php 1294-1321 if ($iscurrentuser) { if (!$iscustomprofilefield) { if (property_exists($USER, $field)) { return $USER->{$field}; } else { // Unknown user field. This should not happen. throw new coding_exception('Requested user profile field does not exist'); } } // Checking if the custom profile fields are already available. if (!isset($USER->profile)) { // Drat! they're not. We need to use a temp object and load them. // We don't use $USER as the profile fields are loaded into the object. $user = new stdClass; $user->id = $USER->id; // This should ALWAYS be set, but just in case we check. require_once($CFG->dirroot.'/user/profile/lib.php'); profile_load_custom_fields($user); if (array_key_exists($field, $user->profile)) { return $user->profile[$field]; } } else if (array_key_exists($field, $USER->profile)) { // Hurrah they're available, this is easy. return $USER->profile[$field]; } // The profile field doesn't exist. return false ; } else { Replace throw new coding_exception('Requested user profile field does not exist'); with return false ;
          Hide
          Ray Lawrence added a comment -

          Odd thing is this used to work at 2.4 launch.
          Not sure I understand the comment about unauthenticated users. Surely this will only be an is where a user is authenticated...

          Show
          Ray Lawrence added a comment - Odd thing is this used to work at 2.4 launch. Not sure I understand the comment about unauthenticated users. Surely this will only be an is where a user is authenticated...
          Hide
          Jody Steele added a comment - - edited

          We discovered this because you can add activities to the front page. One of my co-workers was experimenting with making material only available to instructors, which in our system have their idnumber set to 'E'. He added a label activity to the front page in our dev environment, added the constraint that idnumber starts with 'E', logged out, got the coding error message.

          So yeah, it can be used with unauthenticated users...

          EDIT
          That's how we noticed it anyways. I'm not sure what would have triggered it in your case, but it will happen if $USER isn't setup properly.

          Show
          Jody Steele added a comment - - edited We discovered this because you can add activities to the front page. One of my co-workers was experimenting with making material only available to instructors, which in our system have their idnumber set to 'E'. He added a label activity to the front page in our dev environment, added the constraint that idnumber starts with 'E', logged out, got the coding error message. So yeah, it can be used with unauthenticated users... EDIT That's how we noticed it anyways. I'm not sure what would have triggered it in your case, but it will happen if $USER isn't setup properly.
          Hide
          Ray Lawrence added a comment -

          Thanks. So it's really, really broken.

          Show
          Ray Lawrence added a comment - Thanks. So it's really, really broken.
          Hide
          Frédéric Massart added a comment -

          I've noticed a similar issue on the front page, when not authenticated.

          Coding error detected, it must be fixed by a programmer: Requested user profile field does not exist
          
          More information about this error
          
          Debug info: 
          Error code: codingerror
          Stack trace:
          line 1364 of /lib/conditionlib.php: coding_exception thrown
          line 1058 of /lib/conditionlib.php: call to condition_info_base->get_cached_user_profile_field()
          line 1160 of /lib/modinfolib.php: call to condition_info_base->is_available()
          line 345 of /lib/modinfolib.php: call to cm_info->obtain_dynamic_data()
          line 1393 of /lib/modinfolib.php: call to course_modinfo->__construct()
          line 54 of /filter/activitynames/filter.php: call to get_fast_modinfo()
          line 167 of /lib/filterlib.php: call to filter_activitynames->filter()
          line 205 of /lib/filterlib.php: call to filter_manager->apply_filter_chain()
          line 337 of /lib/filterlib.php: call to filter_manager->filter_text()
          line 1141 of /lib/weblib.php: call to performance_measuring_filter_manager->filter_text()
          line 2203 of /course/renderer.php: call to format_text()
          line 1128 of /course/renderer.php: call to coursecat_helper->get_course_formatted_summary()
          line 1097 of /course/renderer.php: call to core_course_renderer->coursecat_coursebox_content()
          line 1260 of /course/renderer.php: call to core_course_renderer->coursecat_coursebox()
          line 1871 of /course/renderer.php: call to core_course_renderer->coursecat_courses()
          

          And I think the fix is the following (based on master integration):

          diff --git a/lib/conditionlib.php b/lib/conditionlib.php
          index a525149..5e112b9 100644
          --- a/lib/conditionlib.php
          +++ b/lib/conditionlib.php
          @@ -1324,7 +1324,7 @@ abstract class condition_info_base {
                   }
                   $iscurrentuser = $USER->id == $userid;
           
          -        if (isguestuser($userid)) {
          +        if (!isloggedin() || isguestuser($userid)) {
                       // Must be logged in and can't be the guest. (this should never happen anyway)
                       return false;
                   }
          

          The comment was already stating that we should be logged in... but the test was not ensuring that we were. Though that makes me wonder how the logic got that far in the process as we're not even logged in. I guess conditional access is not important at all if we're not even guest!

          Show
          Frédéric Massart added a comment - I've noticed a similar issue on the front page, when not authenticated. Coding error detected, it must be fixed by a programmer: Requested user profile field does not exist More information about this error Debug info: Error code: codingerror Stack trace: line 1364 of /lib/conditionlib.php: coding_exception thrown line 1058 of /lib/conditionlib.php: call to condition_info_base->get_cached_user_profile_field() line 1160 of /lib/modinfolib.php: call to condition_info_base->is_available() line 345 of /lib/modinfolib.php: call to cm_info->obtain_dynamic_data() line 1393 of /lib/modinfolib.php: call to course_modinfo->__construct() line 54 of /filter/activitynames/filter.php: call to get_fast_modinfo() line 167 of /lib/filterlib.php: call to filter_activitynames->filter() line 205 of /lib/filterlib.php: call to filter_manager->apply_filter_chain() line 337 of /lib/filterlib.php: call to filter_manager->filter_text() line 1141 of /lib/weblib.php: call to performance_measuring_filter_manager->filter_text() line 2203 of /course/renderer.php: call to format_text() line 1128 of /course/renderer.php: call to coursecat_helper->get_course_formatted_summary() line 1097 of /course/renderer.php: call to core_course_renderer->coursecat_coursebox_content() line 1260 of /course/renderer.php: call to core_course_renderer->coursecat_coursebox() line 1871 of /course/renderer.php: call to core_course_renderer->coursecat_courses() And I think the fix is the following (based on master integration): diff --git a/lib/conditionlib.php b/lib/conditionlib.php index a525149..5e112b9 100644 --- a/lib/conditionlib.php +++ b/lib/conditionlib.php @@ -1324,7 +1324,7 @@ abstract class condition_info_base { } $iscurrentuser = $USER->id == $userid; - if (isguestuser($userid)) { + if (!isloggedin() || isguestuser($userid)) { // Must be logged in and can't be the guest. (this should never happen anyway) return false; } The comment was already stating that we should be logged in... but the test was not ensuring that we were. Though that makes me wonder how the logic got that far in the process as we're not even logged in. I guess conditional access is not important at all if we're not even guest!
          Hide
          Frédéric Massart added a comment -

          Raising to Blocker as this can now be triggered from the frontpage!

          Show
          Frédéric Massart added a comment - Raising to Blocker as this can now be triggered from the frontpage!
          Hide
          Frédéric Massart added a comment -

          Sending for peer review.

          As Marina pointed it out to me, the function is_available (parent of get_cached_user_profile_field()) should be responsible for cleaning up $userid, and should probably allow for $userid === null, and $userid === 0 (for not logged in, and current user respectively). But as it's a bit late to introduce a beautiful fix which will case regressions, here is a simple and safe solution.

          So I've added a test to confirm that the user is logged in, if we're trying to access the current user information. That was indeed the issue, as a non-logged in user does not have profile fields.

          Show
          Frédéric Massart added a comment - Sending for peer review. As Marina pointed it out to me, the function is_available (parent of get_cached_user_profile_field()) should be responsible for cleaning up $userid, and should probably allow for $userid === null, and $userid === 0 (for not logged in, and current user respectively). But as it's a bit late to introduce a beautiful fix which will case regressions, here is a simple and safe solution. So I've added a test to confirm that the user is logged in, if we're trying to access the current user information. That was indeed the issue, as a non-logged in user does not have profile fields.
          Hide
          Dan Poltawski added a comment -

          Hi Fred,

          While that is one way to solve it, I I am to read the logic in english, it reads a bit like nonsense.

          If not a guest user and not logged in, then by definition, you shouldn't have any userid. So I think that the check should be.. do you have a userid, which from a brief talk on chat you say it is. If the userid id 0, then surely we don't have a user!

          But if you disagree, and think i'm reading the logic wrong, no worries.

          The other comment is that we should really remove the 'this should never happen' comment from that branch of logic, because clearly it does happen!

          Show
          Dan Poltawski added a comment - Hi Fred, While that is one way to solve it, I I am to read the logic in english, it reads a bit like nonsense. If not a guest user and not logged in, then by definition, you shouldn't have any userid. So I think that the check should be.. do you have a userid, which from a brief talk on chat you say it is. If the userid id 0, then surely we don't have a user! But if you disagree, and think i'm reading the logic wrong, no worries. The other comment is that we should really remove the 'this should never happen' comment from that branch of logic, because clearly it does happen!
          Hide
          Frédéric Massart added a comment -

          If you don't have a userid, then we fall back on the current user. If the current user is not logged in, it still has a $USER->id associated. I didn't want to assume that I knew the logic that was present in isloggedin() and so I used $iscurrentuser. I could have used if empty($userid) && !isloggedin() but that's basically doing the same check twice, which assumes I know what isloggedin() does. Where as when I'm the current user, I'm sure that I have to check whether I'm logged in or not. Because if I wasn't myself then I'd be looking at someone's activity setting.

          Anyway, one or the other does not change the result, I just wanted to prevent including a logic that belongs somewhere else.

          Show
          Frédéric Massart added a comment - If you don't have a userid, then we fall back on the current user. If the current user is not logged in, it still has a $USER->id associated. I didn't want to assume that I knew the logic that was present in isloggedin() and so I used $iscurrentuser. I could have used if empty($userid) && !isloggedin() but that's basically doing the same check twice, which assumes I know what isloggedin() does. Where as when I'm the current user, I'm sure that I have to check whether I'm logged in or not. Because if I wasn't myself then I'd be looking at someone's activity setting. Anyway, one or the other does not change the result, I just wanted to prevent including a logic that belongs somewhere else.
          Hide
          Frédéric Massart added a comment -

          Dan Poltawski, are you ok with the patch I had provided or would you like me to implement your comments?

          Show
          Frédéric Massart added a comment - Dan Poltawski , are you ok with the patch I had provided or would you like me to implement your comments?
          Hide
          Dan Poltawski added a comment -

          No, you have explained your rationale, thank Fred.

          Show
          Dan Poltawski added a comment - No, you have explained your rationale, thank Fred.
          Hide
          Frédéric Massart added a comment - - edited

          I am going to assume you answered 'No' to the second question . Pushing for integration again. Cheers!

          Show
          Frédéric Massart added a comment - - edited I am going to assume you answered 'No' to the second question . Pushing for integration again . Cheers!
          Hide
          Damyon Wiese added a comment -

          Thanks Fred,

          This has been integrated to 24 and master. I added a patch which added some assertions to the existing unit tests to cover this case and make the comment clearer.

          Show
          Damyon Wiese added a comment - Thanks Fred, This has been integrated to 24 and master. I added a patch which added some assertions to the existing unit tests to cover this case and make the comment clearer.
          Hide
          Rossiani Wijaya added a comment -

          This is working as expected.

          Tested it for 2.4 and master.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working as expected. Tested it for 2.4 and master. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Did you think this day was not going to arrive ever?

          Your patience has been rewarded, yay, sent upstream, thanks!

          Closing...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Did you think this day was not going to arrive ever? Your patience has been rewarded, yay, sent upstream, thanks! Closing...ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: