diff --git a/app/models/api_key.rb b/app/models/api_key.rb index a009b8c8c25..b55b496cee7 100644 --- a/app/models/api_key.rb +++ b/app/models/api_key.rb @@ -62,10 +62,10 @@ class ApiKey < ActiveRecord::Base 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) } + def request_allowed?(env) + return false if allowed_ips.present? && allowed_ips.none? { |ip| ip.include?(Rack::Request.new(env).ip) } - api_key_scopes.blank? || api_key_scopes.any? { |s| s.permits?(route_param) } + api_key_scopes.blank? || api_key_scopes.any? { |s| s.permits?(env) } end end diff --git a/app/models/api_key_scope.rb b/app/models/api_key_scope.rb index b66bb3b9f20..47582378fdf 100644 --- a/app/models/api_key_scope.rb +++ b/app/models/api_key_scope.rb @@ -77,39 +77,15 @@ class ApiKeyScope < ActiveRecord::Base 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)) + def permits?(env) + RouteMatcher.new(**mapping.except(:urls), allowed_param_values: allowed_parameters).match?(env: env) 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 diff --git a/app/models/user_api_key.rb b/app/models/user_api_key.rb index 951add6d7ac..45b87369f18 100644 --- a/app/models/user_api_key.rb +++ b/app/models/user_api_key.rb @@ -5,21 +5,7 @@ class UserApiKey < ActiveRecord::Base "scopes" # TODO(2020-12-18): remove ] - SCOPES = { - read: [:get], - write: [:get, :post, :patch, :put, :delete], - message_bus: [[:post, 'message_bus']], - push: nil, - one_time_password: nil, - notifications: [[:post, 'message_bus'], [:get, 'notifications#index'], [:put, 'notifications#mark_read']], - session_info: [ - [:get, 'session#current'], - [:get, 'users#topic_tracking_state'], - [:get, 'list#unread'], - [:get, 'list#new'], - [:get, 'list#latest'] - ] - } + REVOKE_MATCHER = RouteMatcher.new(actions: "user_api_keys#revoke", methods: :post, params: [:id]) belongs_to :user has_many :scopes, class_name: "UserApiKeyScope", dependent: :destroy @@ -51,35 +37,7 @@ class UserApiKey < ActiveRecord::Base end def self.available_scopes - @available_scopes ||= Set.new(SCOPES.keys.map(&:to_s)) - end - - def self.allow_permission?(permission, env) - verb, action = permission - actual_verb = env["REQUEST_METHOD"] || "" - - return false unless actual_verb.downcase == verb.to_s - return true unless action - - # not a rails route, special handling - return true if action == "message_bus" && env["PATH_INFO"] =~ /^\/message-bus\/.*\/poll/ - - params = env['action_dispatch.request.path_parameters'] - - return false unless params - - actual_action = "#{params[:controller]}##{params[:action]}" - actual_action == action - end - - def self.allow_scope?(name, env) - if allowed = SCOPES[name.to_sym] - good = allowed.any? do |permission| - allow_permission?(permission, env) - end - - good || allow_permission?([:post, 'user_api_keys#revoke'], env) - end + @available_scopes ||= Set.new(UserApiKeyScopes.all_scopes.keys.map(&:to_s)) end def has_push? @@ -89,9 +47,7 @@ class UserApiKey < ActiveRecord::Base end def allow?(env) - scopes.any? do |s| - UserApiKey.allow_scope?(s.name, env) - end || is_revoke_self_request?(env) + scopes.any? { |s| s.permits?(env) } || is_revoke_self_request?(env) end def self.invalid_auth_redirect?(auth_redirect) @@ -102,8 +58,12 @@ class UserApiKey < ActiveRecord::Base private + def revoke_self_matcher + REVOKE_MATCHER.with_allowed_param_values({ "id" => [nil, id.to_s] }) + end + def is_revoke_self_request?(env) - UserApiKey.allow_permission?([:post, 'user_api_keys#revoke'], env) && (env[:id].nil? || env[:id].to_i == id) + revoke_self_matcher.match?(env: env) end end diff --git a/app/models/user_api_key_scope.rb b/app/models/user_api_key_scope.rb index bae6a928005..c4f99975009 100644 --- a/app/models/user_api_key_scope.rb +++ b/app/models/user_api_key_scope.rb @@ -1,6 +1,34 @@ # frozen_string_literal: true class UserApiKeyScope < ActiveRecord::Base + SCOPES = { + read: [ RouteMatcher.new(methods: :get) ], + write: [ RouteMatcher.new(methods: [:get, :post, :patch, :put, :delete]) ], + message_bus: [ RouteMatcher.new(methods: :post, actions: 'message_bus') ], + push: [], + one_time_password: [], + notifications: [ + RouteMatcher.new(methods: :post, actions: 'message_bus'), + RouteMatcher.new(methods: :get, actions: 'notifications#index'), + RouteMatcher.new(methods: :put, actions: 'notifications#mark_read') + ], + session_info: [ RouteMatcher.new(methods: :get, actions: 'session#current') ] + } + + def self.all_scopes + SCOPES + end + + def permits?(env) + matchers.any? { |m| m.match?(env: env) } + end + + private + + def matchers + @matchers ||= Array(self.class.all_scopes[name.to_sym]) + end + end # == Schema Information diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index 010a7f14d9d..884b000d4c8 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/lib/auth/default_current_user_provider.rb @@ -1,4 +1,5 @@ # frozen_string_literal: true +require_relative '../route_matcher' class Auth::DefaultCurrentUserProvider @@ -20,9 +21,9 @@ class Auth::DefaultCurrentUserProvider BAD_TOKEN ||= "_DISCOURSE_BAD_TOKEN" PARAMETER_API_PATTERNS ||= [ - { - method: :get, - route: [ + RouteMatcher.new( + methods: :get, + actions: [ "posts#latest", "posts#user_posts_feed", "groups#posts_feed", @@ -37,18 +38,18 @@ class Auth::DefaultCurrentUserProvider *[:all, :yearly, :quarterly, :monthly, :weekly, :daily].map { |p| "list#top_#{p}_feed" }, *[:latest, :unread, :new, :read, :posted, :bookmarks].map { |f| "tags#show_#{f}" } ], - format: :rss - }, - { - method: :get, - route: "users#bookmarks", - format: :ics - }, - { - method: :post, - route: "admin/email#handle_mail", - format: "*" - } + formats: :rss + ), + RouteMatcher.new( + methods: :get, + actions: "users#bookmarks", + formats: :ics + ), + RouteMatcher.new( + methods: :post, + actions: "admin/email#handle_mail", + formats: nil + ) ] # do all current user initialization here @@ -335,8 +336,7 @@ 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] - params = @env['action_dispatch.request.parameters'] - unless api_key.request_allowed?(request, params) + unless api_key.request_allowed?(@env) Rails.logger.warn("[Unauthorized API Access] username: #{api_username}, IP address: #{request.ip}") return nil end @@ -370,17 +370,7 @@ class Auth::DefaultCurrentUserProvider # However, in some scenarios it is essential to send them via url parameters # so we need to add some exceptions def api_parameter_allowed? - request_method = @env["REQUEST_METHOD"]&.downcase&.to_sym - request_format = @env['action_dispatch.request.formats']&.first&.symbol - - path_params = @env['action_dispatch.request.path_parameters'] - request_route = "#{path_params[:controller]}##{path_params[:action]}" if path_params - - parameter_api_patterns.any? do |p| - (p[:method] == "*" || Array(p[:method]).include?(request_method)) && - (p[:format] == "*" || Array(p[:format]).include?(request_format)) && - (p[:route] == "*" || Array(p[:route]).include?(request_route)) - end + parameter_api_patterns.any? { |p| p.match?(env: @env) } end def header_api_key? diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb index 882c6184419..6745cbf22e7 100644 --- a/lib/plugin/instance.rb +++ b/lib/plugin/instance.rb @@ -797,15 +797,37 @@ class Plugin::Instance # in a query parameter rather than a header. For example: # # add_api_parameter_route( - # method: :get, - # route: "users#bookmarks", - # format: :ics + # methods: :get, + # actions: "users#bookmarks", + # formats: :ics # ) # # See Auth::DefaultCurrentUserProvider::PARAMETER_API_PATTERNS for more examples # and Auth::DefaultCurrentUserProvider#api_parameter_allowed? for implementation - def add_api_parameter_route(method:, route:, format:) - DiscoursePluginRegistry.register_api_parameter_route({ method: method, route: route, format: format }, self) + def add_api_parameter_route(method: nil, methods: nil, + route: nil, actions: nil, + format: nil, formats: nil) + + if Array(format).include?("*") + Discourse.deprecate("* is no longer a valid api_parameter_route format matcher. Use `nil` instead", drop_from: "2.7") + # Old API used * as wildcard. New api uses `nil` + format = nil + end + + # Backwards compatibility with old parameter names: + if method || route || format + Discourse.deprecate("method, route and format parameters for api_parameter_routes are deprecated. Use methods, actions and formats instead.", drop_from: "2.7") + methods ||= method + actions ||= route + formats ||= format + end + + DiscoursePluginRegistry.register_api_parameter_route( + RouteMatcher.new( + methods: methods, + actions: actions, + formats: formats + ), self) end protected diff --git a/lib/route_matcher.rb b/lib/route_matcher.rb new file mode 100644 index 00000000000..4571373f875 --- /dev/null +++ b/lib/route_matcher.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +class RouteMatcher + attr_reader :actions, :params, :methods, :aliases, :formats, :allowed_param_values + + def initialize(actions: nil, params: nil, methods: nil, formats: nil, aliases: nil, allowed_param_values: nil) + @actions = Array(actions) if actions + @params = Array(params) if params + @methods = Array(methods) if methods + @formats = Array(formats) if formats + @aliases = aliases + @allowed_param_values = allowed_param_values + end + + # Return an identical route matcher, with the allowed_param_values replaced + def with_allowed_param_values(new_allowed_param_values) + RouteMatcher.new( + actions: actions, + params: params, + methods: methods, + formats: formats, + aliases: aliases, + allowed_param_values: new_allowed_param_values + ) + end + + def match?(env:) + request = ActionDispatch::Request.new(env) + + action_allowed?(request) && + params_allowed?(request) && + method_allowed?(request) && + format_allowed?(request) + end + + private + + def action_allowed?(request) + return true if actions.nil? # actions are unrestricted + path_params = request.path_parameters + + # message_bus is not a rails route, special handling + return true if actions.include?("message_bus") && request.fullpath =~ /^\/message-bus\/.*\/poll/ + + actions.include? "#{path_params[:controller]}##{path_params[:action]}" + end + + def params_allowed?(request) + return true if params.nil? || allowed_param_values.blank? # params are unrestricted + + requested_params = request.parameters + + params.all? do |param| + param_alias = aliases&.[](param) + allowed_values = [allowed_param_values[param.to_s]].flatten + + value = requested_params[param.to_s] + alias_value = requested_params[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 extract_category_id(category_slug_with_id) + parts = category_slug_with_id.split('/') + !parts.empty? && parts.last =~ /\A\d+\Z/ ? parts.pop : nil + end + + def method_allowed?(request) + return true if methods.nil? + request_method = request.request_method&.downcase&.to_sym + methods.include?(request_method) + end + + def format_allowed?(request) + return true if formats.nil? + request_format = request.formats&.first&.symbol + formats.include?(request_format) + end +end diff --git a/spec/models/api_key_spec.rb b/spec/models/api_key_spec.rb index c8d5348a1c9..88d0aa82d6f 100644 --- a/spec/models/api_key_spec.rb +++ b/spec/models/api_key_spec.rb @@ -98,55 +98,56 @@ describe ApiKey do 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(:request) { + ActionDispatch::TestRequest.create.tap do |request| + request.path_parameters = { controller: "topics", action: "show", topic_id: "3" } + request.remote_addr = "133.45.67.99" + end + } + + let(:env) { request.env } + 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) + expect(key.request_allowed?(env)).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) + expect(key.request_allowed?(env)).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) + expect(key.request_allowed?(env)).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) + request.path_parameters = { controller: "topics", action: "show", topic_id: "4" } + expect(key.request_allowed?(env)).to eq(false) end it 'accepts the request if one of the parameters match' do - route_path['topic_id'] = '4' + request.path_parameters = { controller: "topics", action: "show", topic_id: "4" } scope.allowed_parameters = { topic_id: %w[3 4] } - - expect(key.request_allowed?(request_mock, route_path)).to eq(true) + expect(key.request_allowed?(env)).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) + request.path_parameters = { controller: "topics", action: "show", id: "3" } + expect(key.request_allowed?(env)).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) + request.path_parameters = { controller: "topics", action: "show", topic_id: "3", id: "3" } + expect(key.request_allowed?(env)).to eq(false) end end end diff --git a/spec/models/user_api_key_spec.rb b/spec/models/user_api_key_spec.rb index 22bda2eb98b..d77dda1df8e 100644 --- a/spec/models/user_api_key_spec.rb +++ b/spec/models/user_api_key_spec.rb @@ -4,37 +4,40 @@ require 'rails_helper' describe UserApiKey do context "#allow?" do + def request_env(method, path, **path_parameters) + ActionDispatch::TestRequest.create.tap do |request| + request.request_method = method + request.path = path + request.path_parameters = path_parameters + end.env + end + it "can look up permissions correctly" do key = UserApiKey.new(scopes: ['message_bus', 'notifications'].map { |name| UserApiKeyScope.new(name: name) }) - expect(key.allow?("PATH_INFO" => "/random", "REQUEST_METHOD" => "GET")).to eq(false) - expect(key.allow?("PATH_INFO" => "/message-bus/1234/poll", "REQUEST_METHOD" => "POST")).to eq(true) + expect(key.allow?(request_env("GET", "/random"))).to eq(false) + expect(key.allow?(request_env("POST", "/message-bus/1234/poll"))).to eq(true) - expect(key.allow?("action_dispatch.request.path_parameters" => { controller: "notifications", action: "mark_read" }, - "PATH_INFO" => "/xyz", "REQUEST_METHOD" => "PUT")).to eq(true) - - expect(key.allow?("action_dispatch.request.path_parameters" => { controller: "user_api_keys", action: "revoke" }, - "PATH_INFO" => "/xyz", "REQUEST_METHOD" => "POST")).to eq(true) + expect(key.allow?(request_env("PUT", "/xyz", controller: "notifications", action: "mark_read"))).to eq(true) + expect(key.allow?(request_env("POST", "/xyz", controller: "user_api_keys", action: "revoke"))).to eq(true) end it "can allow all correct scopes to write" do - key = UserApiKey.new(scopes: ["write"].map { |name| UserApiKeyScope.new(name: name) }) - expect(key.allow?("PATH_INFO" => "/random", "REQUEST_METHOD" => "GET")).to eq(true) - expect(key.allow?("PATH_INFO" => "/random", "REQUEST_METHOD" => "PUT")).to eq(true) - expect(key.allow?("PATH_INFO" => "/random", "REQUEST_METHOD" => "PATCH")).to eq(true) - expect(key.allow?("PATH_INFO" => "/random", "REQUEST_METHOD" => "DELETE")).to eq(true) - expect(key.allow?("PATH_INFO" => "/random", "REQUEST_METHOD" => "POST")).to eq(true) + expect(key.allow?(request_env("GET", "/random"))).to eq(true) + expect(key.allow?(request_env("PUT", "/random"))).to eq(true) + expect(key.allow?(request_env("PATCH", "/random"))).to eq(true) + expect(key.allow?(request_env("DELETE", "/random"))).to eq(true) + expect(key.allow?(request_env("POST", "/random"))).to eq(true) end it "can allow blanket read" do - key = UserApiKey.new(scopes: ["read"].map { |name| UserApiKeyScope.new(name: name) }) - expect(key.allow?("PATH_INFO" => "/random", "REQUEST_METHOD" => "GET")).to eq(true) - expect(key.allow?("PATH_INFO" => "/random", "REQUEST_METHOD" => "PUT")).to eq(false) + expect(key.allow?(request_env("GET", "/random"))).to eq(true) + expect(key.allow?(request_env("PUT", "/random"))).to eq(false) end end end