Message Subscribe currently has a handy $options['entity access'] parameter, which sanely defaults to TRUE, enforcing entity access and preventing sending email notifications to users who don't have access to view a particular entity.

This is a security issue, but since this module does not have a stable release yet we can handle this in public.

However, entity_access() does not take into account users whose accounts have been blocked. It seems reasonable to me that we would expect Message Subscribe to not send notifications to users whose accounts are blocked. Perhaps we would want to override this behavior in some cases, such as to notify a blocked user how to regain access to her account.

I propose that we provide a "notify blocked users" key on the $options parameter to message_subscribe_send_message() and have it default to FALSE.

This might be considered a security issue if Message Subscribe had an official 1.0 release.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ezra-g’s picture

Status: Active » Needs review
Issue tags: +commonslove, +commons 7.x-3.9 radar
ezra-g’s picture

WebSinPat’s picture

the patch in #2 seems to work as described.
- user is active, receives email when new node is created
- user gets blocked, does not receive email when new node is created
- user get unblocked, does receive email again

(I was not running into this problem with a real world use-case that I can test the patch against, so just did the very basic testing above.)

amitaibu’s picture

Thanks.

+++ b/message_subscribe.module
@@ -40,9 +40,15 @@
+ *      about entities which they used to have access to ¶

Trailing space here and below.

Also, since we are dealing with access, this should come with a simpleTest.

ezra-g’s picture

A start at revising with tests. We've got one failing test so far: 'All subscribers from "last uid" and "range" were fetched.'

It's not clear to me whether this is a fault with the tests or whether function message_subscribe_get_subscribers() needs revision.

Leaving as "needs review" to confirm that the testbot sees the same results as I do locally.

Status: Needs review » Needs work

The last submitted patch, 5: 2184567-message-subscribe-blocked-users-test-and-fix-5.patch, failed testing.

ezra-g’s picture

Revised test-only patch and test-with-fix patches.

The last submitted patch, 7: 2184567-message-subscribe-blocked-users-test-only-7.patch, failed testing.

klausi’s picture

Priority: Normal » Critical
Issue summary: View changes
Issue tags: +Security improvements

Security issues are critical.

@Amitaibu: please create a new release when this gets fixed and mark it as security update. Let me or another security team member know then we publish it for you.

amitaibu’s picture

