Moodle
  1. Moodle
  2. MDL-31520

get_users_by_id never returns email, not even to admin user

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.5, 2.2.1, 2.3
    • Fix Version/s: 2.1.7, 2.2.4
    • Component/s: Web Services
    • Labels:
    • Rank:
      38070

      Description

      Hello again, another bug unfortunately

      This function should return the email of the user on some conditions, namely:

      1) if the invoking user was a site admin, or
      2) if the invoking user could check hidden user details (moodle/user:viewhiddendetails , although I'm not 100% certain on this one)

      Neither of the conditions are met, because the underlying function to get user details, namely user_get_user_details() only returns email if those conditions are met:

      1) if the capability 'moodle/course:useremail' is true, which will never be upon this webservice call
      2) if the user has the email visible to everyone
      3) or if the invoking user is in the same course of the requested user and the requested user is allowing email sharing only for members of his courses

      So , the underlying function does not check if the calling user is admin, which I think it's wrong, because the admin should have access to the e-mail of all users, and also the users that have the capability moodle/user:viewhiddendetails should as well, but on this one I am not sure.

      I was developing the implementation for get_users() when I stumbled upon this bug. This might be critical for some integrations I think (at least on mine is), because normally an email is an important key.

      Edit: I forgot to mention that there is only one condition on which the e-mail is returned: when the invoking user is the same as the requested user.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting that.

          Show
          Michael de Raadt added a comment - Thanks for reporting that.
          Hide
          Fábio Souto added a comment -

          Here goes a fix for this one

          https://github.com/fabiomsouto/moodle/compare/MOODLE_22_STABLE...MDL-31520-MOODLE_22_STABLE

          The function user_get_user_details is used at 3 functions, on enrol/externallib.php and user/externallib.php, so these also suffer from the same bug.
          My fix allows the admin user to have access to the email in any situation, which seems to me the correct behaviour.

          The remaining behaviour can be added at the externallib.php functions. For example, I think that the users with the capability moodle/user:viewhiddendetails or moodle/user:update should be able to consult another user's email, but I can be wrong on this one of course. If so, these fields can be added to the answer, like it is being done on user/externallib.php, lines 395- 403 aprox.

          Cheers
          Fabio

          Show
          Fábio Souto added a comment - Here goes a fix for this one https://github.com/fabiomsouto/moodle/compare/MOODLE_22_STABLE...MDL-31520-MOODLE_22_STABLE The function user_get_user_details is used at 3 functions, on enrol/externallib.php and user/externallib.php, so these also suffer from the same bug. My fix allows the admin user to have access to the email in any situation, which seems to me the correct behaviour. The remaining behaviour can be added at the externallib.php functions. For example, I think that the users with the capability moodle/user:viewhiddendetails or moodle/user:update should be able to consult another user's email, but I can be wrong on this one of course. If so, these fields can be added to the answer, like it is being done on user/externallib.php, lines 395- 403 aprox. Cheers Fabio
          Hide
          Jérôme Mouneyrac added a comment -

          Peer reviewed. It seems a good fix to me. There are some cases where no capability are checked (when $course is empty) so I think it is necessary to add a check if the user is admin.

          Cheery-pick into 2.1 and master too.

          Show
          Jérôme Mouneyrac added a comment - Peer reviewed. It seems a good fix to me. There are some cases where no capability are checked (when $course is empty) so I think it is necessary to add a check if the user is admin. Cheery-pick into 2.1 and master too.
          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 -

          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
          Dan Poltawski 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
          Dan Poltawski 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 Fabio, this has been integrated now (sorry about the delay).
          During integration I did make one change worth noting. I moved the is admin check to the front of that queue of or's as an optimisation (bool check is quicker than some of the other checks there).

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Fabio, this has been integrated now (sorry about the delay). During integration I did make one change worth noting. I moved the is admin check to the front of that queue of or's as an optimisation (bool check is quicker than some of the other checks there). Cheers Sam
          Hide
          Andrew Davis added a comment -

          This works fine when I use Moodle's built in test client at /admin/webservice/testclient.php. I eventually got this working with the PHP REST client from github. Passing.

          The testing instructions with this could have been more thorough. There are a bunch of steps that are required but which are not mentioned. In particular the process to create a user token and the need to enable the REST protocol.

          Show
          Andrew Davis added a comment - This works fine when I use Moodle's built in test client at /admin/webservice/testclient.php. I eventually got this working with the PHP REST client from github. Passing. The testing instructions with this could have been more thorough. There are a bunch of steps that are required but which are not mentioned. In particular the process to create a user token and the need to enable the REST protocol.
          Hide
          Fábio Souto added a comment -

          Hello all,

          Thank you yet again. I'm happy it got integrated.

          Cheers,
          Fabio

          Show
          Fábio Souto added a comment - Hello all, Thank you yet again. I'm happy it got integrated. Cheers, Fabio
          Hide
          Eloy Lafuente (stronk7) added a comment -

          We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3.

          Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish.

          Many thanks for your collaboration!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3. Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish. Many thanks for your collaboration! Ciao
          Hide
          Rajesh Taneja added a comment -

          Sorry guys,

          It seems on 21 it was wrongly merged.
          +if ($isadmin
          + of $currentuser

          It should be or and not of

          Can you please rectify this or should this be handled by new issue ?

          Show
          Rajesh Taneja added a comment - Sorry guys, It seems on 21 it was wrongly merged. +if ($isadmin + of $currentuser It should be or and not of Can you please rectify this or should this be handled by new issue ?
          Hide
          Rajesh Taneja added a comment -

          Sorry for false alarm, Seems Petr has fixed this.

          Show
          Rajesh Taneja added a comment - Sorry for false alarm, Seems Petr has fixed this.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: