Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rolling Twilio keys breaks channels without recourse #4227

Open
ericnewcomer opened this issue Jan 4, 2023 · 6 comments
Open

Rolling Twilio keys breaks channels without recourse #4227

ericnewcomer opened this issue Jan 4, 2023 · 6 comments
Assignees
Labels

Comments

@ericnewcomer
Copy link
Member

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
Copy link

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
Copy link
Member

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
Copy link

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants
@ericnewcomer @rowanseymour @norkans7 and others