element-hq / element-integration-manager

Element Integration Manager related issues
6 stars 1 forks source link

Feeds bot (and presumably others) is added with moderator privileges #44

Closed marcusmueller closed 1 year ago

marcusmueller commented 1 year ago

Describe the bug Followed the migration guide for the new RSS bot.

This added the bot to the room I'm admin off, and instantly gave it moderator privileges, without informing me and without having (to the best of my knowledge) any use of these privileges.

From the principles of least privilege and least surprise, this should not be the case

To Reproduce Steps to reproduce the behavior:

  1. Be an admin in a room
  2. Click on "add widgets, bridges and bots" in the element web sidebar
  3. Scroll down to 'Feeds'
  4. Add bot

Expected behavior

Adding the bot is fine and expected, but giving it anything above "Default" privileges should at least come with a warning sign, and since the feeds bot doesn't have to do any channel management, it shouldn't even get a higher-than-necessary-to-post-messages level.

Desktop (please complete the following information):

JokerGermany commented 1 year ago

Why the heck does a RSS Bot need moderator privileges anyway?

erebion commented 1 year ago

Why the heck does a RSS Bot need moderator privileges anyway?

If there is one, we should at least get informed about the reasoning.

marcusmueller commented 1 year ago

@JokerGermany @erebion while I appreciate you agreeing with me, let's keep this issue as concise as possible. Both the things you expressed are explicitly already part of my report (and thus contribute nothing). Also, as a developer myself, I'd appreciate a kinder approach than "what the heck". Thank you!

Half-Shot commented 1 year ago

Hiya, sorry this has taken long to get back to. Launches are always busy.

I'd be glad to go over why we have this system at the moment.

First off, the moderator permission is unfortunate because we actually don't need the ability to moderate the room, we just need the ability to set Matrix state in the room (https://spec.matrix.org/v1.6/client-server-api/#types-of-room-events). We send a state event to map the room to an RSS feed, a GitHub Repo or something else. We call this "Connection State" in Hookshot.

The state events allow us to store basically what we have connected to your room, which is quite useful because:

Now, the problem is that Matrix does not allow regular users to send state event information to the room. You need to at least have the state_default permission in the room in order set it, and that only comes with a power level of 50. Matrix uses a single number to represent a users power, so it's not easy to toggle on and off permissions like you'd get on Discord: You have to give a user a given power level and everything that comes with.

Element calls power level 50 "moderator" because it also happens to be the level at which you can kick and ban users. It's regretful to me that back when Matrix was originally designed, we didn't make these two separate levels. Anyway, effectively this means the integrations need PL 50 in order to do the parts we need them to do.

So yeah, the optics do not look good that the bot requires "Moderator" to do it's thing, but it's not actually going to moderate anything. Unfortunately, it's something in the short term you'll have to trust us on. Although given the way Matrix works, it'll be fairly obvious if we've been malicious in the room and you'd have every right to tell us off.

The short term solution is if you've set up a feed and you're happy with it, you can de-op the user back to the normal power level. The configuration pages won't work, because it won't be able to modify it's status, but you can always put the power levels back or do your own modifications if you like.

Long term, I'd like for Matrix to be much more granular in it's permission models than it is today. I'd really love it if you could say "I'd like this bot to set RSS feed data in my room and nothing else", but we're not quite there yet. The other option is we drop the whole state system and go back to using PostgreSQL databases or what have you in the backend, but I think that would lose us a lot of the Matrixy-ness of the new system, and you'd have to go back to using our custom APIs to do anything, which would suck!

I apologise we weren't more transparent about this during the transition. It's been the case for our new bot "Hookshot" which both Element and the community have been using for a while, and so it just wasn't something we thought about in the move to migrate from Go-NEB.

I hope that clears things up, I'd be happy to chat about it a bit more in here or #hookshot:half-shot.uk if you've got more concerns :+1:

JokerGermany commented 1 year ago

Can you change the permission back to normal or will this break something? (Then the statelevel should be set) (And before you remove the Feed, you have to give him Moderator rights, or delete the state manual)

Half-Shot commented 1 year ago

You can change the power level before and after making changes in the interface yeah, and that should work fine. Doing it automatically is likely to generate a lot of room spam for more casual users so for the moment it's something you'll need to do yourself.

JokerGermany commented 1 year ago

Okay, which permission is needed by the bot? change settings? Perhaps it would be an option to reduce this room permission to e.g. 25 and give the bot this permission :thinking:

Half-Shot commented 1 year ago

