send error if someone wasn't added to admin group correctly. #95
Conversation
| @@ -141,6 +143,13 @@ def is_admin_group_message(self, command: deltachat.Message): | |||
| else: | |||
| logging.info("%s is not allowed to give commands to mailadm.", | |||
| command.get_sender_contact()) | |||
| elif command.chat.is_protected() and not command.is_encrypted(): | |||
There was a problem hiding this comment.
is the command allowed to be unencrypted at all? IOW i guess the first part of the elif expression is not needed.
There was a problem hiding this comment.
if you intended to write "is the admin group allowed to be unprotected at all?" - then you're correct, it isn't and the is_protected() chat isn't needed.
That is, as long as nobody fiddles with the mailadm database and assigns admingrpid to a different, unprotected chat. If that happens, we have a problem 😅 and then we want to catch it in the else class below, rather than say "please readd this person to the admin group".
So I slightly prefer the flow proposed in this PR, but we can also take out the is_protected() check.
A problem I see in this function, which can make questions like this one quite complex: over all, a message can have six different boolean attributes which destinate what should happen with it. So there are 64 different possibilities what kind of a message we have to deal with. I'm fiddling with a pivot table atm to figure out how to refactor this in a future PR.
(for reference, the 6 attributes are:)
- message.starts_with(„/“)
- message.chat.is_group()
- message.chat.is_protected()
- message.is_encrypted()
- message.chat.id == self.admingroup.id
- message.get_sender_contact() in self.admingroup.get_contacts()
|
closed in favor of #99. |
fixes #68
I was too lazy to write a complicated test which reproduces a corrupted verified group, but with the traceback in #68 I think we don't need to reproduce this to fix it. It's quite straightforward; and after all the bug is only annoying, not a security issue or anything. So imho we can merge this.