Join slack channels with slack-id (#23495)

* Join slack channels with slack-id

Fixes https://github.com/metabase/metabase/issues/23229

We upload images to a channel and then send messages to the desired
channel referencing those images from the channel we uploaded. But the
slack bot must be in the channel to upload images.

We handle this in `slack/upload-image!` where we watch the error message
and join the channel if we recognize that is our issue. This special
upload channel is set in the admin section when setting up, typed in by
a human.

Slack now requires us to use the internal id of the channel when
joining. IE we used to use "metabase_files" but now we need to use
something like "C87LQNL0Y23". But we do not have this information and we
don't want humans to have to look this up.

SOLUTION:
change our cache. We currently just get a list of channels and users
```
["#general" "@dan" ...]
```

Change this to
```
{:version 2,
 :channels [{:display-name "#random",
             :name "random",
             :id "CT2FNGZSRPL",
             :type "channel"}
            {:display-name "#general",
             :name "general",
             :id "C87LQNL0Y23",
             :type "channel"}
            {:display-name "@dan",
             :type "user",
             :name "dan",
             :id "UR65C4ZJVIW"}
            ...]}
```

Now we have slack internal ids present. When we attempt to join the
slack channel, look for this id and attempt to use that.

This has some knock-on effects. The UI still lists the channels in a
channel picker when sending pulses. The list sent over the wire still
mimics the previous shape (a flat list) and the choice is still the
human readable name.

In the future we should switch over to using the stable ids rather than
solely channel names. Channel names can be renamed.

I didn't go down this route because of the files channel. It is set at
setup before we have a channel list. We could do some kind of run time
migration but it is difficult because it would change the type of
`slack-files-channel` from string to :json to handle the more complex
type. Or perhaps we could make another setting to hold the json form and
set that when we can positively identify things.

In either case, these changes were not required yet to fix our slack
issue. We just upgrade the information we have about slack channels,
downgrade it when it hits the wire so the UI needs no changes, and use
the extra information in the one spot where we need it.

The cache is populated at startup and every four hours after that. So we
do not need to worry about the old cache shape. If the new code is
running, its the new cache.

* Send #channel and @user forms over wire

We store `{"channel": "#slack-pulses"}` in the pulse_channel.details
column so we should keep those types of values around.

We use the bare portion ("slack-pulses") rather than with the hash on it
so we seem to be mixing usernames and channels. But these sets are
distinct and you cannot create a channel with the same name as a
user. Also, channel names are lowercase while channel-ids are uppercase
so those are also non-overlapping sets.

* Put slack token so slack reports as configured

* Errant tap>

* Alignment and docstring fixes

* Remove slack-cache version information

remove the `:version 2` from the cache. We are always in charge of the
cache, and we compute it on startup so there's little risk of other data
shapes being present.

src/metabase/api/pulse.clj

@@ -167,7 +167,9 @@
                    (future (slack/refresh-channels-and-usernames-when-needed!))
                    (assoc-in chan-types
                              [:slack :fields 0 :options]
-                             (slack/slack-cached-channels-and-usernames))
+                             (->> (slack/slack-cached-channels-and-usernames)
+                                  :channels
+                                  (map :display-name)))
                    (catch Throwable e
                      (assoc-in chan-types [:slack :error] (.getMessage e)))))}))
 

src/metabase/integrations/slack.clj

@@ -163,6 +163,14 @@
           (lazy-seq
            (paged-list-request endpoint response->data (assoc params :cursor next-cursor)))))))))
 
