Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework joined room tracking in Mjolnir. #370

Closed
Gnuxie opened this issue Sep 13, 2022 · 2 comments · Fixed by #385
Closed

Rework joined room tracking in Mjolnir. #370

Gnuxie opened this issue Sep 13, 2022 · 2 comments · Fixed by #385

Comments

@Gnuxie
Copy link
Contributor

Gnuxie commented Sep 13, 2022

The tracking of joined rooms is barely comprehensible at the moment.
This is complicated by the need to track and protect all joined rooms (except those which we can't such as policy lists we are watching but don't moderate).

  • This is just poorly written

    mjolnir/src/Mjolnir.ts

    Lines 474 to 503 in 4d5447c

    private async resyncJoinedRooms(withSync = true) {
    if (!this.config.protectAllJoinedRooms) return;
    const joinedRoomIds = (await this.client.getJoinedRooms()).filter(r => r !== this.managementRoomId);
    const oldRoomIdsSet = new Set(this.protectedJoinedRoomIds);
    const joinedRoomIdsSet = new Set(joinedRoomIds);
    // Remove every room id that we have joined from `this.protectedRooms`.
    for (const roomId of this.protectedJoinedRoomIds) {
    delete this.protectedRooms[roomId];
    this.protectedRoomActivityTracker.removeProtectedRoom(roomId);
    if (!joinedRoomIdsSet.has(roomId)) {
    this.roomJoins.removeRoom(roomId);
    }
    }
    this.protectedJoinedRoomIds = joinedRoomIds;
    // Add all joined rooms back to the permalink object
    for (const roomId of joinedRoomIds) {
    this.protectedRooms[roomId] = Permalinks.forRoom(roomId);
    this.protectedRoomActivityTracker.addProtectedRoom(roomId);
    if (!oldRoomIdsSet.has(roomId)) {
    this.roomJoins.addRoom(roomId);
    }
    }
    this.applyUnprotectedRooms();
    if (withSync) {
    await this.syncLists(this.config.verboseLogging);
    }
    }
  • Unacceptable horrible state tracking that removes watched policy lists retroactively from protected rooms that were added earlier because we were joined.

    mjolnir/src/Mjolnir.ts

    Lines 748 to 753 in 4d5447c

    private applyUnprotectedRooms() {
    for (const roomId of this.unprotectedWatchedListRooms) {
    delete this.protectedRooms[roomId];
    this.protectedRoomActivityTracker.removeProtectedRoom(roomId);
    }
    }
  • Member variables in mjolnir that have a purpose that is difficult to understand

    mjolnir/src/Mjolnir.ts

    Lines 97 to 102 in 4d5447c

    private protectedJoinedRoomIds: string[] = [];
    /**
    * These are rooms that were explicitly said to be protected either in the config, or by what is present in the account data for `org.matrix.mjolnir.protected_rooms`.
    */
    private explicitlyProtectedRoomIds: string[] = [];
    private unprotectedWatchedListRooms: string[] = [];
  • PROTECTED_ROOMS_EVENT_TYPE is difficult to track.
What are all these things doing?
  • Updating protected rooms when we leave a room so we don't keep trying to protect it. One assumption to remember for Mjolnir is that it will only protect a room while it is able to prove it is joined to the room.
    • Something else to consider is whether this assumption extends to watching policy lists, currently it does not.
  • while protectAllJoinedRooms is false: PROTECTED_ROOMS_EVENT_TYPE Persisting the set of rooms Mjolnir protects for when it next starts up.
  • while protectAllJoinedRooms is true: working out which rooms are protected from all joined rooms based on whether they are a watched list and it is determined (arbitrarily) that we don't moderate the list. There might be other reasons not to protect a room.
  • while protectAllJoinedRooms is true: Working out when we have joined a room so it can also be protected.
What needs to change then?

While #371 will factor out protected rooms from the Mjolnir class, it will not change the tracking of joined rooms. It intentionally does not try to synchronize PROTECTED_ROOMS_EVENT_TYPE either. This is because it would contaminate ProtectedRooms which is supposed to be able to work in isolation of Mjolnir and we shouldn't be tied to one group of protected rooms (See #283).

  • Split out the logic that is only there to serve protectAllJoinedRooms from everything else (ie working out new joins, working out from joins which rooms we are currently protecting) like protected rooms. Currently they contaminate each other even when it's not used and it's what makes things horrible.

