Drop public channel requirement for slack images #54276

wotbrew · 2025-02-25T16:10:46Z

Previously, before a file could be embedded in a bot post (e.g. images in a dashboard subscription post), it first had to be uploaded to the slack-files-channel. This PR makes it so that the slack-files-channel is no longer necessary, and images can be embedded directly in posts without additional ceremony.

When setting up the slack integration, users no longer need to specify a files channel name. They now only need provide the oauth token.

To be clear, if this PR is merged we will no longer post images to the configured files channel - so if users somehow depend on that (not only the dashboard/alert posts themselves, but the file post) then we might break them. On balance I consider this unlikely enough that the additional complexity required to support any such use cases would not be worth it.

This has a few advantages:

Fewer requirements of users to get started with slack, less likely to bounce off due to confusion etc
Privacy and security is improved
e.g say you had a dashboard of employee stats, salary, sick days etc - you might hesitate setting up a subscription because all your slack users would be able to see the dashboard images.
Less complexity, tests, documentation needed
We could not get private channels to work as a files channel, this bypasses that problem.

downgrades note This is a code only change, the setting is not deleted from existing instances, so if downgraded - the slack integration will start using the files channel again. If a new slack installation (that has never used the files channel) is downgraded, then the files channel will need to be added before the integration works again.

Closes #53319

Closes WRK-59

Description

This is made possible by dropping the legacy slack 'attachment' used for images, and instead utilising the blockkit image element, which permits sending private files directly using the slack_file field.

For large dashboards, more than one slack post will be made per notification, to work around block limits (50 per message).

How to verify

Describe the steps to verify that the changes are working as expected.

Setup a new slack integration
- admin settings
- notifications
- create slack app (I used my own slack developer workspace)
Create a dashboard
Test a subscription
- click on sharing
- click on subscriptions
- click 'send it to slack'
- choose a channel to post to (private or public is fine)
- click send to slack now
Observe the post in slack, image is shared directly without posting to a files channel

Checklist

Tests have been added/updated to cover changes in this PR.

trunk-io · 2025-02-25T17:56:24Z

Failed Test	Failure Summary	Logs
`multi-row-create-test`	The expected result of the `spec-update/do-update!` function does not match the actual result.	Logs ↗︎

_{View Full Report ↗︎ ⋅ Docs}

src/metabase/channel/impl/slack.clj

docs/configuring-metabase/environment-variables.md

qnkhuat · 2025-02-28T10:40:14Z

Testing and I get this error: clojure.lang.ExceptionInfo: Slack API error: missing_scope {:error-code "missing_scope", :message "Slack API error: missing_scope", :response {:ok false, :error "missing_scope", :needed "files:read", :provided "users:read,channels:read,channels:join,files:write,chat:write,chat:write.customize,chat:write.public"}, :url "https://slack.com/api/files.info"} looks like we need to include files:read scope

crisptrutski

Devils advocating here, but can you imagine any customers being upset at losing the image firehose? Can't imagine a user wanting to be in that channel, but perhaps being used for automation or something.

qnkhuat · 2025-02-28T10:55:04Z

So I've added the files:read scope and it works.

But I have another issue with sending a big dashboard with 34 cards: Caused by: clojure.lang.ExceptionInfo: Slack API error: invalid_blocks {:error-code "invalid_blocks", :message "Slack API error: invalid_blocks", :response {:ok false, :error "invalid_blocks", :errors ["no more than 50 items allowed [json-pointer:/blocks]"], :response_metadata {:messages ["[ERROR] no more than 50 items allowed [json-pointer:/blocks]"]}}}

For your info, I use the example dashboards at http://localhost:3000/dashboard/1-e-commerce-insights.

I think we need to find a way around this, otherwise this will break some existing integration.

… files channel This removes the requirement to have a public files channel so share images or other files in slack bot posts. Simplifying the integration and lessening requirements to get setup. It enables the posting of images files directly to public or private channels without sharing - improving privacy and security.

There was an accidental introduction of files.read. The goal is to preserve the previous behaviour of reusing completeUploadExternal. This limits the oauth scope requirements of the bot token.

- Its easy to mapv - (cond) over nested (if)s - Fix out of date docstring

wotbrew · 2025-03-05T18:37:10Z

Devils advocating here, but can you imagine any customers being upset at losing the image firehose? Can't imagine a user wanting to be in that channel, but perhaps being used for automation or something.

I mean, yes - I can imagine it. I think we win on balance?

qnkhuat

nicely done!

no scope changes, get passed the block limit.

I do find a mention of slack-channel in docs/configuring-metabase/config-template.md which I think you need to remove.

other than that this looks great to me.

Side note: I think we need to mention in the release note that we'll no longer update images to a channel in case people rely on these.

* Prefer slack_file for images, so metabase no longer requires a public files channel This removes the requirement to have a public files channel so share images or other files in slack bot posts. Simplifying the integration and lessening requirements to get setup. It enables the posting of images files directly to public or private channels without sharing - improving privacy and security. * Files channel deprecation note * Drop slack-files-channel requirement from slack /settings API * Remove remaining files-channel calls * Remove files channel input from SlackSetupForm * Remove slack-files-channel from SettingsManagerSettings * Remove slack-files-channel from API docs * Drop files channel configuration guidance * Remove slack channel prompt/description * Instead of polling files.read reuse the complete call There was an accidental introduction of files.read. The goal is to preserve the previous behaviour of reusing completeUploadExternal. This limits the oauth scope requirements of the bot token. * If more than 50 slack blocks, send using multiple messages * Escape mkdwn special-processing chars in card links * Simplify slack attachment uploading - Its easy to mapv - (cond) over nested (if)s - Fix out of date docstring * Remove slack-files-channel from config-template.md * Remove metabase_files check from e2e suite

