REFACTOR: Introduce RouteMatcher class

This consolidates logic used to match routes in ApiKey, UserApiKey and DefaultCurrentUserProvider. This reduces duplicated logic, and will allow UserApiKeysScope to easily re-use the parameter matching logic from ApiKeyScope

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
 

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

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
 

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

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?

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

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

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

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