Notifies sent for messages in inaccessible boards

[iksa]

I'm using SMF 2.0.10. Steps to reproduce:

Preconditions: Users U1 and U2 belong to user group UG and therefore they can access board B
1) User U1 orders email notifications for new messages in board B
2) Administrator removes U1 from group UG, therefore U1 no longer can access B
3) User U2 posts a new message in B

I would expect the following would occur:
2) Notification orders on messages in board B for U1 are removed from the database, and/or
3) U1 does not receive an email notification of U2's message

This is what actually happens:
3) U1 still receives a notification of the new message

I guess this behavior is not by design?

ps. I do have quite a few mods installed, hopefully this one is not caused by any of them.

[JBlaze]

Sounds like the notifications are not being removed upon membergroup change for any boards that may be restricted.

Tracked here: https://github.com/SimpleMachines/SMF2.1/issues/2987

[margarett]

This does not happen on a clean 2.0.10 (and I would assume that it also doesn't in 2.1)

The comment of Oldiesmann in GH issue is correct: SMF *does* check the user being in allowed groups for the notifications:

Code Select Expand

if ($rowmember['id_group'] != 1)
{
$allowed = explode(',', $rowmember['member_groups']);
$rowmember['additional_groups'] = explode(',', $rowmember['additional_groups']);
$rowmember['additional_groups'][] = $rowmember['id_group'];
$rowmember['additional_groups'][] = $rowmember['id_post_group'];

if (count(array_intersect($allowed, $rowmember['additional_groups'])) == 0)
continue;
}


So the only possibility is the notification already be in the mail queue when the member is removed from the group.

[iksa]

Please test that piece of code more carefully.

Let's assume that when execution enters that code block, the initial values are as follows:

$rowmember['member_groups'] = (empty string)
$rowmember['additional_groups'] = (empty string)

When an empty string is exploded into an array, it becomes an array of one element whose value is an empty string. In the condition test at the end those empty string elements in $allowed and $rowmember['additional_groups'] will intersect, no matter what the values of $rowmember['id_group'] and $rowmember['id_post_group'] are. Therefore, a notification will be sent.

Also, how is it taken into account that if a person has notifications on in a board he sees because he is an administrator, not because he belongs to a member group with access to it, shouldn't also he receive the notifications?

The issue applies both to function notifyMembersBoard() in Post.php (notifications on new topics) and function sendNotifications() in Subs-Post.php (notifications on events in an existing topic).

[Illori]

if the user is an admin they can view everything, that means they would always get notifications in boards as they can view all boards.

[Kindred]

in other words, we have tested and we have confirmed that what you report is NOT reproducible with the default/core code.

[iksa]

I have tested with the code you quoted and the issue DOES reproduce with it like I explained in my last post. I suppose that code was taken from default/core SMF, right?

You just need to test with a user that does not belong to any additional member groups after the administrator has removed them from the group that let them see the board at which they set the notification. That will make both $allowed and $rowmember['additional_groups'] contain the same array element, in other words array_intersect() will return a non-zero value and that will lead a notification to be sent.

[margarett]

Actually that was exactly how I tested it.
Don't know how you get that results really...

Just for discussion sake, the primary membergroup is never empty, as you'll have at least 0 there...

[iksa]

Primary membergroup is the value stored in $rowmember['member_groups'], right? Yes, I have an empty value there. In the profile of the user I tested with there is "(no primary membergroup)" set as the value for 'Primary Membergroup' setting, could that explain it? Also some mod I have installed may have caused it to be empty...

Anyway, would it hurt to modify that piece of code so that it works even if any of those group lists are empty?

[margarett]

It might be because of the code you're using
Using empty against a 0 value also returns true.

Yet I tested as you said and I couldn't reproduce it...

[iksa]

Ok, I'll fix this only locally, then.

A couple of questions just out of curiosity:
1) When your execution flow/core SMF enters that code, what do you have as the value of $rowmember['member_groups'] when the board is such that no membergroup (except administrators, of course) is allowed to view it? In my environment it is an empty string according to my debugger (and it is an empty string also in the database), which will become a zero when exploded and that zero is assigned as an array into $allowed. In other words, $allowed now contains membergroup "zero" even though no membergroups were supposed to be contained. In this respect my code is equal to the one you quoted earlier, so I don't understand how this would not happen in your case, too.

