The access control for fee operation links passes wrong arguments on the admin/store/orders/recurring page. Once that is fixed the logic in uc_recurring_user_access() is wrong. This function tries to do too much as it controls access to the user fees overview page and the user fee operations links.

This patch changes:

  • Split uc_recurring_user_access() into two function.
    • uc_recurring_user_fees_options_access() controls fee operation links access
    • uc_recurring_user_fees_access() controls access to user's fee overview page (as local link in my account and on user home page).
  • Correct the arguments passed to uc_recurring_user_fee_options_access() [previously uc_recurring_user_access()]
  • Set default access on fee operation links
    • Cannot charge a fee that is not active
    • Cannot cancel a fee that is canceled or expired
    • Only admin can charge or edit fee
    • Let payment modules decide access for other links
  • Remove no longer needed code in uc_recurrin_hosted.module that hides cancel links
CommentFileSizeAuthor
#4 uc_recurring-fee-op-access-2030267-4.patch6.46 KBstewart.adam
#3 uc_recurring-fee-op-access-2030267-3.patch6.5 KBstewart.adam
#2 uc_recurring-6.x-2.x-fee-op-access-2030267-1.patch6.21 KBtinker
#1 uc_recurring-fee-op-access-2030267-1.patch5.7 KBtinker

Comments

tinker’s picture

tinker commented
Status: Active » Needs review
StatusFileSize
new uc_recurring-fee-op-access-2030267-1.patch5.7 KB

Attached patch

tinker’s picture

tinker commented
StatusFileSize
new uc_recurring-6.x-2.x-fee-op-access-2030267-1.patch6.21 KB

Backport patch for 6.x-2.x

stewart.adam’s picture

stewart.adam commented
Issue summary: View changes
StatusFileSize
new uc_recurring-fee-op-access-2030267-3.patch6.5 KB

Rebase 7.x patch against latest git.

stewart.adam’s picture

stewart.adam commented
StatusFileSize
new uc_recurring-fee-op-access-2030267-4.patch6.46 KB

Removed the check against count($fees) - this resulted in hidden links or a 403 if no fees were present instead of displaying the "no current subscriptions" message.

tinker’s picture

tinker commented

@stewart.adam,

Thx for rebasing the patch. The count($fees) is there on purpose to hide the link to the user's subscription page. I don't see any value in displaying the link or the page when it provides no functionality when there are no recurring fees. What reason do you see to have it visible?

stewart.adam’s picture

stewart.adam commented

Primarily, it's because links to that page (e.g. from emails or instructional documentation) fail if the user has no subscriptions. It looks like a broken link and gives a bad user experience instead of just informing the user they have no active subscriptions.

I haven't tested that this is the case, but something to consider as well would be if the user has an active subscription that expires/gets deleted so the page that was previously visible to them is no longer so.

For this reason, I would advocate that the link stays enabled at all times and users can use hook_menu_alter() if it needs to be disabled when empty.

tinker’s picture

tinker commented

@stewart.adam, Good point about the email links. You should remove the '$fees = uc_recurring_get_user_fees($account->uid);' from uc_recurring_user_fees_access($account) as it become unnecessary database loads.

glynster’s picture

glynster commented

@tinker & @stewart.adam, this patch works well to resolve the issue of the admin recurring tab on an order to render all charges (rather than nadda). The work around before was to go to the original order, then click on recurring tab and you would see all the charges. However if the order state is in pending mode (as more attempts to bill occur) the same work around needs to occur. Not sure why. That said, being able to see all recurring fees from the last order issued is golden. Other than the above user case "fixes the fee operation links".

tinker’s picture

tinker commented

@glynster, This patch fixes the fee operation/option links on admin/store/orders/(order_id)/recurring and user/(user_id)/recurring-fees. It does not directly try to fix anything about the list of associated orders on the admin page. If it has fixed anything there it is a by product of improving access control.

The fee operation links are display in the far right column and can be 'Edit', 'Charge', 'Update', 'Cancel' etc. This patch fixes access callbacks that control which links are displayed. The patch fixes the arguments passed to the access callback, makes the access callback co-operate properly with payment handler modules, and sets up sensible defaults for the operation links such as "don't display a 'cancel' link if the subscription is already canceled".

tinker’s picture

tinker commented

@glynster, I think I found the issue you are talking about. If a renewal order fails payment and is 'pending' it will not show up on the admin recurring orders tab because the reference to the renewal order is only created if payment is successful. I have created a new report with patch for this issue #2579479: New order reference not saved in uc_recurring_orders when payment fails. That patch is not retro active so will only have effect on new renewal orders.

glynster’s picture

glynster commented

@tinker, OK great, if this is the case then +1 to have this committed as it definitely fixes those issues. Tested on Admin & authenticated users.

glynster’s picture

glynster commented

@tinker, been running this patch for 5 days is working perfectly +1 to have this committed to the latest dev.

glynster’s picture

glynster commented

I think it is time to RTBC.