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.
Comment | File | Size | Author |
---|---|---|---|
#23 | innerdiff-2184567-23.txt | 658 bytes | RoySegall |
#23 | 2184567-message-subscribe-blocked-users-23.patch | 8.23 KB | RoySegall |
#21 | innerdiff-2184567.txt | 2.67 KB | RoySegall |
#21 | 2184567-message-subscribe-blocked-users-20.patch | 8.21 KB | RoySegall |
#16 | 2184567-message-subscribe-blocked-users-15-test-and-fix.patch | 7.2 KB | ezra-g |
Comments
Comment #1
ezra-g CreditAttribution: ezra-g commentedComment #2
ezra-g CreditAttribution: ezra-g commentedComment #3
WebSinPat CreditAttribution: WebSinPat commentedthe 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.)
Comment #4
amitaibuThanks.
Trailing space here and below.
Also, since we are dealing with access, this should come with a simpleTest.
Comment #5
ezra-g CreditAttribution: ezra-g commentedA 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.
Comment #7
ezra-g CreditAttribution: ezra-g commentedRevised test-only patch and test-with-fix patches.
Comment #9
klausiSecurity 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.
Comment #10
amitaibuI like the new approach!
Trailing space here and below.
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".
So, this can be removed.
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.
Comment #11
ezra-g CreditAttribution: ezra-g commentedComment #12
ezra-g CreditAttribution: ezra-g commentedI 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.
Comment #14
ezra-g CreditAttribution: ezra-g commentedFail was for test only.
Comment #15
amitaibuI'm not clear why we need this, and can't move this condition into message_subscribe_message_subscribe_get_subscribers()
Comment looks wrong "Otherwise,"
Comment is wrong.
Comment #16
ezra-g CreditAttribution: ezra-g commentedIndeed, 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.
Comment #18
ezra-g CreditAttribution: ezra-g commentedFailed patch was test only.
Comment #19
amitaibuThanks, we are very close :)
Minor - wrong indentation.
If all users are blocked, we will get a notice as $active_uids will be empty. Also can we rename variable to
$result
?I think: If we're notifying => If we're not notifying -- right?
Comment #20
amitaibuSetting to correct status.
Comment #21
RoySegall CreditAttribution: RoySegall commentedI 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.
Comment #22
RoySegall CreditAttribution: RoySegall commentedComment #23
RoySegall CreditAttribution: RoySegall commentedComment #27
amitaibubtw, Feel free to open a PR in the GitHub repo instead of here
#2362663: Move issue queue to Github
Comment #28
amitaibuComment #30
DamienMcKennaThe patch was committed.