I like the new approach!

  1. +++ b/message_subscribe.module
    @@ -40,9 +40,15 @@
    + *      about entities which they used to have access to ¶
    

    Trailing space here and below.

  2. +++ b/message_subscribe.module
    @@ -230,7 +237,13 @@ function message_subscribe_get_subscribers($entity_type, $entity, Message $messa
    +      if (empty($subscribe_options['notify blocked users'])) {
    +        $account = user_load($uid);
    +        if (!$account->status) {
    +          // User doesn't have access to view the entity.
    +          continue;
    +        }
    +      }
    

    Maybe we can move this and the above if, outside of the foreach, so we can load all the users in one go ($account = user_load_multiple(array_keys($uids)) -- and then unsest those that are "illegal".

  3. +++ b/message_subscribe.module
    @@ -401,11 +423,16 @@ function message_subscribe_message_subscribe_get_subscribers(Message $message, $
    +      // If we're notifiying blocked users, we need to take this into account
    +      // in this query so that the range is accurate. Otherwise, ¶
    +      if (empty($subscribe_options['notify blocked users'])) {
    +        $query->addTag('message_subscribe_message_subscribe_get_subscribers_range');
    +      }
    

    So, this can be removed.

  4. +++ b/message_subscribe.test
    @@ -206,20 +214,26 @@ class MessageSubscribeSubscribersTest extends DrupalWebTestCase {
    +    $user5->status = 0;
    

    lets just call it $user_blocked to prevent this naming issue.

    Also, can you add an assert for a case where notify blocked members is allowed.

@klausi, sure.

ezra-g’s picture

Assigned: Unassigned » ezra-g
ezra-g’s picture

I made the revisions described in #10 with the exception of removing the query alter. Even when we change the order of processing in message_subscribe_get_subscribers(), message_subscribe_message_subscribe_get_subscribers() must still take blocked users into account for range queries.

Also, rather than doing a user_load_multiple(), I did an entity field query to subtract the blocked users from the query. If we wanted to nitpick, I suppose it might be more efficient to select blocked users, rather than active ones.

Status: Needs review » Needs work

The last submitted patch, 12: 2184567-message-subscribe-blocked-users-12-test-only.patch, failed testing.

ezra-g’s picture

Status: Needs work » Needs review

Fail was for test only.

amitaibu’s picture

  1. +++ b/message_subscribe.module
    @@ -341,6 +359,15 @@ function message_subscribe_get_basic_context($entity_type, $entity, $subscribe_o
    + * Remove blocked users from message_subscribe_message_subscribe_get_subscribers
    + * queries that include a range.
    + */
    +function message_subscribe_query_message_subscribe_message_subscribe_get_subscribers_range_alter(QueryAlterableInterface $query) {
    +  $query->join('users', 'users', 'users.uid = fc.uid');
    +  $query->condition('users.status', '1', '=');
    +}
    ...
    @@ -401,11 +428,16 @@ function message_subscribe_message_subscribe_get_subscribers(Message $message, $
    

    I'm not clear why we need this, and can't move this condition into message_subscribe_message_subscribe_get_subscribers()

  2. +++ b/message_subscribe.module
    @@ -401,11 +428,16 @@ function message_subscribe_message_subscribe_get_subscribers(Message $message, $
    +      // in this query so that the range is accurate. Otherwise,
    

    Comment looks wrong "Otherwise,"

  3. +++ b/message_subscribe.test
    @@ -220,6 +229,11 @@ class MessageSubscribeSubscribersTest extends DrupalWebTestCase {
    +    // User 5 is blocked for testing purposes. We jump to 5 so we don't
    +    // conflict with the user 3 defined elsewhere in Message Subscribe's tests.
    

    Comment is wrong.

ezra-g’s picture

Indeed, we can eliminate the query alter and move this functionality to message_subscribe_message_subscribe_get_subscribers(). Thanks for the clarification.

Now with the fixes in #15 to this and comments.

The last submitted patch, 16: 2184567-message-subscribe-blocked-users-15-test-only.patch, failed testing.

ezra-g’s picture

Failed patch was test only.

amitaibu’s picture

Thanks, we are very close :)

  1. +++ b/message_subscribe.module
    @@ -212,26 +219,37 @@ function message_subscribe_get_subscribers($entity_type, $entity, Message $messa
    +      $query->entityCondition('entity_type', 'user')
    +      ->propertyCondition('status', 1)
    

    Minor - wrong indentation.

  2. +++ b/message_subscribe.module
    @@ -212,26 +219,37 @@ function message_subscribe_get_subscribers($entity_type, $entity, Message $messa
    +    $uids = array_intersect_key($uids, $active_uids['user']);
    

    If all users are blocked, we will get a notice as $active_uids will be empty. Also can we rename variable to $result?

  3. +++ b/message_subscribe.module
    @@ -401,11 +419,17 @@ function message_subscribe_message_subscribe_get_subscribers(Message $message, $
    +      // If we're notifiying blocked users, we need to take this into account
    

    I think: If we're notifying => If we're not notifying -- right?

amitaibu’s picture

Status: Needs review » Needs work

Setting to correct status.

RoySegall’s picture

I re rolled and apply the fixes. For now the tests fail at my local machine but we can give it a try with Drupal QA servers.

RoySegall’s picture

Status: Needs work » Needs review
RoySegall’s picture

The last submitted patch, 21: 2184567-message-subscribe-blocked-users-20.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 23: 2184567-message-subscribe-blocked-users-23.patch, failed testing.

amitaibu’s picture

btw, Feel free to open a PR in the GitHub repo instead of here
#2362663: Move issue queue to Github

amitaibu’s picture

Status: Needs work » Needs review

DamienMcKenna’s picture

Status: Needs review » Fixed

The patch was committed.

Status: Fixed » Closed (fixed)

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