Moodle
  1. Moodle
  2. MDL-35780

Participants page disclosure of email addresses is inconsistent

    Details

    • Testing Instructions:
      Hide

      Prerequisite

      1. Three students (S1 & S2 & S3) enroled in same course
      2. set moodle/site:viewuseridentity capability for student role.
      3. enable email in showuseridentity user config (Home ► Site administration ► Users ► Permissions ► User policies)

      Set appropriate email display settings for above students

      1. Log in as S1, and edit profile (Home ► My profile settings ► Edit profile)
      2. Change Email display to Hide My email address from everyone
      3. Log in as S2, and edit profile (Home ► My profile settings ► Edit profile)
      4. Change Email display to Allow everyone to see my email address
      5. Log in as S3, and edit profile (Home ► My profile settings ► Edit profile)
      6. Change Email display to Allow only other course members to see my email address.

      Test 1

      1. Log in as S1 and go to course -> participants page and make sure you can see all emails including S1's.
      2. Click on S1 and you should see S1's email.
      3. Log in as S2 and go to course -> participants page and make sure you can see all emails.
      4. Click on S1 and you should see S1's email.
      5. Log in as S3 and go to course -> participants page and make sure you can see all emails.
      6. As admin/teacher unenrol S3 from course and log in as S2.
      7. Go to course -> participants page and make sure you cannot see S3 student and all other emails.

      Test 2

      1. Remove moodle/site:viewuseridentity capability and
      2. Log in as S1 and go to course -> participants page (Select user details on right) and make sure you can see all emails including S1's.
      3. Click on S1 and you should see S1's email.
      4. Log in as S2 and go to course -> participants page (Select user details on right) and make sure you can see all emails except S1's
      5. Click on S1 and you should not see S1's email.
      6. Log in as S3 and go to course -> participants page and make sure you can see all emails except S1's.
      7. As admin/teacher unenrol S3 from course and log in as S2.
      8. Go to course -> participants page and make sure you cannot see S3 student and all other emails except S1's.

      Test 3

      Repeat test 2 by removing moodle/site:viewuseridentity and adding moodle/course:viewhiddenuserfields

      Show
      Prerequisite Three students (S1 & S2 & S3) enroled in same course set moodle/site:viewuseridentity capability for student role. enable email in showuseridentity user config (Home ► Site administration ► Users ► Permissions ► User policies) Set appropriate email display settings for above students Log in as S1, and edit profile (Home ► My profile settings ► Edit profile) Change Email display to Hide My email address from everyone Log in as S2, and edit profile (Home ► My profile settings ► Edit profile) Change Email display to Allow everyone to see my email address Log in as S3, and edit profile (Home ► My profile settings ► Edit profile) Change Email display to Allow only other course members to see my email address. Test 1 Log in as S1 and go to course -> participants page and make sure you can see all emails including S1's. Click on S1 and you should see S1's email. Log in as S2 and go to course -> participants page and make sure you can see all emails. Click on S1 and you should see S1's email. Log in as S3 and go to course -> participants page and make sure you can see all emails. As admin/teacher unenrol S3 from course and log in as S2. Go to course -> participants page and make sure you cannot see S3 student and all other emails. Test 2 Remove moodle/site:viewuseridentity capability and Log in as S1 and go to course -> participants page (Select user details on right) and make sure you can see all emails including S1's. Click on S1 and you should see S1's email. Log in as S2 and go to course -> participants page (Select user details on right) and make sure you can see all emails except S1's Click on S1 and you should not see S1's email. Log in as S3 and go to course -> participants page and make sure you can see all emails except S1's. As admin/teacher unenrol S3 from course and log in as S2. Go to course -> participants page and make sure you cannot see S3 student and all other emails except S1's. Test 3 Repeat test 2 by removing moodle/site:viewuseridentity and adding moodle/course:viewhiddenuserfields
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      wip-mdl-35780-m24
    • Pull Master Branch:
      wip-mdl-35780
    • Rank:
      44532

      Description

      The course participants page (user/index.php) can optionally display a column of student email addresses. Those email addresses show regardless of whether any user has specified, in their profile, that they want to "hide my email address from everyone."

      Site admins can add the extra column by changing $CFG->showuseridentity, which can be found the User Policies section of site configuration.

      Additionally, it is necessary to grant moodle/site:viewuseridentity to Authenticated Users.

      With the site configured as such, imagine two students in the same course:

      Joe configures his profile to "hide my email address from everyone."
      Mary configures her profile to "allow everyone to see my address" or "allow only other course members to see my address."

      When Joe looks at the course participants page, he sees a table of all course members names, email addresses, cities, and countries. That table includes Mary's email address, as it should. If Joe clicks on Mary's course profile, he will see her bio description, and her email address.

      When Mary looks at the course participants page, she sees the same table, which includes Joe's email address. It should not. Interestingly, when Mary clicks on Joe's name to see his course profile, Joe's email address is hidden on that page.

      I believe this inconsistency should be fixed. It's not a major security issue, but it is a disclosure issue. Joe was given the option of hiding his email address from everyone, and Moodle is failing to honor that request.

      I was able to hack up a quick fix to the user/index.php page. My fix blanks out the email address for any user who has elected to "hide from everyone", unless the user viewing the page has the moodle/course:useremail capability (which is granted to instructors by default). Perhaps that is the wrong capability to check.

      My rationale here is that instructors need to have student email addresses, and probably have them anyway using other school databases/systems. I believe students will expect that "hide from everyone" does not mean instructors in their own classes will not be able to see their email addresses.

      Here is my patch, working against Moodle 2.2.5.

      156d155
      < 
      743c742,750
      <                         $data[] = $user->{$field};
      ---
      > 
      >                       // show a null value if the field is the email address and the user wants to hide email from everyone
      >                       // make an exception for instructors (who have a special capability at the course level that lets them email people)
      >                       if (!has_capability('moodle/course:useremail', $coursecontext) && $field==='email' && $user->maildisplay==0) {
      >                           $data[] = '';
      >                       }
      >                       else {
      >                           $data[] = $user->{$field};
      >                       }
      755c762
      < 
      ---
      > 
      

      I suspect that the issue may surface in other places in Moodle. The internal routine, get_external_fields(), is called a dozen times in various places.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          I guess this is a matter of priorities. Should the moodle/site:viewuseridentity override the user's preference to hide their email?

          According to the docs, the purpose of this capability is to show email addresses, so I'm tending to think that by providing this capability to all users you are seeing the expected result.

          If users were seeing email addresses without this capability, then I think this would be a security issue.

          What do you think?

          Show
          Michael de Raadt added a comment - I guess this is a matter of priorities. Should the moodle/site:viewuseridentity override the user's preference to hide their email? According to the docs, the purpose of this capability is to show email addresses, so I'm tending to think that by providing this capability to all users you are seeing the expected result. If users were seeing email addresses without this capability, then I think this would be a security issue. What do you think?
          Hide
          Garret Gengler added a comment - - edited

          > Should the moodle/site:viewuseridentity override the user's preference to hide their email?

          I don't think so. I think it should be the other way around.

          My patch (or something like it*) allows site administrators to make email addresses public, but also allows users to override the setting.

          I think this approach will be simple to implement and intuitive, and is how moodle works now (at least partially).

          On the user/profile.php page, Moodle is honoring the user preferences, regardless of how moodle/site:viewuseridentity is configured.

          Here is the code from user/profile.php, version 2.2.5:

          if (has_capability('moodle/site:viewuseridentity', $context)) {
              $identityfields = array_flip(explode(',', $CFG->showuseridentity));
          } else {
              $identityfields = array();
          }
          ...
          if (isset($identityfields['email']) and ($currentuser
            or $user->maildisplay == 1
            or has_capability('moodle/course:useremail', $context)
            or ($user->maildisplay == 2 and enrol_sharing_course($user, $USER)))) {
              print_row(get_string("email").":", obfuscate_mailto($user->email, ''));
          }
          

          * My patch just fixes the disclosure problem on the Participants page. It would be better to fix this in get_external_fields(), so it applies throughout moodle anywhere users are displayed in a table.

          Show
          Garret Gengler added a comment - - edited > Should the moodle/site:viewuseridentity override the user's preference to hide their email? I don't think so. I think it should be the other way around. My patch (or something like it*) allows site administrators to make email addresses public, but also allows users to override the setting. I think this approach will be simple to implement and intuitive, and is how moodle works now (at least partially). On the user/profile.php page, Moodle is honoring the user preferences, regardless of how moodle/site:viewuseridentity is configured. Here is the code from user/profile.php, version 2.2.5: if (has_capability('moodle/site:viewuseridentity', $context)) { $identityfields = array_flip(explode(',', $CFG->showuseridentity)); } else { $identityfields = array(); } ... if (isset($identityfields['email']) and ($currentuser or $user->maildisplay == 1 or has_capability('moodle/course:useremail', $context) or ($user->maildisplay == 2 and enrol_sharing_course($user, $USER)))) { print_row(get_string("email").":", obfuscate_mailto($user->email, '')); } * My patch just fixes the disclosure problem on the Participants page. It would be better to fix this in get_external_fields(), so it applies throughout moodle anywhere users are displayed in a table.
          Hide
          Michael de Raadt added a comment -

          I don't think a user setting should override a capability. The purpose of capabilities is to allow users to exceed normal users.

          If there is an inconsistency in applying that capability, then perhaps we have an issue. Perhaps the user's email address should be exposed to users with the viewuseridentity capability. The moodle/course:useremail is not as appropriate for this purpose as it refers to the current user's potential to enable/disable another user's email address.

          Show
          Michael de Raadt added a comment - I don't think a user setting should override a capability. The purpose of capabilities is to allow users to exceed normal users. If there is an inconsistency in applying that capability, then perhaps we have an issue. Perhaps the user's email address should be exposed to users with the viewuseridentity capability. The moodle/course:useremail is not as appropriate for this purpose as it refers to the current user's potential to enable/disable another user's email address.
          Hide
          Garret Gengler added a comment -

          Good catch on moodle/course:useremail. From the description, it doesn't sound like the appropriate capability to check for "instructor always needs to email students" rights. My mistake there. I made an assumption because that's what the master code is using in user/profile.php. [A side reaction to that: What a strange capability! I will have to search the forums and the tracker for that one... I wonder why it's considered acceptable for an instructor to disable a student's email site-wide?]

          But back to the inconsistency/disclosure issue...

          Here's how I read the excerpt above from /user/profile.php... am I reading this wrong? I think it already lets user preferences override site config.

          First thing at the top, if the current user has moodle/site:viewuseridentity in the current context, we load up the extra identity fields, as configured in site config -> showuseridentity.

          Then, the email will be displayed if it is one of the extra identify fields and one of the following is true:

          1) the user we are looking at is the current user
          2) the user we are looking at has chosen to share his/her email address with everyone (maildisplay=1)
          3) the current logged in user has a special capability (by default granted to instructors) in the current context that allows them to enable/disable user email addresses
          4) the user we are looking at has chosen to share his/her email address with classmates (maildisplay=2) and the logged in user shares a course

          Note that in the case where maildisplay=0, the email address is not shown, unless you pass test #1 or #3.

          All I'm asking is for Moodle to do the same thing, in other places where the email address might be displayed.

          Thanks for taking time to think this through with me. Sorry if it seems like I'm making a big deal out of a small thing here. We have a number of "directory suppressed" users at our school (folks who often have good reasons for hiding their identity - crime victims, stalker targets, etc.) and many more people who are protective of their personal information. Partly that comes from teaching graduate level studies in information science, copyright law, privacy policies, etc... we are educating people to know and care about these issues. At the same time, we have a large majority of users who want to share their email address – so we can't just turn that off.

          Show
          Garret Gengler added a comment - Good catch on moodle/course:useremail. From the description, it doesn't sound like the appropriate capability to check for "instructor always needs to email students" rights. My mistake there. I made an assumption because that's what the master code is using in user/profile.php. [A side reaction to that: What a strange capability! I will have to search the forums and the tracker for that one... I wonder why it's considered acceptable for an instructor to disable a student's email site-wide?] But back to the inconsistency/disclosure issue... Here's how I read the excerpt above from /user/profile.php... am I reading this wrong? I think it already lets user preferences override site config. First thing at the top, if the current user has moodle/site:viewuseridentity in the current context, we load up the extra identity fields, as configured in site config -> showuseridentity. Then, the email will be displayed if it is one of the extra identify fields and one of the following is true: 1) the user we are looking at is the current user 2) the user we are looking at has chosen to share his/her email address with everyone (maildisplay=1) 3) the current logged in user has a special capability (by default granted to instructors) in the current context that allows them to enable/disable user email addresses 4) the user we are looking at has chosen to share his/her email address with classmates (maildisplay=2) and the logged in user shares a course Note that in the case where maildisplay=0, the email address is not shown, unless you pass test #1 or #3. All I'm asking is for Moodle to do the same thing, in other places where the email address might be displayed. Thanks for taking time to think this through with me. Sorry if it seems like I'm making a big deal out of a small thing here. We have a number of "directory suppressed" users at our school (folks who often have good reasons for hiding their identity - crime victims, stalker targets, etc.) and many more people who are protective of their personal information. Partly that comes from teaching graduate level studies in information science, copyright law, privacy policies, etc... we are educating people to know and care about these issues. At the same time, we have a large majority of users who want to share their email address – so we can't just turn that off.
          Hide
          Michael de Raadt added a comment -

          Hi, Garret.

          I think we have come to a point where we can call this a bug. I agree with your interpretation of what the code is doing and I think we need to review the capability used there.

          When working on this issue, I think it is important that we ensure this behaviour is consistent on the participants list and user profile page (and possibly other areas) and that we are not changing the behaviour expected. If teachers currently have the ability to view student email addresses due to a capability, then they should probably still be able to do so, unless there is a good reason against it.

          Show
          Michael de Raadt added a comment - Hi, Garret. I think we have come to a point where we can call this a bug. I agree with your interpretation of what the code is doing and I think we need to review the capability used there. When working on this issue, I think it is important that we ensure this behaviour is consistent on the participants list and user profile page (and possibly other areas) and that we are not changing the behaviour expected. If teachers currently have the ability to view student email addresses due to a capability, then they should probably still be able to do so, unless there is a good reason against it.
          Hide
          Michael de Raadt added a comment -

          I've modified the summary of this issue slightly to reflect the point we've come to.

          Show
          Michael de Raadt added a comment - I've modified the summary of this issue slightly to reflect the point we've come to.
          Hide
          Rajesh Taneja added a comment - - edited

          Looking at this, it seems information on participants page is fine and email should be visible on profile page.

          AFAIK capability take precedence over individual's selection and hence anyone having moodle/site:viewuseridentity capability should be able to see details.

          As this is default set for teacher/manager role, it is solely used for administration purpose and should be respected over individual's preference.

          Show
          Rajesh Taneja added a comment - - edited Looking at this, it seems information on participants page is fine and email should be visible on profile page. AFAIK capability take precedence over individual's selection and hence anyone having moodle/site:viewuseridentity capability should be able to see details. As this is default set for teacher/manager role, it is solely used for administration purpose and should be respected over individual's preference.
          Hide
          Rajesh Taneja added a comment -

          I have removed security tag from this issue, as less information (email) is displayed to user (no extra information is revealed beyond user capability).

          Show
          Rajesh Taneja added a comment - I have removed security tag from this issue, as less information (email) is displayed to user (no extra information is revealed beyond user capability).
          Hide
          Frédéric Massart added a comment -

          Thanks for working on this Raj, the patch is great and could be pushed as is. I just thought of a few things, which could certainly be raised as another issue.

          Looking at the capability course:useremail, which you removed, and you were right (http://docs.moodle.org/24/en/Capabilities/moodle/course:useremail), I think we should deprecate it as it lost its purpose since dc5586907dc4d when it was introduced and now.

          Also, it is used in a few other places to check if a user can see another's email address, which is wrong. As part of the removal of this capability, we should probably create a function like can_user_see_email() which would prevent duplicating the code which does almost the same checks here and there. Also adding the label docs_required to the raised issue to explain that viewhiddenuserfields and viewuseridentity are also used to control the display of email addresses.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Thanks for working on this Raj, the patch is great and could be pushed as is. I just thought of a few things, which could certainly be raised as another issue. Looking at the capability course:useremail, which you removed, and you were right ( http://docs.moodle.org/24/en/Capabilities/moodle/course:useremail ), I think we should deprecate it as it lost its purpose since dc5586907dc4d when it was introduced and now. Also, it is used in a few other places to check if a user can see another's email address, which is wrong. As part of the removal of this capability, we should probably create a function like can_user_see_email() which would prevent duplicating the code which does almost the same checks here and there. Also adding the label docs_required to the raised issue to explain that viewhiddenuserfields and viewuseridentity are also used to control the display of email addresses. Cheers, Fred
          Hide
          Rajesh Taneja added a comment -

          Thanks Fred,

          I have created issue to deprecate course:useremail capability.

          At this point I am not sure if we can combine these check for all places. Probably a good thing to look at while fixing MDL-37479

          Show
          Rajesh Taneja added a comment - Thanks Fred, I have created issue to deprecate course:useremail capability. At this point I am not sure if we can combine these check for all places. Probably a good thing to look at while fixing MDL-37479
          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
          Sam Hemelryk added a comment -

          Thanks Raj, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Raj, this has been integrated now
          Hide
          Mark Nelson added a comment -

          Hi Raj, Test 1 step 1 failed for me, I did not continue after this. I was not able to see S1's email on the participants page. All the emails are hidden until you change the 'User list' select box to 'User details' (which I will add to the testing instructions). Once I changed this to 'User details' I was able too see S2 and S3's emails, but not S1's.

          Show
          Mark Nelson added a comment - Hi Raj, Test 1 step 1 failed for me, I did not continue after this. I was not able to see S1's email on the participants page. All the emails are hidden until you change the 'User list' select box to 'User details' (which I will add to the testing instructions). Once I changed this to 'User details' I was able too see S2 and S3's emails, but not S1's.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (sending back to testing as requested)

          Show
          Eloy Lafuente (stronk7) added a comment - (sending back to testing as requested)
          Hide
          Mark Nelson added a comment -

          Hi Raj, sorry I did not enable the moodle/site:viewuseridentity capability earlier which is why I prematurely failed the test. However, in Test 2 after removing the capability for the student role I ended up running into the same issue. Logged in as S1 I was not able to see S1's email on the participants page but was able to see it when I clicked on 'Full profile'.

          Show
          Mark Nelson added a comment - Hi Raj, sorry I did not enable the moodle/site:viewuseridentity capability earlier which is why I prematurely failed the test. However, in Test 2 after removing the capability for the student role I ended up running into the same issue. Logged in as S1 I was not able to see S1's email on the participants page but was able to see it when I clicked on 'Full profile'.
          Hide
          Rajesh Taneja added a comment -

          Hello Mark,

          Did you change user list = user details in right top select box?
          If you don't have capability then email column is not shown with user list = brief.

          Show
          Rajesh Taneja added a comment - Hello Mark, Did you change user list = user details in right top select box? If you don't have capability then email column is not shown with user list = brief.
          Hide
          Rajesh Taneja added a comment -

          Thanks for pointing that Mark.

          I have added another commit to fix this. Sam can you please pick this.

          Show
          Rajesh Taneja added a comment - Thanks for pointing that Mark. I have added another commit to fix this. Sam can you please pick this.
          Hide
          Dan Poltawski added a comment -

          Pulled that in as requested.

          Show
          Dan Poltawski added a comment - Pulled that in as requested.
          Hide
          Rajesh Taneja added a comment -

          Thanks Dan and Mark.

          Show
          Rajesh Taneja added a comment - Thanks Dan and Mark.
          Hide
          Mark Nelson added a comment -

          Hi Raj, passes now.

          Show
          Mark Nelson added a comment - Hi Raj, passes now.
          Hide
          Dan Poltawski added a comment -

          Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          Show
          Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: