Currently in both D5 and D6, the Account Panel is a tab that appears on users' pages. Unfortunately, its way to easy for anyone to that has the "access content" permission to view someone's private settings like this as generally, on most sites, even anonymous site users have access content perm.

if you know about this module you can simply type in user/XXXX/pfp_cpanel, and get access to their subscription information.

Personally, I think the access should be changed so that only people with the "view customer" setting (a core Ubercart permission) or the actual current logged in site user can see their own Account Panel page, but no one else's. ....

  global $user;
  if(arg(0) == 'user') {
    $items[] = array(
      'path' => 'user/' . arg(1) . '/pfp_cpanel',
      'title'=> t('Account Panel'),
      'callback' => 'uc_payflowpro_user_panel',
      'type' => MENU_LOCAL_TASK,
      'access' => user_access('view customers') || $user->uid == arg(1),
    );
  }

I've tested this on D5 and the changes above work great.

Comments

kwinters’s picture

kwinters commented

Overall I like the idea for this. I'll check to see if there are any similar places to fix or if there is a better permission to use.

kwinters’s picture

kwinters commented

All of the other 'access content' permission menu items have to do with the actual cart process, so they're fairly public and that's fine.

I'm thinking that "view payments" is a more appropriate permission set for the user panel, since the it tells you more than just that the user is a "customer" and has details related to payments. It's in uc_payment which is core (optional) but required for uc_payflowpro. Thoughts?

kwinters’s picture

kwinters commented
Status: Active » Needs review

Status changed to review.

If I don't hear any cons by the time we're ready to roll a new beta (when the recurring fixes are done), I'll go ahead and put it in.

kwinters’s picture

kwinters commented
Version: 5.x-1.0-beta1 » 6.x-1.0-beta2
Assigned: Unassigned » kwinters
Status: Needs review » Needs work

Unfortunately, there's more work involved here than originally anticipated.

1) There is a second argument to that function (profile), which is not as straight forward to test.
2) Making this work sensibly in D6 requires a heavy rewrite of several functions, so that they will use arguments in a reasonable way.
3) Not sure if this is the case in D5 or not, but in D6 you can't do that kind of logic directly in the menu array because it will get cached incorrectly.

So, what I'm going to do is leave the menu callbacks alone and just add some extra checks in the functions they are calling at first. That will be sufficient for the D5 branch, but long-term the D6 version will need to be refactored to use the menu system callbacks correctly.

kwinters’s picture

kwinters commented
Title: Recurring Payments tab (Account Panel) access control too lenient » Account Panel Recurring Tab Permissions Improvement
Status: Needs work » Fixed

This is now fixed, but it actually wasn't very broken to start with. The permission checks with in place, they were just done in a very roundabout way and didn't use the menu system very well.

In D6, I re-organized the permission checks somewhat so that it would show permission denied rather than just fall back to something you did have permission to see, and made the code for the checks more sensible. I also changed the user account tab base permission to be 'view own orders' to be more consistent, but that is a fairly minor change (only prevents the tab link from appearing when clicking on it would 403 anyway).

In D5 I only made the necessary change, which was to account for profile ID in the single-profile details page and make the permission denied message use the drupal_access_denied function.

Eventually I want to refactor those functions to do all of the access permissions through the menu system callbacks, but that may wait until 7.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.