From 25fda1ecc2e42089d3425e2146862e17b3f8b0b6 Mon Sep 17 00:00:00 2001 From: evazion Date: Sun, 14 Feb 2021 18:05:32 -0600 Subject: [PATCH] api keys: add IP whitelist and API permission system. Add the ability to restrict API keys so that they can only be used with certain IP addresses or certain API endpoints. Restricting your key is useful to limit damage in case it gets leaked or stolen. For example, if your key is on a remote server and it gets hacked, or if you accidentally check-in your key to Github. Restricting your key's API permissions is useful if a third-party app or script wants your key, but you don't want to give full access to your account. If you're an app or userscript developer, and your app needs an API key from the user, you should only request a key with the minimum permissions needed by your app. If you have a privileged account, and you have scripts running under your account, you are highly encouraged to restrict your key to limit damage in case your key gets leaked or stolen. --- CHANGELOG.md | 14 +++++ app/controllers/api_keys_controller.rb | 23 ++++++--- .../src/styles/common/simple_form.scss | 7 +++ app/logical/session_loader.rb | 7 +-- app/models/api_key.rb | 50 +++++++++++++++++- app/models/user.rb | 3 +- app/policies/api_key_policy.rb | 16 ++++++ app/views/api_keys/_form.html.erb | 6 +++ app/views/api_keys/_secondary_links.html.erb | 2 +- app/views/api_keys/edit.html.erb | 8 +++ app/views/api_keys/index.html.erb | 18 +++++-- app/views/api_keys/new.html.erb | 8 +++ config/routes.rb | 4 +- .../20210214101614_add_fields_to_api_keys.rb | 12 +++++ db/structure.sql | 11 +++- test/factories/api_key.rb | 1 + test/functional/api_keys_controller_test.rb | 51 +++++++++++++++---- .../functional/application_controller_test.rb | 47 +++++++++++++++++ test/unit/api_key_test.rb | 30 ++++++++++- 19 files changed, 286 insertions(+), 32 deletions(-) create mode 100644 app/views/api_keys/_form.html.erb create mode 100644 app/views/api_keys/edit.html.erb create mode 100644 app/views/api_keys/new.html.erb create mode 100644 db/migrate/20210214101614_add_fields_to_api_keys.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index a503e7bdc..d00c262e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,17 @@ +## Unreleased + +### API Changes + +* You can now have multiple API keys. +* API keys can be restricted to only work with certain IPs or certain API + endpoints. +* If you're an app or script developer, and you have an app that requests API + keys from users, you're highly encouraged to only request the minimum API + permissions necessary for your app to work. +* If you have a privileged account, and you run scripts under your account, + you're highly encouraged to restrict your API keys to limit damage in case + they get leaked or stolen. + ## 2021-02-05 ### Changes diff --git a/app/controllers/api_keys_controller.rb b/app/controllers/api_keys_controller.rb index 94ddfb448..a9ca5a382 100644 --- a/app/controllers/api_keys_controller.rb +++ b/app/controllers/api_keys_controller.rb @@ -1,23 +1,34 @@ class ApiKeysController < ApplicationController respond_to :html, :json, :xml + def new + @api_key = authorize ApiKey.new(user: CurrentUser.user, **permitted_attributes(ApiKey)) + respond_with(@api_key) + end + def create - @api_key = authorize ApiKey.new(user: CurrentUser.user) + @api_key = authorize ApiKey.new(user: CurrentUser.user, **permitted_attributes(ApiKey)) @api_key.save respond_with(@api_key, location: user_api_keys_path(CurrentUser.user.id)) end + def edit + @api_key = authorize ApiKey.find(params[:id]) + respond_with(@api_key) + end + + def update + @api_key = authorize ApiKey.find(params[:id]) + @api_key.update(permitted_attributes(@api_key)) + respond_with(@api_key, location: user_api_keys_path(CurrentUser.user.id)) + end + def index params[:search][:user_id] = params[:user_id] if params[:user_id].present? @api_keys = authorize ApiKey.visible(CurrentUser.user).paginated_search(params, count_pages: true) respond_with(@api_keys) end - def show - @api_key = authorize ApiKey.find(params[:id]) - respond_with(@api_key) - end - def destroy @api_key = authorize ApiKey.find(params[:id]) @api_key.destroy diff --git a/app/javascript/src/styles/common/simple_form.scss b/app/javascript/src/styles/common/simple_form.scss index b2489cee3..338f316f4 100644 --- a/app/javascript/src/styles/common/simple_form.scss +++ b/app/javascript/src/styles/common/simple_form.scss @@ -69,6 +69,13 @@ form.simple_form { } } } + + &.stacked-hints { + span.hint { + display: block; + padding-left: 0; + } + } } form.inline-form { diff --git a/app/logical/session_loader.rb b/app/logical/session_loader.rb index b3b8ce6f9..9ae708fc4 100644 --- a/app/logical/session_loader.rb +++ b/app/logical/session_loader.rb @@ -60,7 +60,7 @@ class SessionLoader end def has_api_authentication? - request.authorization.present? || params[:login].present? || params[:api_key].present? + request.authorization.present? || params[:login].present? || (params[:api_key].present? && params[:api_key].is_a?(String)) end private @@ -87,9 +87,10 @@ class SessionLoader authenticate_api_key(login, api_key) end - def authenticate_api_key(name, api_key) - user = User.find_by_name(name)&.authenticate_api_key(api_key) + def authenticate_api_key(name, key) + user, api_key = User.find_by_name(name)&.authenticate_api_key(key) raise AuthenticationFailure if user.blank? + raise User::PrivilegeError if !api_key.has_permission?(request.remote_ip, request.params[:controller], request.params[:action]) CurrentUser.user = user end diff --git a/app/models/api_key.rb b/app/models/api_key.rb index 61621ac5b..35b507733 100644 --- a/app/models/api_key.rb +++ b/app/models/api_key.rb @@ -1,6 +1,13 @@ class ApiKey < ApplicationRecord + array_attribute :permissions + array_attribute :permitted_ip_addresses + + normalize :permissions, :normalize_permissions + normalize :name, :normalize_text + belongs_to :user - validates_uniqueness_of :key + validate :validate_permissions, if: :permissions_changed? + validates :key, uniqueness: true, if: :key_changed? has_secure_token :key def self.visible(user) @@ -16,4 +23,45 @@ class ApiKey < ApplicationRecord q = q.apply_default_order(params) q end + + concerning :PermissionMethods do + def has_permission?(ip, controller, action) + ip_permitted?(ip) && action_permitted?(controller, action) + end + + def ip_permitted?(ip) + return true if permitted_ip_addresses.empty? + permitted_ip_addresses.any? { |permitted_ip| ip.in?(permitted_ip) } + end + + def action_permitted?(controller, action) + return true if permissions.empty? + + permissions.any? do |permission| + permission == "#{controller}:#{action}" + end + end + + def validate_permissions + permissions.each do |permission| + if !permission.in?(ApiKey.permissions_list) + errors.add(:permissions, "can't allow invalid permission '#{permission}'") + end + end + end + + class_methods do + def normalize_permissions(permissions) + permissions.compact_blank + end + + def permissions_list + Rails.application.routes.routes.select do |route| + route.defaults[:controller].present? && !route.internal + end.map do |route| + "#{route.defaults[:controller]}:#{route.defaults[:action]}" + end.uniq.sort + end + end + end end diff --git a/app/models/user.rb b/app/models/user.rb index a9fcb7707..6a50b10ba 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -209,7 +209,7 @@ class User < ApplicationRecord def authenticate_api_key(key) api_key = api_keys.find_by(key: key) - api_key.present? && ActiveSupport::SecurityUtils.secure_compare(api_key.key, key) && self + api_key.present? && ActiveSupport::SecurityUtils.secure_compare(api_key.key, key) && [self, api_key] end def authenticate_password(password) @@ -560,6 +560,7 @@ class User < ApplicationRecord ] end + # XXX def api_token api_keys.first.try(:key) end diff --git a/app/policies/api_key_policy.rb b/app/policies/api_key_policy.rb index d43083d81..a1cdd690a 100644 --- a/app/policies/api_key_policy.rb +++ b/app/policies/api_key_policy.rb @@ -1,4 +1,8 @@ class ApiKeyPolicy < ApplicationPolicy + def new? + !user.is_anonymous? + end + def create? !user.is_anonymous? end @@ -7,10 +11,22 @@ class ApiKeyPolicy < ApplicationPolicy !user.is_anonymous? end + def edit? + record.user == user + end + + def update? + record.user == user + end + def destroy? record.user == user end + def permitted_attributes + [:name, :permitted_ip_addresses, permissions: []] + end + def api_attributes super - [:key] end diff --git a/app/views/api_keys/_form.html.erb b/app/views/api_keys/_form.html.erb new file mode 100644 index 000000000..ec83e54c4 --- /dev/null +++ b/app/views/api_keys/_form.html.erb @@ -0,0 +1,6 @@ +<%= edit_form_for(api_key, html: { class: "stacked-hints" }) do |f| %> + <%= f.input :name, as: :string, hint: "An optional name to help you remember what this key is for." %> + <%= f.input :permitted_ip_addresses, label: "IP Addresses", as: :string, hint: "An optional list of IPs allowed to use this key. Leave blank to allow all IPs." %> + <%= f.input :permissions, as: :select, collection: ApiKey.permissions_list, hint: "An optional list of API endpoints this key can use. Ctrl+click to select multiple endpoints. Leave blank to allow all API endpoints.", input_html: { multiple: true, size: 10 } %> + <%= f.submit "Create" %> +<% end %> diff --git a/app/views/api_keys/_secondary_links.html.erb b/app/views/api_keys/_secondary_links.html.erb index d98882116..2900a3b73 100644 --- a/app/views/api_keys/_secondary_links.html.erb +++ b/app/views/api_keys/_secondary_links.html.erb @@ -1,5 +1,5 @@ <% content_for(:secondary_links) do %> <%= subnav_link_to "Listing", user_api_keys_path(CurrentUser.user.id) %> - <%= subnav_link_to "New", user_api_keys_path(CurrentUser.user.id), method: :post %> + <%= subnav_link_to "New", new_user_api_key_path(CurrentUser.user.id) %> <%= subnav_link_to "Help", wiki_page_path("help:api") %> <% end %> diff --git a/app/views/api_keys/edit.html.erb b/app/views/api_keys/edit.html.erb new file mode 100644 index 000000000..1bf651027 --- /dev/null +++ b/app/views/api_keys/edit.html.erb @@ -0,0 +1,8 @@ +<%= render "secondary_links" %> + +
+
+