2) At the same time, what do you have as the value of $rowmember['additional_groups'] if the user does not belong to any additional membergroups (the primary membergroup is not relevant here)? In my environment it is an empty string according to my debugger (and it is an empty string also in the database), which will become a zero when exploded and that zero is assigned as an array into $rowmember['additional_groups'].

Now, in my environment, both $allowed and $rowmember['additional_groups'] contain an array element whose value is zero. In other words, when those two are compared to each other (with array_intersect), it will look like a user who does not belong to any additional membergroups would be allowed to view a board that is not viewable to any membergroups. If this is not the case in your code, then what piece of code exactly prevents that from happening?

Another thing: In my code there was a bug ( both in notifyMembersBoard() and sendNotifications() ) that allowed administrators only when they were administrators by their primary membergroup. Being an administrator by additional membergroup was not taken into account. Maybe you would like to check if such issue exists also in the default code.

Thanks.

[Oldiesmann]

I'm looking further into this now to see what I can figure out about this... The issue with admins not receiving alerts when 1 isn't their primary group does appear to be a bug, and I'll look at fixing that once I determine what else (if anything) needs to be fixed along with it.

[Oldiesmann]

Ok... Upon further inspection, I do see how this can happen, but it's more of a fringe case situation - it can only happen in specific situations.

It can only happen if all of the following are true:
Member has subscribed to a topic in a board they no longer have access to
The board is set such that only admins can see it (eg no groups are selected as being able to see it)
The member in question does not have any additional (secondary) groups

This will result in them receiving a notification since both $allowed and $rowmember['additional_groups'] will contain an empty string.

The easiest way to fix all the issues in question is as follows:

Find

Code Select Expand

if ($rowmember['id_group'] != 1)
{
$allowed = explode(',', $rowmember['member_groups']);
$rowmember['additional_groups'] = explode(',', $rowmember['additional_groups']);
$rowmember['additional_groups'][] = $rowmember['id_group'];
$rowmember['additional_groups'][] = $rowmember['id_post_group'];

if (count(array_intersect($allowed, $rowmember['additional_groups'])) == 0)
continue;
}

Replace

Code Select Expand

$rowmember['additional_groups'] = empty($rowmember['additional_groups']) ? array() : explode(',', $rowmember['additional_groups']);

if ($rowmember['id_group'] != 1 || !in_array(1, $rowmember['additional_groups']))
{
$allowed = explode(',', $rowmember['member_groups']);
$rowmember['additional_groups'][] = $rowmember['id_group'];
$rowmember['additional_groups'][] = $rowmember['id_post_group'];

if (count(array_intersect($allowed, $rowmember['additional_groups'])) == 0)
continue;
}

[Oldiesmann]

Just merged in a fix for 2.1 as well. Thanks for the report

[iksa]

Thanks, finally someone who can read I already yesterday listed all those required conditions in the form of variable values.

Just wondering how many bugs never get fixed as getting a report through requires this great an effort...

[margarett]

I'm sorry that you feel that way, but that's not what happened.

I did try to reproduce the issue you reported but I didn't hit the edge case that was identified. Then I followed up the rest of your report on the phone, that's why I didn't test the rest of the stuff you mentioned.

We didn't discard your report, it just wasn't identified in the first attempt.

Anyway, I'm now moving this to Fixed bugs

[iksa]

Would you like to get some free friendly tips on how to improve customer service and keep the bug reporters motivated for reporting also the next bug they find? If yes, please let me know what would be the correct way and place to provide them. Thanks.

[iksa]

Btw, it should be

Code Select Expand


if ($rowmember['id_group'] != 1 && !in_array(1, $rowmember['additional_groups']))


not

Code Select Expand


if ($rowmember['id_group'] != 1 || !in_array(1, $rowmember['additional_groups']))


[Oldiesmann]

Good catch. Will fix it shortly.