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

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

Open
tomarad opened this issue Sep 23, 2024 · 6 comments
Labels
auto-triage-skip Prevent this issue from being closed due to lack of activity bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:patch

Comments

@tomarad
Copy link

tomarad commented Sep 23, 2024

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 filmaj added question M-T: User needs support to use the project needs info An issue that is claimed to be a bug and hasn't been reproduced, or otherwise needs more info and removed untriaged labels Sep 23, 2024
@filmaj
Copy link
Contributor

filmaj commented Sep 23, 2024

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
Copy link
Author

tomarad commented Sep 23, 2024

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
Copy link
Contributor

filmaj commented Sep 23, 2024

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
Copy link
Author

tomarad commented Sep 23, 2024

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
Copy link
Contributor

filmaj commented Sep 23, 2024

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 filmaj added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:patch auto-triage-skip Prevent this issue from being closed due to lack of activity and removed question M-T: User needs support to use the project needs info An issue that is claimed to be a bug and hasn't been reproduced, or otherwise needs more info labels Sep 23, 2024
@filmaj filmaj changed the title Help with user id mismatch on events bug: buildSource does not take into account authorizations array when sourcing user_id during user-token retrieval/authorization Sep 23, 2024
@filmaj
Copy link
Contributor

filmaj commented Sep 23, 2024

@seratch does the above assessment look correct to you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-triage-skip Prevent this issue from being closed due to lack of activity bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:patch
Projects
None yet
Development

No branches or pull requests

2 participants