Edit API Key

+ <%= render "form", api_key: @api_key %> +
+
diff --git a/app/views/api_keys/index.html.erb b/app/views/api_keys/index.html.erb index 73b1c4ee6..2167be418 100644 --- a/app/views/api_keys/index.html.erb +++ b/app/views/api_keys/index.html.erb @@ -5,7 +5,7 @@

API Keys

- <%= link_to user_api_keys_path(CurrentUser.user.id), class: "button-primary", method: :post do %> + <%= link_to new_user_api_key_path(CurrentUser.user.id), class: "button-primary" do %> <%= plus_icon %> Add <% end %>
@@ -16,7 +16,7 @@

If you're a developer, you can use an API key to access the <%= link_to_wiki "#{Danbooru.config.canonical_app_name} API", "help:api" %>. If you're not a - developer, you probably don't need an API key.

+ developer, and you're not using any third-party apps, then you probably don't need an API key.

Your API key is like your password. Anyone who has it has full access to your account. Don't give your API key to apps or people you don't trust, and don't post your @@ -37,11 +37,20 @@ <% end %> <% if params[:user_id].present? && !@api_keys.present? %> - <%= link_to "Create API key", user_api_keys_path(CurrentUser.user.id), method: :post %> + <%= link_to "Create API key", new_user_api_key_path(CurrentUser.user.id) %> <% else %> <%= table_for @api_keys, width: "100%", class: "striped autofit" do |t| %> + <% t.column :name %> <% t.column :key, td: { class: "col-expand" } %> + <% t.column :permissions do |api_key| %> + <%= safe_join(api_key.permissions, "
".html_safe).presence || "All" %> + <% end %> + + <% t.column "IPs" do |api_key| %> + <%= safe_join(api_key.permitted_ip_addresses, "
".html_safe).presence || "All" %> + <% end %> + <% if !params[:user_id].present? %> <% t.column "User" do |api_key| %> <%= link_to_user api_key.user %> @@ -53,7 +62,8 @@ <% end %> <% t.column column: "control" do |api_key| %> - <%= link_to "Delete", api_key, method: :delete %> + <%= link_to "Edit", edit_api_key_path(api_key) %> + | <%= link_to "Delete", api_key, method: :delete %> <% end %> <% end %> diff --git a/app/views/api_keys/new.html.erb b/app/views/api_keys/new.html.erb new file mode 100644 index 000000000..0ed5593d7 --- /dev/null +++ b/app/views/api_keys/new.html.erb @@ -0,0 +1,8 @@ +<%= render "secondary_links" %> + +

