Add dedicated user_api_key_clients table to allow for 1:many use cases (#28119)

app/controllers/application_controller.rb

@@ -868,7 +868,7 @@ class ApplicationController < ActionController::Base
         return render plain: I18n.t("user_api_key.invalid_public_key")
       end
 
-      if UserApiKey.invalid_auth_redirect?(params[:auth_redirect])
+      if UserApiKeyClient.invalid_auth_redirect?(params[:auth_redirect])
         return render plain: I18n.t("user_api_key.invalid_auth_redirect")
       end
 

app/controllers/user_api_key_clients_controller.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+class UserApiKeyClientsController < ApplicationController
+  layout "no_ember"
+
+  requires_login
+  skip_before_action :check_xhr, :preload_json
+
+  def register
+    require_params
+
+    client = UserApiKeyClient.find_or_initialize_by(client_id: params[:client_id])
+    client.application_name = params[:application_name]
+    client.public_key = params[:public_key]
+    client.auth_redirect = params[:auth_redirect]
+
+    if client.save!
+      render json: success_json
+    else
+      render json: failed_json
+    end
+  end
+
+  def require_params
+    %i[client_id application_name public_key auth_redirect].each { |p| params.require(p) }
+    OpenSSL::PKey::RSA.new(params[:public_key])
+  end
+end

app/controllers/user_api_keys_controller.rb

@@ -17,6 +17,7 @@ class UserApiKeysController < ApplicationController
       return
     end
 
+    find_client
     require_params
     validate_params
 
@@ -36,8 +37,8 @@ class UserApiKeysController < ApplicationController
       return
     end
 
-    @application_name = params[:application_name]
+    @application_name = params[:application_name] || @client&.application_name
-    @public_key = params[:public_key]
+    @public_key = params[:public_key] || @client&.public_key
     @nonce = params[:nonce]
     @client_id = params[:client_id]
     @auth_redirect = params[:auth_redirect]
@@ -49,24 +50,30 @@ class UserApiKeysController < ApplicationController
   end
 
   def create
+    find_client
     require_params
 
     if params.key?(:auth_redirect)
-      raise Discourse::InvalidAccess if UserApiKey.invalid_auth_redirect?(params[:auth_redirect])
+      if UserApiKeyClient.invalid_auth_redirect?(params[:auth_redirect], client: @client)
+        raise Discourse::InvalidAccess
+      end
     end
 
     raise Discourse::InvalidAccess unless meets_tl?
 
     validate_params
-    @application_name = params[:application_name]
     scopes = params[:scopes].split(",")
 
-    UserApiKey.where(client_id: params[:client_id]).destroy_all
+    @client = UserApiKeyClient.new(client_id: params[:client_id]) if @client.blank?
+    @client.application_name = params[:application_name] if params[:application_name].present?
+    @client.public_key = params[:public_key] if params[:public_key].present?
+    @client.save! if @client.new_record? || @client.changed?
+
+    # destroy any old keys the user had with the client
+    @client.keys.where(user_id: current_user.id).destroy_all
 
     key =
-      UserApiKey.create!(
+      @client.keys.create!(
-        application_name: @application_name,
-        client_id: params[:client_id],
         user_id: current_user.id,
         push_url: params[:push_url],
         scopes: scopes.map { |name| UserApiKeyScope.new(name: name) },
@@ -81,7 +88,7 @@ class UserApiKeysController < ApplicationController
       api: AUTH_API_VERSION,
     }.to_json
 
-    public_key = OpenSSL::PKey::RSA.new(params[:public_key])
+    public_key = OpenSSL::PKey::RSA.new(@client.public_key)
     @payload = Base64.encode64(public_key.public_encrypt(@payload))
 
     if scopes.include?("one_time_password")
@@ -102,7 +109,8 @@ class UserApiKeysController < ApplicationController
       respond_to do |format|
         format.html { render :show }
         format.json do
-          instructions = I18n.t("user_api_key.instructions", application_name: @application_name)
+          instructions =
+            I18n.t("user_api_key.instructions", application_name: @client.application_name)
           render json: { payload: @payload, instructions: instructions }
         end
       end
@@ -131,7 +139,9 @@ class UserApiKeysController < ApplicationController
   def create_otp
     require_params_otp
 
-    raise Discourse::InvalidAccess if UserApiKey.invalid_auth_redirect?(params[:auth_redirect])
+    if UserApiKeyClient.invalid_auth_redirect?(params[:auth_redirect])
+      raise Discourse::InvalidAccess
+    end
     raise Discourse::InvalidAccess unless meets_tl?
 
     public_key = OpenSSL::PKey::RSA.new(params[:public_key])
@@ -167,8 +177,14 @@ class UserApiKeysController < ApplicationController
     key
   end
 
+  def find_client
+    @client = UserApiKeyClient.find_by(client_id: params[:client_id])
+  end
+
   def require_params
-    %i[public_key nonce scopes client_id application_name].each { |p| params.require(p) }
+    %i[nonce scopes client_id].each { |p| params.require(p) }
+    params.require(:public_key) if @client&.public_key.blank?
+    params.require(:application_name) if @client&.application_name.blank?
   end
 
   def validate_params
@@ -176,7 +192,7 @@ class UserApiKeysController < ApplicationController
     raise Discourse::InvalidAccess unless UserApiKey.allowed_scopes.superset?(requested_scopes)
 
     # our pk has got to parse
-    OpenSSL::PKey::RSA.new(params[:public_key])
+    OpenSSL::PKey::RSA.new(params[:public_key]) if params[:public_key]
   end
 
   def require_params_otp

app/models/user_api_key.rb

@@ -3,11 +3,14 @@
 class UserApiKey < ActiveRecord::Base
   self.ignored_columns = [
     "scopes", # TODO: Remove when 20240212034010_drop_deprecated_columns has been promoted to pre-deploy
+    "client_id", # TODO: Add post-migration to remove column after 3.4.0 stable release (not before early 2025)
+    "application_name", # TODO: Add post-migration to remove column after 3.4.0 stable release (not before early 2025)
   ]
 
   REVOKE_MATCHER = RouteMatcher.new(actions: "user_api_keys#revoke", methods: :post, params: [:id])
 
   belongs_to :user
+  belongs_to :client, class_name: "UserApiKeyClient", foreign_key: "user_api_key_client_id"
   has_many :scopes, class_name: "UserApiKeyScope", dependent: :destroy
 
   scope :active, -> { where(revoked_at: nil) }
@@ -39,14 +42,13 @@ class UserApiKey < ActiveRecord::Base
 
   def update_last_used(client_id)
     update_args = { last_used_at: Time.zone.now }
-    if client_id.present? && client_id != self.client_id
+    if client_id.present? && client_id != self.client.client_id
-      # invalidate old dupe api key for client if needed
+      new_client =
-      UserApiKey
+        UserApiKeyClient.create!(
-        .where(client_id: client_id, user_id: self.user_id)
+          client_id: client_id,
-        .where("id <> ?", self.id)
+          application_name: self.client.application_name,
-        .destroy_all
+        )
-
+      update_args[:user_api_key_client_id] = new_client.id
-      update_args[:client_id] = client_id
     end
     self.update_columns(**update_args)
   end
@@ -69,13 +71,6 @@ class UserApiKey < ActiveRecord::Base
     scopes.any? { |s| s.permits?(env) } || is_revoke_self_request?(env)
   end
 
-  def self.invalid_auth_redirect?(auth_redirect)
-    SiteSetting
-      .allowed_user_api_auth_redirects
-      .split("|")
-      .none? { |u| WildcardUrlChecker.check_url(u, auth_redirect) }
-  end
-
   private
 
   def revoke_self_matcher
@@ -91,20 +86,20 @@ end
 #
 # Table name: user_api_keys
 #
-#  id               :integer          not null, primary key
+#  id                     :integer          not null, primary key
-#  user_id          :integer          not null
+#  user_id                :integer          not null
-#  client_id        :string           not null
+#  push_url               :string
-#  application_name :string           not null
+#  created_at             :datetime         not null
-#  push_url         :string
+#  updated_at             :datetime         not null
-#  created_at       :datetime         not null
+#  revoked_at             :datetime
-#  updated_at       :datetime         not null
+#  last_used_at           :datetime         not null
-#  revoked_at       :datetime
+#  key_hash               :string           not null
-#  last_used_at     :datetime         not null
+#  user_api_key_client_id :bigint
-#  key_hash         :string           not null
 #
 # Indexes
 #
-#  index_user_api_keys_on_client_id  (client_id) UNIQUE
+#  index_user_api_keys_on_client_id               (client_id) UNIQUE
-#  index_user_api_keys_on_key_hash   (key_hash) UNIQUE
+#  index_user_api_keys_on_key_hash                (key_hash) UNIQUE
-#  index_user_api_keys_on_user_id    (user_id)
+#  index_user_api_keys_on_user_api_key_client_id  (user_api_key_client_id)
+#  index_user_api_keys_on_user_id                 (user_id)
 #

app/models/user_api_key_client.rb

@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+class UserApiKeyClient < ActiveRecord::Base
+  has_many :keys, class_name: "UserApiKey", dependent: :destroy
+
+  def self.invalid_auth_redirect?(auth_redirect, client: nil)
+    return false if client&.auth_redirect == auth_redirect
+    SiteSetting
+      .allowed_user_api_auth_redirects
+      .split("|")
+      .none? { |u| WildcardUrlChecker.check_url(u, auth_redirect) }
+  end
+end
+
+# == Schema Information
+#
+# Table name: user_api_key_clients
+#
+#  id               :bigint           not null, primary key
+#  client_id        :string           not null
+#  application_name :string           not null
+#  public_key       :string
+#  auth_redirect    :string
+#  created_at       :datetime         not null
+#  updated_at       :datetime         not null
+#
+# Indexes
+#
+#  index_user_api_key_clients_on_client_id  (client_id) UNIQUE
+#

app/serializers/user_serializer.rb

@@ -153,7 +153,7 @@ class UserSerializer < UserCardSerializer
         .map do |k|
           {
             id: k.id,
-            application_name: k.application_name,
+            application_name: k.client.application_name,
             scopes: k.scopes.map { |s| I18n.t("user_api_key.scopes.#{s.name}") },
             created_at: k.created_at,
             last_used_at: k.last_used_at,

app/services/post_alerter.rb

@@ -101,13 +101,13 @@ class PostAlerter
       clients =
         user
           .user_api_keys
-          .joins(:scopes)
+          .joins(:scopes, :client)
           .where("user_api_key_scopes.name IN ('push', 'notifications')")
           .where("push_url IS NOT NULL AND push_url <> ''")
           .where("position(push_url IN ?) > 0", SiteSetting.allowed_user_api_push_urls)
           .where("revoked_at IS NULL")
-          .order(client_id: :asc)
+          .order("user_api_key_clients.client_id ASC")
-          .pluck(:client_id, :push_url)
+          .pluck("user_api_key_clients.client_id, user_api_keys.push_url")
 
       return if clients.length == 0
 

config/routes.rb

@@ -1625,6 +1625,8 @@ Discourse::Application.routes.draw do
     get "/user-api-key/otp" => "user_api_keys#otp"
     post "/user-api-key/otp" => "user_api_keys#create_otp"
 
+    post "/user-api-key-client/register" => "user_api_key_clients#register"
+
     get "/safe-mode" => "safe_mode#index"
     post "/safe-mode" => "safe_mode#enter", :as => "safe_mode_enter"
 

db/migrate/20240729084803_create_user_api_key_clients.rb

@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+class CreateUserApiKeyClients < ActiveRecord::Migration[7.1]
+  def up
+    create_table :user_api_key_clients do |t|
+      t.string :client_id, null: false
+      t.string :application_name, null: false
+      t.string :public_key
+      t.string :auth_redirect
+
+      t.timestamps
+    end
+
+    add_index :user_api_key_clients, %i[client_id], unique: true
+
+    execute "INSERT INTO user_api_key_clients (client_id, application_name, created_at, updated_at)
+             SELECT client_id, application_name, created_at, updated_at
+             FROM user_api_keys"
+
+    add_column :user_api_keys, :user_api_key_client_id, :bigint, null: true
+    add_index :user_api_keys, :user_api_key_client_id
+
+    execute "UPDATE user_api_keys keys
+             SET user_api_key_client_id = clients.id
+             FROM user_api_key_clients clients
+             WHERE clients.client_id = keys.client_id"
+
+    change_column_null :user_api_keys, :client_id, true
+    change_column_null :user_api_keys, :application_name, true
+  end
+
+  def down
+    execute "UPDATE user_api_keys keys
+             SET client_id = clients.client_id,
+                 application_name = clients.application_name
+             FROM user_api_key_clients clients
+             WHERE clients.id = keys.user_api_key_client_id"
+
+    remove_column :user_api_keys, :user_api_key_client_id
+    change_column_null :user_api_keys, :client_id, false
+    change_column_null :user_api_keys, :application_name, false
+
+    remove_index :user_api_key_clients, :client_id
+    drop_table :user_api_key_clients
+  end
+end

lib/auth/default_current_user_provider.rb

@@ -187,7 +187,7 @@ class Auth::DefaultCurrentUserProvider
           .active
           .joins(:user)
           .where(key_hash: @hashed_user_api_key)
-          .includes(:user, :scopes)
+          .includes(:user, :scopes, :client)
           .first
 
       raise Discourse::InvalidAccess unless user_api_key_obj

spec/fabricators/user_api_key_fabricator.rb

@@ -2,12 +2,23 @@
 
 Fabricator(:user_api_key) do
   user
-  client_id { SecureRandom.hex }
+
-  application_name "some app"
+  after_create do |key, transients|
+    if key.client.blank?
+      client = Fabricate(:user_api_key_client)
+      key.user_api_key_client_id = client.id
+      key.save!
+    end
+  end
 end
 
 Fabricator(:user_api_key_scope)
 
+Fabricator(:user_api_key_client) do
+  client_id { SecureRandom.hex }
+  application_name "some app"
+end
+
 Fabricator(:readonly_user_api_key, from: :user_api_key) do
   scopes { [Fabricate.build(:user_api_key_scope, name: "read")] }
 end

spec/lib/auth/default_current_user_provider_spec.rb

@@ -602,32 +602,22 @@ RSpec.describe Auth::DefaultCurrentUserProvider do
     fab!(:user)
 
     let(:api_key) do
-      UserApiKey.create!(
+      Fabricate(
-        application_name: "my app",
+        :user_api_key,
-        client_id: "1234",
         scopes: ["read"].map { |name| UserApiKeyScope.new(name: name) },
-        user_id: user.id,
+        user: user,
       )
     end
 
-    it "can clear old duplicate keys correctly" do
+    it "creates a new client if the client id changes" do
-      dupe =
-        UserApiKey.create!(
-          application_name: "my app",
-          client_id: "12345",
-          scopes: ["read"].map { |name| UserApiKeyScope.new(name: name) },
-          user_id: user.id,
-        )
-
       params = {
         "REQUEST_METHOD" => "GET",
         "HTTP_USER_API_KEY" => api_key.key,
-        "HTTP_USER_API_CLIENT_ID" => dupe.client_id,
+        "HTTP_USER_API_CLIENT_ID" => api_key.client.client_id + "1",
       }
-
       good_provider = provider("/", params)
       expect(good_provider.current_user.id).to eq(user.id)
-      expect(UserApiKey.find_by(id: dupe.id)).to eq(nil)
+      expect(UserApiKeyClient.exists?(client_id: api_key.client.client_id + "1")).to eq(true)
     end
 
     it "allows user API access correctly" do

spec/requests/application_controller_spec.rb

@@ -1167,8 +1167,7 @@ RSpec.describe ApplicationController do
 
     it "is included when user API key is rate limited" do
       global_setting :max_user_api_reqs_per_minute, 1
-      user_api_key =
+      user_api_key = UserApiKey.create!(user_id: admin.id)
-        UserApiKey.create!(user_id: admin.id, client_id: "", application_name: "discourseapp")
       user_api_key.scopes =
         UserApiKeyScope.all_scopes.keys.map do |name|
           UserApiKeyScope.create!(name: name, user_api_key_id: user_api_key.id)

spec/requests/notifications_controller_spec.rb

@@ -631,8 +631,6 @@ RSpec.describe NotificationsController do
     fab!(:user)
     let(:user_api_key) do
       UserApiKey.create!(
-        application_name: "my app",
-        client_id: "",
         scopes: ["notifications"].map { |name| UserApiKeyScope.new(name: name) },
         user_id: user.id,
       )

spec/requests/user_api_key_clients_controller_spec.rb

@@ -0,0 +1,62 @@
+# frozen_string_literal: true
+
+RSpec.describe UserApiKeyClientsController do
+  let :public_key do
+    <<~TXT
+    -----BEGIN PUBLIC KEY-----
+    MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDh7BS7Ey8hfbNhlNAW/47pqT7w
+    IhBz3UyBYzin8JurEQ2pY9jWWlY8CH147KyIZf1fpcsi7ZNxGHeDhVsbtUKZxnFV
+    p16Op3CHLJnnJKKBMNdXMy0yDfCAHZtqxeBOTcCo1Vt/bHpIgiK5kmaekyXIaD0n
+    w0z/BYpOgZ8QwnI5ZwIDAQAB
+    -----END PUBLIC KEY-----
+    TXT
+  end
+
+  let :args do
+    {
+      client_id: "x" * 32,
+      auth_redirect: "http://over.the/rainbow",
+      application_name: "foo",
+      public_key: public_key,
+    }
+  end
+
+  describe "#register" do
+    context "without a user" do
+      it "returns a 403" do
+        post "/user-api-key-client/register.json", params: args
+        expect(response.status).to eq(403)
+      end
+    end
+
+    context "with a user" do
+      before { sign_in(Fabricate(:user)) }
+
+      it "registers a client" do
+        post "/user-api-key-client/register.json", params: args
+        expect(response.status).to eq(200)
+        expect(
+          UserApiKeyClient.exists?(
+            client_id: args[:client_id],
+            application_name: args[:application_name],
+            auth_redirect: args[:auth_redirect],
+            public_key: args[:public_key],
+          ),
+        ).to eq(true)
+      end
+
+      it "updates a registered client" do
+        Fabricate(:user_api_key_client, **args)
+        args[:application_name] = "bar"
+        post "/user-api-key-client/register.json", params: args
+        expect(response.status).to eq(200)
+        expect(
+          UserApiKeyClient.exists?(
+            client_id: args[:client_id],
+            application_name: args[:application_name],
+          ),
+        ).to eq(true)
+      end
+    end
+  end
+end

spec/requests/user_api_keys_controller_spec.rb

@@ -295,24 +295,30 @@ RSpec.describe UserApiKeysController do
       expect(uri.to_s).to include(query_str)
     end
 
-    it "revokes API key when client_id used by another user" do
+    context "with a registered client" do
-      user1 = Fabricate(:trust_level_0)
+      let!(:fixed_args) { args }
-      user2 = Fabricate(:trust_level_0)
+      let!(:user) { Fabricate(:user, trust_level: TrustLevel[1]) }
-      key = Fabricate(:user_api_key, user: user1)
+      let!(:client) do
+        Fabricate(
+          :user_api_key_client,
+          client_id: fixed_args[:client_id],
+          application_name: fixed_args[:application_name],
+          public_key: public_key,
+          auth_redirect: fixed_args[:auth_redirect],
+        )
+      end
 
-      SiteSetting.user_api_key_allowed_groups = Group::AUTO_GROUPS[:trust_level_0]
+      before { sign_in(user) }
-      SiteSetting.allowed_user_api_auth_redirects = args[:auth_redirect]
-      SiteSetting.allowed_user_api_push_urls = "https://push.it/here"
-      args[:client_id] = key.client_id
-      args[:scopes] = "push,notifications,message_bus,session_info,one_time_password"
-      args[:push_url] = "https://push.it/here"
 
-      sign_in(user2)
+      it "does not require allowed_user_api_auth_redirects to contain registered auth_redirect" do
+        post "/user-api-key.json", params: fixed_args
+        expect(response.status).to eq(302)
+      end
 
-      post "/user-api-key.json", params: args
+      it "does not require application_name or public_key params" do
-
+        post "/user-api-key.json", params: fixed_args.except(:application_name, :public_key)
-      expect(response.status).to eq(302)
+        expect(response.status).to eq(302)
-      expect(UserApiKey.exists?(key.id)).to eq(false)
+      end
     end
   end
 

spec/services/post_alerter_spec.rb

@@ -61,12 +61,13 @@ RSpec.describe PostAlerter do
 
   def setup_push_notification_subscription_for(user:)
     2.times do |i|
-      UserApiKey.create!(
+      client = Fabricate(:user_api_key_client, client_id: "xxx#{i}", application_name: "iPhone#{i}")
-        user_id: user.id,
+      Fabricate(
-        client_id: "xxx#{i}",
+        :user_api_key,
-        application_name: "iPhone#{i}",
+        user: user,
         scopes: ["notifications"].map { |name| UserApiKeyScope.new(name: name) },
         push_url: "https://site2.com/push",
+        user_api_key_client_id: client.id,
       )
     end
   end
@@ -1484,10 +1485,9 @@ RSpec.describe PostAlerter do
       evil_trout.update_columns(last_seen_at: 31.days.ago)
 
       SiteSetting.allowed_user_api_push_urls = "https://site2.com/push"
-      UserApiKey.create!(
+      Fabricate(
-        user_id: evil_trout.id,
+        :user_api_key,
-        client_id: "xxx#1",
+        user: evil_trout,
-        application_name: "iPhone1",
         scopes: ["notifications"].map { |name| UserApiKeyScope.new(name: name) },
         push_url: "https://site2.com/push",
       )