wotbrew added no-backport .Team/Workflows run-tests labels Feb 25, 2025

wotbrew self-assigned this Feb 25, 2025

wotbrew force-pushed the WRK-59-slack-images-to-private-channels branch from fef2f7c to 1e1a12a Compare February 25, 2025 17:10

wotbrew force-pushed the WRK-59-slack-images-to-private-channels branch 6 times, most recently from 2773f32 to a974236 Compare February 27, 2025 10:27

wotbrew commented Feb 27, 2025

View reviewed changes

src/metabase/channel/impl/slack.clj Outdated Show resolved Hide resolved

wotbrew force-pushed the WRK-59-slack-images-to-private-channels branch from a974236 to faffe90 Compare February 27, 2025 10:43

wotbrew marked this pull request as ready for review February 28, 2025 09:30

wotbrew requested review from a team as code owners February 28, 2025 09:30

wotbrew requested a review from qnkhuat February 28, 2025 09:31

crisptrutski reviewed Feb 28, 2025

View reviewed changes

docs/configuring-metabase/environment-variables.md Show resolved Hide resolved

crisptrutski reviewed Feb 28, 2025

View reviewed changes

wotbrew marked this pull request as draft March 4, 2025 16:55

wotbrew force-pushed the WRK-59-slack-images-to-private-channels branch from 080868c to 962d55a Compare March 5, 2025 16:04

wotbrew added 2 commits March 5, 2025 16:10

Files channel deprecation note

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

e074588

Drop slack-files-channel requirement from slack /settings API

1fe8ffa

wotbrew force-pushed the WRK-59-slack-images-to-private-channels branch from 962d55a to b287dbb Compare March 5, 2025 16:11

wotbrew added 2 commits March 5, 2025 16:28

Remove remaining files-channel calls

59de4ad

Remove files channel input from SlackSetupForm

1ad9fda

wotbrew added 5 commits March 5, 2025 16:28

Remove slack-files-channel from SettingsManagerSettings

12cd391

Remove slack-files-channel from API docs

e4df6b5

Drop files channel configuration guidance

52b5403

Remove slack channel prompt/description

c803da2

Instead of polling files.read reuse the complete call

8aca0f9

There was an accidental introduction of files.read. The goal is to preserve the previous behaviour of reusing completeUploadExternal. This limits the oauth scope requirements of the bot token.

wotbrew force-pushed the WRK-59-slack-images-to-private-channels branch from b287dbb to c2a8623 Compare March 5, 2025 16:29

wotbrew added 2 commits March 5, 2025 17:09

If more than 50 slack blocks, send using multiple messages

e267ddf

Escape mkdwn special-processing chars in card links

Loading
Loading status checks…

07a2bcc

wotbrew force-pushed the WRK-59-slack-images-to-private-channels branch from c2a8623 to 07a2bcc Compare March 5, 2025 17:10

Simplify slack attachment uploading

Loading
Loading status checks…

9b523b3

- Its easy to mapv - (cond) over nested (if)s - Fix out of date docstring

wotbrew marked this pull request as ready for review March 5, 2025 18:38

qnkhuat requested a review from a team March 6, 2025 04:54

qnkhuat approved these changes Mar 6, 2025

View reviewed changes

wotbrew added 2 commits March 6, 2025 09:34

Remove slack-files-channel from config-template.md

Loading
Loading status checks…

bfb7ec3

Remove metabase_files check from e2e suite

Loading
Loading status checks…

65fb0eb

wotbrew requested a review from filiphric as a code owner March 6, 2025 17:59

wotbrew merged commit 1fc4142 into master Mar 7, 2025
168 checks passed

wotbrew deleted the WRK-59-slack-images-to-private-channels branch March 7, 2025 14:11

wotbrew mentioned this pull request Mar 11, 2025

Truncate slack header titles and links if the exceed block limits #54956

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop public channel requirement for slack images #54276

Drop public channel requirement for slack images #54276

wotbrew commented Feb 25, 2025 •

edited

Loading

trunk-io bot commented Feb 25, 2025 •

edited

Loading

qnkhuat commented Feb 28, 2025

crisptrutski left a comment

qnkhuat commented Feb 28, 2025

wotbrew commented Mar 5, 2025

qnkhuat left a comment

Drop public channel requirement for slack images #54276

Drop public channel requirement for slack images #54276

Conversation

wotbrew commented Feb 25, 2025 • edited Loading

Description

How to verify

Checklist

trunk-io bot commented Feb 25, 2025 • edited Loading

qnkhuat commented Feb 28, 2025

crisptrutski left a comment

Choose a reason for hiding this comment

qnkhuat commented Feb 28, 2025

wotbrew commented Mar 5, 2025

qnkhuat left a comment

Choose a reason for hiding this comment

wotbrew commented Feb 25, 2025 •

edited

Loading

trunk-io bot commented Feb 25, 2025 •

edited

Loading