diff --git a/app/assets/javascripts/admin/controllers/admin-api-keys-new.js.es6 b/app/assets/javascripts/admin/controllers/admin-api-keys-new.js.es6 index c04e6abec9b..e4f2869b21b 100644 --- a/app/assets/javascripts/admin/controllers/admin-api-keys-new.js.es6 +++ b/app/assets/javascripts/admin/controllers/admin-api-keys-new.js.es6 @@ -28,12 +28,11 @@ export default Ember.Controller.extend({ }, save() { - this.model - .save() - .then(() => { - this.transitionToRoute("adminApiKeys.show", this.model.id); - }) - .catch(popupAjaxError); + this.model.save().catch(popupAjaxError); + }, + + continue() { + this.transitionToRoute("adminApiKeys.show", this.model.id); } } }); diff --git a/app/assets/javascripts/admin/models/api-key.js.es6 b/app/assets/javascripts/admin/models/api-key.js.es6 index 7a20c288aea..06d861fa4a3 100644 --- a/app/assets/javascripts/admin/models/api-key.js.es6 +++ b/app/assets/javascripts/admin/models/api-key.js.es6 @@ -3,6 +3,7 @@ import AdminUser from "admin/models/admin-user"; import RestModel from "discourse/models/rest"; import { ajax } from "discourse/lib/ajax"; import { computed } from "@ember/object"; +import { fmt } from "discourse/lib/computed"; const ApiKey = RestModel.extend({ user: computed("_user", { @@ -19,17 +20,14 @@ const ApiKey = RestModel.extend({ } }), - @discourseComputed("key") - shortKey(key) { - return `${key.substring(0, 4)}...`; - }, - @discourseComputed("description") shortDescription(description) { if (!description || description.length < 40) return description; return `${description.substring(0, 40)}...`; }, + truncatedKey: fmt("truncated_key", "%@..."), + revoke() { return ajax(`${this.basePath}/revoke`, { type: "POST" diff --git a/app/assets/javascripts/admin/templates/api-keys-index.hbs b/app/assets/javascripts/admin/templates/api-keys-index.hbs index a0d5bf9f77e..7943a5b9e2a 100644 --- a/app/assets/javascripts/admin/templates/api-keys-index.hbs +++ b/app/assets/javascripts/admin/templates/api-keys-index.hbs @@ -19,7 +19,7 @@ {{#if k.revoked_at}}{{d-icon 'times-circle'}}{{/if}} - {{k.shortKey}} + {{k.truncatedKey}} {{k.shortDescription}} diff --git a/app/assets/javascripts/admin/templates/api-keys-new.hbs b/app/assets/javascripts/admin/templates/api-keys-new.hbs index 15f25b1ce17..4ddc37befd4 100644 --- a/app/assets/javascripts/admin/templates/api-keys-new.hbs +++ b/app/assets/javascripts/admin/templates/api-keys-new.hbs @@ -4,24 +4,35 @@ {{/link-to}}
- {{#admin-form-row label="admin.api.description"}} - {{input value=model.description maxlength="255" placeholder=(i18n "admin.api.description_placeholder")}} - {{/admin-form-row}} + {{#if model.id}} + {{#admin-form-row label="admin.api.key"}} +
{{model.key}}
+ {{/admin-form-row}} + {{#admin-form-row}} + {{i18n "admin.api.not_shown_again"}} + {{/admin-form-row}} + {{#admin-form-row}} + {{d-button icon="angle-right" label="admin.api.continue" action=(action "continue") class="btn-primary"}} + {{/admin-form-row}} + {{else}} + {{#admin-form-row label="admin.api.description"}} + {{input value=model.description maxlength="255" placeholder=(i18n "admin.api.description_placeholder")}} + {{/admin-form-row}} - {{#admin-form-row label="admin.api.user_mode"}} - {{combo-box content=userModes value=userMode onSelect=(action "changeUserMode")}} - {{/admin-form-row}} + {{#admin-form-row label="admin.api.user_mode"}} + {{combo-box content=userModes value=userMode onSelect=(action "changeUserMode")}} + {{/admin-form-row}} - {{#if showUserSelector}} - {{#admin-form-row label="admin.api.user"}} - {{user-selector single="true" - usernames=model.username - placeholderKey="admin.api.user_placeholder" - }} + {{#if showUserSelector}} + {{#admin-form-row label="admin.api.user"}} + {{user-selector single="true" + usernames=model.username + placeholderKey="admin.api.user_placeholder" + }} + {{/admin-form-row}} + {{/if}} + {{#admin-form-row}} + {{d-button icon="check" label="admin.api.save" action=(action "save") class="btn-primary"}} {{/admin-form-row}} {{/if}} - - {{#admin-form-row}} - {{d-button icon="check" label="admin.api.save" action=(action "save") class="btn-primary" disabled=saveDisabled}} - {{/admin-form-row}}
\ No newline at end of file diff --git a/app/assets/javascripts/admin/templates/api-keys-show.hbs b/app/assets/javascripts/admin/templates/api-keys-show.hbs index 725c987a22c..7639a438f9d 100644 --- a/app/assets/javascripts/admin/templates/api-keys-show.hbs +++ b/app/assets/javascripts/admin/templates/api-keys-show.hbs @@ -6,7 +6,7 @@
{{#admin-form-row label="admin.api.key"}} {{#if model.revoked_at}}{{d-icon 'times-circle'}}{{/if}} -
{{model.key}}
+ {{model.truncatedKey}} {{/admin-form-row}} {{#admin-form-row label="admin.api.description"}} diff --git a/app/models/api_key.rb b/app/models/api_key.rb index 91722b36030..6abc38b548f 100644 --- a/app/models/api_key.rb +++ b/app/models/api_key.rb @@ -1,22 +1,36 @@ # frozen_string_literal: true class ApiKey < ActiveRecord::Base + class KeyAccessError < StandardError; end + belongs_to :user belongs_to :created_by, class_name: 'User' scope :active, -> { where("revoked_at IS NULL") } scope :revoked, -> { where("revoked_at IS NOT NULL") } - validates_presence_of :key + scope :with_key, ->(key) { + hashed = self.hash_key(key) + where(key_hash: hashed) + } after_initialize :generate_key def generate_key - self.key ||= SecureRandom.hex(32) + if !self.key_hash + @key ||= SecureRandom.hex(32) # Not saved to DB + self.truncated_key = key[0..3] + self.key_hash = ApiKey.hash_key(key) + end end - def truncated_key - self.key[0..3] + def key + raise KeyAccessError.new "API key is only accessible immediately after creation" unless key_available? + @key + end + + def key_available? + @key.present? end def self.last_used_epoch @@ -42,6 +56,10 @@ class ApiKey < ActiveRecord::Base end end end + + def self.hash_key(key) + Digest::SHA256.hexdigest key + end end # == Schema Information @@ -49,7 +67,6 @@ end # Table name: api_keys # # id :integer not null, primary key -# key :string(64) not null # user_id :integer # created_by_id :integer # created_at :datetime not null @@ -59,9 +76,11 @@ end # last_used_at :datetime # revoked_at :datetime # description :text +# key_hash :string not null +# truncated_key :string not null # # Indexes # -# index_api_keys_on_key (key) -# index_api_keys_on_user_id (user_id) +# index_api_keys_on_key_hash (key_hash) +# index_api_keys_on_user_id (user_id) # diff --git a/app/serializers/api_key_serializer.rb b/app/serializers/api_key_serializer.rb index c743b97d52e..310b210a01d 100644 --- a/app/serializers/api_key_serializer.rb +++ b/app/serializers/api_key_serializer.rb @@ -4,6 +4,7 @@ class ApiKeySerializer < ApplicationSerializer attributes :id, :key, + :truncated_key, :description, :last_used_at, :created_at, @@ -16,4 +17,8 @@ class ApiKeySerializer < ApplicationSerializer !object.user_id.nil? end + def include_key? + # Only available when first created. Not stored in db + object.key_available? + end end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 5f9055905fa..dbbdc95de54 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -3394,6 +3394,8 @@ en: new_key: New API Key revoked: Revoked delete: Permenantly Delete + not_shown_again: This key will not be displayed again. Make sure you take a copy before continuing. + continue: Continue web_hooks: title: "Webhooks" diff --git a/db/migrate/20191211170000_add_hashed_api_key.rb b/db/migrate/20191211170000_add_hashed_api_key.rb new file mode 100644 index 00000000000..2811239c5dc --- /dev/null +++ b/db/migrate/20191211170000_add_hashed_api_key.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true +class AddHashedApiKey < ActiveRecord::Migration[6.0] + def up + add_column(:api_keys, :key_hash, :string) + add_column(:api_keys, :truncated_key, :string) + + execute( + <<~SQL + UPDATE api_keys + SET truncated_key = LEFT(key, 4) + SQL + ) + + batch_size = 500 + begin + batch = DB.query <<-SQL + SELECT id, key + FROM api_keys + WHERE key_hash IS NULL + LIMIT #{batch_size} + SQL + + to_update = [] + for row in batch + hashed = Digest::SHA256.hexdigest row.key + to_update << { id: row.id, key_hash: hashed } + end + + if to_update.size > 0 + data_string = to_update.map { |r| "(#{r[:id]}, '#{r[:key_hash]}')" }.join(",") + + DB.exec <<~SQL + UPDATE api_keys + SET key_hash = data.key_hash + FROM (values + #{data_string} + ) as data(id, key_hash) + WHERE api_keys.id = data.id + SQL + end + end until batch.length < batch_size + + change_column_null :api_keys, :key_hash, false + change_column_null :api_keys, :truncated_key, false + + add_index :api_keys, :key_hash + + # The key column will be dropped in a post_deploy migration + # But allow it to be null in the meantime + Migration::SafeMigrate.disable! + change_column_null :api_keys, :key, true + Migration::SafeMigrate.enable! + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index 3bc8957ea33..bdd304ef1b1 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/lib/auth/default_current_user_provider.rb @@ -284,7 +284,7 @@ class Auth::DefaultCurrentUserProvider end def lookup_api_user(api_key_value, request) - if api_key = ApiKey.active.where(key: api_key_value).includes(:user).first + if api_key = ApiKey.active.with_key(api_key_value).includes(:user).first api_username = header_api_key? ? @env[HEADER_API_USERNAME] : request[API_USERNAME] # Check for deprecated api auth @@ -333,7 +333,7 @@ class Auth::DefaultCurrentUserProvider RateLimiter.new( nil, - "admin_api_min_#{api_key}", + "admin_api_min_#{ApiKey.hash_key(api_key)}", GlobalSetting.max_admin_api_reqs_per_key_per_minute, 60 ).performed! diff --git a/lib/tasks/api.rake b/lib/tasks/api.rake index bf8c37ecd31..f6f838d83dc 100644 --- a/lib/tasks/api.rake +++ b/lib/tasks/api.rake @@ -1,9 +1,9 @@ # frozen_string_literal: true -desc "find or generate a master api key with given description" -task "api_key:get_or_create_master", [:description] => :environment do |task, args| +desc "generate a master api key with given description" +task "api_key:create_master", [:description] => :environment do |task, args| raise "Supply a description for the key" if !args[:description] - api_key = ApiKey.find_or_create_by!(description: args[:description], revoked_at: nil, user_id: nil) + api_key = ApiKey.create!(description: args[:description]) puts api_key.key end diff --git a/spec/fabricators/api_key_fabricator.rb b/spec/fabricators/api_key_fabricator.rb index f2f766baee4..2f75d78c0bb 100644 --- a/spec/fabricators/api_key_fabricator.rb +++ b/spec/fabricators/api_key_fabricator.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true Fabricator(:api_key) do - key '1dfb7d427400cb8ef18052fd412781af134cceca5725dd74f34bbc6b9e35ddc9' + end diff --git a/spec/models/api_key_spec.rb b/spec/models/api_key_spec.rb index aaaf7196041..c842f5421cc 100644 --- a/spec/models/api_key_spec.rb +++ b/spec/models/api_key_spec.rb @@ -8,18 +8,32 @@ describe ApiKey do it { is_expected.to belong_to :user } it { is_expected.to belong_to :created_by } - it { is_expected.to validate_presence_of :key } it 'generates a key when saving' do - key = ApiKey.new - key.save! - initial_key = key.key + api_key = ApiKey.new + api_key.save! + initial_key = api_key.key expect(initial_key.length).to eq(64) # Does not overwrite key when saving again - key.description = "My description here" - key.save! - expect(key.reload.key).to eq(initial_key) + api_key.description = "My description here" + api_key.save! + expect(api_key.reload.key).to eq(initial_key) + end + + it 'does not have the key when loading later from the database' do + api_key = ApiKey.create! + expect(api_key.key_available?).to eq(true) + expect(api_key.key.length).to eq(64) + + api_key = ApiKey.find(api_key.id) + expect(api_key.key_available?).to eq(false) + expect { api_key.key }.to raise_error(ApiKey::KeyAccessError) + end + + it "can lookup keys based on their hash" do + key = ApiKey.create!.key + expect(ApiKey.with_key(key).length).to eq(1) end it "can calculate the epoch correctly" do diff --git a/spec/requests/admin/api_controller_spec.rb b/spec/requests/admin/api_controller_spec.rb index 217ef19dc55..9c48d74732e 100644 --- a/spec/requests/admin/api_controller_spec.rb +++ b/spec/requests/admin/api_controller_spec.rb @@ -10,8 +10,8 @@ describe Admin::ApiController do fab!(:admin) { Fabricate(:admin) } - fab!(:key1) { Fabricate(:api_key, description: "my key") } - fab!(:key2) { Fabricate(:api_key, user: admin) } + fab!(:key1, refind: false) { Fabricate(:api_key, description: "my key") } + fab!(:key2, refind: false) { Fabricate(:api_key, user: admin) } context "as an admin" do before do @@ -32,7 +32,8 @@ describe Admin::ApiController do expect(response.status).to eq(200) data = JSON.parse(response.body)["key"] expect(data["id"]).to eq(key1.id) - expect(data["key"]).to eq(key1.key) + expect(data["key"]).to eq(nil) + expect(data["truncated_key"]).to eq(key1.key[0..3]) expect(data["description"]).to eq("my key") end end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 179bedc690f..7c7b370b085 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -815,7 +815,7 @@ describe UsersController do context "with a regular api key" do fab!(:user) { Fabricate(:user) } - fab!(:api_key) { Fabricate(:api_key, user: user) } + fab!(:api_key, refind: false) { Fabricate(:api_key, user: user) } it "won't create the user as active with a regular key" do post "/u.json", @@ -828,7 +828,7 @@ describe UsersController do context "with an admin api key" do fab!(:admin) { Fabricate(:admin) } - fab!(:api_key) { Fabricate(:api_key, user: admin) } + fab!(:api_key, refind: false) { Fabricate(:api_key, user: admin) } it "creates the user as active with a an admin key" do SiteSetting.send_welcome_message = true @@ -915,7 +915,7 @@ describe UsersController do context "with a regular api key" do fab!(:user) { Fabricate(:user) } - fab!(:api_key) { Fabricate(:api_key, user: user) } + fab!(:api_key, refind: false) { Fabricate(:api_key, user: user) } it "won't create the user as staged with a regular key" do post "/u.json", params: post_user_params.merge(staged: true, api_key: api_key.key) @@ -928,7 +928,7 @@ describe UsersController do context "with an admin api key" do fab!(:user) { Fabricate(:admin) } - fab!(:api_key) { Fabricate(:api_key, user: user) } + fab!(:api_key, refind: false) { Fabricate(:api_key, user: user) } it "creates the user as staged with a regular key" do post "/u.json", params: post_user_params.merge(staged: true, api_key: api_key.key)