Add Support for FileInstallationStore #1279

misscoded · 2021-07-02T00:21:00Z

Summary

Currently, the default (and only) InstallationStore made available to developers out of the box is the MemoryInstallationStore. This store isn't ideal for all development scenarios, as data is stored in memory and thus cleared out when the server is restarted.

We should introduce a FileInstallationStore, reasonably following the existing pattern(s) found on the Python and Java SDK side of things.

Exportability for Choice

Though MemoryInstallationStore remains the default if not otherwise specified, to allow developers the choice between using MemoryInstallationStore and FileInstallationStore, both are now exported for direct use in their projects (set to the installationStore constructor option):

const { InstallProvider, MemoryInstallationStore, FileInstallationStore } = require('@slack/oauth');

const installer = new InstallProvider({
  clientId: process.env.SLACK_CLIENT_ID,
  clientSecret: process.env.SLACK_CLIENT_SECRET,
  authVersion: 'v2',
  stateSecret: 'my-state-secret',
  // installationStore: new MemoryInstallationStore(),
  installationStore: new FileInstallationStore({ baseDir: '.', historicalDataEnabled: false }),
});

Options include: baseDir, clientId, and historicalDataEnabled. Developers can go with the defaults, or override to suit their needs.

Installation Directory + File Pattern

For the most part, implementation has stayed true to what is found in Python, but some differences are detailed below:

The enterprise_id and team_id are used to create the installation directory. If both present, the pattern is enterprise_id-team_id. If only one of the two are present, only that ID is used for the directory. This differs slightly from Python's use of none in the directory name if one is not passed.
When storeInstallation is called, if historicalDataEnabled is enabled (which it is by default), 4 records will be created: an app-latest, user-[user_id]-latest, app-[currentUTC] and user-[user_id]-[currentUTC]. Every subsequent install will overwrite the data in latest and save two additional currentUTC records. If set to false by the developer, only *-latest records will be saved.

Python goes a step further by saving the same information into both installer-latest / installer-[currentUTC] and the user-[user_id]-* records. I omitted the former records because it wasn't clear to me how the duplication was value-adding, but am open to introducing it if folks feel it's important or if I've misunderstood its purpose.
Python also makes use of a to_bot() method to shape the data prior to writing it to file, but for a similar reason to the above, I forwent that. Also equally happy to introduce it if it's the better (or necessary) approach.

Directory/File Restructuring

New /stores directory. Since we now have two stores, I've pulled out the MemoryInstallationStore class into a stores directory, alongside the new FileInstallationStore. These two are then made available in a dedicated index.js so that the import is cleaner (both importable directly from /stores).

Requirements (place an `x` in each `[ ]`)

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.

packages/oauth/src/index.ts

packages/oauth/src/stores/file-store.ts

misscoded · 2021-07-02T00:30:35Z

packages/oauth/src/stores/file-store.ts

