Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-31520

get_users_by_id never returns email, not even to admin user

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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:

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting that.

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting that.
            Hide
            fabiomsouto 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
            fabiomsouto 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
            jerome 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
            jerome 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
            stronk7 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
            stronk7 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
            stronk7 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
            stronk7 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
            poltawski 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
            poltawski 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
            samhemelryk 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
            samhemelryk 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
            andyjdavis 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
            andyjdavis 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
            fabiomsouto Fábio Souto added a comment -

            Hello all,

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

            Cheers,
            Fabio

            Show
            fabiomsouto Fábio Souto added a comment - Hello all, Thank you yet again. I'm happy it got integrated. Cheers, Fabio
            Hide
            stronk7 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
            stronk7 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
            rajeshtaneja 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
            rajeshtaneja 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
            rajeshtaneja Rajesh Taneja added a comment -

            Sorry for false alarm, Seems Petr has fixed this.

            Show
            rajeshtaneja 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:
                  Fix Release Date:
                  9/Jul/12