slackapi / bolt-js

A framework to build Slack apps using JavaScript
https://tools.slack.dev/bolt-js/
MIT License
2.74k stars 393 forks source link

bug: `buildSource` does not take into account `authorizations` array when sourcing `user_id` during user-token retrieval/authorization #2271

Open tomarad opened 1 month ago

tomarad commented 1 month ago

Hey there, We're developing a Slack app, allowing multiple installations per team, and we've run into a problem when using events.

@slack/bolt version

3.21.4

Node.js runtime version

v20.15.0

Steps to reproduce:

  1. Create a Slack app that can subscribe to the member_joined_channel user event.
  2. Install the app through two members of the same team - let's call them Alice and Bob.
  3. Bob opens a private channel.
  4. Bob invites Carol (a third member) to the channel.
  5. The member_joined_channel is fired, and after the buildSource() function, installationStore.fetchInstallation() is called with the user_id equivalent to Carol's id (inside authorize()).
  6. The problem lies with the fact that we cannot know which installation will be able to perform API calls on this private channel - since only Bob has access to it and Alice doesn't, yet we can't know from the source object which installation to use.

After debugging for a while it seems that we can use the authorizations property in body to know which installation is authorized, but it doesn't seem to be passed to fetchInstallation(). Is there any reason for this, or another way to solve the problem?

filmaj commented 1 month ago

So I understand correctly: your app requires user tokens, yes? Presumably to have access to private multi-person DMs that do not include your app?

If so, have you seen our documentation on user tokens in OAuth? I assume your app requires each user to run through the OAuth flow. If so, check the docs out. Your Installation Store implementation will need to take into account the user ID for each fetchInstallation call in order to retrieve the appropriate user-specific access token as each event is sent to your app.

tomarad commented 1 month ago

You got it right @filmaj. We are keeping the user in our installation store, the problem is that on the member_joined_channel event for example, the user_id that in the query object in fetchInstallation() is the user id of the member who joined the channel, and not a user that installed the app. Let me know if it makes sense :)

filmaj commented 1 month ago

Is there a field on the member_joined_channel event that represents the "source" user in this case? Have you inspected all the fields?

tomarad commented 1 month ago

Yeah, I could only find relevant information in body.authorizations.user_id

{
  "token": "...",
  "team_id": "T05UK5GDLLF",
  "context_team_id": "T07E4UY2V3N",
  "context_enterprise_id": null,
  "api_app_id": "...",
  "event": {
    "type": "member_joined_channel",
    "user": "U07EF4VQKJM", // Invited user. Didn't install the app. This gets passed to `fetchInstallation()`.
    "channel": "C07NDDC2KL6",
    "channel_type": "C",
    "team": "T07E4UY2V3N",
    "inviter": "U07DN185RRD", // Inviter user. Omitted.
    "event_ts": "1727111934.000800"
  },
  "type": "event_callback",
  "event_id": "Ev07N7H18B39",
  "event_time": 1727111934,
  "authorizations": [
    {
      "enterprise_id": null,
      "team_id": "T05UK5GDLLF",
      "user_id": "U05UT2A9H6J", // The correct user id, the one who installed the slack app.
      "is_bot": false,
      "is_enterprise_install": false
    }
  ],
  "is_ext_shared_channel": true,
  "event_context": "..."
}

Does passing the whole authorizations array with the query param to fetchInstallation() make sense? Should be pretty hermetic.

filmaj commented 1 month ago

Thanks for that. I think you are right that this is a bug in how user IDs are extracted during buildSource().

In this code, which extracts the relevant team_id from the event, you can see that the code looks at the authorizations array.

However, in this code, which extracts the relevant user_id, it does not look at the authorizations array.

That's my admittedly-quick assessment. Going to set this as a bug that needs fixing. However, a proper fix may need to wait until bolt v4 is released (we are actively working on it), because it feels to me this could be a risky change. Specifically, I feel like there is potential for this adversely affecting other kinds of events. Would need good test coverage to ensure everything works as expected.

filmaj commented 1 month ago

@seratch does the above assessment look correct to you?