seratch / slack-bolt-extensions

Collection of bolt-js InstallationStore/Receiver implementations + Next.js support module
MIT License
33 stars 5 forks source link

InstallationStore.fetchInstallation can fetch different user's token if userId is specified in the query and the user's installation is not found #12

Open forestaa opened 5 months ago

forestaa commented 5 months ago

Hi, thank you for your awesome extensions!

I have a question about the behavior of PrismaInstallationStore.fetchInstallation (and other implementations in this repository), which is different from FileInstallationStore.fetchInstallation's if userId is specified in the query. FileInstallationStore.fetchInstallation returns installation data without user.token if the specified user's installation data is not found. (see https://github.com/slackapi/node-slack-sdk/blob/664db4e58d5a7b7082d3006b969e82a4b459402e/packages/oauth/src/installation-stores/file-store.ts#L84-L87) However, PrismaInstallationStore.fetchInstallation returns full installation data with different user's token even if the specified user's installation data is not found.

Is this behavior is expected? (There will be no problem as long as all users install the app before use it because everyone's installation is always found, I think.)

Here is sample code.

import { PrismaClient } from "@prisma/client";
import { PrismaInstallationStore } from "@seratch_/bolt-prisma";
import { FileInstallationStore } from "@slack/bolt";

(async () => {
  const installation = {
    team: {
      id: "team",
      name: "test",
    },
    enterprise: { id: "test" },
    user: { id: "user1", token: "userToken", scopes: [] },
    bot: { id: "bot1", token: "botToken", scopes: [], userId: "botUser1" },
  };

  const prismaClient = new PrismaClient();
  const prismaInstallationStore = new PrismaInstallationStore({
    prismaTable: prismaClient.slackAppInstallation,
    clientId: "00000",
  });
  await prismaInstallationStore.storeInstallation(installation);
  const prismaInstallation = await prismaInstallationStore.fetchInstallation({
    teamId: "team",
    enterpriseId: "test",
    isEnterpriseInstall: false,
    userId: "user2", // different from uesr1
  });
  console.log(`prismaInstallationStore.fetchInstallation: ${JSON.stringify(prismaInstallation)}`);

  const fileInstallationStore = new FileInstallationStore({ clientId: "00000", baseDir: "./bolt-js-app-installation" });
  await fileInstallationStore.storeInstallation(installation);
  const fileInstallation = await fileInstallationStore.fetchInstallation({
    teamId: "team",
    enterpriseId: "test",
    isEnterpriseInstall: false,
    userId: "user2",
  });
  console.log(`fileInstallationStore.fetchInstallation: ${JSON.stringify(fileInstallation)}`);
})();

Execution result: fileInstallationStore.fetchInstallation deletes user.token and user.scopes, but PrismaInstallationStore doesn't and return user1's token.

prismaInstallationStore.fetchInstallation: {"team":{"id":"team","name":"test"},"enterprise":{"id":"test"},"user":{"id":"user1","token":"userToken","scopes":[""]},"bot":{"id":"bot1","userId":"botUser1","token":"botToken","scopes":[""]},"tokenType":"bot","isEnterpriseInstall":false,"authVersion":"v2"}
fileInstallationStore.fetchInstallation: {"team":{"id":"team","name":"test"},"enterprise":{"id":"test"},"user":{"id":"user1"},"bot":{"id":"bot1","token":"botToken","scopes":[],"userId":"botUser1"}}
seratch commented 5 months ago

Hi @forestaa, thanks for sharing this! In this scenario, the user token data must be removed, so the Prisma implementation needs improvements!

forestaa commented 5 months ago

Thank you for your quick response! I understand this behavior is not expected.

I have another question. Should fetchInstallation return an user.token if the userId is not specified in query? Currently both FileInstallationStore.fetchInstallation and PrismaInstallationStore.fetchInstallation return an user.token in such case, but I'm not sure if this is appropriate behavior. (I should have open an issue in https://github.com/slackapi/node-slack-sdk, but what do you think?)

seratch commented 5 months ago

I don't think the user token for the app installer should be always removed when a user ID is not given in query. It depends on your requirements. For example, when you build an app that utilizes an admin user's previledges only when certain conditions are surtisfied (e.g., submit a request using modal and then use admin APIs with an admin's user token when it's safe enough), having both bot and user tokens in authorize result should be convenient.

If you think the user token should be consistently omitted, you can implement your own InstallationStore in the way. There is no issue that can prevent you from implementing it on the node-slack-side side.

Regarding this project, my opinion is that 1) when the query has a user ID, the user token must be the user's one, 2) when the query does not have a user ID, the full information including the installed user's token should be kept as-is. Adding an option to customize 2) behavior like you mentioned may be a good addition too, though.

forestaa commented 5 months ago

I indeed understand it is convenient in such a case. Thank you for the detailed explanation. I really appreciate it.