Expiring roles are not deleted if removed using Drupal

Patch attached

Hi TR. The issue is that this conditional will never be true:
if (!in_array($rid, array_keys($edit['roles'])) && $rid != DRUPAL_AUTHENTICATED_RID) as $edit['roles'] has every role available.

I can't speak to whether or not it worked before but, thinking about it, it's possible that it didn't and no one noticed. I came across this with what I would consider an extreme edge case:
1. Admin was removing a role the Drupal way
2. If site user didn't have role but there was an expiration set for this role on this user, a custom module was granting the role(s) again when the site user visited their 'dashboard'

Because this custom module was checking the expiration, it led me to this implementation of hook_user_presave(). I can imagine that a site admin would think they were revoking a role the Drupal way and never check - the role would be revoked, but the expiration would not be. Therefore, it could stand to reason that role expirations were never properly done but, because the role was revoked, no one was ever the wiser. If there is an "expiring roles" admin report of sorts, I would imagine it would be incorrect.

I am yet to write test cases so I'll try to get some decent ones for you.

Yes it would be :). I went ahead and put it in a patch

Sorry about this TR, I completely forgot about this after implementing a fix. I think this is definitely a bug so I'm going to switch it back (if only to make sure you see it again), I apologize if that's out-of-order for me to do.

To restate the issue: If a user has an existing expiration (stored in uc_roles_expirations) and an admin manually removes the role by unchecking it on the user profile form, that expiration is not removed from uc_roles_expirations which would be the expected behavior as indicated in uc_roles.module.

The issue is only somewhat mitigated by the fact that the user no longer has a role, so access checks work as expected. However, in a situation where an admin removes a role that had an expiration date with 20 days left, for example, and then the user makes another purchase which grants that same role - that purchased expiration date is *added* to the existing expiration (the one that should have been deleted).

Unfortunately, I only tested this on the user profile form - in that case my statement is true: `$edit['roles'] `always contains every role except for anonymous (#options on that form element is built using `user_roles(TRUE)` ). Therefore, the role expiration will never be deleted if done from the user profile page.

However, I failed to consider when that form is submitted programmatically with a direct call to user_login_submit(). If the $form argument is passed as an empty array(), then we get the error the test produced. In this instance, your modifications to the patch actually delete the role expirations when they should not be.

I'd like to propose the attached patch which accounts for the valid use case of calling `user_login_submit(array(), $form_state)`. It just maintains the old conditional around your revision to my patch.

To reproduce this I just did a fresh install with only these modules:

chrisrockwell@RockwellMBPro  ~/Sites/quick-drupal-20160220154705/drupal-7.x/sites/all/modules/ubercart  drush pml --status=enabled --no-core
Package                     Name                          Type    Version
 Chaos tool suite            Chaos tools (ctools)          Module  7.x-1.9
 Development                 Devel (devel)                 Module  7.x-1.5
 Other                       Entity API (entity)           Module  7.x-1.6
 Other                       Entity tokens (entity_token)  Module  7.x-1.6
 Rules                       Rules (rules)                 Module  7.x-2.9
 Rules                       Rules UI (rules_admin)        Module  7.x-2.9
 Ubercart - core             Cart (uc_cart)                Module  7.x-3.9+19-dev
 Ubercart - core             Order (uc_order)              Module  7.x-3.9+19-dev
 Ubercart - core             Product (uc_product)          Module  7.x-3.9+19-dev
 Ubercart - core             Store (uc_store)              Module  7.x-3.9+19-dev
 Ubercart - core (optional)  Roles (uc_roles)              Module  7.x-3.9+19-dev
 Views                       Views (views)                 Module  7.x-3.13

I'll need to spend some time going through the tests; if it's getting caught on offset 1 I think that means it's checking anonymous users as well, should it be? (I'm sorry, I'm completely unfamiliar with tests so it'll take me sometime to figure it out). It looks like a new method in uc_roles.test would be the correct place to start.

This checks to make sure uc_roles even has control over the role in question; I thought about doing if ($rid != DRUPAL_AUTHENTICATED_RID && $rid != DRUPAL_ANONYMOUS_RID && !edit['roles'][$rid] but I feel the anonymous user rid should never be there anyway as uc_roles shouldn't have the ability to take that role away.

@gauravmanerkar, did #16 not work?

I can reproduce #2770193 and I see why.

1. user_admin_account_submit() calls user_multiple_role_edit(), which only passes the roles the user *should* have (see the array_diff at line 3329).
2. user_profile_form_submit() passes #edit['roles'] with all available roles, in the format 'role_id' => (bool)

To prevent the warning, we need to check the following:
1. The role id is not in $edit['roles']
2. The role id is in $edit['roles'] but is FALSE

There is an alternative to the now-3-part conditional, and that's to use hook_user_update() which always includes *only* the roles the user should have. In that case we could just use array_key_exists.

I'm not sure what to change the status to on this (needs review and back to 7.x?)

Fixed path

This should produce one undefined offset exception in uc_roles_user_presave on line 340.

This one includes test (#37) and patch (#33).