From 07cf0f94605c21795269a1a30d72a49e12cbf904 Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Wed, 17 Feb 2021 14:42:44 -0300 Subject: [PATCH] FIX: Allow plugins to correctly extend API key scopes. (#12113) Adding a scope from a plugin was broken. This commit fixes it and adds a test. It also documents the instance method and renames the serialized "id" attribute to "scope_id" to avoid a conflict when the scope also has a parameter with the same name. --- app/controllers/admin/api_controller.rb | 4 ++-- app/models/api_key_scope.rb | 13 +++++++++---- lib/plugin/instance.rb | 7 +++++++ spec/components/plugin/instance_spec.rb | 9 +++++++++ spec/requests/admin/api_controller_spec.rb | 8 ++++---- 5 files changed, 31 insertions(+), 10 deletions(-) diff --git a/app/controllers/admin/api_controller.rb b/app/controllers/admin/api_controller.rb index 7712cc308ef..513aa4b4802 100644 --- a/app/controllers/admin/api_controller.rb +++ b/app/controllers/admin/api_controller.rb @@ -27,7 +27,7 @@ class Admin::ApiController < Admin::AdminController memo.tap do |m| m[resource] = actions.map do |k, v| { - id: "#{resource}:#{k}", + scope_id: "#{resource}:#{k}", key: k, name: k.to_s.gsub('_', ' '), params: v[:params], @@ -99,7 +99,7 @@ class Admin::ApiController < Admin::AdminController def build_scopes params.require(:key)[:scopes].to_a.map do |scope_params| - resource, action = scope_params[:id].split(':') + resource, action = scope_params[:scope_id].split(':') mapping = ApiKeyScope.scope_mappings.dig(resource.to_sym, action.to_sym) raise Discourse::InvalidParameters if mapping.nil? # invalid mapping diff --git a/app/models/api_key_scope.rb b/app/models/api_key_scope.rb index d691965089c..56443ac835e 100644 --- a/app/models/api_key_scope.rb +++ b/app/models/api_key_scope.rb @@ -36,7 +36,7 @@ class ApiKeyScope < ActiveRecord::Base users: { bookmarks: { actions: %w[users#bookmarks], params: %i[username] }, sync_sso: { actions: %w[admin/users#sync_sso], params: %i[sso sig] }, - show: { actions: %w[users#show], params: %i[username external_id] }, + show: { actions: %w[users#show], params: %i[username external_id external_provider] }, check_emails: { actions: %w[users#check_emails], params: %i[username] }, update: { actions: %w[users#update], params: %i[username] }, log_out: { actions: %w[admin/users#log_out] }, @@ -61,10 +61,15 @@ class ApiKeyScope < ActiveRecord::Base plugin_mappings = DiscoursePluginRegistry.api_key_scope_mappings default_mappings.tap do |mappings| - plugin_mappings.each do |mapping| - mapping[:urls] = find_urls(mapping[:actions]) + plugin_mappings.each do |resource| - mappings.deep_merge!(mapping) + resource.each_value do |resource_actions| + resource_actions.each_value do |action_data| + action_data[:urls] = find_urls(action_data[:actions]) + end + end + + mappings.deep_merge!(resource) end end end diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb index 5af08b24e53..a8fd07dadf5 100644 --- a/lib/plugin/instance.rb +++ b/lib/plugin/instance.rb @@ -791,6 +791,13 @@ class Plugin::Instance end end + # Register a new API key scope. + # + # Example: + # add_api_key_scope(:groups, { delete: { actions: %w[groups#add_members], params: %i[id] } }) + # + # This scope lets you add members to a group. Additionally, you can specify which group ids are allowed. + # The delete action is added to the groups resource. def add_api_key_scope(resource, action) DiscoursePluginRegistry.register_api_key_scope_mapping({ resource => action }, self) end diff --git a/spec/components/plugin/instance_spec.rb b/spec/components/plugin/instance_spec.rb index 0f43099eaa2..57baa660d90 100644 --- a/spec/components/plugin/instance_spec.rb +++ b/spec/components/plugin/instance_spec.rb @@ -593,4 +593,13 @@ describe Plugin::Instance do expect(ReviewableScore.types.values.max).to eq(highest_flag_id + 2) end end + + describe '#add_api_key_scope' do + it 'adds a custom api key scope' do + actions = %w[admin/groups#create] + subject.add_api_key_scope(:groups, create: { actions: actions }) + + expect(ApiKeyScope.scope_mappings.dig(:groups, :create, :actions)).to contain_exactly(*actions) + end + end end diff --git a/spec/requests/admin/api_controller_spec.rb b/spec/requests/admin/api_controller_spec.rb index 3478349f574..792136438a9 100644 --- a/spec/requests/admin/api_controller_spec.rb +++ b/spec/requests/admin/api_controller_spec.rb @@ -137,7 +137,7 @@ describe Admin::ApiController do post "/admin/api/keys.json", params: { key: { description: "master key description", - scopes: [{ id: 'topics:write', topic_id: '55' }] + scopes: [{ scope_id: 'topics:write', topic_id: '55' }] } } expect(response.status).to eq(200) @@ -154,7 +154,7 @@ describe Admin::ApiController do post "/admin/api/keys.json", params: { key: { description: "master key description", - scopes: [{ id: 'topics:write', topic_id: '55,33' }] + scopes: [{ scope_id: 'topics:write', topic_id: '55,33' }] } } expect(response.status).to eq(200) @@ -170,7 +170,7 @@ describe Admin::ApiController do post "/admin/api/keys.json", params: { key: { description: "master key description", - scopes: [{ id: 'topics:write', fake_id: '55' }] + scopes: [{ scope_id: 'topics:write', fake_id: '55' }] } } @@ -186,7 +186,7 @@ describe Admin::ApiController do post "/admin/api/keys.json", params: { key: { description: "master key description", - scopes: [{ id: 'something:else' }] + scopes: [{ scope_id: 'something:else' }] } }