Installation does not model situations where multiple Incoming Webhooks are created in a single workspace/organization

[aoberoi]

Description

Currently the Installation object contains a single incomingWebhook property, as an object with several properties. That works when an OAuth flow completes and the installation needs to be stored since a single installation can generate at most one Incoming Webhook. However, when another installation in the same workspace or organization occurs, it will create a new Incoming Webhook. This might also be fine if you're happy with the InstallationStore storing a new record for this installation, even though most of the data in it is duplicative. However when fetching that installation, the InstallationStore returns a single Installation object, which can only contain one Incoming Webhook. That means one of the two Incoming Webhooks is not reachable.

One possible solution is to update the interface for Installation so that the incomingWebhook property becomes incomingWebhooks, which contains an array of the created webhooks instead of just one. However we'd have to figure out how InstallationStores are meant to merge the individual installations into one. This might require a redesign of that interface, and updated implementations.

Another solution is to update the return value of fetchInstallation() to be an array of Installation objects. This would be simpler to implement, but would encourage storing duplicative data .

There may be other good solutions available. Let's brainstorm and discuss.

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

• I've read and understood the Contributing guidelines and have done my best effort to follow them.
• I've read and agree to the Code of Conduct.
• I've searched for any related issues and avoided creating a duplicate issue.

[seratch]

Thanks for bringing a great topic to discuss!

One possible solution is to update the interface for Installation so that the incomingWebhook property becomes incomingWebhooks, which contains an array of the created webhooks instead of just one.

In my opinion, an Installation should simply represent a Slack app installation. Specifically, it should be in 1:1 relationship with oauth.v2.access/oauth.access results.

Having all available incoming webhooks in a single installation data (as suggested here) means merging multiple installation data rows into one row in a database. Doing so will bring more complexity to the SDKs, plus, will make implementing valid custom installation stores harder.

Here is one example database table that stores historical data of all installations in Python SDK. https://github.com/slackapi/python-slack-sdk/blob/v3.0.0/slack_sdk/oauth/installation_store/sqlite3/__init__.py#L55-L72

With this database table design, appending a new row for each installation is recommended over updating a single installation in the table. In my understanding, the general recommendation for implementing the storage layer for Installation in Node SDK should the same. Having all the histories is crucial for operating Slack apps. For example, requested scopes can be changed in subsequent installations. For properly managing tokens and doing investigations for troubles, historical data is greatly helpful.

So, my comments/suggestions here are:

• On the assumption that Node SDK's InstallationStore stores all histories of installations, the query to fetch the latest installation for user/team/org should be select * from slack_installations where enterprise_id = ? and team_id = ? order by installed_at = ? desc (note: we do not need to have installed_at field in Installation model - it can exist only under the hood).
• If the Node SDK's InstallationStore does not recommend storing all the installation histories with the current installation data model, it should be corrected. Updating a single installation per workspace/org is not recommended as the operation deletes existing incoming webhooks.
• Fetching all incoming webhooks is not a so common use case particularly for Bolt apps' listeners or similar. If a developer really needs to do so, running a simple query would be enough. For instance, if a developer would like to fetch all the incoming webhook URLs added to a workspace, a query like select incoming_webhook_url from slack_installations where enterprise_id = ? and team_id = ? works. I don't think InstallationStore should support the functionality.

Another solution is to update the return value of fetchInstallation() to be an array of Installation objects.

In this case, the method name should be fetchInstallations (plural). It may be helpful for some use cases but I don't think all InstallationStore implementations should support this method.

Overall, I don't see the necessity to change InstallationStore / Installation interface. If we see more needs for a handy way to fetch all incoming webhooks, we may want to consider adding fetchIncomingWebhooks separately.

[aoberoi]

Great points about the utility of having a historical record of individual installations! I see a lot of value there and would like to preserve that too.

In my view, InstallationStore doesn't have any opinion what your data model/schema should or should not be. It doesn't prescribe a relational model, or a document based model, or anything. It doesn't even say the object you chose to implement the InstallationStore interface should solely be responsible for fetching and storing installations - it could be any object in your model that happens to at least have those two methods on it. (I think it would be a really interesting simple design to just have one object that implements the InstallationStore and ConversationStore interfaces in addition to methods specific to your app's own domain model. You'd just need one instance to retain the database connection and credentials).

Therefore I believe the InstallationStore interface should be about describing the needs of our library. Decisions like storing installed_at (or not) are completely left up to the implementation. We just need a service that knows how to do a couple operations: storeInstallation() and fetchInstallation(). We don't actually need to fetchIncomingWebhooks() to provide this library's functionality, so I don't think we should include it in the interface. In contrast though, I think we do need to know the best set of authorization info (tokens, user ID, and bot user ID) in order to provide this library's functionality.

In order to understand which authorization info is the best we must consider all of the installations which match the query (team ID, enterprise ID, user ID, conversation ID), right? I had hoped that our library could just fetch a single Installation and pull out the relevant authorization info and it would be the best. But now I'm realizing that determining which authorization info is not that simple. We can't even assume it's the latest installation. In a shared channel, if you have the conversation ID, you might have two different installations with two different team IDs (and two different enterprise IDs) but you might want to use the authorization info for the one where you were installed first because it simplifies the user experience to get all messages from the same bot user instead of two different bot users.