+
+

New API Key

+ <%= render "form", api_key: @api_key %> +
+
diff --git a/config/routes.rb b/config/routes.rb index 4812a498d..eefc84a2f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -56,7 +56,7 @@ Rails.application.routes.draw do end end - resources :api_keys, only: [:create, :index, :destroy] + resources :api_keys, only: [:new, :create, :edit, :update, :index, :destroy] resources :artists do member do @@ -246,7 +246,7 @@ Rails.application.routes.draw do post :send_confirmation end resource :password, only: [:edit, :update] - resources :api_keys, only: [:create, :index, :destroy] + resources :api_keys, only: [:new, :create, :edit, :update, :index, :destroy] collection do get :custom_style diff --git a/db/migrate/20210214101614_add_fields_to_api_keys.rb b/db/migrate/20210214101614_add_fields_to_api_keys.rb new file mode 100644 index 000000000..f076f546d --- /dev/null +++ b/db/migrate/20210214101614_add_fields_to_api_keys.rb @@ -0,0 +1,12 @@ +class AddFieldsToApiKeys < ActiveRecord::Migration[6.1] + def change + change_table :api_keys do |t| + t.string :name, null: false, default: "" + t.string :permissions, array: true, null: false, default: "{}" + t.inet :permitted_ip_addresses, array: true, null: false, default: "{}" + t.integer :uses, null: false, default: 0 + t.timestamp :last_used_at, null: true + t.inet :last_ip_address, null: true + end + end +end diff --git a/db/structure.sql b/db/structure.sql index a82f535b0..e7549407b 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -454,7 +454,13 @@ CREATE TABLE public.api_keys ( user_id integer NOT NULL, key character varying NOT NULL, created_at timestamp without time zone NOT NULL, - updated_at timestamp without time zone NOT NULL + updated_at timestamp without time zone NOT NULL, + name character varying DEFAULT ''::character varying NOT NULL, + permissions character varying[] DEFAULT '{}'::character varying[] NOT NULL, + permitted_ip_addresses inet[] DEFAULT '{}'::inet[] NOT NULL, + uses integer DEFAULT 0 NOT NULL, + last_used_at timestamp without time zone, + last_ip_address inet ); @@ -7957,6 +7963,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20210123112752'), ('20210127000201'), ('20210127012303'), -('20210214095121'); +('20210214095121'), +('20210214101614'); diff --git a/test/factories/api_key.rb b/test/factories/api_key.rb index 6df1623fd..21ea55c39 100644 --- a/test/factories/api_key.rb +++ b/test/factories/api_key.rb @@ -1,5 +1,6 @@ FactoryBot.define do factory(:api_key) do user + name { FFaker::Name.first_name } end end diff --git a/test/functional/api_keys_controller_test.rb b/test/functional/api_keys_controller_test.rb index b433be149..730fe5a9e 100644 --- a/test/functional/api_keys_controller_test.rb +++ b/test/functional/api_keys_controller_test.rb @@ -4,13 +4,10 @@ class ApiKeysControllerTest < ActionDispatch::IntegrationTest context "An api keys controller" do setup do @user = create(:user) + @api_key = create(:api_key, user: @user) end context "#index action" do - setup do - @api_key = create(:api_key, user: @user) - end - should "let a user see their own API keys" do get_auth user_api_keys_path(@user.id), @user @@ -40,20 +37,54 @@ class ApiKeysControllerTest < ActionDispatch::IntegrationTest end end + context "#new action" do + should "render for a Member user" do + get_auth new_user_api_key_path(@user.id), @user + assert_response :success + end + + should "fail for an Anonymous user" do + get new_user_api_key_path(@user.id) + assert_response 403 + end + end + context "#create action" do should "create a new API key" do - post_auth user_api_keys_path(@user.id), @user + post_auth user_api_keys_path(@user.id), @user, params: { api_key: { name: "blah" }} assert_redirected_to user_api_keys_path(@user.id) - assert_equal(true, @user.api_keys.last.present?) + assert_equal("blah", @user.api_keys.last.name) + end + end + + context "#edit action" do + should "render for the API key owner" do + get_auth edit_api_key_path(@api_key.id), @user + assert_response :success + end + + should "fail for someone else" do + get_auth edit_api_key_path(@api_key.id), create(:user) + assert_response 403 + end + end + + context "#update action" do + should "render for the API key owner" do + put_auth api_key_path(@api_key.id), @user, params: { api_key: { name: "blah" }} + + assert_redirected_to user_api_keys_path(@user.id) + assert_equal("blah", @api_key.reload.name) + end + + should "fail for someone else" do + put_auth api_key_path(@api_key.id), create(:user) + assert_response 403 end end context "#destroy" do - setup do - @api_key = create(:api_key, user: @user) - end - should "delete the user's API key" do delete_auth api_key_path(@api_key.id), @user diff --git a/test/functional/application_controller_test.rb b/test/functional/application_controller_test.rb index 8b39e994c..3c9de79e0 100644 --- a/test/functional/application_controller_test.rb +++ b/test/functional/application_controller_test.rb @@ -108,6 +108,53 @@ class ApplicationControllerTest < ActionDispatch::IntegrationTest end end + context "for an API key with restrictions" do + should "restrict requests to the permitted IP addresses" do + @api_key = create(:api_key, permitted_ip_addresses: ["192.168.0.1", "10.0.0.1/24", "2600::1/64"]) + + ActionDispatch::Request.any_instance.stubs(:remote_ip).returns("192.168.0.1") + get posts_path, params: { login: @api_key.user.name, api_key: @api_key.key } + assert_response :success + + ActionDispatch::Request.any_instance.stubs(:remote_ip).returns("10.0.0.42") + get posts_path, params: { login: @api_key.user.name, api_key: @api_key.key } + assert_response :success + + ActionDispatch::Request.any_instance.stubs(:remote_ip).returns("2600::1234:0:0:1") + get posts_path, params: { login: @api_key.user.name, api_key: @api_key.key } + assert_response :success + + ActionDispatch::Request.any_instance.stubs(:remote_ip).returns("127.0.0.2") + get posts_path, params: { login: @api_key.user.name, api_key: @api_key.key } + assert_response 403 + + ActionDispatch::Request.any_instance.stubs(:remote_ip).returns("10.0.1.0") + get posts_path, params: { login: @api_key.user.name, api_key: @api_key.key } + assert_response 403 + + ActionDispatch::Request.any_instance.stubs(:remote_ip).returns("2600:dead:beef::1") + get posts_path, params: { login: @api_key.user.name, api_key: @api_key.key } + assert_response 403 + end + + should "restrict requests to the permitted endpoints" do + @post = create(:post) + @api_key = create(:api_key, permissions: ["posts:index", "posts:show"]) + + get posts_path, params: { login: @api_key.user.name, api_key: @api_key.key } + assert_response :success + + get post_path(@post), params: { login: @api_key.user.name, api_key: @api_key.key } + assert_response :success + + get tags_path, params: { login: @api_key.user.name, api_key: @api_key.key } + assert_response 403 + + put post_path(@post), params: { login: @api_key.user.name, api_key: @api_key.key, post: { rating: "s" }} + assert_response 403 + end + end + context "with cookie-based authentication" do should "not allow non-GET requests without a CSRF token" do # get the csrf token from the login page so we can login diff --git a/test/unit/api_key_test.rb b/test/unit/api_key_test.rb index 6122114e9..889c99d78 100644 --- a/test/unit/api_key_test.rb +++ b/test/unit/api_key_test.rb @@ -1,18 +1,44 @@ require 'test_helper' class ApiKeyTest < ActiveSupport::TestCase - context "in all cases a user" do + context "ApiKey:" do setup do @user = create(:user) @api_key = create(:api_key, user: @user) end + context "During validation" do + subject { build(:api_key) } + + context "of permissions" do + should allow_value([]).for(:permissions) + should allow_value(["posts:index"]).for(:permissions) + should allow_value(["posts:index", "posts:show"]).for(:permissions) + + should_not allow_value(["blah"]).for(:permissions) + should_not allow_value(["posts:blah"]).for(:permissions) + should_not allow_value(["blah:index"]).for(:permissions) + end + + context "of IP addresses" do + should allow_value([]).for(:permitted_ip_addresses) + should allow_value(["1.2.3.4"]).for(:permitted_ip_addresses) + should allow_value(["1.2.3.4/24"]).for(:permitted_ip_addresses) + should allow_value(["1.2.3.4/24 4.5.6.7/24"]).for(:permitted_ip_addresses) + should allow_value(["0.0.0.0/0"]).for(:permitted_ip_addresses) + should allow_value(["2600::1/64"]).for(:permitted_ip_addresses) + + #should_not allow_value(["blah"]).for(:permitted_ip_addresses) + #should_not allow_value(["1.2.3.4/64"]).for(:permitted_ip_addresses) + end + end + should "generate a unique key" do assert_not_nil(@api_key.key) end should "authenticate via api key" do - assert_equal(@user, @user.authenticate_api_key(@api_key.key)) + assert_equal([@user, @api_key], @user.authenticate_api_key(@api_key.key)) end should "not authenticate with the wrong api key" do