[drupal-devel] [bug] user_access returns a string not a boolean as specified

[moshe weitzman]

Issue status update for http://drupal.org/node/21566

 Project:      Drupal
 Version:      cvs
 Component:    user system
 Category:     bug reports
 Priority:     minor
 Assigned to:  deekayen
 Reported by:  gjf
 Updated by:   moshe weitzman
 Status:       patch

and the fact that you obviously did not test your patch with any user
beside uid=1. considering that the code branch you changed doesn't even
apply to uid=1, i'd say that this was pretty reckless.


other than that, welcome to the project. we look forward to your future
patches ...




moshe weitzman



Previous comments:
------------------------------------------------------------------------

April 29, 2005 - 01:53 : gjf

user_access() function  returns a string not a boolean.
This caused memory consumption problems for us due to a bug elsewhere
in our code.
i.e.
return strstr($perm[$account->uid], "$string, ");
should probably be
if ( strstr($perm[$account->uid], "$string, "))
{
   return true;
} else
{
   return false;
}




------------------------------------------------------------------------

June 20, 2005 - 03:35 : deekayen

Attachment: http://drupal.org/files/issues/user_access.returnbooleans.diff (1.28 KB)

Attached patch has a spelling correction and makes user_access() return
only booleans. Diff'ed against HEAD.




------------------------------------------------------------------------

June 20, 2005 - 03:55 : deekayen

Attachment: http://drupal.org/files/issues/user_access.returnbooleans_0.diff (1.59 KB)

It bothered me to have a string search instead of an array search for
permissions. This patch does what the previous patch does, but it uses
in_array() on the static $perm instead of strstr() on a string.




------------------------------------------------------------------------

June 20, 2005 - 04:02 : killes at www.drop.org

Can you please provide a benchmark test of the strstr approach vs the
in_array one? I am concerned that it might be slower. user_access gets
called quite often.




------------------------------------------------------------------------

June 20, 2005 - 04:47 : deekayen

That's a fair request. The output of the following code on my WinXP,
PHP5.1-dev, 2.133 Ghz Athlon is:


0.29749894142151
0.29712684249878
1.0804200172424
0.56814384460449
0.10924220085144
0.12113380432129
0.13284420967102


<?php
function utime($a = '')
{
  static $utime;
  $time  = explode(' ',  microtime());
  $usec  = (double)$time[0];
  $sec   = (double)$time[1];
  switch($a) {
    case 'bm':
      $to_return = $usec + $sec - $utime;
      break;
    default:
      $utime = $usec + $sec;
      return $utime;
      break;
  }
  $utime = $usec + $sec;
  return $to_return;
}
$account->uid = 1;
utime();
for($i=0; $i<500000; $i+=1) {
  if ($account->uid == 1) {
    // blah
  }
}
echo utime('bm') .'<br />';
for($i=0; $i<500000; $i+=1) {
  if ((int)$account->uid === 1) {
    // blah
  }
}
echo utime('bm') .'<br />';
for($i=0; $i<50; $i+=1) {
  $result[$i] = '';
  for($j=0; $j>10; $j+=1) {
    $result[$i] .= chr(rand(65-126));
  }
}
$perm[$account->uid] = '';
utime();
for($i=0; $i<2000; $i+=1) {
  foreach($result as $row) {
    $perm[$account->uid] .= "$row, ";
  }
  isset($perm[$account->uid]) && strstr($perm[$account->uid],
"{$result['45']}, ");
}
echo utime('bm') .'<br />';
$perm[$account->uid] = array();
utime();
for($i=0; $i<2000; $i+=1) {
  foreach($result as $row) {
    $perm[$account->uid][] = $row;
  }
  isset($perm[$account->uid]) && in_array($result['45'],
$perm[$account->uid]);
}
echo utime('bm') .'<br />';
for($i=0; $i<500000; $i+=1) {
  // blah
}
echo utime('bm') .'<br />';
for($i=0; $i<500000; $i++) {
  // blah
}
echo utime('bm') .'<br />';
for($i=0; $i<500000; $i = $i+1) {
  // blah
}
echo utime('bm') .'<br />';
?>




------------------------------------------------------------------------

June 20, 2005 - 05:01 : deekayen

Actually, there's a bug in the benchmark code that causes $result to not
populate. Switch the following:


  for($j=0; $j>10; $j+=1) {


to


  for($j=0; $j<10; $j+=1) {


Then the string search is more like 8.1727991104126 seconds verses like
0.46953892707825 for the array search, an even bigger difference.




------------------------------------------------------------------------

June 21, 2005 - 13:45 : Dries

Committed to HEAD.




------------------------------------------------------------------------

June 22, 2005 - 15:31 : killes at www.drop.org

Please revert this patch, it isn't working at all. Drupal HEad is
non-workign for all users which aren't No. 1.




------------------------------------------------------------------------

June 22, 2005 - 15:37 : moshe weitzman

yeah - this breaks HEAD completely. here is a possible fix, if we still
want to use in_array() for checking. this is in user_access() where we
build the permission array:



<?php
while ($row = db_fetch_object($result)) {
      foreach (explode(',', $row->perm) as $p) {
        $perm[$account->uid][] = trim($p);
      }
}
?>






------------------------------------------------------------------------

June 22, 2005 - 18:10 : deekayen

Attachment: http://drupal.org/files/issues/user_access.returnbooleans_1.diff (763 bytes)

I haven't found a case where trim() would be necessary if you explode on
', ' (with the space). I made a patch so things are hopefully less
broken. I'm working on the benchmark people keep asking me for in
#drupal.




------------------------------------------------------------------------

June 22, 2005 - 20:35 : deekayen

Attachment: http://drupal.org/files/issues/user_access.returnbooleans_2.diff (884 bytes)

in_array() was faster, but explode() is slow, so this reverts back to
strstr. I hope I can blame my newbieness to Drupal internals on this,
but I should have looked to see how permissions were stored before just
assuming each loop through db_fetch_object was a separate permission
instead of a string of permissions.