[drupal-devel] [bug] Remove code containing user access controls
from profile theme functions
robertDouglass
drupal-devel at drupal.org
Sat Sep 3 07:45:16 UTC 2005
Issue status update for
http://drupal.org/node/27949
Post a follow up:
http://drupal.org/project/comments/add/27949
Project: Drupal
Version: cvs
Component: profile.module
Category: bug reports
Priority: normal
Assigned to: robertDouglass
Reported by: robertDouglass
Updated by: robertDouglass
Status: patch (ready to be committed)
PS why didn't your follow up send a mail to the devel-list, Dries?
robertDouglass
Previous comments:
------------------------------------------------------------------------
Mon, 01 Aug 2005 12:32:07 +0000 : robertDouglass
Attachment: http://drupal.org/files/issues/profile_fix_acces_control_in_theme.txt (2.36 KB)
The two theme functions in profile.module both violate good theming
practice by running user control logic in the middle of them. Worse
yet, this isn't immediately visible since it happens in yet another
function. Thus themers overriding these functions to style profile
pages[1] inadvertently break access control, thus leading to the
misperception that overriding theme functions is inherently
dangerous[2].
[1] http://drupal.org/node/16011
[2] http://drupal.org/node/16821
------------------------------------------------------------------------
Thu, 18 Aug 2005 12:37:40 +0000 : robertDouglass
patch still applies. Anyone care to review?
------------------------------------------------------------------------
Thu, 18 Aug 2005 12:41:46 +0000 : Dublin Drupaller
Would like to review and help Robert, but I don't have a CVS version of
drupal installed..will the patch work with 4.6.x?
Dub
------------------------------------------------------------------------
Thu, 18 Aug 2005 12:45:25 +0000 : chx
Just by looking at the code: -1. Never write $user we use $account in
these places. There is a global $user and you do not want accidental
mixup.
If you change that, I think I'll like it :)
------------------------------------------------------------------------
Thu, 18 Aug 2005 13:43:33 +0000 : robertDouglass
chx: $account is already in use and I didn't want to replace it
($account = user_load()) - or do you think that would be ok in this
case?
------------------------------------------------------------------------
Thu, 18 Aug 2005 20:26:20 +0000 : chx
feel free to overwrite, it's common with nodes to do similar.
------------------------------------------------------------------------
Fri, 19 Aug 2005 15:10:39 +0000 : robertDouglass
Attachment: http://drupal.org/files/issues/profile_fix_acces_control_in_theme2.txt (2.95 KB)
updated with $account
------------------------------------------------------------------------
Fri, 02 Sep 2005 16:12:21 +0000 : killes at www.drop.org
Attachment: http://drupal.org/files/issues/profile_2.patch (3.09 KB)
I've re-rolled th epatch from Drupal root dir and without DOS line
endings. I have no objections against this patch and I think the
intention is a good one..
------------------------------------------------------------------------
Fri, 02 Sep 2005 22:28:38 +0000 : Dries
I don't see why a theme function can't call helper functions? At least
you can by-pass the helper function to theme things differently. If
you pass pre-rendered fields to the theme function, the themer has less
flexibility. Please re-open if I missed the point.
------------------------------------------------------------------------
Sat, 03 Sep 2005 07:41:49 +0000 : robertDouglass
The helper function in quetion, profile_view_field($user, $field)) ,
implements all of the access permission checks for the entire module's
output. It does two things; it sees whether the current user is allowed
to see the field, and it builds the standard output for the field.
Building the ouput usually means calling functions like check_plain()
or l().
The entire point of the patch can be seen here:
Before
foreach ($fields as $field) {
if ($value = profile_view_field($user, $field)) {
$output .= " <div class=\"field\">$value</div>\n";
}
}
In this code, the profile_view_field($user, $field) call filters the
$field array based on user permissions. It returns a non-themable, pre
built piece of output called $value.
After
foreach ($fields as $field) {
if ($field->value) {
$output .= " <div class=\"field\">$value</div>\n";
}
}
The $fields array has already been filtered using profile_view_field so
that only the fields that the current user is allowed to see are in it.
What does a themer do with a theme function like this? In the first
version, they take the $fields array and start extracting values from
it to display the unthemable information as desired. Oops, user access
goes out the window. In the second version, the themer extracts *the
same*, unthemable information from the $fields array to display it as
desired. No problems with access control.
People who were theming this using the current theme functions were
ending up with broken permissions and were, as a result, re-programming
them at the theme level and writing in the handbook and forums that
PHPTemplate doesn't respect permissions.
More information about the drupal-devel
mailing list