But still, I'd like to save our developers the effort of having to describe the complex choice of which authorization info is best. I think if we were to update the InstallationStore interface to have a fetchInstallations() method (plural) (or just update the return type of fetchInstallation() to Installation | Installation[]), then we could try to determine which authorization info is best for the developer using some naive logic (whichever installation matches most of the InstallationQuery properties and is later in the array), and still let them express their own understanding of best (by implementing their own logic inside fetchInstallations() and just returning a single installation. Whatever they decide to return, we could make it available in the Bolt context so they don't have to perform another fetch later in their listeners.

What do you think about this?

[seratch]

But now I'm realizing that determining which authorization info is not that simple. We can't even assume it's the latest installation.

In the case of bot tokens, this is quite simple. The latest one is always the right one. That's one of the reasons why I recommend having a kind of bots table for a bot token and its associated data in Python and Java. In the case of installations, that depends and queries may not be so simple as you mentioned.

In a shared channel, if you have the conversation ID, you might have two different installations with two different team IDs (and two different enterprise IDs) but you might want to use the authorization info for the one where you were installed first because it simplifies the user experience to get all messages from the same bot user instead of two different bot users.

I may not fully understand what the situation you mentioned yet. Are you assuming a situation as below?

• A conversation ID is stored in a database and an app's bot user posts messages to the channel
• The conversation can be a shared channel with other workspaces/orgs
• The same app may be installed into the other sides of the shared channel

If so, I'm not sure what InstallationStore should do for this. If a conversation ID is correctly associated with an installed workspace, it should just work. If you want to ensure only one bot user behaves in the shared channel, such coordination needs to be done by some extra code. For instance, you can check if there is another bot user from the same app in the channel by web API results and/or Events API payloads.

When it comes to Events API handling, you don't need to worry about it. In the case where the same app is installed in both sides (or multiple workspaces/orgs) in a shared channel, your app does not receive doubled/multiplied events (I'm sure about this at least for message events). Consistently, one bot user in a particular workspace behaves there.

If you are specifically talking about ConversationStore, I haven't done thorough consideration about the compatibility with shared channels yet. In any case, I think we can discuss it separately as it's not directly related to InstallationStore.

or just update the return type of fetchInstallation() to Installation | Installation[])

This sounds ... very confusing to me. Also, it's impossible to follow this design in an idiomatic way in Java. In Python, the
return type can be Union[Installation, List[Installation]] but it's not simple to handle. If we decided to go with an array, installation store should always return an array even when it consists of a single element.

Regarding the last paragraph, I think developers can set additional fields in AuthorizeResult, not Installation. That being said, I understand it may be convenient if a developer could customize only InstallationStore. My alternative idea would be:

• Using the combination of saveInstallation and fetchInstallation is the simplest way - with fetchInstallation, you can always expect the latest installation data.
• If you need additional custom logic, we recommend using fetchCustomInstallation() (we can find a better name) rather than fetchInstallation. In this method, the returned data is not necessarily compatible with Installation data model except the very basic fields like botToken, teamId, and so on.

Or, just allowing developers to have any additional fields in Installation may be fine too.

[aoberoi]

Are you assuming a situation as below?

Yup, that was the situation I was using as an example. I admit, the example is a little contrived. But the general problem here is pretty easy to state: If the framework/library already needs to query the InstallationStore (database) to find the right authorization info (a subset of Installation), it's wasteful to throw away the remaining data in that installation since it can be useful in middleware and listeners.

By extension, its also wasteful to throw away other candidate installations when several are matching the query, because the fetch for one or a small set will likely cost about the same (it might even cost slightly more in some cases to sort by latest and limit 1 than to return the full result). The other installations can also be useful in middleware and listeners. This is similar to how we don't just discard the userToken when there's a botToken supplied - we append it to context because it might be useful to a listener.

I'm trying to find a way to eliminate this waste, while keeping it easy to implement a simplistic fetchInstallation() (just return any one installation that matches the query, most likely the latest makes the most sense). I think we're both aligned on this.

I think the difference is that I've proposed something like an overload for the fetchInstallation() method (which technically cannot be an overload in languages like Java), and you've proposed a separate named method. I think a function with a singular or plural return type is reasonable in JS, and maybe even in Python (we have options like scope that are string | string[] for convenience). In my opinion, this is better than another optional function, which forces people who implement the interface to think "do I implement this or not? if i do implement this, how will it interact with the other existing functions." If the single and plural fetch were semantically different, I'd agree that another function is a more clear design. But in my opinion the semantics are the same, there's just a convenience applied for single values so you don't need to wrap it in an array before you return it.

Can we get the best of both? What if in JS and Python, we did the same fetchInstallation() method with single or array return types? And in Java we added an another function which returns an array of installations as an alternative to fetchInstallation(), that takes priority over fetchInstallation() when it's defined? Does this still feel confusing?

[seratch]

The reference implementation of installation management in the Node SDK does not support multiple installations with incoming webhooks yet. My recommendation is to have historical data for all the installations and to provide a way to access them as necessary. See also: #1272 (comment)

[github-actions]

👋 It looks like this issue has been open for 30 days with no activity. We'll mark this as stale for now, and wait 10 days for an update or for further comment before closing this issue out.