Needs work
Project:
User Points
Version:
7.x-2.x-dev
Component:
Code: userpoints
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
14 Oct 2010 at 05:00 UTC
Updated:
6 Jan 2012 at 06:38 UTC
Jump to comment: Most recent file
Comments
Comment #1
recrit commentedthe attached patch adds a permission check to userpoints_preprocess_author_pane()
To better utilize the static cache with this additional access check, I revamped the code to ensure that the check only happens once for multiple author panes of the same account id. The original code's cache was not correctly being used so some cleanup was needed, ie
$cached_userpoints_points = $variables['userpoints_points'];should have been$cached_userpoints_points[$account_id].Comment #2
michelleI wish you hadn't combined two issues into one. That makes it a pain for issues like this where I want to move one of the issues. I'll file a separate issue for the broken caching.
As for the permissions, AP is just using the Userpoints API. The API should have permission checking in it. When I check for a user's posts in User Stats, for example, it returns FALSE if the user does not have permission to view them. I don't think there even were permissions when I implemented the API so changes like this is exactly why it needs to be encapsulated.
This is yet another example of why I want the modules that are integrating with Author Pane to have their integration code with the module that's being integrated. I don't even use User Points and so never find out about changes to it until someone comes along and says there's a bug in AP because they changed something. On the flip side, I actively make sure any maintainers of AP integration code are aware of any changes on the AP end by providing updated code so all they have to do is commit it.
Michelle
Comment #3
recrit commentedI agree the author_pane includes should be maintained by their respective modules. This should be handled in the author-pane.inc since userpoints_get_current_points() gets called many other places where they are just looking for the points for a given account, and not looking for any context access check to be applied, ie return points for account X when user Y is viewing.
Since this is adding a file and not just a patch, I have attached the full userpoints.author-pane.inc patched with #1 (author_pane-941158-userpoints-access.patch). To add, rename to "userpoints.author-pane.inc" and add to the userpoints directory. The author_pane_include() will include the correct file.
The access check added to userpoints_preprocess_author_pane is an exact copy of how its handled in userpoints_user($op="view").
Comment #4
michelleIf the function is called in other places, then there should be a wrapper function so that some other module calling the API doesn't need to be aware of permissions.
I wouldn't get your hopes up on getting this added to User Points. ISTR they were firmly against it. The Author Pane module was created because of my frustrations with trying to get other maintainers to add a bit of code.
Anyway, if it comes down to it, I'll add the permission check to AP. I'm not that stubborn. But I wanted to at least take a crack at getting it solved correctly first.
Michelle
Comment #5
michelleI forgot about this issue and now it's popped up again in my queue. I have no dev environment set up to work on Author Pane and am not really maintaining it anymore. Before I go through all that hassle and likely have to do a security release as well given the crap I went through with Location, I thought I'd give this one last bump and see if there's any chance of User Points getting a better API rather than me hacking around it.
Michelle
Comment #6
berdirYes, there should be a *separate* api function to check view permissions, but userpoints_get_current_points() should not check permissions. That introduces just as many new problems as it solves existing ones. E.g. doing stuff with points during cron runs and so on.
However, don't expect that anything is going to happen in 6.x-1.x, I just don't have time to maintain that version properly.
Comment #7
michelleMy point is that there should be a simple API call that modules can use that want to integrate User Points. I should just be able to say, "Hey, UP, here's a user wanting points from this account. Give me them to me." and it should return FALSE or something if that user isn't allowed to see them. A wrapper for an internal function you use on cron would work fine.
I know what you mean about not having time for D6. Unfortunately, this has put me in the position of having to set up a whole D6 dev environment to fix this because it's technically a security issue even though it's been publicly known for over a year. :(
I'm going to bump this to D7 because the issue is there for AP as well and I will not add User Points integration to Artesian without this. This is the second time I've had to scramble to deal with permissions in someone else's module and I'm not going to have a third.
Michelle