+(defn channel-transform
+  "Transformation from slack's api representation of a channel to our own."
+  [channel]
+  {:display-name (str \# (:name channel))
+   :name         (:name channel)
+   :id           (:id channel)
+   :type         "channel"})
+
 (defn conversations-list
   "Calls Slack API `conversations.list` and returns list of available 'conversations' (channels and direct messages).
   By default only fetches channels, and returns them with their # prefix. Note the call to [[paged-list-request]] will
@@ -171,19 +179,16 @@
   (let [params (merge {:exclude_archived true, :types "public_channel"} query-parameters)]
     (paged-list-request "conversations.list"
                         ;; response -> channel names
-                        #(->> %
-                              :channels
-                              (map (fn [channel]
-                                     (str \# (:name channel)))))
+                        #(->> % :channels (map channel-transform))
                         params)))
 
 (defn channel-exists?
   "Returns true if the channel it exists."
   [channel-name]
-  (and channel-name
-       (contains?
-        (set (slack-cached-channels-and-usernames))
-        (str \# channel-name))))
+  (let [channel-names (into #{} (comp (map (juxt :name :id))
+                                      cat)
+                            (:channels (slack-cached-channels-and-usernames)))]
+    (and channel-name (contains? channel-names channel-name))))
 
 (s/defn valid-token?
   "Check whether a Slack token is valid by checking if the `conversations.list` Slack api accepts it."
@@ -196,16 +201,21 @@
         false
         (throw e)))))
 
+(defn user-transform
+  "Tranformation from slack api user to our own internal representation."
+  [member]
+  {:display-name (str \@ (:name member))
+   :type         "user"
+   :name         (:name member)
+   :id           (:id member)})
+
 (defn users-list
   "Calls Slack API `users.list` endpoint and returns the list of available users with their @ prefix. Note the call
   to [[paged-list-request]] will only fetch the first [[max-list-results]] items."
   [& {:as query-parameters}]
   (->> (paged-list-request "users.list"
                            ;; response -> user names
-                           #(->> %
-                                 :members
-                                 (map (fn [member]
-                                        (str \@ (:name member)))))
+                           #(->> % :members (map user-transform))
                            query-parameters)
        ;; remove deleted users and bots. At the time of this writing there's no way to do this in the Slack API
        ;; itself so we need to do it after the fact.
@@ -223,7 +233,7 @@
   "Clear the Slack channels cache, and reset its last-updated timestamp to its default value (the Unix epoch)."
   []
   (slack-channels-and-usernames-last-updated! zoned-time-epoch)
-  (slack-cached-channels-and-usernames! []))
+  (slack-cached-channels-and-usernames! {:channels []}))
 
 (defn refresh-channels-and-usernames!
   "Refreshes users and conversations in slack-cache. finds both in parallel, sets
@@ -233,7 +243,7 @@
     (log/info "Refreshing slack channels and usernames.")
     (let [users (future (vec (users-list)))
           conversations (future (vec (conversations-list)))]
-      (slack-cached-channels-and-usernames! (concat @conversations @users))
+      (slack-cached-channels-and-usernames! {:channels (concat @conversations @users)})
       (slack-channels-and-usernames-last-updated! (t/zoned-date-time)))))
 
 (defn refresh-channels-and-usernames-when-needed!
@@ -272,6 +282,18 @@
   [channel-id :- su/NonBlankString]
   (POST "conversations.join" {:form-params {:channel channel-id}}))
 
+(defn- maybe-lookup-id
+  "Slack requires the slack app to be in the channel that we post all of our attachments to. Slack changed (around June
+  2022 #23229) the \"conversations.join\" api to require the internal slack id rather than the common name. This makes
+  a lot of sense to ensure we continue to operate despite channel renames. Attempt to look up the channel-id in the
+  list of channels to obtain the internal id. Fallback to using the current channel-id."
+  [channel-id cached-channels]
+  (let [name->id    (into {} (comp (filter (comp #{"channel"} :type))
+                                   (map (juxt :name :id)))
+                          (:channels cached-channels))
+        channel-id' (get name->id channel-id channel-id)]
+    channel-id'))
+
 (s/defn upload-file!
   "Calls Slack API `files.upload` endpoint and returns the URL of the uploaded file."
   [file :- NonEmptyByteArray, filename :- su/NonBlankString, channel-id :- su/NonBlankString]
@@ -285,7 +307,9 @@
                      ;; If file upload fails with a "not_in_channel" error, we join the channel and try again.
                      ;; This is expected to happen the first time a Slack subscription is sent.
                      (if (= "not_in_channel" (:error-code (ex-data e)))
-                       (do (join-channel! channel-id)
+                       (do (-> channel-id
+                               (maybe-lookup-id (slack-cached-channels-and-usernames))
+                               join-channel!)
                            (POST "files.upload" request))
                        (throw e))))]
     (u/prog1 (get-in response [:file :url_private])

src/metabase/task/refresh_slack_channel_user_cache.clj

@@ -14,7 +14,7 @@
           start-ms (System/currentTimeMillis)
           _        (slack/refresh-channels-and-usernames!)]
       (log/info (trs "Slack user/channel startup cache refreshed with {0} entries, took {1}ms."
-                     (count (slack/slack-cached-channels-and-usernames))
+                     (count (:channels (slack/slack-cached-channels-and-usernames)))
                      (- (System/currentTimeMillis) start-ms))))
     (log/info (trs "Slack is not configured, not refreshing slack user/channel cache."))))
 

test/metabase/api/pulse_test.clj

@@ -972,17 +972,28 @@
 (deftest form-input-test
   (testing "GET /api/pulse/form_input"
     (testing "Check that Slack channels come back when configured"
-      (mt/with-temporary-setting-values [slack-token nil
-                                         slack-app-token "something"]
-        (with-redefs [slack/conversations-list (constantly ["#foo" "#two" "#general"])
-                      slack/users-list         (constantly ["@bar" "@baz"])]
-          ;; set the cache to these values
-          (slack/slack-cached-channels-and-usernames! (concat (slack/conversations-list) (slack/users-list)))
-          ;; don't let the cache refresh itself (it will refetch if it is too old)
-          (slack/slack-channels-and-usernames-last-updated! (t/zoned-date-time))
-          (is (= [{:name "channel", :type "select", :displayName "Post to", :options ["#foo" "#two" "#general" "@bar" "@baz"], :required true}]
-                 (-> (mt/user-http-request :rasta :get 200 "pulse/form_input")
-                     (get-in [:channels :slack :fields])))))))
+      (mt/with-temporary-setting-values [slack/slack-channels-and-usernames-last-updated
+                                         (t/zoned-date-time)
+
+                                         slack/slack-app-token "test-token"
+
+                                         slack/slack-cached-channels-and-usernames
+                                         {:channels [{:type "channel"
+                                                      :name "foo"
+                                                      :display-name "#foo"
+                                                      :id "CAAS3DD9XND"}
+                                                     {:type "channel"
+                                                      :name "general"
+                                                      :display-name "#general"
+                                                      :id "C3MJRZ9EUVA"}
+                                                     {:type "user"
+                                                      :name "user1"
+                                                      :id "U1DYU9W3WZ2"
+                                                      :display-name "@user1"}]}]
+        (is (= [{:name "channel", :type "select", :displayName "Post to",
+                 :options ["#foo" "#general" "@user1"], :required true}]
+               (-> (mt/user-http-request :rasta :get 200 "pulse/form_input")
+                   (get-in [:channels :slack :fields]))))))
 
     (testing "When slack is not configured, `form_input` returns no channels"
       (mt/with-temporary-setting-values [slack-token nil

test/metabase/api/slack_test.clj

@@ -10,9 +10,9 @@
   (testing "PUT /api/slack/settings"
     (testing "An admin can set a valid Slack app token to the slack-app-token setting, and any value in the
              `slack-token` setting is cleared"
-      (with-redefs [slack/valid-token? (constantly true)
-                    slack/channel-exists? (constantly true)
-                    slack/refresh-channels-and-usernames! (constantly nil)
+      (with-redefs [slack/valid-token?                                (constantly true)
+                    slack/channel-exists?                             (constantly true)
+                    slack/refresh-channels-and-usernames!             (constantly nil)
                     slack/refresh-channels-and-usernames-when-needed! (constantly nil)]
         (mt/with-temporary-setting-values [slack-app-token nil
                                            slack-token     "fake-token"]
@@ -22,16 +22,17 @@
 
     (testing "A 400 error is returned if the Slack app token is invalid"
       (mt/with-temporary-setting-values [slack-app-token nil]
-        (with-redefs [slack/valid-token? (constantly false)
+        (with-redefs [slack/valid-token?                                (constantly false)
                       ;; Token validation is skipped by default in test environments; overriding `is-test?` ensures
                       ;; that validation occurs
-                      config/is-test?    false
-                      slack/refresh-channels-and-usernames! (constantly nil)
+                      config/is-test?                                   false
+                      slack/refresh-channels-and-usernames!             (constantly nil)
                       slack/refresh-channels-and-usernames-when-needed! (constantly nil)]
           (let [response (mt/user-http-request :crowberto :put 400 "slack/settings" {:slack-app-token "fake-token"})]
             (is (= {:slack-app-token "invalid token"} (:errors response)))
             (is (= nil (slack/slack-app-token)))
-            (is (= [] (slack/slack-cached-channels-and-usernames)))))))
+            (is (= {:channels []}
+                   (slack/slack-cached-channels-and-usernames)))))))
 
     (testing "The Slack files channel setting can be set by an admin, and the leading # is stripped if it is present"
       (mt/with-temporary-setting-values [slack-files-channel                       nil
@@ -59,7 +60,8 @@
         ;; The files channel is reset to its default value
         (is (= "metabase_files" (slack/slack-files-channel)))
         ;; The cache is empty, and its last-updated value is reset to its default value
-        (is (= [] (slack/slack-cached-channels-and-usernames)))
+        (is (= {:channels []}
+               (slack/slack-cached-channels-and-usernames)))
         (is (= @#'slack/zoned-time-epoch (slack/slack-channels-and-usernames-last-updated)))))
 
 

test/metabase/integrations/slack_test.clj

 (ns metabase.integrations.slack-test
   (:require [cheshire.core :as json]
             [clj-http.fake :as http-fake]
-            [clojure.java.io :as io]
             [clojure.test :refer :all]
             [medley.core :as m]
             [metabase.email.messages :as messages]
@@ -10,7 +9,6 @@
             [metabase.test.util :as tu]
             [schema.core :as s])
   (:import java.nio.charset.Charset
-           org.apache.commons.io.IOUtils
            org.apache.http.client.utils.URLEncodedUtils
            org.apache.http.NameValuePair))
 
@@ -83,9 +81,8 @@
 
     (testing "should be able to fetch channels and paginate"
       (http-fake/with-fake-routes {conversations-endpoint (comp mock-200-response mock-conversations-response-body)}
-        (let [expected-result (map
-                               (comp #(str \# %) :name)
-                               (concat (mock-conversations) (mock-conversations)))]
+        (let [expected-result (map slack/channel-transform
+                                   (concat (mock-conversations) (mock-conversations)))]
           (tu/with-temporary-setting-values [slack-token "test-token"
                                              slack-app-token nil]
             (is (= expected-result
@@ -136,9 +133,8 @@
 
     (testing "should be able to fetch list of users and page"
       (http-fake/with-fake-routes {users-endpoint (comp mock-200-response mock-users-response-body)}
-        (let [expected-result (map
-                               (comp #(str \@ %) :name)
-                               (concat (mock-users) (mock-users)))]
+        (let [expected-result (map slack/user-transform
+                                   (concat (mock-users) (mock-users)))]
           (tu/with-temporary-setting-values [slack-token     nil
                                              slack-app-token "test-token"]
             (is (= expected-result
@@ -152,10 +148,14 @@
   (testing "files-channel"
     (testing "Should be able to get the files-channel from the cache (if it exists)"
       (tu/with-temporary-setting-values [slack-files-channel "general"
-                                         slack-cached-channels-and-usernames ["#general" "#random" "#off-topic" "#cooking" "@john" "@james" "@jordan"]]
+                                         slack-cached-channels-and-usernames
+                                         {:channels (mapv (fn [c] {:name c :id c})
+                                                          ["general" "random" "off-topic"
+                                                           "cooking" "john" "james" "jordan"])}]
         (is (= "general" (slack/files-channel))))
       (tu/with-temporary-setting-values [slack-files-channel "not_in_the_cache"
-                                         slack-cached-channels-and-usernames ["#general"]]
+                                         slack-cached-channels-and-usernames
+                                         {:channels [{:name "general" :id "C0G9QKBBL"}]}]
         (is (thrown-with-msg?
              clojure.lang.ExceptionInfo
              #"Slack channel named.*is missing.*"
@@ -163,8 +163,7 @@
 
 (deftest upload-file!-test
   (testing "upload-file!"
-    (let [image-bytes (with-open [is (io/input-stream (io/resource "frontend_client/favicon.ico"))]
-                        (IOUtils/toByteArray is))
+    (let [image-bytes (.getBytes "fake-picture")
           filename    "wow.gif"
           channel-id  "C13372B6X"]
       (http-fake/with-fake-routes {#"^https://slack.com/api/files\.upload.*"
@@ -181,7 +180,65 @@
         (tu/with-temporary-setting-values [slack-token nil
                                            slack-app-token "test-token"]
           (is (= "https://files.slack.com/files-pri/T078VLEET-F017C3TSBK6/wow.gif"
-                 (slack/upload-file! image-bytes filename channel-id))))))))
+                 (slack/upload-file! image-bytes filename channel-id)))))))
+  (testing (str "upload-file! will attempt to join channels by internal slack id"
+                " but we can continue to use the channel name for posting")
+    (let [filename    "wow.gif"
+          channel-id  "metabase_files"
+          slack-id    "CQXPZKNQ3RK"
+          joined?     (atom false)
+          channel-info [{:display-name "#random",
+                         :name "random",
+                         :id "CT2FNGZSRPL",
+                         :type "channel"}
+                        {:display-name "#general",
+                         :name "general",
+                         :id "C4Q6LXLRA46",
+                         :type "channel"}
+                        {:display-name "#metabase_files",
+                         :name channel-id,
+                         ;; must look up "metabase_files" and find the id below
+                         :id slack-id,
+                         :type "channel"}]]
+      (tu/with-temporary-setting-values [slack/slack-app-token "slack-configured?"
+                                         slack/slack-cached-channels-and-usernames
+                                         {:channels channel-info}]
+        (with-redefs [slack/POST (fn [endpoint payload]
+                                   (case endpoint
+                                     "files.upload"
+                                     (if @joined?
+                                       {:file {:url_private filename}}
+                                       (throw (ex-info "Not in that channel"
+                                                       {:error-code "not_in_channel"})))
+                                     "conversations.join"
+                                     (reset! joined? (= (-> payload :form-params :channel)
+                                                        slack-id))))]
+          (slack/upload-file! (.getBytes "fake-picture") filename channel-id)
+          (is @joined? (str "Did not attempt to join with slack-id " slack-id)))))))
+
+(deftest maybe-lookup-id-test
+  (let [f (var-get #'slack/maybe-lookup-id)]
+    (testing "On new v2 shape"
+      (testing "Returns original if not found"
+        (is (= "needle"
+               (f "needle" {:channels [{:display-name "#other1"
+                                        :name         "other1"
+                                        :type         "channel"
+                                        :id           "CR65C4ZJVIW"}
+                                       {:display-name "#other2"
+                                        :name         "other2"
+                                        :type         "channel"
+                                        :id           "C87LQNL0Y23"}]}))))
+      (testing "Returns the slack internal id if found"
+        (is (= "slack-id"
+               (f "needle" {:channels [{:display-name "#other1"
+                                        :name         "other1"
+                                        :type         "channel"
+                                        :id           "CR65C4ZJVIW"}
+                                       {:display-name "#needle"
+                                        :name         "needle"
+                                        :type         "channel"
+                                        :id           "slack-id"}]})))))))
 
 (deftest post-chat-message!-test
   (testing "post-chat-message!"