diff --git a/app/controllers/user_api_key_clients_controller.rb b/app/controllers/user_api_key_clients_controller.rb index d312b65f0fe..421ddfc0476 100644 --- a/app/controllers/user_api_key_clients_controller.rb +++ b/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 + skip_before_action :check_xhr, :preload_json, :verify_authenticity_token - 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 diff --git a/app/controllers/user_api_keys_controller.rb b/app/controllers/user_api_keys_controller.rb index 425c719c7ea..bc42e6dc30a 100644 --- a/app/controllers/user_api_keys_controller.rb +++ b/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] diff --git a/app/jobs/scheduled/clean_up_unused_registered_user_api_key_clients.rb b/app/jobs/scheduled/clean_up_unused_registered_user_api_key_clients.rb new file mode 100644 index 00000000000..319daa34692 --- /dev/null +++ b/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 diff --git a/app/models/user_api_key_client.rb b/app/models/user_api_key_client.rb index 1085e553c60..7ac2be29143 100644 --- a/app/models/user_api_key_client.rb +++ b/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 diff --git a/app/models/user_api_key_client_scope.rb b/app/models/user_api_key_client_scope.rb new file mode 100644 index 00000000000..a3f62f67810 --- /dev/null +++ b/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 +# diff --git a/config/routes.rb b/config/routes.rb index c5c58e40852..3d9e2a8b3af 100644 --- a/config/routes.rb +++ b/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" diff --git a/config/site_settings.yml b/config/site_settings.yml index 5f55c60b831..d7e7c0ed67e 100644 --- a/config/site_settings.yml +++ b/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: diff --git a/db/migrate/20241112124552_create_user_api_key_client_scopes.rb b/db/migrate/20241112124552_create_user_api_key_client_scopes.rb new file mode 100644 index 00000000000..d52857ac1d4 --- /dev/null +++ b/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 diff --git a/spec/fabricators/user_api_key_fabricator.rb b/spec/fabricators/user_api_key_fabricator.rb index ba64f207fc5..41d9b0f9142 100644 --- a/spec/fabricators/user_api_key_fabricator.rb +++ b/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 diff --git a/spec/jobs/clean_up_unused_user_api_key_clients_spec.rb b/spec/jobs/clean_up_unused_user_api_key_clients_spec.rb new file mode 100644 index 00000000000..f87a5ded867 --- /dev/null +++ b/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 diff --git a/spec/requests/user_api_key_clients_controller_spec.rb b/spec/requests/user_api_key_clients_controller_spec.rb index 3c36ceb1ec9..4340a776471 100644 --- a/spec/requests/user_api_key_clients_controller_spec.rb +++ b/spec/requests/user_api_key_clients_controller_spec.rb @@ -21,41 +21,69 @@ RSpec.describe UserApiKeyClientsController do } 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) + describe "#show" do + context "with a registered client" do + before { Fabricate(:user_api_key_client, **args) } + + 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 - before { sign_in(Fabricate(:user)) } + context "without a registered client" do + 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 - 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) + describe "#create" do + context "without scopes" do + it "returns a 400" do + post "/user-api-key-client.json", params: args + expect(response.status).to eq(400) + end + end + + context "with scopes" do + let!(:args_with_scopes) { args.merge(scopes: "user_status") } + + 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 - 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) + context "when scopes are allowed" do + before { SiteSetting.allow_user_api_key_client_scopes = "user_status" } + + it "registers a client" do + post "/user-api-key-client.json", params: args_with_scopes + expect(response.status).to eq(200) + client = + UserApiKeyClient.find_by( + client_id: args_with_scopes[:client_id], + application_name: args_with_scopes[:application_name], + 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 diff --git a/spec/requests/user_api_keys_controller_spec.rb b/spec/requests/user_api_keys_controller_spec.rb index f74ce896552..1a154bd0f12 100644 --- a/spec/requests/user_api_keys_controller_spec.rb +++ b/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 - post "/user-api-key.json", params: fixed_args - expect(response.status).to eq(302) + context "with allowed scopes" do + 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 + + 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 - post "/user-api-key.json", params: fixed_args.except(:application_name, :public_key) - expect(response.status).to eq(302) + context "without allowed scopes" do + let!(:invalid_scope_args) do + 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