Yeah, if you want to set the state_default to something like 25. This is the "Change settings" permission in the Roles & Permissions dialog in Element.

As mentioned in my essay above, the defaults of 50 means we kinda have to expect most people won't have this, sadly.

JokerGermany commented 1 year ago

Yeah as default i can understand the decision (but don't like it anyway^^)

But as an more Advanced Matrix user i would tweak it.

marcusmueller commented 1 year ago

@Half-Shot yeah, that makes sense; wasn't aware you used room metadata to store the bridging/bot information. Makes sense!

Then again, from an architectural point of view, the party actually initiating and intending these state changes is the user (typically, a human) adding Feeds to the room, and configuring them. Why not let that user modify the room state instead?

opk12 commented 1 year ago

Is it possible to have a brief mention in the config page that the fix is to give mod again to the bot, and perhaps a link to this issue? I initially thought that the bot becoming mod was a bug and made it PL 0, then I tried to subscribe and got a generic Failed to create connection: Unknown error which was not very helpful (and I'm sure I'll forget the details of this issue in a few weeks, so it would save me from doing another search).

jjj333-p commented 1 year ago

perhaps a solution is if the integration is managed through the element integration anyways, you could have the user write the state events and the bot just follows the room state?

msrd0 commented 1 year ago

It might be a good idea to actually let users know about this when adding the bot - I just got an error saying "bot has PL 50 but needs 100" when I try to subscribe to a feed. I'm not going to make that bot an admin, but it would be nice if I didn't have to scroll through GitHub issues to find which permission it actually needs.

Half-Shot commented 1 year ago

Yeah, I think a lot of these issues come down to us providing better errors. I actually think we shipped a fix recently to resolve some of the "Unknown error" issues. The trouble with describing the thing the bot needs is that there isn't a standard way to name the setting it needs in the client:

image

I suspect what we might do in the short term is have a help page somewhere to explain what the setting might look like.

perhaps a solution is if the integration is managed through the element integration anyways, you could have the user write the state events and the bot just follows the room state?

It's certainly something we might do once the APIs land for that kind of thing yep.

marcusmueller commented 1 year ago

The trouble with describing the thing the bot needs

Exactly! I really didn't open this issue because there's an unclear error message (there isn't an error at all), but because the level of privileges the bot gets (without me being informed beforehand) is unreasonably high :)

I suspect what we might do in the short term is have a help page somewhere to explain what the setting might look like.

I know this is an unreasonable thing to propose, but: I disagree! The right thing to do here isn't to fix the documentation, but to fix the mechanism. Maybe having, in parallel to the level-based privileges setting, a kind of a privilege whitelist, so that especially bots can have, e.g., level 0 in a room, but the special exception to post message events, and room status events of certain scope. (Other bots might have no right to post messages, but to invite and kick, for example).

The beauty in the privilege level system is that it's easy for humans to understand that if I'm level 80 I can do everything I want to level 79 and below, up to elevating them to my level. And it's very future-proof: A new kind of thing that needs permissions just gets assigned a default minimum level, and nobody has to keep a user ACL that explicitly lists the thing each roomuser can do.

Neither advantage applies to bots: the privileges they need aren't "linear" in the sense that they need something and everything below; they need to do A, B and C, and nothing else. If there's a new thing Z, which they hadn't the capability to do before, nothing needs to change, they still can't use that capability (which is true, since the bot hasn't changed, and was not aware of that capability before. Unlike "real" users, for bots, user and client form a unit.)

So, what I'd propose is, for the (far) future:

Half-Shot commented 1 year ago

I agree entirely that Matrix's linear permission system currently sucks and isn't fit for this purpose, and I loathe that we have to grant as many permissions as we do to a bot that doesn't need them all. The changes required to Matrix in order to support a permissions system as you suggest are entirely reasonable, but very very challenging to introduce into the spec.

At a high level it would need a new room version and a big upheaval in how we do event authorisation, and then for clients/bots and servers to all agree on and understand the format. We're realistically talking a lot of effort for it.

Which isn't to say we shouldn't do it, and I suspect that now bots are doing more than just posting messages into rooms means the changes will inevitably come. Immediately though I think it's going to need us to document the reason for the present system, as well as looking at how to get away from it.

marcusmueller commented 1 year ago

That's why I assessed my own request to be unreasonable: Systems are big and complex and that makes such changes quite intense :) Yeah, you're right. Documenting the current system up to the point it might change one day is definitely more desirable than not doing it. Thanks!

Half-Shot commented 1 year ago

I think this has probably answered the question for now.