seratch / slack-edge

Slack app development framework for edge functions with streamlined TypeScript support
https://github.com/seratch/slack-edge-app-template
MIT License
87 stars 5 forks source link

SLACK_BOT_SCOPES and SLACK_USER_SCOPES should not be Env variables #6

Closed zhawtof closed 8 months ago

zhawtof commented 9 months ago

Maybe I'm missing the intention of making scopes environment variables but I can see some clear disadvantages:

  1. Using Slack APIs sometimes requires an update to the scopes for an application. They should be written into the code as constants so that they can be tracked across version control. Rarely are scopes environment specific between development, QA, and prod.
  2. Requiring a global set of scopes prevents the developer from using a limited subset of scopes for some users and a more expansive set of scopes for others. Scopes as an environment variable prevent variations at runtime.
seratch commented 9 months ago

@zhawtof The intention of having the values in env is mainly to manage app configuration in one place.

Requiring a global set of scopes prevents the developer from using a limited subset of scopes for some users and a more expansive set of scopes for others. Scopes as an environment variable prevent variations at runtime.

When having multiple patterns of installations, you can override the scope variables inside your fetch function. Also, the scope variables are optional, so you don't need to set them if you don't want to have them in env. Here is a simple example code to override them:

export default {
  async fetch(
    request: Request,
    env: Env,
    ctx: ExecutionContext,
  ): Promise<Response> {
    env.SLACK_BOT_SCOPES = "commands,chat:write";
    const app = new SlackOAuthApp({
      env,
      installationStore: new KVInstallationStore(env, env.SLACK_INSTALLATIONS),
      stateStore: new KVStateStore(env.SLACK_OAUTH_STATES),
      oidc: {
        callback: async (token, req) => {
          const handler = defaultOpenIDConnectCallback(env);
          return await handler(token, req);
        },
      },
    })

I hope this clarifies.

zhawtof commented 8 months ago

Thanks Kaz. I didn't think about being able to override the env variable. That makes a lot more sense