MMRIZE / MMM-TelegramBot

TelegramBot module for remote-controlling MagicMirror
MIT License
26 stars 8 forks source link

Allow Users only for telecast or per command #33

Closed Rdlgrmpf closed 4 years ago

Rdlgrmpf commented 4 years ago

This module ist really great so far! What do you think of implementing permissions either per command and person or having separate users for the telecast command?

I would really appreciate, if I could configure allowed users, that can use the /telecast command but not the rest, like /status or /screenshot.

eouia commented 4 years ago

Good idea. I'll consider, but I'm also worrying, the configuration would be too complex if each command could have it's own allowed user list.

eouia commented 4 years ago

And I'm regarding this allowed members would be at least your family or company members, not such public. So I'll consider to implement it or not.

Rdlgrmpf commented 4 years ago

Yes exactly. Maybe have two categories or only do this filter for telecast.

Rdlgrmpf commented 4 years ago

I created a pullrequest tonight. please review it :-)

It should be rather straight forward for you. #35

eouia commented 4 years ago

I've added your request feature. Thanks.

Rdlgrmpf commented 4 years ago

Your code definitely looks cleaner :)

Just one thing: wouldn't it be better to have a whitelist? Right now it looks like a Whitelist with a blacklist functionality. So everybody who is not in the list, can't execute the command, but the rest can still execute every other command. It would be more secure, if allowedUsers could only execute the commands, where they are in a list but not the rest.

In your example that would be:

Edit: What about this line. wouldn't it process the command twice?: https://github.com/eouia/MMM-TelegramBot/blob/2fc91ae77845aa77de471c99079eec779fe398e7/node_helper.js#L113

eouia commented 4 years ago

??? Sorry, I might have failed to address exact meanings. English is not my mother tongue.

allowedUser: ["me", "john", "jane"],
commandAllowed: {
  "telecast": ["me", "john"],
  "mychatid": ["me"],
  "modules": [],
}

jane can execute no commands no one can execute /modules command no one can execute /screenshot as it is not mentioned here.

Totally not. Everybody(Jane) in default allowedUser can execute /modules, /screenshot or any other commands(except /telecast and /mychatid).

Only /telecast and mychatid will be affected by this commandAllowed option. So it could be regarded "Whitelist of Whitelist for specific commands". (Am I right? ^^)

Edit: What about this line. wouldn't it process the command twice?:

Not twice, but... difficult to explain. That is some tricky way to handle telecast as a command and normal chat from specific room together. node_helper only check commandLike things, not command itself. Then send only normal commandLike things to front MMM-TelegramBot.js. Front js determine it is a real command or not and do its job. It was OK before telecast implementation. But telecast feature needs normal chat (which was ignored before) also to send to front. So, I use a trick, if /telecast is triggered, send it to node_helper again and regulate it as a normal telecast chat, (including download photos...) then return it to front as a CHAT, not a COMMAND.

Rdlgrmpf commented 4 years ago

Totally not. Everybody(Jane) in default allowedUser can execute /modules, /screenshot or any other commands(except /telecast and /mychatid).

Exactly. That is how I understood it as well. So that means jane can execute the commands where she is in the list and all commands that don't have a specific list, right? This means the default is "can execute command"

I think it would be better if she could only execute commands where she is in the list and that's it. The default here would be "cannot execute command" unless she is in a list of the specific user. pseudocode:

if (allowedUser.contains(username) && commandAllowed[command].contains(username)) {
execute
} else {
errormesdage}
eouia commented 4 years ago

Hmmm. it would be for usage policy. Basically, I regard users would be family members or colleagues of the company where this MagicMirror is installed. The member is not too much to handle. And usually, this bot will be used within these TRUSTABLE members. Maybe the usual case is using this bot in a dedicated chat room with these members. I don't think people use this bot in public anonymous room or with business partners. :D Even in those cases, you can restrict users by allowedUser.

So, frankly says, I couldn't imagine the situation Jane should be allowed only for /screenshot. Why she isn't allowed for other commands? Maybe /telecast, /shell or /allowuser would be more important so she could be restricted, but for other commands? If I adopt your suggestion, I should opt-in my kids to allow normal dozens of commands (including other modules' commands and my own custom commands), I cannot agree this is efficient.

Or plz explain possible use-case scenario.

eouia commented 4 years ago

Anyway, I can add one more configuration option commandNotAllowed:{} for that purpose. I'll consider.

Rdlgrmpf commented 4 years ago

You do have a point. My usecase would be to have all commands to myself, but only allow some users to send telecasts. So people that can send stuff to my mirror at home, but cannot control it in any way. The difference to having a chat room would be that they don't see each other or their messages. I guess with friends the chatroom is also a good option.

Another scenario is just to have a basic rights management. Who can do what. Say in a company I have an admin that manages the mirror. He should be able to do everything. Then there are employees that can also get screenshots or similar stuff. Then guests or external contractors that should only telecast.

Basically just extended security control.

Edit: Chatroom does still need the /telecast in front of the message, despite what the wiki says. Is that correct or a bug?

eouia commented 4 years ago

? telecast: <chat room id> doesn't work? Mine is working.

Rdlgrmpf commented 4 years ago

It is working, but in the chat I still have to type "/telecast hello world" instead of just "hello world". Also the user sending to the group still needs to be in allowedUsers. (I guess that last part is intentional)

eouia commented 4 years ago

How about this?

allowedUser: ["john", "jane", "tom"],
userPermission: {
  "john" : {
    type: "restrict",
    except: ["help", "modules"]
  },
  "jane" : {
    type: "allow",
    except: ["help", "modules"]
  }
}

How do you think about? If you satisfy, I'll update.


in the chat I still have to type "/telecast hello world" instead of just "hello world". Also the user sending to the group still needs to be in allowedUsers. (I guess that last part is intentional)

Weird. For group chat without /telecast, ('telecast: "-1234567"is set. nottelecast:true`)

I suspect your telecast:<chat id> is not valid real chat room id but anyway it is regarded as true.

Rdlgrmpf commented 4 years ago

That looks great, very fine and flexible control! I think this will also be very useful for a lot of users. Would be great if you could add that :-)

You are right. Tried it again and now it works fine. I probably got confused with the branches and testing for my PR.