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

Add support for deletion of an installation (InstallationStore / MemoryInstallationStore) #1272

Merged
merged 3 commits into from
Jun 18, 2021

Conversation

misscoded
Copy link
Contributor

Summary

Fixes #1271

Adds deleteInstallation to the MemoryInstallationStore class, as well as to the InstallationStore interface.

Requirements (place an x in each [ ])

@misscoded misscoded added enhancement M-T: A feature request for new functionality pkg:oauth applies to `@slack/oauth-helper` labels Jun 17, 2021
// returns nothing
deleteInstallation: async (installQuery) => {
// replace myDB.get with your own database or OEM getter
if (query.isEnterpriseInstall && query.enterpriseId !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fix the same error for fetchInstallation method?

Suggested change
if (query.isEnterpriseInstall && query.enterpriseId !== undefined) {
if (installQuery.isEnterpriseInstall && installQuery.enterpriseId !== undefined) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on both of these. Will update!

// org wide app installation deletion
return await myDB.delete(installQuery.enterpriseId);
}
if (query.teamId !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (query.teamId !== undefined) {
if (installQuery.teamId !== undefined) {

@seratch
Copy link
Member

seratch commented Jun 17, 2021

The changes are fine. While reviewing this, I noticed that memory store does not support user tokens. We may want to add some comments to DevDatabase like this:

// using a javascript object as a makeshift database for development
// storing user tokens is not supported
interface DevDatabase {
  [teamIdOrEnterpriseId: string]: Installation;
}

@seratch seratch added this to the [email protected] milestone Jun 17, 2021
@misscoded misscoded merged commit e771849 into slackapi:main Jun 18, 2021
@@ -456,6 +456,8 @@ export interface InstallationStore {
logger?: Logger): Promise<void>;
fetchInstallation:
(query: InstallationQuery<boolean>, logger?: Logger) => Promise<Installation<'v1' | 'v2', boolean>>;
deleteInstallation:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider making this optional incase folks have created their own installationStores. By making it required, it would mean their installationStores are invalid when they upgrade.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, great point. Thanks.

@stevengill
Copy link
Member

@seratch how does memoryStore not support userTokens? It stores the entire installation object which should contain the userToken. That would then get attached to context in bolt

@seratch
Copy link
Member

seratch commented Jun 18, 2021

@stevengill If the internal datastore supports user tokens too, the key should not be teamIdOrEnterpriseId. Your app can have multiple user tokens per workspace/org. The current interface stores only the latest installation for a workpace.

interface DevDatabase {
  [teamIdOrEnterpriseId: string]: Installation;
}

Your app may ask an admin to install the bot and ask other end users to install the app with a set of user token scopes (this is a common scenario). Your app may want to look up user tokens by a user ID as necessary. The simplest interface (which does not support historical data) would be like this:

interface DevDatabase {
  [teamIdOrEnterpriseId: string]: {
    bot?: Installation,
    [userId: string]: Installation
  }
}

@stevengill
Copy link
Member

Ooo good point! We should update our memory install store and examples to this!

@seratch
Copy link
Member

seratch commented Jun 18, 2021

@stevengill Also, as the DevDatabase interface does not keep all the installations for bot, it does not support incoming webhooks in a good way. With the current implementation, an app can keep only the latest one in database. The developers of the app can still get other URLs in the Slack app admin pages, though.

Python/Java's built-in installation stores support historical data as the default way to cover any use cases including webhooks (custom implementations do not need to have historical data). Here are examples: Python SQLAlchemy, Java S3

The DevDatabase is not production-ready. That being said, the reference implementation should properly support incoming webhooks too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality pkg:oauth applies to `@slack/oauth-helper` semver:minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add deleteInstallation to MemoryInstallationStore
3 participants