From 97e2b353f6b92039d7d30e46930fabd0e41ac8a0 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Mon, 15 Jul 2024 13:07:36 +1000
Subject: [PATCH] FEATURE: Allow for multiple GitHub onebox tokens (#27887)

Followup 560e8aff75e4bde67bb162e8fdea52e704a19f81

GitHub auth tokens cannot be made with permissions to
access multiple organisations. This is quite limiting.
This commit changes the site setting to be a "secret list"
type, which allows for a key/value mapping where the value
is treated like a password in the UI.

Now when a GitHub URL is requested for oneboxing, the
org name from the URL is used to determine which token
to use for the request.

Just in case anyone used the old site setting already,
there is a migration to create a `default` entry
with that token in the new list setting, and for
a period of time we will consider that token valid to
use for all GitHub oneboxes as well.
---
 .../admin/addon/components/site-setting.hbs   | 18 ++++++++++---
 config/locales/server.en.yml                  |  2 +-
 config/site_settings.yml                      |  4 ++-
 ...40712050324_default_github_onebox_token.rb | 25 +++++++++++++++++++
 lib/onebox/engine/github_actions_onebox.rb    |  2 +-
 lib/onebox/engine/github_blob_onebox.rb       | 10 ++++----
 lib/onebox/engine/github_commit_onebox.rb     |  9 +++----
 lib/onebox/engine/github_issue_onebox.rb      |  2 +-
 .../engine/github_pull_request_onebox.rb      | 12 ++++-----
 lib/onebox/engine/gitlab_blob_onebox.rb       |  2 +-
 lib/onebox/mixins/git_blob_onebox.rb          |  2 +-
 lib/onebox/mixins/github_auth_header.rb       | 15 ++++++++---
 lib/oneboxer.rb                               |  2 +-
 .../engine/github_actions_onebox_spec.rb      |  8 +++---
 .../onebox/engine/github_blob_onebox_spec.rb  |  4 +--
 .../engine/github_commit_onebox_spec.rb       |  8 +++---
 .../onebox/engine/github_issue_onebox_spec.rb |  4 +--
 .../engine/github_pull_request_onebox_spec.rb |  4 +--
 18 files changed, 90 insertions(+), 43 deletions(-)
 create mode 100644 db/migrate/20240712050324_default_github_onebox_token.rb

diff --git a/app/assets/javascripts/admin/addon/components/site-setting.hbs b/app/assets/javascripts/admin/addon/components/site-setting.hbs
index 3b9b663c64a..bb8228fd44d 100644
--- a/app/assets/javascripts/admin/addon/components/site-setting.hbs
+++ b/app/assets/javascripts/admin/addon/components/site-setting.hbs
@@ -50,12 +50,24 @@
 
 {{#if this.dirty}}
   <div class="setting-controls">
-    <DButton class="ok" @action={{this.update}} @icon="check" />
-    <DButton class="cancel" @action={{this.cancel}} @icon="times" />
+    <DButton
+      @action={{this.update}}
+      @icon="check"
+      class="ok setting-controls__ok"
+    />
+    <DButton
+      @action={{this.cancel}}
+      @icon="times"
+      class="cancel setting-controls__cancel"
+    />
   </div>
 {{else if this.overridden}}
   {{#if this.setting.secret}}
-    <DButton @action={{this.toggleSecret}} @icon="far-eye-slash" />
+    <DButton
+      @action={{this.toggleSecret}}
+      @icon="far-eye-slash"
+      class="setting-toggle-secret"
+    />
   {{/if}}
 
   <DButton
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index b8ed26fc4d2..4d5031a8f45 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -1758,7 +1758,7 @@ en:
     force_custom_user_agent_hosts: "Hosts for which to use the custom onebox user agent on all requests. (Especially useful for hosts that limit access by user agent)."
     max_oneboxes_per_post: "Set the maximum number of oneboxes that can be included in a single post. Oneboxes provide a preview of linked content within the post."
     facebook_app_access_token: "A token generated from your Facebook app ID and secret. Used to generate Instagram oneboxes."
-    github_onebox_access_token: "A GitHub access token which is used to generate GitHub oneboxes for private repos, commits, pull requests, issues, and file contents. Without this, only public GitHub URLs will be oneboxed."
+    github_onebox_access_tokens: "A mapping of a GitHub organisation or user to a GitHub access token which is used to generate GitHub oneboxes for private repos, commits, pull requests, issues, and file contents. Without this, only public GitHub URLs will be oneboxed."
 
     logo: "The logo image at the top left of your site. Use a wide rectangular image with a height of 120 and an aspect ratio greater than 3:1. If left blank, the site title text will be shown."
     logo_small: "The small logo image at the top left of your site, seen when scrolling down. Use a square 120 × 120 image. If left blank, a home glyph will be shown."
diff --git a/config/site_settings.yml b/config/site_settings.yml
index d20d8d60ec1..5a7cf33840d 100644
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@@ -2125,9 +2125,11 @@ onebox:
   cache_onebox_user_agent:
     default: ""
     hidden: true
-  github_onebox_access_token:
+  github_onebox_access_tokens:
     default: ""
+    type: list
     secret: true
+    list_type: secret
 spam:
   add_rel_nofollow_to_user_content: true
   hide_post_sensitivity:
diff --git a/db/migrate/20240712050324_default_github_onebox_token.rb b/db/migrate/20240712050324_default_github_onebox_token.rb
new file mode 100644
index 00000000000..99169140617
--- /dev/null
+++ b/db/migrate/20240712050324_default_github_onebox_token.rb
@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+class DefaultGithubOneboxToken < ActiveRecord::Migration[7.1]
+  def up
+    existing_token =
+      DB.query_single(
+        "SELECT value FROM site_settings WHERE name = 'github_onebox_access_token'",
+      ).first
+
+    # 8 is the data type for a list
+    execute <<~SQL if existing_token.present?
+      INSERT INTO site_settings (name, data_type, value, created_at, updated_at)
+      VALUES ('github_onebox_access_tokens', 8, 'default|#{existing_token}', CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)
+      ON CONFLICT DO NOTHING
+    SQL
+
+    execute <<~SQL
+      DELETE FROM site_settings WHERE name = 'github_onebox_access_token'
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end
diff --git a/lib/onebox/engine/github_actions_onebox.rb b/lib/onebox/engine/github_actions_onebox.rb
index f2494151c9e..20f25a1ddcf 100644
--- a/lib/onebox/engine/github_actions_onebox.rb
+++ b/lib/onebox/engine/github_actions_onebox.rb
@@ -65,7 +65,7 @@ module Onebox
       end
 
       def data
-        result = raw(github_auth_header).clone
+        result = raw(github_auth_header(match[:org])).clone
 
         status = "unknown"
         if result["status"] == "completed"
diff --git a/lib/onebox/engine/github_blob_onebox.rb b/lib/onebox/engine/github_blob_onebox.rb
index 47c240affbd..b396f1998d9 100644
--- a/lib/onebox/engine/github_blob_onebox.rb
+++ b/lib/onebox/engine/github_blob_onebox.rb
@@ -28,19 +28,19 @@ module Onebox
       end
 
       def raw_regexp
-        %r{github\.com/(?<user>[^/]+)/(?<repo>[^/]+)/blob/(?<sha1>[^/]+)/(?<file>[^#]+)(#(L(?<from>[^-]*)(-L(?<to>.*))?))?}mi
+        %r{github\.com/(?<org>[^/]+)/(?<repo>[^/]+)/blob/(?<sha1>[^/]+)/(?<file>[^#]+)(#(L(?<from>[^-]*)(-L(?<to>.*))?))?}mi
       end
 
-      def raw_template(m)
-        "https://raw.githubusercontent.com/#{m[:user]}/#{m[:repo]}/#{m[:sha1]}/#{m[:file]}"
+      def raw_template(match)
+        "https://raw.githubusercontent.com/#{match[:org]}/#{match[:repo]}/#{match[:sha1]}/#{match[:file]}"
       end
 
       def title
         Sanitize.fragment(Onebox::Helpers.uri_unencode(link).sub(%r{^https?\://github\.com/}, ""))
       end
 
-      def auth_headers
-        github_auth_header
+      def auth_headers(match)
+        github_auth_header(match[:org])
       end
     end
   end
diff --git a/lib/onebox/engine/github_commit_onebox.rb b/lib/onebox/engine/github_commit_onebox.rb
index 0954617f4fd..ba073071218 100644
--- a/lib/onebox/engine/github_commit_onebox.rb
+++ b/lib/onebox/engine/github_commit_onebox.rb
@@ -16,7 +16,7 @@ module Onebox
       always_https
 
       def url
-        "https://api.github.com/repos/#{match[:owner]}/#{match[:repository]}/commits/#{match[:sha]}"
+        "https://api.github.com/repos/#{match[:org]}/#{match[:repository]}/commits/#{match[:sha]}"
       end
 
       private
@@ -24,18 +24,17 @@ module Onebox
       def match
         return @match if defined?(@match)
 
-        @match =
-          @url.match(%{github\.com/(?<owner>[^/]+)/(?<repository>[^/]+)/commit/(?<sha>[^/]+)})
+        @match = @url.match(%{github\.com/(?<org>[^/]+)/(?<repository>[^/]+)/commit/(?<sha>[^/]+)})
         @match ||=
           @url.match(
-            %{github\.com/(?<owner>[^/]+)/(?<repository>[^/]+)/pull/(?<pr>[^/]+)/commit/(?<sha>[^/]+)},
+            %{github\.com/(?<org>[^/]+)/(?<repository>[^/]+)/pull/(?<pr>[^/]+)/commit/(?<sha>[^/]+)},
           )
 
         @match
       end
 
       def data
-        result = raw(github_auth_header).clone
+        result = raw(github_auth_header(match[:org])).clone
 
         lines = result["commit"]["message"].split("\n")
         result["title"] = lines.first
diff --git a/lib/onebox/engine/github_issue_onebox.rb b/lib/onebox/engine/github_issue_onebox.rb
index 9f57a3e214d..5212208acc1 100644
--- a/lib/onebox/engine/github_issue_onebox.rb
+++ b/lib/onebox/engine/github_issue_onebox.rb
@@ -37,7 +37,7 @@ module Onebox
       end
 
       def data
-        result = raw(github_auth_header).clone
+        result = raw(github_auth_header(match[:org])).clone
         created_at = Time.parse(result["created_at"])
         closed_at = Time.parse(result["closed_at"]) if result["closed_at"]
         body, excerpt = compute_body(result["body"])
diff --git a/lib/onebox/engine/github_pull_request_onebox.rb b/lib/onebox/engine/github_pull_request_onebox.rb
index e86d0fe1818..91ba259cf16 100644
--- a/lib/onebox/engine/github_pull_request_onebox.rb
+++ b/lib/onebox/engine/github_pull_request_onebox.rb
@@ -18,18 +18,18 @@ module Onebox
       always_https
 
       def url
-        "https://api.github.com/repos/#{match[:owner]}/#{match[:repository]}/pulls/#{match[:number]}"
+        "https://api.github.com/repos/#{match[:org]}/#{match[:repository]}/pulls/#{match[:number]}"
       end
 
       private
 
       def match
         @match ||=
-          @url.match(%r{github\.com/(?<owner>[^/]+)/(?<repository>[^/]+)/pull/(?<number>[^/]+)})
+          @url.match(%r{github\.com/(?<org>[^/]+)/(?<repository>[^/]+)/pull/(?<number>[^/]+)})
       end
 
       def data
-        result = raw(github_auth_header).clone
+        result = raw(github_auth_header(match[:org])).clone
         result["link"] = link
 
         created_at = Time.parse(result["created_at"])
@@ -78,7 +78,7 @@ module Onebox
       def load_commit(link)
         if commit_match = link.match(%r{commits/(\h+)})
           load_json(
-            "https://api.github.com/repos/#{match[:owner]}/#{match[:repository]}/commits/#{commit_match[1]}",
+            "https://api.github.com/repos/#{match[:org]}/#{match[:repository]}/commits/#{commit_match[1]}",
           )
         end
       end
@@ -86,7 +86,7 @@ module Onebox
       def load_comment(link)
         if comment_match = link.match(/#issuecomment-(\d+)/)
           load_json(
-            "https://api.github.com/repos/#{match[:owner]}/#{match[:repository]}/issues/comments/#{comment_match[1]}",
+            "https://api.github.com/repos/#{match[:org]}/#{match[:repository]}/issues/comments/#{comment_match[1]}",
           )
         end
       end
@@ -94,7 +94,7 @@ module Onebox
       def load_review(link)
         if review_match = link.match(/#discussion_r(\d+)/)
           load_json(
-            "https://api.github.com/repos/#{match[:owner]}/#{match[:repository]}/pulls/comments/#{review_match[1]}",
+            "https://api.github.com/repos/#{match[:org]}/#{match[:repository]}/pulls/comments/#{review_match[1]}",
           )
         end
       end
diff --git a/lib/onebox/engine/gitlab_blob_onebox.rb b/lib/onebox/engine/gitlab_blob_onebox.rb
index 56071692c14..51cf05858e3 100644
--- a/lib/onebox/engine/gitlab_blob_onebox.rb
+++ b/lib/onebox/engine/gitlab_blob_onebox.rb
@@ -34,7 +34,7 @@ module Onebox
         Sanitize.fragment(Onebox::Helpers.uri_unencode(link).sub(%r{^https?\://gitlab\.com/}, ""))
       end
 
-      def auth_headers
+      def auth_headers(_match)
         {}
       end
     end
diff --git a/lib/onebox/mixins/git_blob_onebox.rb b/lib/onebox/mixins/git_blob_onebox.rb
index 229d35581f0..a6685bcf9cc 100644
--- a/lib/onebox/mixins/git_blob_onebox.rb
+++ b/lib/onebox/mixins/git_blob_onebox.rb
@@ -173,7 +173,7 @@ module Onebox
               contents =
                 URI
                   .parse(self.raw_template(m))
-                  .open({ read_timeout: timeout }.merge(self.auth_headers))
+                  .open({ read_timeout: timeout }.merge(self.auth_headers(m)))
                   .read
 
               if contents.encoding == Encoding::BINARY || contents.bytes.include?(0)
diff --git a/lib/onebox/mixins/github_auth_header.rb b/lib/onebox/mixins/github_auth_header.rb
index 1f6f37964ab..c96872f20e4 100644
--- a/lib/onebox/mixins/github_auth_header.rb
+++ b/lib/onebox/mixins/github_auth_header.rb
@@ -3,9 +3,18 @@
 module Onebox
   module Mixins
     module GithubAuthHeader
-      def github_auth_header
-        return {} if SiteSetting.github_onebox_access_token.blank?
-        { "Authorization" => "Bearer #{SiteSetting.github_onebox_access_token}" }
+      def github_auth_header(github_org)
+        return {} if SiteSetting.github_onebox_access_tokens.blank?
+        org_tokens =
+          SiteSetting.github_onebox_access_tokens.split("\n").map { |line| line.split("|") }.to_h
+
+        # Use the default token if no token is found for the org,
+        # this will be the token that used to be stored in the old
+        # github_onebox_access_token site setting if it was configured.
+        token = org_tokens[github_org] || org_tokens["default"]
+        return {} if token.blank?
+
+        { "Authorization" => "Bearer #{token}" }
       end
     end
   end
diff --git a/lib/oneboxer.rb b/lib/oneboxer.rb
index 6482437c03b..6270c8924a0 100644
--- a/lib/oneboxer.rb
+++ b/lib/oneboxer.rb
@@ -685,7 +685,7 @@ module Oneboxer
     # FinalDestination to request the final URL because no auth headers
     # are sent. In this case we can ignore redirects and go straight to
     # using Onebox.preview
-    if SiteSetting.github_onebox_access_token.present? && uri.hostname == "github.com"
+    if SiteSetting.github_onebox_access_tokens.present? && uri.hostname == "github.com"
       fd_options[:ignore_redirects] << "https://github.com"
     end
 
diff --git a/spec/lib/onebox/engine/github_actions_onebox_spec.rb b/spec/lib/onebox/engine/github_actions_onebox_spec.rb
index f1e655b735c..f3997a525eb 100644
--- a/spec/lib/onebox/engine/github_actions_onebox_spec.rb
+++ b/spec/lib/onebox/engine/github_actions_onebox_spec.rb
@@ -35,13 +35,13 @@ RSpec.describe Onebox::Engine::GithubActionsOnebox do
     end
 
     context "when github_onebox_access_token is configured" do
-      before { SiteSetting.github_onebox_access_token = "1234" }
+      before { SiteSetting.github_onebox_access_tokens = "discourse|github_pat_1234" }
 
       it "sends it as part of the request" do
         html
         expect(WebMock).to have_requested(:get, run_uri).with(
           headers: {
-            "Authorization" => "Bearer #{SiteSetting.github_onebox_access_token}",
+            "Authorization" => "Bearer github_pat_1234",
           },
         )
       end
@@ -78,13 +78,13 @@ RSpec.describe Onebox::Engine::GithubActionsOnebox do
     end
 
     context "when github_onebox_access_token is configured" do
-      before { SiteSetting.github_onebox_access_token = "1234" }
+      before { SiteSetting.github_onebox_access_tokens = "discourse|github_pat_1234" }
 
       it "sends it as part of the request" do
         html
         expect(WebMock).to have_requested(:get, pr_run_uri).with(
           headers: {
-            "Authorization" => "Bearer #{SiteSetting.github_onebox_access_token}",
+            "Authorization" => "Bearer github_pat_1234",
           },
         )
       end
diff --git a/spec/lib/onebox/engine/github_blob_onebox_spec.rb b/spec/lib/onebox/engine/github_blob_onebox_spec.rb
index 473798a28f2..03826367ec0 100644
--- a/spec/lib/onebox/engine/github_blob_onebox_spec.rb
+++ b/spec/lib/onebox/engine/github_blob_onebox_spec.rb
@@ -46,13 +46,13 @@ RSpec.describe Onebox::Engine::GithubBlobOnebox do
     end
 
     context "when github_onebox_access_token is configured" do
-      before { SiteSetting.github_onebox_access_token = "1234" }
+      before { SiteSetting.github_onebox_access_tokens = "discourse|github_pat_1234" }
 
       it "sends it as part of the request" do
         html
         expect(WebMock).to have_requested(:get, raw_uri).with(
           headers: {
-            "Authorization" => "Bearer #{SiteSetting.github_onebox_access_token}",
+            "Authorization" => "Bearer github_pat_1234",
           },
         )
       end
diff --git a/spec/lib/onebox/engine/github_commit_onebox_spec.rb b/spec/lib/onebox/engine/github_commit_onebox_spec.rb
index b1082d80719..c41b51c044a 100644
--- a/spec/lib/onebox/engine/github_commit_onebox_spec.rb
+++ b/spec/lib/onebox/engine/github_commit_onebox_spec.rb
@@ -55,14 +55,14 @@ RSpec.describe Onebox::Engine::GithubCommitOnebox do
     end
 
     context "when github_onebox_access_token is configured" do
-      before { SiteSetting.github_onebox_access_token = "1234" }
+      before { SiteSetting.github_onebox_access_tokens = "discourse|github_pat_1234" }
 
       it "sends it as part of the request" do
         html
         expect(WebMock).to have_requested(
           :get,
           "https://api.github.com/repos/discourse/discourse/commits/803d023e2307309f8b776ab3b8b7e38ba91c0919",
-        ).with(headers: { "Authorization" => "Bearer #{SiteSetting.github_onebox_access_token}" })
+        ).with(headers: { "Authorization" => "Bearer github_pat_1234" })
       end
     end
   end
@@ -122,14 +122,14 @@ RSpec.describe Onebox::Engine::GithubCommitOnebox do
     end
 
     context "when github_onebox_access_token is configured" do
-      before { SiteSetting.github_onebox_access_token = "1234" }
+      before { SiteSetting.github_onebox_access_tokens = "discourse|github_pat_1234" }
 
       it "sends it as part of the request" do
         html
         expect(WebMock).to have_requested(
           :get,
           "https://api.github.com/repos/discourse/discourse/commits/803d023e2307309f8b776ab3b8b7e38ba91c0919",
-        ).with(headers: { "Authorization" => "Bearer #{SiteSetting.github_onebox_access_token}" })
+        ).with(headers: { "Authorization" => "Bearer github_pat_1234" })
       end
     end
   end
diff --git a/spec/lib/onebox/engine/github_issue_onebox_spec.rb b/spec/lib/onebox/engine/github_issue_onebox_spec.rb
index 4801889ff47..298df414a20 100644
--- a/spec/lib/onebox/engine/github_issue_onebox_spec.rb
+++ b/spec/lib/onebox/engine/github_issue_onebox_spec.rb
@@ -24,13 +24,13 @@ RSpec.describe Onebox::Engine::GithubIssueOnebox do
     end
 
     context "when github_onebox_access_token is configured" do
-      before { SiteSetting.github_onebox_access_token = "1234" }
+      before { SiteSetting.github_onebox_access_tokens = "discourse|github_pat_1234" }
 
       it "sends it as part of the request" do
         html
         expect(WebMock).to have_requested(:get, issue_uri).with(
           headers: {
-            "Authorization" => "Bearer #{SiteSetting.github_onebox_access_token}",
+            "Authorization" => "Bearer github_pat_1234",
           },
         )
       end
diff --git a/spec/lib/onebox/engine/github_pull_request_onebox_spec.rb b/spec/lib/onebox/engine/github_pull_request_onebox_spec.rb
index c787d75db43..7d366a677c8 100644
--- a/spec/lib/onebox/engine/github_pull_request_onebox_spec.rb
+++ b/spec/lib/onebox/engine/github_pull_request_onebox_spec.rb
@@ -92,13 +92,13 @@ RSpec.describe Onebox::Engine::GithubPullRequestOnebox do
   end
 
   context "when github_onebox_access_token is configured" do
-    before { SiteSetting.github_onebox_access_token = "1234" }
+    before { SiteSetting.github_onebox_access_tokens = "discourse|github_pat_1234" }
 
     it "sends it as part of the request" do
       html
       expect(WebMock).to have_requested(:get, api_uri).with(
         headers: {
-          "Authorization" => "Bearer #{SiteSetting.github_onebox_access_token}",
+          "Authorization" => "Bearer github_pat_1234",
         },
       )
     end