diff --git a/app/controllers/user_api_keys_controller.rb b/app/controllers/user_api_keys_controller.rb index d4f2ae3d2b2..6de32f04afc 100644 --- a/app/controllers/user_api_keys_controller.rb +++ b/app/controllers/user_api_keys_controller.rb @@ -71,7 +71,6 @@ class UserApiKeysController < ApplicationController client_id: params[:client_id], user_id: current_user.id, push_url: params[:push_url], - key: SecureRandom.hex, scopes: scopes ) @@ -146,7 +145,7 @@ class UserApiKeysController < ApplicationController revoke_key = find_key if params[:id] if current_key = request.env['HTTP_USER_API_KEY'] - request_key = UserApiKey.find_by(key: current_key) + request_key = UserApiKey.with_key(current_key).first revoke_key ||= request_key if request_key && request_key.id != revoke_key.id && !request_key.scopes.include?("write") raise Discourse::InvalidAccess diff --git a/app/models/user_api_key.rb b/app/models/user_api_key.rb index 7d696246ca2..e937c44a8e0 100644 --- a/app/models/user_api_key.rb +++ b/app/models/user_api_key.rb @@ -20,6 +20,28 @@ class UserApiKey < ActiveRecord::Base belongs_to :user + scope :active, -> { where(revoked_at: nil) } + scope :with_key, ->(key) { where(key_hash: ApiKey.hash_key(key)) } + + after_initialize :generate_key + + def generate_key + if !self.key_hash + @key ||= SecureRandom.hex + self.key = @key + self.key_hash = ApiKey.hash_key(@key) + end + end + + def key + raise ApiKey::KeyAccessError.new "API key is only accessible immediately after creation" unless key_available? + @key + end + + def key_available? + @key.present? + end + def self.allowed_scopes Set.new(SiteSetting.allow_user_api_key_scopes.split("|")) end @@ -88,6 +110,7 @@ end # revoked_at :datetime # scopes :text default([]), not null, is an Array # last_used_at :datetime not null +# key_hash :string not null # # Indexes # diff --git a/db/migrate/20200403100259_add_key_hash_to_user_api_key.rb b/db/migrate/20200403100259_add_key_hash_to_user_api_key.rb new file mode 100644 index 00000000000..2e998fc2558 --- /dev/null +++ b/db/migrate/20200403100259_add_key_hash_to_user_api_key.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +class AddKeyHashToUserApiKey < ActiveRecord::Migration[6.0] + def up + add_column :user_api_keys, :key_hash, :string + + batch_size = 500 + loop do + rows = DB + .query("SELECT id, key FROM user_api_keys WHERE key_hash IS NULL LIMIT #{batch_size}") + .map { |row| { id: row.id, key_hash: Digest::SHA256.hexdigest(key) } } + + break if rows.size == 0 + + data_string = rows.map { |r| "(#{r[:id]}, '#{r[:key_hash]}')" }.join(",") + execute <<~SQL + UPDATE user_api_keys + SET key_hash = data.key_hash + FROM (VALUES #{data_string}) AS data(id, key_hash) + WHERE user_api_keys.id = data.id + SQL + + break if rows.size < batch_size + end + + change_column_null :user_api_keys, :key_hash, false + end + + def down + drop_column :user_api_keys, :key_hash, :string + end +end diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index c43580c042e..fc7f8500260 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/lib/auth/default_current_user_provider.rb @@ -260,7 +260,7 @@ class Auth::DefaultCurrentUserProvider protected def lookup_user_api_user_and_update_key(user_api_key, client_id) - if api_key = UserApiKey.where(key: user_api_key, revoked_at: nil).includes(:user).first + if api_key = UserApiKey.active.with_key(user_api_key).includes(:user).first unless api_key.allow?(@env) raise Discourse::InvalidAccess end diff --git a/spec/integration/user_api_keys_spec.rb b/spec/integration/user_api_keys_spec.rb index 8cd4edcd7a8..5bbdff319ec 100644 --- a/spec/integration/user_api_keys_spec.rb +++ b/spec/integration/user_api_keys_spec.rb @@ -3,12 +3,12 @@ require 'rails_helper' describe 'user api keys integration' do - fab!(:user_api_key) { Fabricate(:readonly_user_api_key) } - it 'updates last used time on use' do freeze_time + user_api_key = Fabricate(:readonly_user_api_key) user_api_key.update_columns(last_used_at: 7.days.ago) + get '/session/current.json', headers: { HTTP_USER_API_KEY: user_api_key.key, } diff --git a/spec/requests/user_api_keys_controller_spec.rb b/spec/requests/user_api_keys_controller_spec.rb index aa965b9a11c..8148bef88a6 100644 --- a/spec/requests/user_api_keys_controller_spec.rb +++ b/spec/requests/user_api_keys_controller_spec.rb @@ -205,7 +205,7 @@ describe UserApiKeysController do expect(parsed["nonce"]).to eq(args[:nonce]) expect(parsed["push"]).to eq(true) - api_key = UserApiKey.find_by(key: parsed["key"]) + api_key = UserApiKey.with_key(parsed["key"]).first expect(api_key.user_id).to eq(user.id) expect(api_key.scopes.sort).to eq(["push", "message_bus", "notifications", "session_info", "one_time_password"].sort) @@ -242,7 +242,7 @@ describe UserApiKeysController do encrypted = Base64.decode64(payload) key = OpenSSL::PKey::RSA.new(private_key) parsed = JSON.parse(key.private_decrypt(encrypted)) - api_key = UserApiKey.find_by(key: parsed["key"]) + api_key = UserApiKey.with_key(parsed["key"]).first expect(api_key.user_id).to eq(user.id) end @@ -259,7 +259,7 @@ describe UserApiKeysController do encrypted = Base64.decode64(payload) key = OpenSSL::PKey::RSA.new(private_key) parsed = JSON.parse(key.private_decrypt(encrypted)) - api_key = UserApiKey.find_by(key: parsed["key"]) + api_key = UserApiKey.with_key(parsed["key"]).first expect(api_key.user_id).to eq(user.id) end