From dc9110cc434b0c0f3d6bec8d992c6caea31e273c Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Tue, 3 Sep 2019 18:10:29 +1000 Subject: [PATCH] FEATURE: track date api key was last used Start tracking the date an api key was last used. This has already been the case for user_api_keys. This information can provide us with the ability to automatically expire unused api keys after N days. --- app/models/api_key.rb | 1 + ...90903073730_add_last_used_at_to_api_key.rb | 6 +++++ lib/auth/default_current_user_provider.rb | 23 ++++++++++++------- .../default_current_user_provider_spec.rb | 11 +++++++-- 4 files changed, 31 insertions(+), 10 deletions(-) create mode 100644 db/migrate/20190903073730_add_last_used_at_to_api_key.rb diff --git a/app/models/api_key.rb b/app/models/api_key.rb index f6a48199ed2..6062025519a 100644 --- a/app/models/api_key.rb +++ b/app/models/api_key.rb @@ -35,6 +35,7 @@ end # updated_at :datetime not null # allowed_ips :inet is an Array # hidden :boolean default(FALSE), not null +# last_used_at :datetime # # Indexes # diff --git a/db/migrate/20190903073730_add_last_used_at_to_api_key.rb b/db/migrate/20190903073730_add_last_used_at_to_api_key.rb new file mode 100644 index 00000000000..46889808a60 --- /dev/null +++ b/db/migrate/20190903073730_add_last_used_at_to_api_key.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true +class AddLastUsedAtToApiKey < ActiveRecord::Migration[5.2] + def change + add_column :api_keys, :last_used_at, :datetime + end +end diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index 27fb47a3276..9e8d88faa06 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/lib/auth/default_current_user_provider.rb @@ -292,15 +292,22 @@ class Auth::DefaultCurrentUserProvider return nil end - if api_key.user - api_key.user if !api_username || (api_key.user.username_lower == api_username.downcase) - elsif api_username - User.find_by(username_lower: api_username.downcase) - elsif user_id = header_api_key? ? @env[HEADER_API_USER_ID] : request["api_user_id"] - User.find_by(id: user_id.to_i) - elsif external_id = header_api_key? ? @env[HEADER_API_USER_EXTERNAL_ID] : request["api_user_external_id"] - SingleSignOnRecord.find_by(external_id: external_id.to_s).try(:user) + user = + if api_key.user + api_key.user if !api_username || (api_key.user.username_lower == api_username.downcase) + elsif api_username + User.find_by(username_lower: api_username.downcase) + elsif user_id = header_api_key? ? @env[HEADER_API_USER_ID] : request["api_user_id"] + User.find_by(id: user_id.to_i) + elsif external_id = header_api_key? ? @env[HEADER_API_USER_EXTERNAL_ID] : request["api_user_external_id"] + SingleSignOnRecord.find_by(external_id: external_id.to_s).try(:user) + end + + if user + api_key.update_columns(last_used_at: Time.zone.now) end + + user end end diff --git a/spec/components/auth/default_current_user_provider_spec.rb b/spec/components/auth/default_current_user_provider_spec.rb index d6320a919d2..62761fa181d 100644 --- a/spec/components/auth/default_current_user_provider_spec.rb +++ b/spec/components/auth/default_current_user_provider_spec.rb @@ -56,11 +56,14 @@ describe Auth::DefaultCurrentUserProvider do it "raises for a user pretending" do user = Fabricate(:user) user2 = Fabricate(:user) - ApiKey.create!(key: "hello", user_id: user.id, created_by_id: -1) + key = ApiKey.create!(key: "hello", user_id: user.id, created_by_id: -1) expect { provider("/?api_key=hello&api_username=#{user2.username.downcase}").current_user }.to raise_error(Discourse::InvalidAccess) + + key.reload + expect(key.last_used_at).to eq(nil) end it "raises for a user with a mismatching ip" do @@ -74,8 +77,10 @@ describe Auth::DefaultCurrentUserProvider do end it "allows a user with a matching ip" do + freeze_time + user = Fabricate(:user) - ApiKey.create!(key: "hello", user_id: user.id, created_by_id: -1, allowed_ips: ['100.0.0.0/24']) + key = ApiKey.create!(key: "hello", user_id: user.id, created_by_id: -1, allowed_ips: ['100.0.0.0/24']) found_user = provider("/?api_key=hello&api_username=#{user.username.downcase}", "REMOTE_ADDR" => "100.0.0.22").current_user @@ -86,6 +91,8 @@ describe Auth::DefaultCurrentUserProvider do "HTTP_X_FORWARDED_FOR" => "10.1.1.1, 100.0.0.22").current_user expect(found_user.id).to eq(user.id) + key.reload + expect(key.last_used_at).to eq_time(Time.zone.now) end it "finds a user for a correct system api key" do