Related:

Gnuxie added a commit that referenced this issue Sep 13, 2022
Gnuxie added a commit that referenced this issue Sep 13, 2022
Gnuxie added a commit that referenced this issue Sep 29, 2022
@Gnuxie
Copy link
Contributor Author

Gnuxie commented Sep 30, 2022

image

@Gnuxie
Copy link
Contributor Author

Gnuxie commented Oct 3, 2022

So, what I'm thinking is that ProtectedRooms should be renamed ProtectedRoomsSet.
Then ProtectedRoomsConfig should be made that basically reads account data and sets up the ProtectedRoomsSet.
Then later on if we want protected rooms to be added via spaces we only need to change ProtectedRoomsConfig.

We then also don't have a horrible set of conflicting room tracking utils in the Mjolnir class as all this can be moved to ProtectedRoomsConfig.

Gnuxie added a commit that referenced this issue Oct 14, 2022
Gnuxie added a commit that referenced this issue Oct 14, 2022
Gnuxie added a commit that referenced this issue Oct 17, 2022
Gnuxie added a commit that referenced this issue Oct 17, 2022
The combination of `resyncJoinedRooms`, `unprotectedWatchedListRooms`,
`explicitlyProtectedRoomIds`, `protectedJoinedRoomIds` was incomprehensible.
#370

Separating out the management of `explicitlyProtectedRoomIds`, then
making sure all policy lists have to be explicitly protected
(in either setting of `config.protectAllJoinedRooms`) will make
this code much much simpler.
We will later change the `status` command to explicitly show
which lists are watched and which are watched and protected.
Gnuxie added a commit that referenced this issue Oct 17, 2022
Gnuxie added a commit that referenced this issue Oct 18, 2022
The combination of `resyncJoinedRooms`, `unprotectedWatchedListRooms`,
`explicitlyProtectedRoomIds`, `protectedJoinedRoomIds` was incomprehensible.
#370

Separating out the management of `explicitlyProtectedRoomIds`, then
making sure all policy lists have to be explicitly protected
(in either setting of `config.protectAllJoinedRooms`) will make
this code much much simpler.
We will later change the `status` command to explicitly show
which lists are watched and which are watched and protected.
Gnuxie added a commit that referenced this issue Oct 18, 2022
Gnuxie added a commit that referenced this issue Oct 19, 2022
The combination of `resyncJoinedRooms`, `unprotectedWatchedListRooms`,
`explicitlyProtectedRoomIds`, `protectedJoinedRoomIds` was incomprehensible.
#370

Separating out the management of `explicitlyProtectedRoomIds`, then
making sure all policy lists have to be explicitly protected
(in either setting of `config.protectAllJoinedRooms`) will make
this code much much simpler.
We will later change the `status` command to explicitly show
which lists are watched and which are watched and protected.
Gnuxie added a commit that referenced this issue Oct 19, 2022
Gnuxie added a commit that referenced this issue Oct 19, 2022
The combination of `resyncJoinedRooms`, `unprotectedWatchedListRooms`,
`explicitlyProtectedRoomIds`, `protectedJoinedRoomIds` was incomprehensible.
#370

Separating out the management of `explicitlyProtectedRoomIds`, then
making sure all policy lists have to be explicitly protected
(in either setting of `config.protectAllJoinedRooms`) will make
this code much much simpler.
We will later change the `status` command to explicitly show
which lists are watched and which are watched and protected.
Gnuxie added a commit that referenced this issue Oct 19, 2022
Gnuxie added a commit that referenced this issue Oct 19, 2022
Gnuxie added a commit that referenced this issue Oct 19, 2022
Gnuxie added a commit that referenced this issue Oct 19, 2022
The combination of `resyncJoinedRooms`, `unprotectedWatchedListRooms`,
`explicitlyProtectedRoomIds`, `protectedJoinedRoomIds` was incomprehensible.
#370

Separating out the management of `explicitlyProtectedRoomIds`, then
making sure all policy lists have to be explicitly protected
(in either setting of `config.protectAllJoinedRooms`) will make
this code much much simpler.
We will later change the `status` command to explicitly show
which lists are watched and which are watched and protected.
Gnuxie added a commit that referenced this issue Oct 19, 2022
Gnuxie added a commit that referenced this issue Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant