diff --git a/app/assets/javascripts/admin/adapters/api-key.js b/app/assets/javascripts/admin/adapters/api-key.js index a473f66e085..9777518ba22 100644 --- a/app/assets/javascripts/admin/adapters/api-key.js +++ b/app/assets/javascripts/admin/adapters/api-key.js @@ -1,6 +1,8 @@ import RESTAdapter from "discourse/adapters/rest"; export default RESTAdapter.extend({ + jsonMode: true, + basePath() { return "/admin/api/"; }, diff --git a/app/assets/javascripts/admin/controllers/admin-api-keys-new.js b/app/assets/javascripts/admin/controllers/admin-api-keys-new.js index eaddcfcd825..bd9c4463485 100644 --- a/app/assets/javascripts/admin/controllers/admin-api-keys-new.js +++ b/app/assets/javascripts/admin/controllers/admin-api-keys-new.js @@ -9,6 +9,8 @@ export default Controller.extend({ { id: "all", name: I18n.t("admin.api.all_users") }, { id: "single", name: I18n.t("admin.api.single_user") } ], + useGlobalKey: false, + scopes: null, @discourseComputed("userMode") showUserSelector(mode) { @@ -31,6 +33,16 @@ export default Controller.extend({ }, save() { + if (!this.useGlobalKey) { + const selectedScopes = Object.values(this.scopes) + .flat() + .filter(action => { + return action.selected; + }); + + this.model.set("scopes", selectedScopes); + } + this.model.save().catch(popupAjaxError); }, diff --git a/app/assets/javascripts/admin/models/api-key.js b/app/assets/javascripts/admin/models/api-key.js index 06d861fa4a3..e5f9724feef 100644 --- a/app/assets/javascripts/admin/models/api-key.js +++ b/app/assets/javascripts/admin/models/api-key.js @@ -41,7 +41,7 @@ const ApiKey = RestModel.extend({ }, createProperties() { - return this.getProperties("description", "username"); + return this.getProperties("description", "username", "scopes"); }, @discourseComputed() diff --git a/app/assets/javascripts/admin/routes/admin-api-keys-new.js b/app/assets/javascripts/admin/routes/admin-api-keys-new.js index 3969615c9fe..63b248cd1a7 100644 --- a/app/assets/javascripts/admin/routes/admin-api-keys-new.js +++ b/app/assets/javascripts/admin/routes/admin-api-keys-new.js @@ -1,7 +1,17 @@ import Route from "@ember/routing/route"; +import { ajax } from "discourse/lib/ajax"; export default Route.extend({ model() { return this.store.createRecord("api-key"); + }, + + setupController(controller, model) { + ajax("/admin/api/keys/scopes.json").then(data => { + controller.setProperties({ + scopes: data.scopes, + model + }); + }); } }); diff --git a/app/assets/javascripts/admin/templates/api-keys-new.hbs b/app/assets/javascripts/admin/templates/api-keys-new.hbs index d48da805e51..28095418a98 100644 --- a/app/assets/javascripts/admin/templates/api-keys-new.hbs +++ b/app/assets/javascripts/admin/templates/api-keys-new.hbs @@ -31,8 +31,40 @@ }} {{/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 label="admin.api.use_global_key"}} + {{input type="checkbox" checked=useGlobalKey}} {{/admin-form-row}} + + {{#unless useGlobalKey}} + {{#each-in scopes as |resource actions|}} + + + + + + + + + + {{#each actions as |act|}} + + + + + + {{/each}} + +
{{resource}}{{i18n "admin.api.scopes.optional_allowed_parameters"}}
{{input type="checkbox" checked=act.selected}}{{act.name}} + {{#each act.params as |p|}} +
+ {{input maxlength="255" value=(get act p) placeholder=p}} +
+ {{/each}} +
+ {{/each-in}} + {{/unless}} + + {{d-button icon="check" label="admin.api.save" action=(action "save") class="btn-primary" disabled=saveDisabled}} {{/if}} diff --git a/app/assets/javascripts/admin/templates/api-keys-show.hbs b/app/assets/javascripts/admin/templates/api-keys-show.hbs index d1d35da0175..2ffd5c4e025 100644 --- a/app/assets/javascripts/admin/templates/api-keys-show.hbs +++ b/app/assets/javascripts/admin/templates/api-keys-show.hbs @@ -79,4 +79,39 @@ {{/if}} {{/admin-form-row}} + + {{#if model.api_key_scopes.length}} + {{#admin-form-row label="admin.api.scopes.title"}} + {{/admin-form-row}} + + + + + + + + + + + {{#each model.api_key_scopes as |scope|}} + + + + + + {{/each}} + +
{{i18n "admin.api.scopes.resource"}}{{i18n "admin.api.scopes.action"}}{{i18n "admin.api.scopes.allowed_parameters"}}
{{scope.resource}}{{scope.action}} + {{#each scope.parameters as |p|}} +
+ {{p}}: + {{#if (get scope.allowed_parameters p)}} + {{get scope.allowed_parameters p}} + {{else}} + {{i18n "admin.api.scopes.any_parameter"}} + {{/if}} +
+ {{/each}} +
+ {{/if}} diff --git a/app/assets/stylesheets/common/admin/api.scss b/app/assets/stylesheets/common/admin/api.scss index d2574606d04..4e77cb41855 100644 --- a/app/assets/stylesheets/common/admin/api.scss +++ b/app/assets/stylesheets/common/admin/api.scss @@ -95,7 +95,6 @@ table.api-keys { .api-key { padding: 10px; margin-bottom: 10px; - border-bottom: 1px solid $primary-low; .form-element, .form-element-desc { float: left; @@ -127,6 +126,9 @@ table.api-keys { width: 50%; } } + .scopes-table { + margin: 20px 0 20px 0; + } } // Webhook diff --git a/app/controllers/admin/api_controller.rb b/app/controllers/admin/api_controller.rb index 42ec4035eb7..d2f8339bf78 100644 --- a/app/controllers/admin/api_controller.rb +++ b/app/controllers/admin/api_controller.rb @@ -18,10 +18,20 @@ class Admin::ApiController < Admin::AdminController end def show - api_key = ApiKey.find_by!(id: params[:id]) + api_key = ApiKey.includes(:api_key_scopes).find_by!(id: params[:id]) render_serialized(api_key, ApiKeySerializer, root: 'key') end + def scopes + scopes = ApiKeyScope.scope_mappings.reduce({}) do |memo, (resource, actions)| + memo.tap do |m| + m[resource] = actions.map { |k, v| { id: "#{resource}:#{k}", name: k, params: v[:params] } } + end + end + + render json: { scopes: scopes } + end + def update api_key = ApiKey.find_by!(id: params[:id]) ApiKey.transaction do @@ -44,6 +54,7 @@ class Admin::ApiController < Admin::AdminController api_key = ApiKey.new(update_params) ApiKey.transaction do api_key.created_by = current_user + api_key.api_key_scopes = build_scopes if username = params.require(:key).permit(:username)[:username].presence api_key.user = User.find_by_username(username) raise Discourse::NotFound unless api_key.user @@ -78,6 +89,31 @@ class Admin::ApiController < Admin::AdminController private + def build_scopes + params.require(:key)[:scopes].to_a.map do |scope_params| + resource, action = scope_params[:id].split(':') + + mapping = ApiKeyScope.scope_mappings.dig(resource.to_sym, action.to_sym) + raise Discourse::InvalidParameters if mapping.nil? # invalid mapping + + ApiKeyScope.new( + resource: resource, + action: action, + allowed_parameters: build_params(scope_params, mapping[:params]) + ) + end + end + + def build_params(scope_params, params) + return if params.nil? + + scope_params.slice(*params).tap do |allowed_params| + allowed_params.each do |k, v| + v.blank? ? allowed_params.delete(k) : allowed_params[k] = v.split(',') + end + end + end + def update_params editable_fields = [:description] permitted_params = params.permit(key: [*editable_fields])[:key] diff --git a/app/models/api_key.rb b/app/models/api_key.rb index 6abc38b548f..a009b8c8c25 100644 --- a/app/models/api_key.rb +++ b/app/models/api_key.rb @@ -3,6 +3,7 @@ class ApiKey < ActiveRecord::Base class KeyAccessError < StandardError; end + has_many :api_key_scopes belongs_to :user belongs_to :created_by, class_name: 'User' @@ -60,6 +61,12 @@ class ApiKey < ActiveRecord::Base def self.hash_key(key) Digest::SHA256.hexdigest key end + + def request_allowed?(request, route_param) + return false if allowed_ips.present? && allowed_ips.none? { |ip| ip.include?(request.ip) } + + api_key_scopes.blank? || api_key_scopes.any? { |s| s.permits?(route_param) } + end end # == Schema Information diff --git a/app/models/api_key_scope.rb b/app/models/api_key_scope.rb new file mode 100644 index 00000000000..66fecc5a0e6 --- /dev/null +++ b/app/models/api_key_scope.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +class ApiKeyScope < ActiveRecord::Base + validates_presence_of :resource + validates_presence_of :action + + class << self + def list_actions + actions = [] + + TopTopic.periods.each do |p| + actions.concat(["list#category_top_#{p}", "list#top_#{p}", "list#top_#{p}_feed"]) + end + + %i[latest unread new top].each { |f| actions.concat(["list#category_#{f}", "list##{f}"]) } + + actions + end + + def default_mappings + { + topics: { + write: { actions: %w[posts#create topics#feed], params: %i[topic_id] }, + read: { actions: %w[topics#show], params: %i[topic_id], aliases: { topic_id: :id } }, + read_lists: { actions: list_actions, params: %i[category_id], aliases: { category_id: :category_slug_path_with_id } } + } + } + end + + def scope_mappings + plugin_mappings = DiscoursePluginRegistry.api_key_scope_mappings + + default_mappings.tap do |mappings| + plugin_mappings.each do |mapping| + mappings.deep_merge!(mapping) + end + end + end + end + + def permits?(route_param) + path_params = "#{route_param['controller']}##{route_param['action']}" + + mapping[:actions].include?(path_params) && (allowed_parameters.blank? || params_allowed?(route_param)) + end + + private + + def params_allowed?(route_param) + mapping[:params].all? do |param| + param_alias = mapping.dig(:aliases, param) + allowed_values = [allowed_parameters[param.to_s]].flatten + value = route_param[param.to_s] + alias_value = route_param[param_alias.to_s] + + return false if value.present? && alias_value.present? + + value = value || alias_value + value = extract_category_id(value) if param_alias == :category_slug_path_with_id + + allowed_values.blank? || allowed_values.include?(value) + end + end + + def mapping + @mapping ||= self.class.scope_mappings.dig(resource.to_sym, action.to_sym) + end + + def extract_category_id(category_slug_with_id) + parts = category_slug_with_id.split('/') + + !parts.empty? && parts.last =~ /\A\d+\Z/ ? parts.pop : nil + end +end + +# == Schema Information +# +# Table name: api_key_scopes +# +# id :bigint not null, primary key +# api_key_id :integer not null +# resource :string not null +# action :string not null +# allowed_parameters :json +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# index_api_key_scopes_on_api_key_id (api_key_id) +# diff --git a/app/serializers/api_key_scope_serializer.rb b/app/serializers/api_key_scope_serializer.rb new file mode 100644 index 00000000000..aa71a727472 --- /dev/null +++ b/app/serializers/api_key_scope_serializer.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class ApiKeyScopeSerializer < ApplicationSerializer + + attributes :resource, + :action, + :parameters, + :allowed_parameters + + def parameters + ApiKeyScope.scope_mappings.dig(object.resource.to_sym, object.action.to_sym, :params).to_a + end +end diff --git a/app/serializers/api_key_serializer.rb b/app/serializers/api_key_serializer.rb index 310b210a01d..ab2a7a2e4d5 100644 --- a/app/serializers/api_key_serializer.rb +++ b/app/serializers/api_key_serializer.rb @@ -12,6 +12,7 @@ class ApiKeySerializer < ApplicationSerializer :revoked_at has_one :user, serializer: BasicUserSerializer, embed: :objects + has_many :api_key_scopes, serializer: ApiKeyScopeSerializer, embed: :objects def include_user_id? !object.user_id.nil? diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index d83831e1ce6..66bb5492f45 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -3610,6 +3610,15 @@ en: delete: Permenantly Delete not_shown_again: This key will not be displayed again. Make sure you take a copy before continuing. continue: Continue + use_global_key: Global Key (allows all actions) + scopes: + title: Scopes + resource: Resource + action: Action + allowed_parameters: Allowed Parameters + optional_allowed_parameters: Allowed Parameters (optional) + any_parameter: (any parameter) + web_hooks: title: "Webhooks" diff --git a/config/routes.rb b/config/routes.rb index 2848255f58d..a6c8b81c953 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -269,6 +269,10 @@ Discourse::Application.routes.draw do resources :api, only: [:index], constraints: AdminConstraint.new do collection do resources :keys, controller: 'api', only: [:index, :show, :update, :create, :destroy] do + collection do + get 'scopes' => 'api#scopes' + end + member do post "revoke" => "api#revoke_key" post "undo-revoke" => "api#undo_revoke_key" diff --git a/db/migrate/20200520124359_add_api_key_scopes.rb b/db/migrate/20200520124359_add_api_key_scopes.rb new file mode 100644 index 00000000000..33c9d21285f --- /dev/null +++ b/db/migrate/20200520124359_add_api_key_scopes.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class AddApiKeyScopes < ActiveRecord::Migration[6.0] + def change + create_table :api_key_scopes do |t| + t.integer :api_key_id, null: false + t.string :resource, null: false + t.string :action, null: false + t.json :allowed_parameters + t.timestamps + end + + add_index :api_key_scopes, :api_key_id + end +end diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index f79728de188..b97b866e371 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/lib/auth/default_current_user_provider.rb @@ -330,7 +330,8 @@ class Auth::DefaultCurrentUserProvider 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] - if api_key.allowed_ips.present? && !api_key.allowed_ips.any? { |ip| ip.include?(request.ip) } + params = @env['action_dispatch.request.parameters'] + unless api_key.request_allowed?(request, params) Rails.logger.warn("[Unauthorized API Access] username: #{api_username}, IP address: #{request.ip}") return nil end diff --git a/lib/discourse_plugin_registry.rb b/lib/discourse_plugin_registry.rb index 73838496614..8beb5c33123 100644 --- a/lib/discourse_plugin_registry.rb +++ b/lib/discourse_plugin_registry.rb @@ -78,6 +78,8 @@ class DiscoursePluginRegistry define_filtered_register :topic_thumbnail_sizes + define_filtered_register :api_key_scope_mappings + def self.register_auth_provider(auth_provider) self.auth_providers << auth_provider end diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb index fcbe75c58d2..2a9b4c8245f 100644 --- a/lib/plugin/instance.rb +++ b/lib/plugin/instance.rb @@ -738,6 +738,10 @@ class Plugin::Instance end end + def add_api_key_scope(resource, action) + DiscoursePluginRegistry.register_api_key_scope_mapping({ resource => action }, self) + end + protected def self.js_path diff --git a/spec/models/api_key_spec.rb b/spec/models/api_key_spec.rb index c842f5421cc..c8d5348a1c9 100644 --- a/spec/models/api_key_spec.rb +++ b/spec/models/api_key_spec.rb @@ -79,4 +79,74 @@ describe ApiKey do expect(used_recently.revoked_at).to eq(nil) end + describe 'API Key scope mappings' do + it 'maps api_key permissions' do + api_key_mappings = ApiKeyScope.scope_mappings[:topics] + + assert_responds_to(api_key_mappings.dig(:write, :actions)) + assert_responds_to(api_key_mappings.dig(:read, :actions)) + assert_responds_to(api_key_mappings.dig(:read_lists, :actions)) + end + + def assert_responds_to(mappings) + mappings.each do |m| + controller, method = m.split('#') + controller_name = "#{controller.capitalize}Controller" + expect(controller_name.constantize.method_defined?(method)).to eq(true) + end + end + end + + describe "#request_allowed?" do + let(:request_mock) { OpenStruct.new(ip: '133.45.67.99') } + let(:route_path) { { 'controller' => 'topics', 'action' => 'show', 'topic_id' => '3' } } + let(:scope) do + ApiKeyScope.new(resource: 'topics', action: 'read', allowed_parameters: { topic_id: '3' }) + end + let(:key) { ApiKey.new(api_key_scopes: [scope]) } + + it 'allows the request if there are no allowed IPs' do + key.allowed_ips = nil + key.api_key_scopes = [] + + expect(key.request_allowed?(request_mock, route_path)).to eq(true) + end + + it 'rejects the request if the IP is not allowed' do + key.allowed_ips = %w[115.65.76.87] + + expect(key.request_allowed?(request_mock, route_path)).to eq(false) + end + + it 'allow the request if there are not allowed params' do + scope.allowed_parameters = nil + + expect(key.request_allowed?(request_mock, route_path)).to eq(true) + end + + it 'rejects the request when params are different' do + route_path['topic_id'] = '4' + + expect(key.request_allowed?(request_mock, route_path)).to eq(false) + end + + it 'accepts the request if one of the parameters match' do + route_path['topic_id'] = '4' + scope.allowed_parameters = { topic_id: %w[3 4] } + + expect(key.request_allowed?(request_mock, route_path)).to eq(true) + end + + it 'allow the request when the scope has an alias' do + route_path = { 'controller' => 'topics', 'action' => 'show', 'id' => '3' } + + expect(key.request_allowed?(request_mock, route_path)).to eq(true) + end + + it 'rejects the request when the main parameter and the alias are both used' do + route_path = { 'controller' => 'topics', 'action' => 'show', 'topic_id' => '3', 'id' => '4' } + + expect(key.request_allowed?(request_mock, route_path)).to eq(false) + end + end end diff --git a/spec/requests/admin/api_controller_spec.rb b/spec/requests/admin/api_controller_spec.rb index 029801dbcfc..d7dc8c68eae 100644 --- a/spec/requests/admin/api_controller_spec.rb +++ b/spec/requests/admin/api_controller_spec.rb @@ -131,6 +131,67 @@ describe Admin::ApiController do expect(UserHistory.last.action).to eq(UserHistory.actions[:api_key_create]) expect(UserHistory.last.subject).to eq(key.truncated_key) end + + describe 'Scopes' do + it 'creates an scope with allowed parameters' do + post "/admin/api/keys.json", params: { + key: { + description: "master key description", + scopes: [{ id: 'topics:write', topic_id: '55' }] + } + } + expect(response.status).to eq(200) + + data = response.parsed_body + scope = ApiKeyScope.find_by(api_key_id: data.dig('key', 'id')) + + expect(scope.resource).to eq('topics') + expect(scope.action).to eq('write') + expect(scope.allowed_parameters['topic_id']).to contain_exactly('55') + end + + it 'allows multiple parameters separated by a comma' do + post "/admin/api/keys.json", params: { + key: { + description: "master key description", + scopes: [{ id: 'topics:write', topic_id: '55,33' }] + } + } + expect(response.status).to eq(200) + + data = response.parsed_body + scope = ApiKeyScope.find_by(api_key_id: data.dig('key', 'id')) + + expect(scope.allowed_parameters['topic_id']).to contain_exactly('55', '33') + end + end + + it 'ignores invalid parameters' do + post "/admin/api/keys.json", params: { + key: { + description: "master key description", + scopes: [{ id: 'topics:write', fake_id: '55' }] + } + } + + expect(response.status).to eq(200) + + data = response.parsed_body + scope = ApiKeyScope.find_by(api_key_id: data.dig('key', 'id')) + + expect(scope.allowed_parameters['fake_id']).to be_nil + end + + it 'fails when the scope is invalid' do + post "/admin/api/keys.json", params: { + key: { + description: "master key description", + scopes: [{ id: 'something:else' }] + } + } + + expect(response.status).to eq(400) + end end describe "#revoke and #undo_revoke" do @@ -154,6 +215,16 @@ describe Admin::ApiController do expect(UserHistory.last.details).to eq(I18n.t("staff_action_logs.api_key.restored")) end end + + describe '#scopes' do + it 'includes scopes' do + get '/admin/api/keys/scopes.json' + + scopes = response.parsed_body['scopes'] + + expect(scopes.keys).to contain_exactly('topics') + end + end end context "as a moderator" do