-
-
Notifications
You must be signed in to change notification settings - Fork 833
Conversation
Opening a PR to gather early feedback, but tests haven't been written yet. |
Hey @Yoric 👋 Does this relate to any issue? If so, can you please link it? If not can you describe your changes (from user point of view)? |
Oh, right. I've just expanded the description. |
56b6847
to
5ba6949
Compare
With this PR, invites that are specified by MSC3847 to be ignored are hidden.
5ba6949
to
f73c431
Compare
Now with tests. |
Not sure what's wrong with Static Analysis. All the errors it shows me are in parts of the code that I'm not hitting. Furthermore, Same for |
private async updateStickyRoom(val: Room) { | ||
await this.doUpdateStickyRoom(val); | ||
this._lastStickyRoom = null; // clear to indicate we're done changing | ||
} | ||
|
||
private doUpdateStickyRoom(val: Room) { | ||
private async doUpdateStickyRoom(val: Room) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These cannot be async, this causes race conditions, please see git history around this file. You would need to introduce a semaphore to prevent updates racing with each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g. #6391
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch. Will do, thanks!
@@ -38,7 +39,7 @@ export class VisibilityProvider { | |||
await VoipUserMapper.sharedInstance().onNewInvitedRoom(room); | |||
} | |||
|
|||
public isRoomVisible(room?: Room): boolean { | |||
public async isRoomVisible(room?: Room): Promise<boolean> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change, existing customisations will fail to compile with this change due to the return type changing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move it into the js-sdk as a background operation, so consumers don't have to make all their APIs async. Feels a bit strange for getRuleForInvite
to be an async function which actually creates rooms, it seems like that one should be read-only, and something else should create and uphold the policy rooms, emitting "PolicyUpdated" or similar events for consumers to subscribe and react to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also looks like getOrCreateTargetRoom
is flawed if called multiple times whilst the room is being created, it'll stack multiple creations instead of being idempotent. This could also occur with this PR where multiple operations overlap and call isRoomVisible causing overlapping calls to getOrCreateTargetRoom
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also looks like
getOrCreateTargetRoom
is flawed if called multiple times whilst the room is being created, it'll stack multiple creations instead of being idempotent. This could also occur with this PR where multiple operations overlap and call isRoomVisible causing overlapping calls togetOrCreateTargetRoom
You're right, I should at least lock it to avoid that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move it into the js-sdk as a background operation, so consumers don't have to make all their APIs async. Feels a bit strange for
getRuleForInvite
to be an async function which actually creates rooms, it seems like that one should be read-only, and something else should create and uphold the policy rooms, emitting "PolicyUpdated" or similar events for consumers to subscribe and react to.
That was the design at some point but I dropped it because it felt like I would be reimplementing a non-trivial subset of the state event store. On the other hand, it is probably needed to achieve any kind of performance anyway, so now might be the right time to revisit this.
I'll think this over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case someone else reads this, this conversation has spawned a patch in matrix-js-sdk: matrix-org/matrix-js-sdk#2653 .
Experience and feedback from matrix-org/matrix-react-sdk#9255 indicates that we cannot integrate an async `IgnoredInvites.getRuleForInvite`. This PR: - rewrites `getRuleForInvite` to be sync; - adds some locking within `IgnoredInvites` to avoid race conditions noticed by @t3chguy.
Experience and feedback from matrix-org/matrix-react-sdk#9255 indicates that we cannot integrate an async `IgnoredInvites.getRuleForInvite`. This PR: - rewrites `getRuleForInvite` to be sync; - adds some locking within `IgnoredInvites` to avoid race conditions noticed by @t3chguy.
This code is part of an early Proof of Concept for hiding invite spam. The general idea is that rejecting invites doesn't work, because spam bots simply use that rejection to send more invite spam.
The general idea is:
Checklist
Here's what your changelog entry will look like:
✨ Features