Moodle
  1. Moodle
  2. MDL-43916

User email addresses shown when setting and capabilities do not allow it

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.5.4, 2.6.1
    • Fix Version/s: 2.4.9, 2.5.5, 2.6.2
    • Component/s: Forum, Quiz
    • Labels:
    • Testing Instructions:
      Hide
      1. Set $CFG->showuseridentity to include email.
      2. Set non-editing tetcher role to not have moodle/site:viewuseridentity in some course (this may be default?)
      3. Enrol a student in that course.
      4. As admin, go to the two places affected by this patch, and verify that you can see the email addresses.
        • Manage forum subscribers.
        • Quiz add user override.
      5. As non-editing teacher, go to those two pages (this may require overriding some capabilities) and verify that you cannot see the email addresses there.
      Show
      Set $CFG->showuseridentity to include email. Set non-editing tetcher role to not have moodle/site:viewuseridentity in some course (this may be default?) Enrol a student in that course. As admin, go to the two places affected by this patch, and verify that you can see the email addresses. Manage forum subscribers. Quiz add user override. As non-editing teacher, go to those two pages (this may require overriding some capabilities) and verify that you cannot see the email addresses there.
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE

      Description

      The browse list of users in "Forum Subscriber" (Forum administration > Show/edit current subscribers) and the user selector when changing a quiz setting for a particular user (Quiz administration > User overrides > Add user override - Override user field) is not taking into account the $CFG->showuseridentity config settings and the moodle/site:viewuseridentity capability to see emails.

      Steps to reproduce it:
      1. Create a course
      2. Create a quiz in the course.
      3. Go to Site administration > Users > Permissions > User policies and make sure you have all checkboxes unchecked for the "Show user identity" setting.
      4. Make sure the capability to see full user identity in lists (moodle/site:viewuseridentity) is set to 'No'
      5. Go to the course and click on "News forum", then go to Forum administration > Show/edit current subscribers
      Result: The list of subscribers and their emails.
      Expected: The list of subscribers without their emails.
      6. Go to the course an click over the quiz created in the step #2, then go to Quiz administration > User overrides > Add user override and see the Override user field.
      Result: The user selector contains a list of users and their emails.
      Expected: The user selector contains a list of users with just their full names (without emails).

        Gliffy Diagrams

          Activity

          Hide
          Maria Torres added a comment -

          A patch for this issue for your review

          Show
          Maria Torres added a comment - A patch for this issue for your review
          Hide
          Michael de Raadt added a comment -

          Thanks for this (very good) report and for providing a patch.

          I can verify this is a problem.

          Show
          Michael de Raadt added a comment - Thanks for this (very good) report and for providing a patch. I can verify this is a problem.
          Hide
          Tim Hunt added a comment -

          Maria does not seem to be a developer in the tracker, so I can't assign this to her.

          Also, a comment that while this is technically a security issue. I don't see how anyone could exploit it to do harm, so I see no point in delaying integration.

          Comments on the code to follow.

          Show
          Tim Hunt added a comment - Maria does not seem to be a developer in the tracker, so I can't assign this to her. Also, a comment that while this is technically a security issue. I don't see how anyone could exploit it to do harm, so I see no point in delaying integration. Comments on the code to follow.
          Hide
          Tim Hunt added a comment -

          As Michael says. Thank you for not just detecting this, but also for the patch.

          The patch looks good, though being really picky, could I ask for a change of coding style in the quiz bit:

          • There was no reason to change the line-wrap on the if statement.
          • I don't like the ? : operator. It is not good for readability. I think a normal if statement hear would be clearer.

          One other thought: You had to repeat very similar logic in two places for $canviewemail. Is it worth adding a library method can_view_user_email($context)? You could add such a function next to get_extra_user_fields in moodelib.php.

          Thanks!

          Show
          Tim Hunt added a comment - As Michael says. Thank you for not just detecting this, but also for the patch. The patch looks good, though being really picky, could I ask for a change of coding style in the quiz bit: There was no reason to change the line-wrap on the if statement. I don't like the ? : operator. It is not good for readability. I think a normal if statement hear would be clearer. One other thought: You had to repeat very similar logic in two places for $canviewemail. Is it worth adding a library method can_view_user_email($context)? You could add such a function next to get_extra_user_fields in moodelib.php. Thanks!
          Hide
          Maria Torres added a comment -

          Hi Tim, thanks for your comments.

          I've changed the code based on your suggestions. Here the new patch

          Regards,
          Maria

          Show
          Maria Torres added a comment - Hi Tim, thanks for your comments. I've changed the code based on your suggestions. Here the new patch Regards, Maria
          Hide
          Tim Hunt added a comment -

          Thanks Maria. I think it is ready to be integrated.

          To INTEGRATORS, not sure this is really a security thing. I think it could be integrated now, rather than held?

          Show
          Tim Hunt added a comment - Thanks Maria. I think it is ready to be integrated. To INTEGRATORS, not sure this is really a security thing. I think it could be integrated now, rather than held?
          Hide
          CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          Maria Torres added a comment -

          I miss something in the patch. In the can_view_user_email($context) function when I ask for has_capability it should be $context and no $this->context. Please change it. Sorry, I copied and pasted and I didn't realise this mistake.

          Regards,
          Maria

          Show
          Maria Torres added a comment - I miss something in the patch. In the can_view_user_email($context) function when I ask for has_capability it should be $context and no $this->context. Please change it. Sorry, I copied and pasted and I didn't realise this mistake. Regards, Maria
          Hide
          Tim Hunt added a comment -

          Oops! I should have spotted that. Hopefully Marina can fix that during integration.

          Show
          Tim Hunt added a comment - Oops! I should have spotted that. Hopefully Marina can fix that during integration.
          Hide
          Marina Glancy added a comment -

          hi Maria and Tim,
          I'm making a review anyway regardless whether we consider it security or not.

          first thing that I want to notice is that 'moodle/site:viewuseridentity' is a course-level capability and there is no point of retrieving forum module context to check it, we can just use course context. Besides, get_coursemodule_from_instance() performs extra DB queries and it is very not recommended for performance reasons. FYI if you are operating with the current course, use get_fast_modinfo()

          second, I'm not sure that function can_view_user_email($context) is necessary at all. There is an existing way to quickly to check it:

          in_array('email', get_extra_user_fields($context))
          

          Show
          Marina Glancy added a comment - hi Maria and Tim, I'm making a review anyway regardless whether we consider it security or not. first thing that I want to notice is that 'moodle/site:viewuseridentity' is a course-level capability and there is no point of retrieving forum module context to check it, we can just use course context. Besides, get_coursemodule_from_instance() performs extra DB queries and it is very not recommended for performance reasons. FYI if you are operating with the current course, use get_fast_modinfo() second, I'm not sure that function can_view_user_email($context) is necessary at all. There is an existing way to quickly to check it: in_array('email', get_extra_user_fields($context))
          Hide
          Tim Hunt added a comment -

          Marina.

          1. It is necessary to check it in the Forum context. (Although you cannot override the capability in the Forum context, the user could be assigned an extra 'Moderator' role in just one forum, which which case they may get the ability to manage subscribers, but without being allowed to see email addresses.)

          2. Looks like using get_extra_user_fields is a better way.

          Show
          Tim Hunt added a comment - Marina. 1. It is necessary to check it in the Forum context. (Although you cannot override the capability in the Forum context, the user could be assigned an extra 'Moderator' role in just one forum, which which case they may get the ability to manage subscribers, but without being allowed to see email addresses.) 2. Looks like using get_extra_user_fields is a better way.
          Hide
          Maria Torres added a comment -

          Yes, the in_array('email', get_extra_user_fields($context)) does all the validations.
          Thanks for this tip and the use of get_fast_modinfo().

          Very useful information.

          Show
          Maria Torres added a comment - Yes, the in_array('email', get_extra_user_fields($context)) does all the validations. Thanks for this tip and the use of get_fast_modinfo(). Very useful information.
          Hide
          Marina Glancy added a comment -

          Tim, sorry, I did not understand your comment about the forum context here. How can extra role on module level overwrite the course-level capability 'moodle/site:viewuseridentity' ?

          Show
          Marina Glancy added a comment - Tim, sorry, I did not understand your comment about the forum context here. How can extra role on module level overwrite the course-level capability 'moodle/site:viewuseridentity' ?
          Hide
          Tim Hunt added a comment -

          Marina. It can. It would be more educational if you work this out yourself. If you really want, I can explain.

          Show
          Tim Hunt added a comment - Marina. It can. It would be more educational if you work this out yourself. If you really want, I can explain.
          Hide
          Marina Glancy added a comment -

          Tim, I'm only talking about those lines added in the patch:

          +            $cm = get_coursemodule_from_instance('forum', $forum->id, $course->id, false, MUST_EXIST);
          +            $canviewemail = can_view_user_email(context_module::instance($cm->id));
          

          they can be replaced with:

          $canviewemail = can_view_user_email(context_course::instance($course->id));
          

          (rather get_extra_user_fields() but this is not the point here).

          I'm not talking about checking the capability to manage subscribers, this is a separate check and it is performed on forum context as it should.

          Show
          Marina Glancy added a comment - Tim, I'm only talking about those lines added in the patch: + $cm = get_coursemodule_from_instance('forum', $forum->id, $course->id, false, MUST_EXIST); + $canviewemail = can_view_user_email(context_module::instance($cm->id)); they can be replaced with: $canviewemail = can_view_user_email(context_course::instance($course->id)); (rather get_extra_user_fields() but this is not the point here). I'm not talking about checking the capability to manage subscribers, this is a separate check and it is performed on forum context as it should.
          Hide
          Simon Coggins added a comment -

          I think what Tim is saying is:

          Imagine a course with a 'teacher' role that has both 'manage subscribers' and 'view user identity' capabilities set to "allow". If a user has the 'teacher' role in the course context they will be able to manage all subscribers and view all their email addresses in any forum inside that course.

          Now imagine a custom, new role called "moderator", which has 'view user identity' set to PROHIBIT. If this role was assigned to the same teacher in the context of one particular forum (not at the course level), then they WOULD be able to get the manage subscribers page for that forum, but they SHOULDN'T see the user's email address (in that forum only).

          That will only happen correctly if the capability is checked in the module context.

          The fact that 'view user identity' is a course level capability prevents it being overridden in the module context but it doesn't prevent a completely different role being assigned in that context.

          Show
          Simon Coggins added a comment - I think what Tim is saying is: Imagine a course with a 'teacher' role that has both 'manage subscribers' and 'view user identity' capabilities set to "allow". If a user has the 'teacher' role in the course context they will be able to manage all subscribers and view all their email addresses in any forum inside that course. Now imagine a custom, new role called "moderator", which has 'view user identity' set to PROHIBIT. If this role was assigned to the same teacher in the context of one particular forum (not at the course level), then they WOULD be able to get the manage subscribers page for that forum, but they SHOULDN'T see the user's email address (in that forum only). That will only happen correctly if the capability is checked in the module context. The fact that 'view user identity' is a course level capability prevents it being overridden in the module context but it doesn't prevent a completely different role being assigned in that context.
          Hide
          Marina Glancy added a comment -

          ok, convinced thanks for explanation Simon

          Show
          Marina Glancy added a comment - ok, convinced thanks for explanation Simon
          Hide
          Marina Glancy added a comment -

          Maria, Tim, we are holding this issue until the next minor release because it is marked as security.

          Please amend the patch as discussed above meanwhile. Do not delay it because there might be an emergency minor release after the next integration cycle.

          Show
          Marina Glancy added a comment - Maria, Tim, we are holding this issue until the next minor release because it is marked as security. Please amend the patch as discussed above meanwhile. Do not delay it because there might be an emergency minor release after the next integration cycle.
          Hide
          Maria Torres added a comment -

          Tim, Marina. I've attached the patch with the amendments.

          Please let me know if it is ok or I have to change something.

          Thanks,
          Maria

          Show
          Maria Torres added a comment - Tim, Marina. I've attached the patch with the amendments. Please let me know if it is ok or I have to change something. Thanks, Maria
          Hide
          Dan Poltawski added a comment -

          Thanks Maria - integrated to master, 26, 25 and 24

          Show
          Dan Poltawski added a comment - Thanks Maria - integrated to master, 26, 25 and 24
          Hide
          Marina Glancy added a comment -

          Thanks all. Test passed

          Show
          Marina Glancy added a comment - Thanks all. Test passed
          Hide
          Eloy Lafuente (stronk7) added a comment -

          For fun: http://www.youtube.com/watch?v=IGENkpaPkgw

          Many thanks for your hard work, this is now part of Moodle!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - For fun: http://www.youtube.com/watch?v=IGENkpaPkgw Many thanks for your hard work, this is now part of Moodle! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: