Require permitted scopes when registering a client (#29718)

app/controllers/user_api_key_clients_controller.rb

@@ -2,26 +2,53 @@
 class UserApiKeyClientsController < ApplicationController
   layout "no_ember"
 
-  requires_login
+  skip_before_action :check_xhr, :preload_json, :verify_authenticity_token
-  skip_before_action :check_xhr, :preload_json
 
-  def register
+  def show
+    params.require(:client_id)
+    client = UserApiKeyClient.find_by(client_id: params[:client_id])
+    raise Discourse::InvalidParameters unless client && client.auth_redirect.present?
+    head :ok
+  end
+
+  def create
+    rate_limit
     require_params
+    validate_params
+    ensure_new_client
 
-    client = UserApiKeyClient.find_or_initialize_by(client_id: params[:client_id])
+    client = UserApiKeyClient.new(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!
+    ActiveRecord::Base.transaction do
+      client.save!
+      @scopes.each { |scope| client.scopes.create!(name: scope) }
+    end
+
+    if client.persisted?
       render json: success_json
     else
       render json: failed_json
     end
   end
 
+  def rate_limit
+    RateLimiter.new(nil, "user-api-key-clients-#{request.remote_ip}", 1, 24.hours).performed!
+  end
+
   def require_params
-    %i[client_id application_name public_key auth_redirect].each { |p| params.require(p) }
+    %i[client_id application_name public_key auth_redirect scopes].each { |p| params.require(p) }
+    @scopes = params[:scopes].split(",")
+  end
+
+  def validate_params
+    raise Discourse::InvalidAccess unless UserApiKeyClientScope.allowed.superset?(Set.new(@scopes))
     OpenSSL::PKey::RSA.new(params[:public_key])
   end
+
+  def ensure_new_client
+    raise Discourse::InvalidAccess if UserApiKeyClient.where(client_id: params[:client_id]).exists?
+  end
 end

app/controllers/user_api_keys_controller.rb

@@ -66,7 +66,6 @@ class UserApiKeysController < ApplicationController
 
     @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
@@ -88,7 +87,8 @@ class UserApiKeysController < ApplicationController
       api: AUTH_API_VERSION,
     }.to_json
 
-    public_key = OpenSSL::PKey::RSA.new(@client.public_key)
+    public_key_str = @client.public_key.present? ? @client.public_key : params[:public_key]
+    public_key = OpenSSL::PKey::RSA.new(public_key_str)
     @payload = Base64.encode64(public_key.public_encrypt(@payload))
 
     if scopes.include?("one_time_password")
@@ -190,6 +190,9 @@ class UserApiKeysController < ApplicationController
   def validate_params
     requested_scopes = Set.new(params[:scopes].split(","))
     raise Discourse::InvalidAccess unless UserApiKey.allowed_scopes.superset?(requested_scopes)
+    if @client&.scopes.present? && !@client.allowed_scopes.superset?(requested_scopes)
+      raise Discourse::InvalidAccess
+    end
 
     # our pk has got to parse
     OpenSSL::PKey::RSA.new(params[:public_key]) if params[:public_key]

app/jobs/scheduled/clean_up_unused_registered_user_api_key_clients.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+module Jobs
+  class CleanUpUnusedRegisteredUserApiKeyClients < ::Jobs::Scheduled
+    every 1.day
+
+    def execute(args)
+      if SiteSetting.unused_registered_user_api_key_clients_days > 0
+        destroy_days_ago = SiteSetting.unused_registered_user_api_key_clients_days.days.ago
+
+        clients =
+          UserApiKeyClient
+            .where("auth_redirect IS NOT NULL")
+            .where(
+              "id NOT IN (SELECT user_api_key_client_id FROM user_api_keys WHERE user_api_keys.last_used_at > ?)",
+              destroy_days_ago,
+            )
+            .distinct
+            .destroy_all
+      end
+    end
+  end
+end

app/models/user_api_key_client.rb

@@ -2,6 +2,14 @@
 
 class UserApiKeyClient < ActiveRecord::Base
   has_many :keys, class_name: "UserApiKey", dependent: :destroy
+  has_many :scopes,
+           class_name: "UserApiKeyClientScope",
+           foreign_key: "user_api_key_client_id",
+           dependent: :destroy
+
+  def allowed_scopes
+    Set.new(scopes.map(&:name))
+  end
 
   def self.invalid_auth_redirect?(auth_redirect, client: nil)
     return false if client&.auth_redirect == auth_redirect

app/models/user_api_key_client_scope.rb

@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+class UserApiKeyClientScope < ActiveRecord::Base
+  belongs_to :client, class_name: "UserApiKeyClient", foreign_key: "user_api_key_client_id"
+
+  validates :name,
+            inclusion: {
+              in: UserApiKeyScope.all_scopes.keys.map(&:to_s),
+              message: "%{value} is not a valid scope",
+            }
+
+  def self.allowed
+    Set.new(SiteSetting.allow_user_api_key_client_scopes.split("|"))
+  end
+end
+
+# == Schema Information
+#
+# Table name: user_api_key_client_scopes
+#
+#  id                     :bigint           not null, primary key
+#  user_api_key_client_id :bigint           not null
+#  name                   :string(100)      not null
+#  created_at             :datetime         not null
+#  updated_at             :datetime         not null
+#

config/routes.rb

@@ -1631,7 +1631,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 "/user-api-key-client" => "user_api_key_clients#show"
+    post "/user-api-key-client" => "user_api_key_clients#create"
 
     get "/safe-mode" => "safe_mode#index"
     post "/safe-mode" => "safe_mode#enter", :as => "safe_mode_enter"

config/site_settings.yml

@@ -3120,6 +3120,10 @@ user_api:
   allow_user_api_key_scopes:
     default: "read|write|message_bus|push|notifications|session_info|one_time_password"
     type: list
+  allow_user_api_key_client_scopes:
+    default: ""
+    type: list
+    hidden: true
   push_api_secret_key:
     default: ""
     hidden: true
@@ -3147,6 +3151,10 @@ user_api:
     default: 0
     max: 36500
     hidden: true
+  unused_registered_user_api_key_clients_days:
+    default: 30
+    max: 36500
+    hidden: true
 
 tags:
   tagging_enabled:

db/migrate/20241112124552_create_user_api_key_client_scopes.rb

@@ -0,0 +1,10 @@
+# frozen_string_literal: true
+class CreateUserApiKeyClientScopes < ActiveRecord::Migration[7.1]
+  def change
+    create_table :user_api_key_client_scopes do |t|
+      t.bigint :user_api_key_client_id, null: false
+      t.string :name, null: false, limit: 100
+      t.timestamps
+    end
+  end
+end

spec/fabricators/user_api_key_fabricator.rb

@@ -14,9 +14,19 @@ end
 
 Fabricator(:user_api_key_scope)
 
+Fabricator(:user_api_key_client_scope)
+
 Fabricator(:user_api_key_client) do
+  transient :scopes
+
   client_id { SecureRandom.hex }
   application_name "some app"
+
+  after_create do |client, transients|
+    if transients[:scopes].present?
+      [*transients[:scopes]].each { |scope| client.scopes.create!(name: scope) }
+    end
+  end
 end
 
 Fabricator(:readonly_user_api_key, from: :user_api_key) do

spec/jobs/clean_up_unused_user_api_key_clients_spec.rb

@@ -0,0 +1,56 @@
+# frozen_string_literal: true
+
+RSpec.describe Jobs::CleanUpUnusedRegisteredUserApiKeyClients do
+  let!(:client1) { Fabricate(:user_api_key_client, auth_redirect: "https://remote.com/redirect") }
+  let!(:client2) do
+    Fabricate(:user_api_key_client, auth_redirect: "https://another-remote.com/redirect")
+  end
+  let!(:client3) { Fabricate(:user_api_key_client) }
+  let!(:key1) { Fabricate(:readonly_user_api_key, client: client1, last_used_at: 1.hour.ago) }
+  let!(:key2) { Fabricate(:readonly_user_api_key, client: client1, last_used_at: 1.hour.ago) }
+  let!(:key3) { Fabricate(:readonly_user_api_key, client: client2, last_used_at: 1.hour.ago) }
+  let!(:key4) { Fabricate(:readonly_user_api_key, client: client3, last_used_at: 1.hour.ago) }
+
+  before do
+    SiteSetting.unused_registered_user_api_key_clients_days = 1
+    freeze_time
+  end
+
+  context "when registered client has used and unused keys" do
+    before { key1.update!(last_used_at: 2.days.ago) }
+
+    it "does not destroy client or keys" do
+      expect { described_class.new.execute({}) }.to not_change {
+        UserApiKeyClient.count
+      }.and not_change { UserApiKey.count }
+    end
+  end
+
+  context "when registered client has only unused keys" do
+    before do
+      key1.update!(last_used_at: 2.days.ago)
+      key2.update!(last_used_at: 2.days.ago)
+    end
+
+    it "destroys registered client and associated keys" do
+      described_class.new.execute({})
+      expect(UserApiKeyClient.exists?(client1.id)).to eq(false)
+      expect(UserApiKey.exists?(key1.id)).to eq(false)
+      expect(UserApiKey.exists?(key2.id)).to eq(false)
+      expect(UserApiKeyClient.exists?(client2.id)).to eq(true)
+      expect(UserApiKey.exists?(key3.id)).to eq(true)
+      expect(UserApiKeyClient.exists?(client3.id)).to eq(true)
+      expect(UserApiKey.exists?(key4.id)).to eq(true)
+    end
+  end
+
+  context "when unregistered client has only unused keys" do
+    before { key4.update!(last_used_at: 2.days.ago) }
+
+    it "does not destroy client or keys" do
+      expect { described_class.new.execute({}) }.to not_change {
+        UserApiKeyClient.count
+      }.and not_change { UserApiKey.count }
+    end
+  end
+end

spec/requests/user_api_key_clients_controller_spec.rb

@@ -21,41 +21,69 @@ RSpec.describe UserApiKeyClientsController do
     }
   end
 
-  describe "#register" do
+  describe "#show" do
-    context "without a user" do
+    context "with a registered client" do
-      it "returns a 403" do
+      before { Fabricate(:user_api_key_client, **args) }
-        post "/user-api-key-client/register.json", params: args
+
-        expect(response.status).to eq(403)
+      it "succeeds" do
+        head "/user-api-key-client.json", params: { client_id: args[:client_id] }
+        expect(response.status).to eq(200)
       end
     end
 
-    context "with a user" do
+    context "without a registered client" do
-      before { sign_in(Fabricate(:user)) }
+      it "returns a 400" do
+        head "/user-api-key-client.json", params: { client_id: args[:client_id] }
+        expect(response.status).to eq(400)
+      end
+    end
+  end
 
-      it "registers a client" do
+  describe "#create" do
-        post "/user-api-key-client/register.json", params: args
+    context "without scopes" do
-        expect(response.status).to eq(200)
+      it "returns a 400" do
-        expect(
+        post "/user-api-key-client.json", params: args
-          UserApiKeyClient.exists?(
+        expect(response.status).to eq(400)
-            client_id: args[:client_id],
+      end
-            application_name: args[:application_name],
+    end
-            auth_redirect: args[:auth_redirect],
+
-            public_key: args[:public_key],
+    context "with scopes" do
-          ),
+      let!(:args_with_scopes) { args.merge(scopes: "user_status") }
-        ).to eq(true)
+
+      context "when scopes are not allowed" do
+        before { SiteSetting.allow_user_api_key_client_scopes = "" }
+
+        it "returns a 403" do
+          post "/user-api-key-client.json", params: args_with_scopes
+          expect(response.status).to eq(403)
+        end
       end
 
-      it "updates a registered client" do
+      context "when scopes are allowed" do
-        Fabricate(:user_api_key_client, **args)
+        before { SiteSetting.allow_user_api_key_client_scopes = "user_status" }
-        args[:application_name] = "bar"
+
-        post "/user-api-key-client/register.json", params: args
+        it "registers a client" do
-        expect(response.status).to eq(200)
+          post "/user-api-key-client.json", params: args_with_scopes
-        expect(
+          expect(response.status).to eq(200)
-          UserApiKeyClient.exists?(
+          client =
-            client_id: args[:client_id],
+            UserApiKeyClient.find_by(
-            application_name: args[:application_name],
+              client_id: args_with_scopes[:client_id],
-          ),
+              application_name: args_with_scopes[:application_name],
-        ).to eq(true)
+              auth_redirect: args_with_scopes[:auth_redirect],
+              public_key: args_with_scopes[:public_key],
+            )
+          expect(client.present?).to eq(true)
+          expect(client.scopes.map(&:name)).to match_array(["user_status"])
+        end
+
+        context "if the client is already registered" do
+          before { Fabricate(:user_api_key_client, **args) }
+
+          it "returns a 403" do
+            post "/user-api-key-client.json", params: args_with_scopes
+            expect(response.status).to eq(403)
+          end
+        end
       end
     end
   end

spec/requests/user_api_keys_controller_spec.rb

@@ -305,19 +305,34 @@ RSpec.describe UserApiKeysController do
           application_name: fixed_args[:application_name],
           public_key: public_key,
           auth_redirect: fixed_args[:auth_redirect],
+          scopes: "read",
         )
       end
 
       before { sign_in(user) }
 
-      it "does not require allowed_user_api_auth_redirects to contain registered auth_redirect" do
+      context "with allowed scopes" do
-        post "/user-api-key.json", params: fixed_args
+        it "does not require allowed_user_api_auth_redirects to contain registered auth_redirect" do
-        expect(response.status).to eq(302)
+          post "/user-api-key.json", params: fixed_args
+          expect(response.status).to eq(302)
+        end
+
+        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)
+        end
       end
 
-      it "does not require application_name or public_key params" do
+      context "without allowed scopes" do
-        post "/user-api-key.json", params: fixed_args.except(:application_name, :public_key)
+        let!(:invalid_scope_args) do
-        expect(response.status).to eq(302)
+          fixed_args[:scopes] = "write"
+          fixed_args
+        end
+
+        it "returns a 403" do
+          post "/user-api-key.json", params: invalid_scope_args
+          expect(response.status).to eq(403)
+        end
       end
     end
   end