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

Does messaging need to check a capbility before displaying user full name's?

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.4, 2.3, 2.4
    • Fix Version/s: 2.2.5, 2.3.2
    • Component/s: Messages
    • Labels:

      Description

      There are a number of places within /message where we call fullname(). Do we need to be doing a capability check? If so, what do we display instead?

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              salvetore Michael de Raadt added a comment -

              The use of fullname() is good. If we are not using fullname(), that's a problem.

              The other possible problem is if the display format of the name is overridden by sending a second argument with true value to fullname(). If this is happening without checking for the capability moodle/site:viewfullnames, then this is a problem.

              I had a quick scan through /message and I found...

              edit.php, line 183

              $userfullname     = fullname($user, true);

              There are some capability checks earlier in the file, but they are not for moodle/site:viewfullnames, so I'm not sure if this is sufficient.

              All other calls to fullname() in that directory don't use a second argument.

              Show
              salvetore Michael de Raadt added a comment - The use of fullname() is good. If we are not using fullname(), that's a problem. The other possible problem is if the display format of the name is overridden by sending a second argument with true value to fullname(). If this is happening without checking for the capability moodle/site:viewfullnames, then this is a problem. I had a quick scan through /message and I found... edit.php, line 183 $userfullname = fullname($user, true); There are some capability checks earlier in the file, but they are not for moodle/site:viewfullnames, so I'm not sure if this is sufficient. All other calls to fullname() in that directory don't use a second argument.
              Hide
              salvetore Michael de Raadt added a comment -

              Interestingly, that variable from edit.php is not used later in the file, so the statement could probably be removed.

              Show
              salvetore Michael de Raadt added a comment - Interestingly, that variable from edit.php is not used later in the file, so the statement could probably be removed.
              Hide
              andyjdavis Andrew Davis added a comment - - edited

              I've removed the unnecessary fullname() call. Other than that I think we're using fullname correctly and are not outputting user names without using fullname().

              Putting this up for peer review.

              Show
              andyjdavis Andrew Davis added a comment - - edited I've removed the unnecessary fullname() call. Other than that I think we're using fullname correctly and are not outputting user names without using fullname(). Putting this up for peer review.
              Hide
              ankit_frenz Ankit Agarwal added a comment -

              Looks good Andrew.
              +1 for integration.
              Thanks

              Show
              ankit_frenz Ankit Agarwal added a comment - Looks good Andrew. +1 for integration. Thanks
              Hide
              andyjdavis Andrew Davis added a comment -

              Adding branches and putting up for integration.

              Show
              andyjdavis Andrew Davis added a comment - Adding branches and putting up for integration.
              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 -

              (taking rid of the security flag for this)

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - (taking rid of the security flag for this)
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Integrated (22, 23 & master), thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (22, 23 & master), thanks!
              Hide
              phalacee Jason Fowler added a comment -

              works without errors Andrew, thanks

              Show
              phalacee Jason Fowler added a comment - works without errors Andrew, thanks
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              I'm so proud...of you, many thanks!

              http://youtu.be/n64CdfDRnZY

              Closing as fixed, ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - I'm so proud...of you, many thanks! http://youtu.be/n64CdfDRnZY Closing as fixed, ciao

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    10/Sep/12