diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 2e1b6a74c16..0e0a54206f9 100644 --- a/app/controllers/application_controller.rb +++ b/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 diff --git a/app/controllers/user_api_key_clients_controller.rb b/app/controllers/user_api_key_clients_controller.rb new file mode 100644 index 00000000000..d312b65f0fe --- /dev/null +++ b/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 diff --git a/app/controllers/user_api_keys_controller.rb b/app/controllers/user_api_keys_controller.rb index 792f3000b34..425c719c7ea 100644 --- a/app/controllers/user_api_keys_controller.rb +++ b/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] - @public_key = params[:public_key] + @application_name = params[:application_name] || @client&.application_name + @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!( - application_name: @application_name, - client_id: params[:client_id], + @client.keys.create!( 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 diff --git a/app/models/user_api_key.rb b/app/models/user_api_key.rb index 9d004cf3bd9..b02d6093495 100644 --- a/app/models/user_api_key.rb +++ b/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 - # invalidate old dupe api key for client if needed - UserApiKey - .where(client_id: client_id, user_id: self.user_id) - .where("id <> ?", self.id) - .destroy_all - - update_args[:client_id] = client_id + if client_id.present? && client_id != self.client.client_id + new_client = + UserApiKeyClient.create!( + client_id: client_id, + application_name: self.client.application_name, + ) + update_args[:user_api_key_client_id] = new_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 -# user_id :integer not null -# client_id :string not null -# application_name :string not null -# push_url :string -# created_at :datetime not null -# updated_at :datetime not null -# revoked_at :datetime -# last_used_at :datetime not null -# key_hash :string not null +# id :integer not null, primary key +# user_id :integer not null +# push_url :string +# created_at :datetime not null +# updated_at :datetime not null +# revoked_at :datetime +# last_used_at :datetime not null +# key_hash :string not null +# user_api_key_client_id :bigint # # Indexes # -# 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_user_id (user_id) +# 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_user_api_key_client_id (user_api_key_client_id) +# index_user_api_keys_on_user_id (user_id) # diff --git a/app/models/user_api_key_client.rb b/app/models/user_api_key_client.rb new file mode 100644 index 00000000000..1085e553c60 --- /dev/null +++ b/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 +# diff --git a/app/serializers/user_serializer.rb b/app/serializers/user_serializer.rb index 509d1656af5..d2c2d063bb5 100644 --- a/app/serializers/user_serializer.rb +++ b/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, diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index acfaa05611d..532ad2a4afc 100644 --- a/app/services/post_alerter.rb +++ b/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) - .pluck(:client_id, :push_url) + .order("user_api_key_clients.client_id ASC") + .pluck("user_api_key_clients.client_id, user_api_keys.push_url") return if clients.length == 0 diff --git a/config/routes.rb b/config/routes.rb index 6c99110abb9..66dc52052bc 100644 --- a/config/routes.rb +++ b/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" diff --git a/db/migrate/20240729084803_create_user_api_key_clients.rb b/db/migrate/20240729084803_create_user_api_key_clients.rb new file mode 100644 index 00000000000..b0139509604 --- /dev/null +++ b/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 diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index b8b73608dc2..ba461a74fd5 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/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 diff --git a/spec/fabricators/user_api_key_fabricator.rb b/spec/fabricators/user_api_key_fabricator.rb index fc2e805ecb2..ba64f207fc5 100644 --- a/spec/fabricators/user_api_key_fabricator.rb +++ b/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 diff --git a/spec/lib/auth/default_current_user_provider_spec.rb b/spec/lib/auth/default_current_user_provider_spec.rb index 7247694f038..ef165a27156 100644 --- a/spec/lib/auth/default_current_user_provider_spec.rb +++ b/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!( - application_name: "my app", - client_id: "1234", + Fabricate( + :user_api_key, scopes: ["read"].map { |name| UserApiKeyScope.new(name: name) }, - user_id: user.id, + user: user, ) end - it "can clear old duplicate keys correctly" do - dupe = - UserApiKey.create!( - application_name: "my app", - client_id: "12345", - scopes: ["read"].map { |name| UserApiKeyScope.new(name: name) }, - user_id: user.id, - ) - + it "creates a new client if the client id changes" do 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 diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb index fec81163bf7..52f71416363 100644 --- a/spec/requests/application_controller_spec.rb +++ b/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 = - UserApiKey.create!(user_id: admin.id, client_id: "", application_name: "discourseapp") + user_api_key = UserApiKey.create!(user_id: admin.id) user_api_key.scopes = UserApiKeyScope.all_scopes.keys.map do |name| UserApiKeyScope.create!(name: name, user_api_key_id: user_api_key.id) diff --git a/spec/requests/notifications_controller_spec.rb b/spec/requests/notifications_controller_spec.rb index 2c06d3801c1..204424c5cf2 100644 --- a/spec/requests/notifications_controller_spec.rb +++ b/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, ) diff --git a/spec/requests/user_api_key_clients_controller_spec.rb b/spec/requests/user_api_key_clients_controller_spec.rb new file mode 100644 index 00000000000..3c36ceb1ec9 --- /dev/null +++ b/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 diff --git a/spec/requests/user_api_keys_controller_spec.rb b/spec/requests/user_api_keys_controller_spec.rb index 194c84dc9cf..f74ce896552 100644 --- a/spec/requests/user_api_keys_controller_spec.rb +++ b/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 - user1 = Fabricate(:trust_level_0) - user2 = Fabricate(:trust_level_0) - key = Fabricate(:user_api_key, user: user1) + context "with a registered client" do + let!(:fixed_args) { args } + let!(:user) { Fabricate(:user, trust_level: TrustLevel[1]) } + 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] - 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" + before { sign_in(user) } - 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 - - expect(response.status).to eq(302) - expect(UserApiKey.exists?(key.id)).to eq(false) + 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 end diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index f2b1cdb754d..9afcdd164db 100644 --- a/spec/services/post_alerter_spec.rb +++ b/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!( - user_id: user.id, - client_id: "xxx#{i}", - application_name: "iPhone#{i}", + client = Fabricate(:user_api_key_client, client_id: "xxx#{i}", application_name: "iPhone#{i}") + Fabricate( + :user_api_key, + 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!( - user_id: evil_trout.id, - client_id: "xxx#1", - application_name: "iPhone1", + Fabricate( + :user_api_key, + user: evil_trout, scopes: ["notifications"].map { |name| UserApiKeyScope.new(name: name) }, push_url: "https://site2.com/push", )