Smf2.1: Mention didn't check if user is able to access this topic

[albertlast]

When you mention a person X in a topic where he had no access,
he keep getting the notification that he was mention:

d3vcho was able to reproduce this error:
https://i.gyazo.com/b72d180e8bd8b1922b559cdcf9f2fd82.png

the post is censored.

Possible root of this evil would be here:
https://github.com/SimpleMachines/SMF2.1/blob/release-2.1/Sources/tasks/CreatePost-Notify.php#L51

Missing check of the array $msgOptions['mentioned_members'] if the mention user is able to access.
other placce could be https://github.com/SimpleMachines/SMF2.1/blob/release-2.1/Sources/Subs-Post.php
that we didn't add any person here how had no access.

[frandominguez03]

To reproduce this, you just need to create (or edit) a board's permissions to make it invisible to a group of users and then just mention an user who is not able to see that board.

[albertlast]

when i look on elkarte side they fixed on notify side:
https://github.com/elkarte/Elkarte/blob/development/sources/subs/MentionType/MentionBoardAccessAbstract.php#L60
(i believe that is the code how do the check)

[PortaMx]

😀 this we fixed it weeks ago ..
and I can say, it's very simple 😋 .. for mentions and quotes ..

[XinYenFon]

@PortaMx unless you are planning to contribute that fix towards this software please do not flood.

[PortaMx]

I have posted many bug and also fixes .. but nothing of them was accepted by the people here.
So I stop reporting of bug and solved code and make my own work .. you understand ? yes ? no ? shit happens 😋
But .. I can you say that the fix is very simple .. a little bit code and moving exist code at a other place ..that's all

[MissAllSunday]

I had a similar issue with Breeze custom notifications: https://github.com/MissAllSunday/Breeze/blob/develop/Sources/Breeze/BreezeLog.php#L85

It isn't as nice as I would like it to be but it works, the extra query cannot be avoided unless you want to add some kind of dynamic cache or had previously loaded the needed data. I mitigated it by grouping as much messages as I was able to group together, thus avoiding doing multiple queries.

@PortaMx Your code wasn't accepted because it didn't meet the minimal criteria for SMF standards, it was almost incomplete or just plain wrong

[PortaMx]

@MissAllSunday .. well, I don't known what criteria SMF have, but .. when I see the code in 2.1 ... uhhh..
I dont known how many code I have fixed / redesigned .. but it was not less ..
And the "just plain wrong" code works on our Forum version perfectly ;-)
And .. Interesting, that this issue opened just 2 days after I have posted that in our Forum ...
A rogue who thinks evil :-)
In German: "Ein Schelm wer Böses dabei denkt.."

[albertlast]

What i don't understand why we not drop alerts which are not able to access here: https://github.com/SimpleMachines/SMF2.1/blob/release-2.1/Sources/Profile-View.php#L302
we only "censor" them.

[MissAllSunday]

Yeah, thats the way to go, you cannot do it on alert creation, it has to be done on alert "display".

The reason why alerts are "censored" rather than "dropped" is because an user might gain access to those boards.

An example would be people on subscriptions, they might lose access to X boards but gain it after X period of time.

Another reason would be to just let the user decide what to do with their own alerts, let them chose when to delete them. On Breeze I have a setting for mark an alert as "unread" even old ones, I used that option as a reminder of things I needed to do. In SMF's case if alerts are deleted people won't be able to do anything with them.

I can also see the "if people cannot see it whats the point" angle and its a valid one too, (IE do not allow alert creation if the mentioned user cannot see the topic) but since the code already heavely favors the other way around (create the alert no matter what and deal with it later) its better to keep on following that path.

[albertlast]

Did we know on display side from which notify this alert came?
When yes we could define a function in every notify which allow to define every notify how react.

[MissAllSunday]

I don't remember if that info is stored but it should be, at least the topic ID since it has to be used to build a link back to it. For other type of alerts I suppose storing a permission name string could be done, that way when displaying the alert a call to isAllowed() or isAllowedTo() (can't remember which one doesn't do a fatal error) can be done to determinate is the user can see that page or action or whatever the permission allows them to do.

For recurring alerts a cached array can be build to avoid the constant calls to the DB.

[jdarwood007]

20:42:54 < albertlast> Oldiesmann: exists a reason why you keep this open? https://github.com/SimpleMachines/SMF2.1/issues/4287
20:44:06 <+Oldiesmann> Because I forgot to close it when I merged the fix and nobody else has seen it
20:48:16 < albertlast> in short: no
20:48:17 < albertlast> :)