Moodle
  1. Moodle
  2. MDL-37792

Conditional Resource based on Profile Interest with a non-existant string

    Details

    • Rank:
      47527

      Description

      Created a conditional activity 'Label' and set it to only be visible to users who had put a particular string into their Interests field in their User Profile.

      There were no users who had that string in the field.

      This created a nasty loop where I could not refresh or move onto another page without the error message popping up.

      Unfortunately I was not smart enough to grab a screen shot before I reverted to a nice fresh install of Moodle.

      I'm guessing for those who have a backup, if this happens a back up to something prior to creating this logic loop would work.

      Better would be to have Moodle not enact the condition until the condition was met.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this.

          Could you please add more information to your report such as replication instructions, error messages and screenshots. If you can determine the cause of the problem in code, or even determine a solution, that will be very valuable.

          Show
          Michael de Raadt added a comment - Thanks for reporting this. Could you please add more information to your report such as replication instructions, error messages and screenshots. If you can determine the cause of the problem in code, or even determine a solution, that will be very valuable.
          Hide
          Michael de Raadt added a comment -

          I've set this as a minor security issue for now. If it is possible to bring down a server using replicatable steps, then it should stay that way.

          Show
          Michael de Raadt added a comment - I've set this as a minor security issue for now. If it is possible to bring down a server using replicatable steps, then it should stay that way.
          Hide
          Susannah Brown added a comment - - edited

          The four basic steps to replicate the bug have been uploaded as screen shots.

          The text of the error is-:
          "Coding error detected, it must be fixed by a programmer: Requested user profile field does not exist"

          Show
          Susannah Brown added a comment - - edited The four basic steps to replicate the bug have been uploaded as screen shots. The text of the error is-: "Coding error detected, it must be fixed by a programmer: Requested user profile field does not exist"
          Hide
          Michael de Raadt added a comment -

          Thanks for adding some more information. I've taken the screenshots out of your zip to make them easier to view.

          Show
          Michael de Raadt added a comment - Thanks for adding some more information. I've taken the screenshots out of your zip to make them easier to view.
          Hide
          Michael de Raadt added a comment -

          I was able to replicate the problem. With caching involved, it's not a simple matter of removing something from the DB to work around the problem.

          Sam: it would be great if you could look at this.

          Show
          Michael de Raadt added a comment - I was able to replicate the problem. With caching involved, it's not a simple matter of removing something from the DB to work around the problem. Sam: it would be great if you could look at this.
          Hide
          Sam Marshall added a comment -

          Hi - sorry I have been very busy for the last few weeks.

          I doubt it brings down the server but it probably kills the course - this probably doesn't count as a security issue since you need editing permissions to that course and there are other ways to kill the course (like deleting it). However, I'll confirm that when I work out the fix.

          Show
          Sam Marshall added a comment - Hi - sorry I have been very busy for the last few weeks. I doubt it brings down the server but it probably kills the course - this probably doesn't count as a security issue since you need editing permissions to that course and there are other ways to kill the course (like deleting it). However, I'll confirm that when I work out the fix.
          Hide
          Sam Marshall added a comment -

          I can confirm:

          1. This does not kill the server, only the course.

          2. The problem is caused because somebody included interests on the list of supported user profile fields, but there is actually no 'interests' field (it's something complicated in the tags table). In other words the user interface that lets users select the feature exist is present, but it was never implemented.

          3. It would be possible to implement this but I don't think there is going to be a frequent need for conditions on the 'interests' non-field. As a result, I propose to fix the problem by simply removing the 'interests' field from the list so that users can't choose it.

          Show
          Sam Marshall added a comment - I can confirm: 1. This does not kill the server, only the course. 2. The problem is caused because somebody included interests on the list of supported user profile fields, but there is actually no 'interests' field (it's something complicated in the tags table). In other words the user interface that lets users select the feature exist is present, but it was never implemented. 3. It would be possible to implement this but I don't think there is going to be a frequent need for conditions on the 'interests' non-field. As a result, I propose to fix the problem by simply removing the 'interests' field from the list so that users can't choose it.
          Hide
          Sam Marshall added a comment -

          Requesting peer review. To recap my comment above, the 'interests' field option appears never to have been implemented, so removing it from the list does not reduce functionality. Just stops this bug happening!

          Show
          Sam Marshall added a comment - Requesting peer review. To recap my comment above, the 'interests' field option appears never to have been implemented, so removing it from the list does not reduce functionality. Just stops this bug happening!
          Hide
          Frédéric Massart added a comment -

          Hi Sam,

          thanks for the patch. While I think you can freely push this for integration, how would you feel about writing a small unit test to assert that:

          • The list of items returned by get_condition_user_fields are actually valid fields, or custom fields?
          • Conditions are respected while using profile fields? (Out of the scope of this issue, but always useful)

          Many thanks!
          Fred

          Show
          Frédéric Massart added a comment - Hi Sam, thanks for the patch. While I think you can freely push this for integration, how would you feel about writing a small unit test to assert that: The list of items returned by get_condition_user_fields are actually valid fields, or custom fields? Conditions are respected while using profile fields? (Out of the scope of this issue, but always useful) Many thanks! Fred
          Hide
          Sam Marshall added a comment -

          Thanks Fred!

          You are right that it would be nice to have a test that detects this issue in case there are any other problem fields (I had already looked at the list and it looked ok but that's not the most reliable way).

          I have added a test which checks (a) that all fields in the list, it can actually check the value of (the test that would detect this problem) and (b) that custom fields are included in the list.

          Now submitting for integration review as the test should more than cover the change.

          I didn't do a check for the comparison operators, which as you say is a separate issue; I decided it was out of scope for this and I hope somebody else adds it! (Other than this one I've just written, there seem to be no unit tests at all for the user-field-condition feature which is rather poor. I can't remember if I reviewed this code or not, but whoever it was should have spotted that...)

          Show
          Sam Marshall added a comment - Thanks Fred! You are right that it would be nice to have a test that detects this issue in case there are any other problem fields (I had already looked at the list and it looked ok but that's not the most reliable way). I have added a test which checks (a) that all fields in the list, it can actually check the value of (the test that would detect this problem) and (b) that custom fields are included in the list. Now submitting for integration review as the test should more than cover the change. I didn't do a check for the comparison operators, which as you say is a separate issue; I decided it was out of scope for this and I hope somebody else adds it! (Other than this one I've just written, there seem to be no unit tests at all for the user-field-condition feature which is rather poor. I can't remember if I reviewed this code or not, but whoever it was should have spotted that...)
          Hide
          Sam Marshall added a comment -

          Removing the 'security issue' marker, this is not a security issue for the reasons I gave some way above (I forgot to do this earlier).

          Show
          Sam Marshall added a comment - Removing the 'security issue' marker, this is not a security issue for the reasons I gave some way above (I forgot to do this earlier).
          Hide
          Damyon Wiese 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.

          Thanks!

          Show
          Damyon Wiese 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. Thanks!
          Hide
          Damyon Wiese added a comment -

          Thanks Sam,

          The patch makes sense and the unit test is good.

          I think it would be worth fixing any existing courses that have been broken by this bug. Can you please add an upgrade step that deletes from section_avail_fields and course_modules_avail_fields any condition using this broken field? (I imagine there would be a lecturer somewhere who would be eternally grateful for this!)

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Thanks Sam, The patch makes sense and the unit test is good. I think it would be worth fixing any existing courses that have been broken by this bug. Can you please add an upgrade step that deletes from section_avail_fields and course_modules_avail_fields any condition using this broken field? (I imagine there would be a lecturer somewhere who would be eternally grateful for this!) Thanks, Damyon
          Hide
          Sam Marshall added a comment -

          OK - to confirm, I am coding the upgrade step now.

          Show
          Sam Marshall added a comment - OK - to confirm, I am coding the upgrade step now.
          Hide
          Sam Marshall added a comment -

          I have updated both branches with an upgrade step. As this is a change to the main db/upgrade.php and version.php the version numbers might be wrong - please correct these when integrating.

          Show
          Sam Marshall added a comment - I have updated both branches with an upgrade step. As this is a change to the main db/upgrade.php and version.php the version numbers might be wrong - please correct these when integrating.
          Hide
          Damyon Wiese added a comment -

          Thanks - taking another look now.

          Show
          Damyon Wiese added a comment - Thanks - taking another look now.
          Hide
          Damyon Wiese added a comment -

          $DB->delete_records('course_sections_avail_fields', array('userfield' => 'idnumber'));

          should be

          $DB->delete_records('course_sections_avail_fields', array('userfield' => 'interests'));

          Show
          Damyon Wiese added a comment - $DB->delete_records('course_sections_avail_fields', array('userfield' => 'idnumber')); should be $DB->delete_records('course_sections_avail_fields', array('userfield' => 'interests'));
          Hide
          Damyon Wiese added a comment -

          I'll add a patch to fix the upgrade commands - that's the only issue I spotted.

          Show
          Damyon Wiese added a comment - I'll add a patch to fix the upgrade commands - that's the only issue I spotted.
          Hide
          Damyon Wiese added a comment -

          Thanks Sam,

          I have integrated this for 24 and master with an additional patch to fix the upgrade code.

          Thanks for working on this important bug fix!

          Show
          Damyon Wiese added a comment - Thanks Sam, I have integrated this for 24 and master with an additional patch to fix the upgrade code. Thanks for working on this important bug fix!
          Hide
          David Monllaó added a comment -

          Tested in master and 24, it passes

          Show
          David Monllaó added a comment - Tested in master and 24, it passes
          Hide
          Sam Marshall added a comment -

          Can't believe I made that mistake! I tested the upgrade step didn't give an error, but I didn't test it actually worked... Thanks for fixing it.

          Show
          Sam Marshall added a comment - Can't believe I made that mistake! I tested the upgrade step didn't give an error, but I didn't test it actually worked... Thanks for fixing it.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Because

          A
          MARVELOUS
          A       U
          Z  YOU  P
          I  ARE  E
          N  PPL  R
          G       B
            TNKS! 
          

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Because A MARVELOUS A U Z YOU P I ARE E N PPL R G B TNKS! Closing, ciao
          Hide
          Vidya Kumar added a comment -

          Hi,

          I'm using Moodle 2.4.1 (Build: 20130114).

          I created a label that I wanted to be visible only to users who have logged in. So I added a condition that the email ID field should have "@". (This may not have been the best or the smartest approach, but I couldn't think of another way to hide the label for those who haven't logged in.)

          Now the installation seems to have crashed. At least I'm not able to even access my instance. The only screenshot I have is here.

          I'm not sure if this is the best place to log this issue, but any help would be appreciated.

          Thanks...

          Show
          Vidya Kumar added a comment - Hi, I'm using Moodle 2.4.1 (Build: 20130114). I created a label that I wanted to be visible only to users who have logged in. So I added a condition that the email ID field should have "@". (This may not have been the best or the smartest approach, but I couldn't think of another way to hide the label for those who haven't logged in.) Now the installation seems to have crashed. At least I'm not able to even access my instance. The only screenshot I have is here. I'm not sure if this is the best place to log this issue, but any help would be appreciated. Thanks...
          Hide
          Vidya Kumar added a comment -

          Hi,

          I'm using Moodle 2.4.1 (Build: 20130114).

          I created a label that I wanted to be visible only to users who have logged in. So I added a condition that the email ID field should have "@". (This may not have been the best or the smartest approach, but I couldn't think of another way to hide the label for those who haven't logged in.)

          Now the installation seems to have crashed. At least I'm not able to even access my instance. The only screenshot I have is here.

          I'm not sure if this is the best place to log this issue, but any help would be appreciated.

          Thanks...

          Show
          Vidya Kumar added a comment - Hi, I'm using Moodle 2.4.1 (Build: 20130114). I created a label that I wanted to be visible only to users who have logged in. So I added a condition that the email ID field should have "@". (This may not have been the best or the smartest approach, but I couldn't think of another way to hide the label for those who haven't logged in.) Now the installation seems to have crashed. At least I'm not able to even access my instance. The only screenshot I have is here. I'm not sure if this is the best place to log this issue, but any help would be appreciated. Thanks...
          Hide
          Sam Marshall added a comment -

          Vidya: That looks like you are describing this same issue. If you upgrade to the latest Moodle 2.4 stable version (or wait for 2.4.2) then you should find that the problem goes away.

          Show
          Sam Marshall added a comment - Vidya: That looks like you are describing this same issue. If you upgrade to the latest Moodle 2.4 stable version (or wait for 2.4.2) then you should find that the problem goes away.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: