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

Can Set State #800

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Conversation

nvrWhere
Copy link
Collaborator

Upstream canSetState and canSendEvent from NeoChat and use to check before setting state to avoid pinging the server with no chance of success.

@KitsuneRal
Copy link
Member

KitsuneRal commented Sep 15, 2024

Looks like we are chasing the same animal :) see #799. I actually made a bit more involved can*() methods in the earlier take; but then realised that merely checking power levels is not enough, as it can give both false positives (not that bad, the server will reject them anyway) and false negatives (bad, because we end up forbidding the user to perform legitimate operations). So I decided to step back for the moment and implement those methods more carefully, following relevant parts of auth rules. If you already explored that path, I'm happy to learn about it.

@KitsuneRal
Copy link
Member

Actually, the test failures here seem to be for the same reason I had with my implementation - those false negatives.

@nvrWhere
Copy link
Collaborator Author

Actually, the test failures here seem to be for the same reason I had with my implementation - those false negatives.

I think I see the issue in the test. With no real server a power level event won't be set and of course powerLevelForUser() doesn't have the room context to resolve that and give the creator an automatic power level of 100.

We should be able to resolve the issue by doing all the can stuff in the room with memberEffectivePowerLevel() available.

…efore setting state to avoid pinging the server with no chance of success.
@nvrWhere
Copy link
Collaborator Author

So further investigation once I got the tests working locally (I realise now there is a server) but for whatever reason we're falling through to the final return 0 in memberEffectivePowerLevel in the failing enable encryption test, i.e. no RoomPowerLevelsEvent or creation event found.

@KitsuneRal
Copy link
Member

Ah, I was looking at the wrong failure :( that one indeed is about the test being dirty: basically, it doesn't wait for the room to load completely, and createRoom() creates the initial Room object as soon as it receives its id, without waiting for the first sync to arrive. And then, without even RoomCreateEvent around, the thing runs very wrong obviously.
As for the failure I was actually looking for - that one didn't manifest here because the CI didn't run quotest. Basically, your implementation of canSetState() doesn't take into account, in particular, that room members can change their own names even if their power level is lower than state_default (or whatever is set for m.room.member). This is one of a long list of conditions that should be checked as a part of auth rules, what I mentioned in my first comment.

@KitsuneRal
Copy link
Member

This is the list we should take relevant items from if we want to implement authorization rules (=rules to allow or disallow sending certain events or perform certain actions) properly: https://spec.matrix.org/v1.11/rooms/v11/#authorization-rules

@nvrWhere
Copy link
Collaborator Author

So I agree having had a look I probably need to go back to the drawing board on this one. It looks like for a complete picture we need to be able to see the event that they want to send which in some ways goes against what i was trying to achieve. I.e. giving an indication before someone even tries to create an event, especially for thing like UI and greying out buttons.

I'm going to have a deeper think about this. Maybe we can create something which has 3 output states yes|no|don't know because for some events we can totally give the indication while some we can't.

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 this pull request may close these issues.

2 participants