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

@@ -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

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

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 {

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)
+  def authenticate_api_key(name, key)
-    user = User.find_by_name(name)&.authenticate_api_key(api_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
 

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

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

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

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 %>

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 %>

app/views/api_keys/edit.html.erb

@@ -0,0 +1,8 @@
+<%= render "secondary_links" %>
+
+<div id="c-api-keys">
+  <div id="a-edit">
+    <h1>Edit API Key</h1>
+    <%= render "form", api_key: @api_key %>
+  </div>
+</div>

app/views/api_keys/index.html.erb

@@ -5,7 +5,7 @@
     <div class="page-heading">
       <h1>API Keys</h1>
 
-      <%= 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 %>
     </div>
@@ -16,7 +16,7 @@
 
         <p>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.</p>
+        developer, and you're not using any third-party apps, then you probably don't need an API key.</p>
 
         <p><strong>Your API key is like your password</strong>. 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, "<br>".html_safe).presence || "All" %>
+        <% end %>
+
+        <% t.column "IPs" do |api_key| %>
+          <%= safe_join(api_key.permitted_ip_addresses, "<br>".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 %>
 

app/views/api_keys/new.html.erb

@@ -0,0 +1,8 @@
+<%= render "secondary_links" %>
+
+<div id="c-api-keys">
+  <div id="a-new">
+    <h1>New API Key</h1>
+    <%= render "form", api_key: @api_key %>
+  </div>
+</div>

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

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

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');
 
 

test/factories/api_key.rb

@@ -1,5 +1,6 @@
 FactoryBot.define do
   factory(:api_key) do
     user
+    name { FFaker::Name.first_name }
   end
 end

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)
-    end
-
-    context "#index action" do
-      setup do
       @api_key = create(:api_key, user: @user)
     end
 
+    context "#index action" do
       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
 

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

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