+      if (this.historicalDataEnabled) {
+        const currentUTC = Date.now();
+        writeToFile(`${installationDir}/app-${currentUTC}`, installationData);
+        writeToFile(`${installationDir}/user-${user.id}-${currentUTC}`, installationData);


I mentioned this in the summary, but this is where it's applicable, so carrying it over:

The Python FileStoreInstallation goes a step further by saving the same information into both installer-latest / installer-[currentUTC] and the user-[user_id]-* records. I omitted the former records because it wasn't clear to me how the duplication was value-adding, but am open to introducing it if folks feel it's important or if I've misunderstood its purpose.

Specifically talking about this file based implementation, not having all the historical data is also fine as most developers won't use it for production.

The reason why we store all installations in Python / Java is that having the data can be helpful for troubleshooting / system operations in production. When you do investigation about an issue related to a user who installed the app multiple times, checking each installation's timestamp, the difference on its scopes, and the existence of enterprise_id may help.

If we think this file based one is a kind of reference implementation, having this consideration may be worth. Not a strong opinion here, though. Just having a comment about this context there is also fine.

Totally get it and am with you regarding the importance. 👍🏼

I think what I'm not understanding is the difference between these two records, other than the extra formatting done to the first, and why having both is necessary or beneficial.

Should we consider setting saving historical data default to false? Users could set it to true if they want to save historical data.

I don't have a strong opinion on this. Happy to go with whatever others deem the best default.

I think what I'm not understanding is the difference between these two records, other than the extra formatting done to the first, and why having both is necessary or beneficial.

Even after revoking all the user installations, bot token must still exist. This is one of the major improvements in GBP (Granular Bot Permissions). This is the reason why Python / Java SDK have bots table separately.

If there is a better alternative way, we can go with a different approach in Node SDK first and implement the same in others, though. Having is_user_token_revoked flag in the installation table plus trying to fetch a bot token even when all installations are marked as deleted may work. Do we think this is a better approach? 🤔

Should we consider setting saving historical data default to false?

In Python / Java SDK, the historical data support is enabled by default. For consistency and encouraging a similar approach for production app design, I prefer turning it on but I am open to change this default for Node SDK (and possibly for others in the future major versions).

@misscoded @stevengill For enabling the historical data support by default or not, I'm happy to leave the decision to you all. I've already shared my thoughts on it in the above comment. If we want to change the defaults for Python / Java, we may want to hold off doing so until the next major releases (we won't do at least this year for both).

For having bots and user installations, as I mentioned above, having something similar (or user_token_revocation flag) should be necessary to keep a bot token even after the user(s) who installed the app are deactivated or all the user tokens are revoked. For these two store implementations, I think it's okay not to support the use case.

I think it makes sense to have historical data enabled as the default, especially to maintain parity between the three languages. So we're good on that point! I'll keep it as is.

I'd like to revisit the other point you bring up when I circle back around to sort out the lack of support for user tokens as a whole, unless you think we shouldn't move forward with introducing this until that's sorted. I think I'm still fuzzy on the details.

misscoded · 2021-07-02T00:34:30Z

packages/oauth/src/stores/file-store.ts

+  }
+
+  public async fetchInstallation(query: InstallationQuery<boolean>, logger?: Logger): Promise<Installation> {
+    const { enterpriseId, teamId } = query;


MemoryInstallationStore doesn't take into consideration user_id, so I omitted it here, but now that I'm reading over it in this context..

Given that user.id has been introduced in storeInstallation above (edit: and deleteInstallation below), my gut feeling is that it needs to be introduced here, as well, but confirmation of this from others would be useful.

How about having TODO comments on user token scope installation support?

@seratch so to support user tokens, it seems we would want to fetch /user-{user_id}-latest from the installationDirectory. How do you determine if you want to return this over app-latest? Do you look for the existence of user_id in the query and /user-{user_id}-latest on the file system and return it over app-latest? How about if you were explicitly looking for a user token existing, in that scenario if /user-{user_id}-latest didn't exist, you would want to throw an error instead of getting app-latest which might contain a user token belonging to a different user.

@stevengill Performing an additional query would be necessary for it. Here are the ways we do in Python / Java.

https://github.com/slackapi/bolt-python/blob/v1.6.1/slack_bolt/authorization/authorize.py#L137-L167

https://github.com/slackapi/java-slack-sdk/blob/v1.8.1/bolt/src/main/java/com/slack/api/bolt/middleware/builtin/MultiTeamsAuthorization.java#L128-L138

@seratch this is very helpful! So the idea being do two fetchinstallation calls in authorize (or do this logic in fetchInstallatino itself). Roughly, in this JS implementation, the first fetchInstallation call would grab app-latest. We would want to strip out user_id from query so it doesn't try to grab a user-{user_id}-latest instead. The second call to fetchInstallation we would pass in the userId and it would look for user-{user_id}-latest. It would pull out the user portion of the installation Object and attach it to the original installationObject we get back from app-latest. It covers some of the edge cases I was worried about. Nice solution.

To limit the scope, I'm in favor of leaving a TODO to address the user token support at a later time if others are.

packages/oauth/src/stores/file-store.ts

packages/oauth/src/stores/index.ts

packages/oauth/src/index.ts

seratch

I haven't verified if it works fine but at first glance, the changes look good to me. Let a few comments.

packages/oauth/src/stores/file-store.ts

seratch · 2021-07-02T03:29:00Z

packages/oauth/src/stores/file-store.ts

+      if (this.historicalDataEnabled) {
+        const currentUTC = Date.now();
+        writeToFile(`${installationDir}/app-${currentUTC}`, installationData);
+        writeToFile(`${installationDir}/user-${user.id}-${currentUTC}`, installationData);


Specifically talking about this file based implementation, not having all the historical data is also fine as most developers won't use it for production.

The reason why we store all installations in Python / Java is that having the data can be helpful for troubleshooting / system operations in production. When you do investigation about an issue related to a user who installed the app multiple times, checking each installation's timestamp, the difference on its scopes, and the existence of enterprise_id may help.

If we think this file based one is a kind of reference implementation, having this consideration may be worth. Not a strong opinion here, though. Just having a comment about this context there is also fine.

packages/oauth/src/stores/file-store.ts

seratch · 2021-07-02T03:34:04Z

packages/oauth/src/stores/file-store.ts

+  }
+
+  public async fetchInstallation(query: InstallationQuery<boolean>, logger?: Logger): Promise<Installation> {
+    const { enterpriseId, teamId } = query;


How about having TODO comments on user token scope installation support?

packages/oauth/src/stores/file-store.ts

packages/oauth/src/index.ts

seratch · 2021-07-02T03:50:13Z

This pull request seems to hav conflicts with the latest revision in main branch. Resolving it before you finalize this work would make things easier.

docs/_packages/oauth.md

seratch · 2021-07-13T09:55:47Z

@misscoded
I don't have any further comments apart from #1279 (comment)

If we are comfortable to have this change in the next minor (v2.2 - the main update is the token rotation support), you can merge this pull request. Otherwise, you can move this PR and its issue to v2.3 milestone!

misscoded · 2021-07-13T22:59:33Z

@seratch

If we are comfortable to have this change in the next minor (v2.2 - the main update is the token rotation support), you can merge this pull request. Otherwise, you can move this PR and its issue to v2.3 milestone!

I think we're good, just waiting on Steve to give the final go ahead on what's here.

Add support for FileInstallationStore

fbd1603

misscoded added semver:minor enhancement M-T: A feature request for new functionality pkg:oauth applies to `@slack/oauth-helper` draft labels Jul 2, 2021

misscoded self-assigned this Jul 2, 2021

misscoded commented Jul 2, 2021

View reviewed changes

packages/oauth/src/index.ts Show resolved Hide resolved

misscoded commented Jul 2, 2021

View reviewed changes

packages/oauth/src/stores/file-store.ts Show resolved Hide resolved

misscoded commented Jul 2, 2021

View reviewed changes

packages/oauth/src/stores/file-store.ts Show resolved Hide resolved

misscoded commented Jul 2, 2021

View reviewed changes

packages/oauth/src/stores/index.ts Show resolved Hide resolved

misscoded commented Jul 2, 2021

View reviewed changes

packages/oauth/src/index.ts Outdated Show resolved Hide resolved

misscoded requested review from stevengill, kian2attari, mwbrooks, seratch and srajiang July 2, 2021 00:39

seratch reviewed Jul 2, 2021

View reviewed changes

misscoded added 3 commits July 2, 2021 13:31

Merge master, resolve conflicts

4ca4a97

update docs + incorporate PR feedback

21be180

installQuery doc fixes

9f07eff

misscoded commented Jul 2, 2021

View reviewed changes

docs/_packages/oauth.md Outdated Show resolved Hide resolved

seratch mentioned this pull request Jul 6, 2021

[FEATURE] Org-aware app slackapi/node-tasks-app#7

Open

6 tasks

doc fixes, test fixes, adjust delete functionality

e062678

misscoded removed the draft label Jul 9, 2021

misscoded marked this pull request as ready for review July 9, 2021 00:48

seratch added this to the [email protected] milestone Jul 12, 2021

misscoded requested a review from seratch July 13, 2021 23:01

stevengill approved these changes Jul 14, 2021

View reviewed changes

misscoded merged commit 1a4497f into slackapi:main Jul 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support for FileInstallationStore #1279

Add Support for FileInstallationStore #1279

misscoded commented Jul 2, 2021 •

edited

Loading

misscoded Jul 2, 2021

seratch Jul 2, 2021 •

edited

Loading

misscoded Jul 2, 2021

stevengill Jul 8, 2021

misscoded Jul 9, 2021

seratch Jul 9, 2021

seratch Jul 13, 2021

misscoded Jul 13, 2021

misscoded Jul 2, 2021 •

edited

Loading

seratch Jul 2, 2021

stevengill Jul 8, 2021

seratch Jul 8, 2021

stevengill Jul 8, 2021

misscoded Jul 8, 2021

seratch left a comment

seratch Jul 2, 2021 •

edited

Loading

seratch Jul 2, 2021

seratch commented Jul 2, 2021

seratch commented Jul 13, 2021

misscoded commented Jul 13, 2021 •

edited

Loading

Add Support for FileInstallationStore #1279

Add Support for FileInstallationStore #1279

Conversation

misscoded commented Jul 2, 2021 • edited Loading

Summary

Exportability for Choice

Installation Directory + File Pattern

Requirements (place an x in each [ ])

Choose a reason for hiding this comment

seratch Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misscoded Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch left a comment

Choose a reason for hiding this comment

seratch Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch commented Jul 2, 2021

seratch commented Jul 13, 2021

misscoded commented Jul 13, 2021 • edited Loading

misscoded commented Jul 2, 2021 •

edited

Loading

Requirements (place an `x` in each `[ ]`)

seratch Jul 2, 2021 •

edited

Loading

misscoded Jul 2, 2021 •

edited

Loading

seratch Jul 2, 2021 •

edited

Loading

misscoded commented Jul 13, 2021 •

edited

Loading