search

slackapi / java-slack-sdk

Slack Developer Kit (including Bolt for Java) for any JVM language
https://slack.dev/java-slack-sdk/
MIT License
573 stars 214 forks source link

Make saveBot required in the InstallationService interface #1253

Open Moh-inc opened 9 months ago

Moh-inc commented 9 months ago

Make saveBot required in the InstallationService interface.

I had to read the source code to understand why token rotation was not working and it was because in the InstallationService that i built, it didn't implement the saveBot which is used in the MultiTeamsAuthorization for token rotation. Hence it shouldn't have a default implementation.

Category (place an x in each of the [ ])

Requirements

Please make sure if this topic is specific to this SDK. For general questions/issues about Slack API platform or its server-side, could you submit questions at https://my.slack.com/help/requests/new instead. :bow:

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you agree to those rules.

hello-ashleyintech commented 9 months ago

hi, @Moh-inc! thanks for submitting this! 😄

I took at look at the InstallationService interface and saw that saveBot is supposed to throw an error if not implemented correctly:

"To run this app, your InstallationService must implement this method properly."

Just wanted to verify if you ran into this error when you were missing an implementation of saveBot in your custom InstallationService.

On my end, I'll reach out to a member of my team internally to see if there's any historical context behind saveBot being made default! This question will be updated once we get a clearer answer about whether this is something that should remain the same or not. 🙌

Moh-inc commented 9 months ago

Thank you for getting back to me, I do think it's a little misleading especially when it's needed in oauth token rotation and can only be found when testing out the application rather than be more convenient and have it implemented by default.

seratch commented 9 months ago

Hi @Moh-inc, thank you so much for taking the time to share this feedback and we are sorry for the confusion you've faced when enabling token rotation for your app.

Indeed, having the default implementation for saveBot in the interface can lead to inconsistency and confusion with the requisites of other methods. However, modifying this now could bring breaking changes for a large number of existing apps that don't use token rotations. For this reason, we are unable to consider the improvement in the short term. We may revisit this as necessary when releasing the next major version (the major release won't come in the short term though).

Thanks again for the feedback. We will keep this issue open to hear from other developers for the future.

Moh-inc commented 9 months ago

Thank you for taking it into consideration!