[#138] Resolve "BUG: Allow broadcast messages beginning with command keywords"
Closes #138 (closed)
Context
Users reported sending normal broadcast messages that got unexpected responses because they started with reserved command words.
Changes
This MR establishes the following (sensible) desired behavior for messages starting with "no-payload" commands:
- GIVEN an admin on a channel
- WHEN I send any of the following:
hello world
join us tomorrow
goodbye world
leave that one to me
info is on the way
help us get out of this kettle!
responses on now!
responses off now!
vouching on now
vouching off now
- THEN that message is broadcast to all members of the channel
Implementation Notes
- to accomplish the above, we introduce a
validatePayload
helper function intocommands.parse.findCommandMatch
. we use to squash the bug we want to eliminate, but also to clean up code by moving all payload validation (including phone numbers) intocommands.parse
, which eliminates some duplication, and -- we hope -- will help out @mari with #137 (closed) - for the correct handling of broadcast messages begining with "no payload commands", we:
- add a branch for all no payload commands to a case statement in
varlidatePayload
and delegate tovalidateNoPayload
helper - use that helper to inspect whether a no-payload command is followed by text, if so, we return
NOOP
to signal a broadcast message, if not, we pass the originally parsed command through
- add a branch for all no payload commands to a case statement in
- for phone number validation, we:
- introduce a
ParseError
type and specify thatparse
may return either anExecutable
or aParseError
- add a series of case statements to
validatePayload
that fall through tovalidatePhoneNumber
-
validatePhoneNumber
then tries to parse a valid e164 phone number (see: https://www.twilio.com/docs/glossary/what-e164) from the payload.- it returns a
CommandMatch
with the parsed phone number substituted into thematches
field if it can - else it returns a
ParseError
which bubbles up to the return value ofparse
- it returns a
- introduce a
- more generally,
parse
andexecute
now have a slightly different contract:-
parse.parseExecutable
now returns aParseError
if a payload cannot be correctly parsed -
execute.processCommand
now returns early and notifies the command sender if a correctly parsed command does not have a correctly parsed payload (ie: if it is given aParseError
as its first argument - the hope is that @mari can use this new contract to add a
validateIntengerBelow(10)
helper to the switch statements and use it to return early if a user ever sends a command likeVOUCH LEVEL foobar
orVOUCH LEVEL 1000
-
Left out of Scope
@margot i considered rolling #124 (closed) into this but ultimately opted against it. (#directaction
)
reasoning:
- the likelihood that a subscriber or admin wants to send a one word message that is actually a message seems moderately high
- if we always block those, we make the most likely person the most sad (they don't really have a way to do what they want to do).
- the likelihood that an admin might send a malformed message is also moderately high (maybe somewhat lower? particularly after repeated use?)
- however, the consqequences of the failure mode are relatively low: if they send out a message that gets broadcast, it's embarassing maybe but not the end of the world and they can still recover to do what they wanted, unlike the user in the above case, who does not have a way to do the thing they actually wanted to do (send a one word message), and is less likely to be well-versed on how signalboost works (and hence more confused and less likely to try again via the circuitous route of sending another message)
- for all those reasons leaving as
fix if squeaky
for now. but if we hear squeaks, we now have a nice little helper function invalidatePayload
that would be the perfect place to implement it! as well as a new implementation ofexcecute.processCommand
that will handle the error we were talking about using in this case!