nyaruka / rapidpro

TextIt is a hosted service allowing organizations to visually build scalable interactive messaging applications.
Other
36 stars 23 forks source link

Rolling Twilio keys breaks channels without recourse #4227

Open ericnewcomer opened 1 year ago

ericnewcomer commented 1 year ago

Describe the bug A clear and concise description of what the bug is.

To Reproduce Steps to reproduce the behavior:

  1. Add Twilio keys to account and add a channel
  2. Roll Twilio keys
  3. Update org config with new Twilio keys
  4. Send messages with existing channel from step 1

Messages aren't delivered with errors that the from number is not part of the account. When channels are added, the current account level twilio config is stored on the channel. When updating the account level config, the existing channel is not updated with the new keys.

Expected behavior We need to either self-cure or give the user a way to fix their channels without deleting and recreated them.

norkans7 commented 1 year ago

I guess the solution is to allow people to update the keys for a channel, I remember we started adding the credentials to the channel as we needed to allow someone to connect numbers from different Twilio account to the same workspace without breaking the first channel

rowanseymour commented 1 year ago

Chatted a bit with @ericnewcomer about this and #4251 is mostly the direction we want to go but... I don't see any reason for this to be a Twilio only thing. Seems like you should be able to edit credentials on any channel. So I think we want to have a think about how this can be generalized across channel types.

We also want to get rid of having Twilio and Vonage configured on the org. When you add a Twilio channel we can store the keys on the org to re-use next time you add a Twilio channel, but all the views live on the channel type.

However we can also be smart and if you're editing credentials for one channel, we can update the new key on all the channels with the same SID.

Is there a good reason to let people edit the SID on a channel? That then requires testing that phone number belongs to that SID. Maybe you can only edit the secret key? SIDs don't typically change right?

norkans7 commented 1 year ago

We can move the views to the channel types or utils(since they are shared among multiple channel types)

For the SID, I thought the number could have been moved on a subaccount on Twilio and that would cause the account SID to change, so we are allowing the update without requiring the channel to be removed and added back,

I also think the view can be used whenever something on the Twilio side was changed and broken the channel, we make sure the channel will set the app and webhook URLS again in a state that we are sure the channel will work

ericnewcomer commented 1 year ago

I think the case when a number is migrated from one Twilio account to another, I am fine with that being a case where you need to remove/readd the channel. I think in this case, we need to create the TwiML app, etc anyways, so updating the SID isn't likely going to be enough.

rowanseymour commented 1 year ago

Yeah I was thinking the same, and if we keep the SID the same, it's also going to make it easier to show the user how this update will effect other channels wit the same SID, e.g. This will update the secret key for your 3 other channels connected to this Twilio account

rowanseymour commented 1 year ago

I think a useful step 1 to implementing changing keys across channel types, would be ChannelType having a method like check_credentials(config: dict) -> bool: which can be called when claiming or updating a channel... or even as something a user can trigger from the UI. That way we don't duplicate the logic to check the validity of the credentials.

Always good to think of ways to make large changes more incremental...