diff --git a/.gitignore b/.gitignore index 596f30e..d1685f3 100644 --- a/.gitignore +++ b/.gitignore @@ -3,6 +3,7 @@ /_yardoc/ /coverage/ /doc/ +/docs/ /pkg/ /spec/reports/ /tmp/ diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e04ce5..6c18156 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,35 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- ActiveAdmin `member_action`, `collection_action` and `batch_action` + definitions can be exposed as MCP tools by adding an `mcp:` option to them. + Actions are opt-in: nothing is exposed without that option. Execution runs + through the real ActiveAdmin controller, so `before_action` chains, + authorization and callbacks all apply, and an optional `permission:` proc can + narrow access further. + + `tools/list` is user-specific: an action is advertised only when the + authenticated MCP user passes the resource's authorization adapter, and a + zero-argument `permission:` proc is evaluated at listing time in controller + context (so `current_admin_user` and `can?` work there as they do at call + time). A param's `suggestions:` proc, which runs application code against the + database, is never evaluated for a user who is not authorized for the action. + + Batch action calls additionally run the submitted ids through the adapter's + `scope_collection` and refuse the entire call if any id falls outside it, + rather than silently acting on fewer records than the client asked for. + + A batch action param declared under `mcp:` but missing from the action's + ActiveAdmin `form:` hash is now a declaration error. ActiveAdmin slices + submitted inputs to the `form:` keys, so such a param was advertised, + required and validated, and then silently dropped before the block ran. + + Action failures return a generic error naming the resource and action; the + underlying exception message is written to the log instead of being sent to + the MCP client, where it could disclose SQL, table names or file paths. + ### Changed - **Breaking:** the minimum supported Ruby is now 4.0 and the minimum Rails is diff --git a/CLAUDE.md b/CLAUDE.md index 41b8b34..cbd7761 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -10,6 +10,16 @@ over HTTP and is mounted at `/mcp` by default. The tools it offers — `list_resources`, `query`, `update` — are defined in `lib/activeadmin_mcp/request_handler.rb`. +## Ruby version + +This gem requires Ruby 4.0 and CI runs 4.0.7. There is no `.ruby-version`, so +if your shell defaults to an older Ruby every `bundle` command fails with a +resolution error that does not mention the Ruby version as the cause. Prefix +commands with the version rather than debugging the symptom: + + RBENV_VERSION=4.0.7 bundle exec rake spec + RBENV_VERSION=4.0.7 bundle exec rake e2e + ## Testing MCP actions **Every MCP action must be covered by an end-to-end test, not only by unit @@ -54,6 +64,42 @@ a snapshot taken after migrating and seeding. So examples must not depend on what another one left behind, and the suite runs in random order to keep that honest. Write each one as though it runs alone, because it might. +### Writing an e2e example + +`E2E::McpClient` speaks the protocol: `tools_list` returns the `tools/list` +result, and `call_tool(name, arguments)` unwraps both layers of a tool result +— the JSON-RPC envelope and the pretty-printed JSON inside the text content +block — and hands back the payload. A tool that refuses returns a hash with an +`"error"` key rather than raising, so assert on that key. + +**Assert the side effect, not only the response.** An example that checks a +refusal came back has not shown the action was prevented: the same assertion +passes whether the call was refused before dispatch or ran and then reported +an error. Read the record back with `query` and assert it is unchanged. The +same applies in reverse for a successful call — assert the change landed, not +merely that no error came back. + +**When an example is about which records were affected, assert an untouched +control record.** A batch example that only checks the selected rows changed +cannot tell "acted on the ones I asked for" from "acted on everything". Seed +or pick a record that must not change, and assert it did not. + +**Prefer `include` to exact lists when asserting on the tool listing.** The +fixture application opts actions in to MCP, so the listing legitimately grows +when someone adds one; `contain_exactly` there turns an unrelated addition +into a failure in a file that has nothing to do with it. + +**Environment variables set for seeding are not set for the running server.** +The seeds read credentials from the environment, but the application boots as +a separate process without them. A fixture that needs the seeded admin's +identity at request time has to hard-code it to match `AppBuilder`, not read +`ENV`. + +Things this suite has caught that the unit specs structurally could not: that +Rails' forgery protection refuses the synthesized request every non-GET action +depends on, and that a `permission:` proc evaluated outside controller context +raises `NameError` and silently hides a tool. Both looked fine against mocks. + ### Write e2e descriptions out in full Give e2e examples and their enclosing blocks descriptions verbose enough that diff --git a/README.md b/README.md index 94a02e9..7eb1a6d 100644 --- a/README.md +++ b/README.md @@ -70,6 +70,7 @@ read/query setup without authentication. | `list_resources` | List the ActiveAdmin resources the current user may read, along with their attributes. | | `query` | Query a resource the current user may read, using Ransack syntax, scoped to the records they may access (`limit` defaults to 25, capped at 100). | | `update` | Update an existing record, honouring ActiveAdmin's permitted params and authorization. | +| *(per action)* | Any ActiveAdmin member, collection or batch action the application has opted in with an `mcp:` option, exposed as its own tool. | ### Query examples @@ -98,6 +99,94 @@ The `update` tool applies the same rules as the ActiveAdmin UI: - **Permitted fields only** — attributes are filtered through the resource's `permit_params`; fields the admin form doesn't accept are silently dropped. +### Running member, collection and batch actions + +ActiveAdmin actions are **not** exposed by default. An action becomes an MCP +tool only when you add an `mcp:` option to it: + +```ruby +ActiveAdmin.register Volunteer do + member_action :create_warning, method: :post, mcp: { + description: "Record a warning against a volunteer", + permission: ->(volunteer) { volunteer.active? && can?(:warn, volunteer) }, + params: { + reason: { type: :string, required: true, + hint: "Short free-text summary shown to the volunteer" }, + severity: { type: :string, enum: %w[low medium high] }, + category: { type: :string, + suggestions: -> { WarningCategory.pluck(:name) } } + } + } do + # your existing action body, unchanged + end +end +``` + +That registers a `volunteer_create_warning` tool. Batch actions opt in the same +way, and inherit their param types from the `form:` hash you already declare: + +```ruby +batch_action :suspend, form: { reason: :text }, + mcp: { description: "Suspend the selected volunteers" } do |ids, inputs| + # ... +end +``` + +**Declaring params** + +- `type:` — one of `:string`, `:integer`, `:number`, `:boolean`, `:array`, `:object`. +- `required:` — refuses the call when the value is missing. +- `hint:` — static guidance shown to the agent. Always a plain string. +- `enum:` — **binding**. A value outside the list is refused before dispatch. +- `suggestions:` — a proc evaluated when tools are listed. **Advisory only**, + never enforced, so use it for live values from the database. If it raises, + the tool is still listed without suggestions. + +On a batch action, every declared param must also appear in the action's `form:` +hash. ActiveAdmin slices submitted inputs down to the declared `form:` keys +before calling the block, so a param declared only under `mcp:` would be +advertised to the client and then dropped; the declaration is refused instead. + +**Authorization** + +`permission:` is an *additional* gate, never a replacement. Every call first +passes your ActiveAdmin authorization adapter exactly as `query` and `update` +do; the proc can only narrow access further, never widen it. It is evaluated in +controller context, so `current_admin_user`, `can?` and the usual admin helpers +are available. Return `false` to refuse, or a `String` to refuse with a reason +the agent can act on. + +Tools are also listed per user: an action whose resource the adapter refuses is +left out of `tools/list` entirely, and a `permission:` proc that takes no +arguments is evaluated at listing time (in the same controller context) so the +tool is hidden rather than offered and then refused. + +For **batch actions** the adapter check is necessarily resource-level — there is +no single record to authorize — so it is `authorized?(:, YourModel)` +rather than a per-record policy evaluation. To stop that being a hole, the ids +the client submits are run back through the adapter's `scope_collection`, and +the whole call is refused if any of them falls outside the scope. Nothing is +narrowed silently: the call either acts on every id you asked for or on none. + +One caveat on authentication. Dispatch neutralises the namespace's +`authentication_method` callback, because the MCP request has already +authenticated by bearer token and that callback would otherwise redirect to a +login page. If your `authentication_method` is a *combined* authentication-and- +authorization method — one that also, say, rejects non-superusers — then +neutralising it disables that authorization half too. Resource-level +authorization still runs through the adapter, but the "we only skip +authentication" framing is not universal; keep authorization in the adapter, +not in the authentication callback. + +**What you get back** + +Actions are executed through your real ActiveAdmin controller, so the action's +`before_action` chain, authorization and callbacks all run. The tool returns the +response status, the redirect target and any flash messages — not the rendered +HTML. Redirect-style (submit-side) actions are the supported case; a `GET` +action that renders a full admin view is best-effort and may fail for want of a +view context. + ## Connecting a client `activeadmin_mcp` has been tested with **Claude Code** (Anthropic) over the diff --git a/lib/activeadmin_mcp.rb b/lib/activeadmin_mcp.rb index 502a1e4..6ba5345 100644 --- a/lib/activeadmin_mcp.rb +++ b/lib/activeadmin_mcp.rb @@ -1,6 +1,13 @@ require_relative "activeadmin_mcp/version" require_relative "activeadmin_mcp/configuration" require_relative "activeadmin_mcp/authorization" +require_relative "activeadmin_mcp/active_admin_ext" +require_relative "activeadmin_mcp/action_definition" +require_relative "activeadmin_mcp/action_schema" +require_relative "activeadmin_mcp/action_params" +require_relative "activeadmin_mcp/action_catalog" +require_relative "activeadmin_mcp/controller_dispatcher" +require_relative "activeadmin_mcp/action_runner" require_relative "activeadmin_mcp/resource_registry" require_relative "activeadmin_mcp/form_field_collector" require_relative "activeadmin_mcp/record_updater" diff --git a/lib/activeadmin_mcp/action_catalog.rb b/lib/activeadmin_mcp/action_catalog.rb new file mode 100644 index 0000000..55355f1 --- /dev/null +++ b/lib/activeadmin_mcp/action_catalog.rb @@ -0,0 +1,64 @@ +module ActiveadminMcp + # Finds every ActiveAdmin action an application has opted in to MCP. + # + # Nothing is cached: ActiveAdmin reloads resources in development, and a + # stale catalog would advertise tools that no longer exist. + module ActionCatalog + # Tool names the engine reserves for itself, including names Phase 2 and 3 + # will take, so an application cannot silently shadow one later. + RESERVED = %w[list_resources query update create describe_form].freeze + + class << self + def all + ResourceRegistry.resources.flat_map { |entry| definitions_for(entry[:config]) } + end + + def find(tool_name) + all.find { |definition| definition.tool_name == tool_name } + end + + private + + def definitions_for(config) + candidates(config).filter_map do |action, kind| + definition = ActionDefinition.build(config: config, action: action, kind: kind) + next unless definition + + next warn_and_skip(definition.errors.join("; ")) unless definition.valid? + next warn_and_skip("#{definition.tool_name} collides with a built-in tool") if reserved?(definition) + + definition + end + end + + def candidates(config) + pairs = [] + pairs.concat(safe_actions(config, :member_actions).map { |a| [a, :member] }) + pairs.concat(safe_actions(config, :collection_actions).map { |a| [a, :collection] }) + pairs.concat(safe_actions(config, :batch_actions).map { |a| [a, :batch] }) if batch_enabled?(config) + pairs.select { |action, _kind| action.respond_to?(:mcp_options) } + end + + def safe_actions(config, reader) + config.respond_to?(reader) ? Array(config.public_send(reader)) : [] + end + + # ActiveAdmin keeps registered batch actions even when the namespace has + # batch actions switched off, and hides them in the UI. Match that. + def batch_enabled?(config) + return false unless config.respond_to?(:batch_actions_enabled?) + + config.batch_actions_enabled? + end + + def reserved?(definition) + RESERVED.include?(definition.tool_name) + end + + def warn_and_skip(message) + warn("[activeadmin_mcp] ignoring action: #{message}") + nil + end + end + end +end diff --git a/lib/activeadmin_mcp/action_definition.rb b/lib/activeadmin_mcp/action_definition.rb new file mode 100644 index 0000000..21e1779 --- /dev/null +++ b/lib/activeadmin_mcp/action_definition.rb @@ -0,0 +1,183 @@ +module ActiveadminMcp + # One ActiveAdmin action that an application has opted in to MCP, normalised + # so the rest of the engine does not care whether it came from a + # member_action, a collection_action or a batch_action. + # + # Batch actions are the special case: ActiveAdmin already knows their input + # names and widget types via `form:`, so those become param types and the + # `mcp:` declaration only layers descriptions and hints on top. + class ActionDefinition + KINDS = %i[member collection batch].freeze + + # JSON Schema scalar types an application may declare. + TYPES = %i[string integer number boolean array object].freeze + + # ActiveAdmin batch action form widgets -> JSON Schema types. + FORM_TYPES = { + text: :string, + string: :string, + select: :string, + datepicker: :string, + number: :number, + checkbox: :boolean, + }.freeze + + attr_reader :config, :action, :kind, :errors + + def self.build(config:, action:, kind:) + options = action.mcp_options + return nil unless options.is_a?(Hash) + + new(config: config, action: action, kind: kind, options: options) + end + + def initialize(config:, action:, kind:, options:) + @config = config + @action = action + @kind = kind + @options = options + @errors = [] + validate! + end + + def action_name + (@kind == :batch ? @action.sym : @action.name).to_sym + end + + def resource_name + @config.resource_class.name + end + + def tool_name + "#{resource_name.underscore.tr('/', '_')}_#{action_name}" + end + + def description + @options[:description] + end + + def permission + @options[:permission] + end + + def http_verb + return :post if @kind == :batch + + Array(@action.http_verb).first&.to_sym || :get + end + + # Declared params, with batch actions inheriting their types from the + # ActiveAdmin `form:` hash underneath anything the declaration says. + def params + @params ||= inherited_params.merge(declared_params) do |_key, inherited, declared| + inherited.merge(declared) + end + end + + def valid? + @errors.empty? + end + + private + + def declared_params + raw = @options[:params] + return {} unless raw.is_a?(Hash) + + raw.each_with_object({}) { |(name, spec), acc| acc[name.to_sym] = spec } + end + + def inherited_params + return {} unless @kind == :batch + return {} unless @action.respond_to?(:inputs) + + form = @action.inputs + return {} unless form.is_a?(Hash) + + form.each_with_object({}) do |(name, widget), acc| + acc[name.to_sym] = { type: FORM_TYPES.fetch(widget.to_sym, :string) } + end + end + + def validate! + @errors << "#{tool_name}: mcp declaration needs a description" if description.to_s.strip.empty? + @errors << "#{tool_name}: unknown kind #{@kind}" unless KINDS.include?(@kind) + + reserved = reserved_param_name + form_keys = batch_form_keys + params.each do |name, spec| + unless spec.is_a?(Hash) + @errors << "#{tool_name}: param #{name} must be a Hash" + next + end + + @errors << "#{tool_name}: param #{name} is reserved" if name == reserved + + # ActiveAdmin's own batch_action controller method slices the submitted + # inputs down to the keys of the batch action's `inputs` (its `form:` + # hash) before calling the block: `inputs.slice(*valid_keys)`. When + # there is no `form:` at all, `inputs` is nil, so `valid_keys` is nil, + # and `slice(*nil)` is `slice()` — which drops EVERY input, not none. + # So a form-less batch action permits nothing, and any declared param + # is a declaration error. A Proc form is evaluated by ActiveAdmin in + # controller context (MethodOrProcHelper.render_in_context), so we + # cannot know its keys here and skip the check rather than guess. + case form_keys + when :unknown_proc_form + nil + when :no_form + @errors << "#{tool_name}: param #{name} cannot be declared because this batch action " \ + "has no form: hash, so ActiveAdmin drops every input before the action runs" + when nil + nil # not a batch action, or no `inputs` method at all: no rule applies + else + unless form_keys.include?(name) + @errors << "#{tool_name}: param #{name} is not among the batch action's declared " \ + "form: keys, so ActiveAdmin would drop it before the action runs" + end + end + + type = spec[:type] + @errors << "#{tool_name}: param #{name} has unknown type #{type}" if type && !TYPES.include?(type.to_sym) + end + + @errors << "#{tool_name}: permission must be callable" if permission && !permission.respond_to?(:call) + end + + # The permitted param keys for a batch action's `inputs` (its `form:` + # hash), distinguishing three outcomes the caller must treat differently: + # + # * not a batch action, or the action has no `inputs` method at all -> + # nil, the batch form: rule does not apply. + # * `inputs` is a Hash -> its keys, the permitted set ActiveAdmin will + # slice submitted params down to. + # * `inputs` is nil (no `form:` declared) -> :no_form. ActiveAdmin still + # slices, against a nil key list, which yields an EMPTY permitted set + # (`hash.slice(*nil)` is `hash.slice()` == `{}`), so every declared + # param here is a declaration error. + # * `inputs` is a Proc -> :unknown_proc_form. ActiveAdmin evaluates it in + # controller context via `render_in_context`, so we cannot know its + # keys at declaration time. We skip the check rather than guess. + def batch_form_keys + return nil unless @kind == :batch + return nil unless @action.respond_to?(:inputs) + + form = @action.inputs + return form.keys.map(&:to_sym) if form.is_a?(Hash) + return :unknown_proc_form if form.is_a?(Proc) + + :no_form + end + + def reserved_param_name + case @kind + when :member + :id + when :batch + :ids + else + nil + end + end + end +end diff --git a/lib/activeadmin_mcp/action_params.rb b/lib/activeadmin_mcp/action_params.rb new file mode 100644 index 0000000..8eba4bb --- /dev/null +++ b/lib/activeadmin_mcp/action_params.rb @@ -0,0 +1,78 @@ +module ActiveadminMcp + # Validates and coerces a tool call's arguments against its ActionDefinition + # before anything reaches the controller. + # + # Undeclared arguments are dropped rather than passed through: the declaration + # is the contract, and forwarding unknown keys into a controller action would + # let a client reach parameters the application never opted in to. + class ActionParams + class CoercionError < StandardError; end + + def initialize(definition) + @definition = definition + end + + def call(arguments) + arguments ||= {} + result = { params: {} } + + case @definition.kind + when :member + id = arguments["id"] + return { error: "id is required" } if id.nil? || id.to_s.empty? + + result[:record_id] = id.to_s + when :batch + ids = Array(arguments["ids"]).reject { |id| id.to_s.empty? } + return { error: "ids is required" } if ids.empty? + + result[:record_ids] = ids.map(&:to_s) + end + + @definition.params.each do |name, spec| + value = arguments[name.to_s] + + if value.nil? || value.to_s.empty? + return { error: "#{name} is required" } if spec[:required] + + next + end + + begin + coerced_value = coerce(value, spec[:type]) + rescue CoercionError + return { error: "#{name} must be a #{spec[:type]}" } + end + + if spec[:enum].is_a?(Array) && !spec[:enum].include?(coerced_value) + return { error: "#{name} must be one of: #{spec[:enum].join(', ')}" } + end + + result[:params][name] = coerced_value + end + + result + end + + private + + def coerce(value, type) + case type&.to_sym + when :integer + Integer(value) + when :number + Float(value) + when :boolean + case value + when true, "true", "1", 1 then true + when false, "false", "0", 0 then false + else raise CoercionError, "invalid boolean value" + end + else + value + end + rescue ArgumentError, TypeError => e + raise CoercionError, e.message + end + end +end diff --git a/lib/activeadmin_mcp/action_runner.rb b/lib/activeadmin_mcp/action_runner.rb new file mode 100644 index 0000000..cbc7c83 --- /dev/null +++ b/lib/activeadmin_mcp/action_runner.rb @@ -0,0 +1,163 @@ +module ActiveadminMcp + # Runs one opted-in ActiveAdmin action for an MCP client. + # + # Two gates gate every call, in this order, both before anything is + # dispatched: + # + # 1. ActiveAdmin's own authorization adapter - the same check `query` and + # `update` make today. + # 2. The action's optional `permission:` proc. + # 3. For batch actions only, the submitted ids are run back through the + # adapter's scope_collection, since gate 1 could only authorize the + # resource class. + # + # The proc can only ever narrow access. A record the MCP user cannot touch + # stays untouchable whether or not a proc is declared, and the controller's + # own before_action chain runs again during dispatch regardless. + class ActionRunner + def initialize(definition:, current_user:) + @definition = definition + @current_user = current_user + end + + def call(arguments) + parsed = ActionParams.new(@definition).call(arguments) + return parsed if parsed[:error] + + record = find_record(parsed[:record_id]) + return { error: "Record not found: #{@definition.resource_name}##{parsed[:record_id]}" } if record_missing?(record, parsed) + + subject = record || @definition.config.resource_class + return { error: "Not authorized to run #{@definition.tool_name}" } unless authorized?(subject) + + dispatcher = ControllerDispatcher.new(config: @definition.config, current_user: @current_user) + + refusal = permission_refusal(dispatcher, record, parsed) + return { error: refusal } if refusal + + out_of_scope = unauthorized_batch_ids(parsed) + return { error: out_of_scope } if out_of_scope + + dispatcher.call( + action: dispatch_action, + path: path_for(record, parsed), + verb: @definition.http_verb, + params: dispatch_params(parsed), + path_params: path_params(parsed) + ) + end + + private + + def find_record(id) + return nil unless id + + @definition.config.resource_class.find_by(id: id) + end + + def record_missing?(record, parsed) + parsed.key?(:record_id) && record.nil? + end + + def authorized?(subject) + Authorization.for(@definition.config, @current_user) + .authorized?(@definition.action_name, subject) + end + + # Evaluated the way ActiveAdmin evaluates batch action `:if` procs, so + # current_admin_user, can? and the usual admin helpers are in scope. + # Returns a refusal message, or nil when the proc allows the call. + def permission_refusal(dispatcher, record, parsed) + permission = @definition.permission + return nil unless permission + + controller = dispatcher.controller_with_mcp_user + outcome = ::MethodOrProcHelper.render_in_context(controller, permission, *permission_args(permission, record, parsed)) + + return outcome if outcome.is_a?(String) + return nil if outcome + + "Not permitted to run #{@definition.tool_name}" + rescue StandardError => e + # Keep the detail server-side: the message can carry SQL, paths and other + # internals the MCP client has no business seeing. + warn("[activeadmin_mcp] permission proc for #{@definition.tool_name} raised #{e.class}: #{e.message}") + "Permission check failed for #{@definition.tool_name}" + end + + # Gate 1 can only authorize the resource class for a batch action, because + # there is no single record. That leaves the submitted ids unchecked, so a + # client could name records the adapter's scope_collection excludes. Run the + # ids back through the scope and refuse the whole call if any falls outside + # it — narrowing silently would let a client believe it acted on records it + # never touched. + def unauthorized_batch_ids(parsed) + return nil unless @definition.kind == :batch + + ids = Array(parsed[:record_ids]) + return nil if ids.empty? + + klass = @definition.config.resource_class + key = klass.primary_key + scoped = Authorization.for(@definition.config, @current_user) + .scope_collection(klass.where(key => ids), @definition.action_name) + permitted = scoped.pluck(key).map(&:to_s) + refused = ids - permitted + return nil if refused.empty? + + "Not authorized to run #{@definition.tool_name} on #{@definition.resource_name} " \ + "#{refused.join(', ')} (not found, or outside your permitted scope)" + rescue StandardError => e + warn("[activeadmin_mcp] scoping batch ids for #{@definition.tool_name} raised #{e.class}: #{e.message}") + "Not authorized to run #{@definition.tool_name}" + end + + # render_in_context instance_execs the proc AND passes args along, so a + # zero-arity lambda would raise ArgumentError if we always handed it a + # record. Match what the proc actually accepts. + def permission_args(permission, record, parsed) + return [] if permission.respond_to?(:arity) && permission.arity.zero? + + case @definition.kind + when :member then [record] + when :batch then [parsed[:record_ids]] + else [] + end + end + + # Batch actions enter through ActiveAdmin's own batch_action controller + # method, so its slicing of inputs to the declared form: keys applies + # exactly as it does in the admin UI. + def dispatch_action + @definition.kind == :batch ? :batch_action : @definition.action_name + end + + def dispatch_params(parsed) + return parsed[:params] unless @definition.kind == :batch + + { + batch_action: @definition.action_name.to_s, + collection_selection: parsed[:record_ids], + batch_action_inputs: JSON.generate(parsed[:params]), + } + end + + def path_params(parsed) + parsed[:record_id] ? { id: parsed[:record_id] } : {} + end + + def path_for(record, parsed) + config = @definition.config + + case @definition.kind + when :member then config.route_member_action_path(@definition.action_name, record) + # RouteBuilder#batch_action_path calls `.permit!` on the params it is + # given, which a plain Hash (its own default argument) does not + # respond to. Pass ActionController::Parameters explicitly to avoid + # tripping over ActiveAdmin's own default. + when :batch then config.route_batch_action_path(ActionController::Parameters.new) + else config.route_collection_path + end + end + end +end diff --git a/lib/activeadmin_mcp/action_schema.rb b/lib/activeadmin_mcp/action_schema.rb new file mode 100644 index 0000000..352fabf --- /dev/null +++ b/lib/activeadmin_mcp/action_schema.rb @@ -0,0 +1,66 @@ +module ActiveadminMcp + # Builds the JSON Schema an MCP client sees for one opted-in action. + # + # The binding/advisory split matters: a static `enum:` is enforced before we + # dispatch, while `suggestions:` is only ever a hint to the model. A + # suggestions proc runs application code on every tools/list call, so it is + # wrapped — a raising proc costs its suggestions, never the whole listing. + class ActionSchema + def initialize(definition) + @definition = definition + end + + def to_h + properties = record_properties + required = properties.keys.map(&:to_s) + + @definition.params.each do |name, spec| + properties[name] = property_for(spec) + required << name.to_s if spec[:required] + end + + { type: "object", properties: properties, required: required.uniq } + end + + private + + def record_properties + case @definition.kind + when :member + { id: { type: "string", description: "Primary key of the record to act on" } } + when :batch + { ids: { type: "array", items: { type: "string" }, + description: "Primary keys of the records to act on" } } + else + {} + end + end + + def property_for(spec) + property = { type: (spec[:type] || :string).to_s } + + descriptions = [] + descriptions << spec[:hint] if spec[:hint] + + property[:enum] = spec[:enum] if spec[:enum].is_a?(Array) + + suggestions = resolve_suggestions(spec[:suggestions]) + if suggestions&.any? + property[:examples] = suggestions + descriptions << "Suggested values: #{suggestions.join(', ')}" + end + + property[:description] = descriptions.join(". ") unless descriptions.empty? + property + end + + # Advisory only. A proc that blows up must not take the tool listing with it. + def resolve_suggestions(suggestions) + return nil unless suggestions.respond_to?(:call) + + Array(suggestions.call) + rescue StandardError + nil + end + end +end diff --git a/lib/activeadmin_mcp/active_admin_ext.rb b/lib/activeadmin_mcp/active_admin_ext.rb new file mode 100644 index 0000000..ec21330 --- /dev/null +++ b/lib/activeadmin_mcp/active_admin_ext.rb @@ -0,0 +1,39 @@ +module ActiveadminMcp + # Every patch this gem applies to ActiveAdmin lives in this one file, so the + # coupling has a single place to check when ActiveAdmin is upgraded. + # + # ActiveAdmin stores an action's option hash but exposes only the keys it + # uses itself (:method, :title, :form, :if, ...). MCP metadata is declared as + # an extra `mcp:` key on that same hash, which ActiveAdmin carries through + # untouched, so all we need is a reader to get it back out. + module ActiveAdminExt + module ActionOptions + def mcp_options + options = instance_variable_get(:@options) + options.is_a?(Hash) ? options[:mcp] : nil + end + end + + # ActiveAdmin::BatchAction only exists once ActiveAdmin's before_load hooks + # have run, so this is called from ActiveAdmin.after_load rather than at + # require time. + def self.apply! + unless applicable? + # Without these readers every opted-in action silently vanishes from + # tools/list, because ActionCatalog can no longer see an mcp: option + # anywhere. Say so rather than shipping a feature that is quietly off. + warn("[activeadmin_mcp] ActiveAdmin::ControllerAction / ActiveAdmin::BatchAction not found: " \ + "MCP action options cannot be read, so no opted-in actions will be exposed as tools.") + return false + end + + ::ActiveAdmin::ControllerAction.include(ActionOptions) + ::ActiveAdmin::BatchAction.include(ActionOptions) + true + end + + def self.applicable? + defined?(::ActiveAdmin::ControllerAction) && defined?(::ActiveAdmin::BatchAction) ? true : false + end + end +end diff --git a/lib/activeadmin_mcp/controller_dispatcher.rb b/lib/activeadmin_mcp/controller_dispatcher.rb new file mode 100644 index 0000000..d143c17 --- /dev/null +++ b/lib/activeadmin_mcp/controller_dispatcher.rb @@ -0,0 +1,121 @@ +module ActiveadminMcp + # Runs a real ActiveAdmin controller action out of band, so an MCP tool call + # goes through the same before_action chain, authorization and callbacks as a + # click in the admin UI. + # + # We synthesize the request rather than route one. Routing would mean issuing + # a second HTTP request against the app, which would have to authenticate as + # the MCP user by forging an admin session — a back door this gem should not + # have. Building the request by hand lets us inject the already-authenticated + # MCP user directly onto the controller instead. + class ControllerDispatcher + def initialize(config:, current_user:) + @config = config + @current_user = current_user + end + + def call(action:, path:, verb: :get, params: {}, path_params: {}) + controller = controller_with_mcp_user + request = build_request(path: path, verb: verb, params: params, action: action, path_params: path_params) + response = ActionDispatch::Response.new + + controller.set_request!(request) + controller.set_response!(response) + controller.process(action) + + capture(request, response) + rescue StandardError => e + # The exception text can carry internals — SQL fragments, table names, + # file paths. It belongs in the application's log, not in a tool result + # that goes to an MCP client. + warn("[activeadmin_mcp] #{@config.resource_class.name}##{action} raised #{e.class}: #{e.message}") + { error: "#{@config.resource_class.name}##{action} failed" } + end + + # A controller instance for this resource with the MCP user injected, ready + # either to process a request or to serve as the evaluation context for an + # action's `permission:` proc. Public because listing-time permission checks + # need exactly the same context a dispatched call gets. + def controller_with_mcp_user + user = @current_user + controller = @config.controller.new + + current_user_methods.each do |method_name| + controller.define_singleton_method(method_name) { user } + end + + # The namespace's authentication_method (typically Devise's + # authenticate_admin_user!) would redirect us to a login page. The MCP + # request has already authenticated by bearer token, so it is a no-op + # here — this does NOT skip authorization, which still runs in full. + auth_method = @config.namespace.authentication_method + controller.define_singleton_method(auth_method) { true } if auth_method + + # The synthesized request carries no session-bound CSRF token, so + # Rails' own forgery protection would refuse every non-GET action + # (member actions declared `method: :post`, and every batch action, + # which always dispatches as one). The MCP request has already + # authenticated by bearer token; this does NOT skip authorization, + # which still runs in full. + controller.define_singleton_method(:verified_request?) { true } + + controller + end + + private + + # An application can point ActiveadminMcp at one current-user method and the + # ActiveAdmin namespace at another. Stub both, so an action body calling + # either gets the MCP user rather than nil. uniq keeps us from defining the + # same singleton method twice when they agree. + def current_user_methods + namespace_method = @config.namespace.current_user_method if @config.namespace.respond_to?(:current_user_method) + + [ + ActiveadminMcp.config.current_user_method, + namespace_method, + :current_active_admin_user, + ].select { |name| name.respond_to?(:to_sym) }.map(&:to_sym).uniq + end + + def build_request(path:, verb:, params:, action:, path_params:) + env = Rack::MockRequest.env_for( + path, + method: verb.to_s.upcase, + params: params.transform_keys(&:to_s) + ) + # Flash needs somewhere to live; without a session the action raises. + env["rack.session"] = {} + env["action_dispatch.request.path_parameters"] = + { controller: controller_path, action: action.to_s }.merge(path_params) + + ActionDispatch::Request.new(env) + end + + def controller_path + @config.controller.name.underscore.sub(/_controller\z/, "") + end + + def capture(request, response) + result = { status: response.status } + + if response.redirect? + result[:redirect_to] = response.location + else + # Deliberately not the body: admin HTML is large and almost entirely + # chrome, and would swamp the client's context for no benefit. + result[:rendered] = true + end + + flash = extract_flash(request) + result[:flash] = flash if flash&.any? + result + end + + def extract_flash(request) + request.flash.to_hash + rescue StandardError + nil + end + end +end diff --git a/lib/activeadmin_mcp/engine.rb b/lib/activeadmin_mcp/engine.rb index a771593..cf1b450 100644 --- a/lib/activeadmin_mcp/engine.rb +++ b/lib/activeadmin_mcp/engine.rb @@ -14,5 +14,11 @@ class Engine < ::Rails::Engine end end end + + initializer "activeadmin_mcp.active_admin_ext" do + ActiveSupport.on_load(:after_initialize) do + ActiveAdmin.after_load { ActiveadminMcp::ActiveAdminExt.apply! } if defined?(::ActiveAdmin) + end + end end end diff --git a/lib/activeadmin_mcp/request_handler.rb b/lib/activeadmin_mcp/request_handler.rb index a6fb0ca..c2993b0 100644 --- a/lib/activeadmin_mcp/request_handler.rb +++ b/lib/activeadmin_mcp/request_handler.rb @@ -38,45 +38,97 @@ def initialize_result end def tools_list - { - tools: [ - { - name: "list_resources", - description: "List the ActiveAdmin resources the authenticated user is authorized " \ - "to read, with their attributes", - inputSchema: { type: "object", properties: {} }, - }, - { - name: "query", - description: "Query an ActiveAdmin resource using Ransack syntax. Respects ActiveAdmin " \ - "authorization: the resource must be readable by the authenticated user, " \ - "and results are scoped to the records they may access.", - inputSchema: { - type: "object", - properties: { - resource: { type: "string", description: "Resource name (e.g., 'User', 'Post')" }, - q: { type: "object", description: "Ransack query (e.g., {name_cont: 'john'})" }, - limit: { type: "integer", description: "Max records (default: 25)" }, - }, - required: ["resource"], + { tools: built_in_tools + action_tools } + end + + # Both gates run before the schema is built, never after: ActionSchema calls + # the application's `suggestions:` procs, which read the database. Filtering + # an already-assembled list would mean those procs had already run — and + # their values already been read — for a user authorized for none of it. + def action_tools + ActionCatalog.all.filter_map do |definition| + next unless authorized_to_run?(definition) + next unless authorized_to_list?(definition) + + { + name: definition.tool_name, + description: definition.description, + inputSchema: ActionSchema.new(definition).to_h, + } + end + end + + # The same authorization adapter check ActionRunner makes before dispatch, + # and the one list_resources makes for reads. There is no record at listing + # time, so the subject is the resource class. + def authorized_to_run?(definition) + Authorization.for(definition.config, @current_user) + .authorized?(definition.action_name, definition.config.resource_class) + rescue StandardError => e + warn("[activeadmin_mcp] hiding #{definition.tool_name}: authorization check raised #{e.class}: #{e.message}") + false + end + + # Collection and batch actions whose permission proc takes no record can be + # resolved now, so the tool is simply hidden. A member action's proc needs a + # record, so its tool stays listed and refusal happens at call time. + # + # The proc is evaluated in controller context, exactly as ActionRunner + # evaluates it at call time, so `current_admin_user`, `can?` and the rest of + # the admin helpers are in scope. Evaluating it bare would make every such + # proc raise NameError and hide its tool from everybody. + def authorized_to_list?(definition) + permission = definition.permission + return true unless permission + return true unless permission.respond_to?(:arity) && permission.arity.zero? + + controller = ControllerDispatcher.new(config: definition.config, current_user: @current_user) + .controller_with_mcp_user + !!::MethodOrProcHelper.render_in_context(controller, permission) + rescue StandardError => e + # One broken proc hides its own tool and nothing else, but it says so. + warn("[activeadmin_mcp] hiding #{definition.tool_name}: permission proc raised #{e.class}: #{e.message}") + false + end + + def built_in_tools + [ + { + name: "list_resources", + description: "List the ActiveAdmin resources the authenticated user is authorized " \ + "to read, with their attributes", + inputSchema: { type: "object", properties: {} }, + }, + { + name: "query", + description: "Query an ActiveAdmin resource using Ransack syntax. Respects ActiveAdmin " \ + "authorization: the resource must be readable by the authenticated user, " \ + "and results are scoped to the records they may access.", + inputSchema: { + type: "object", + properties: { + resource: { type: "string", description: "Resource name (e.g., 'User', 'Post')" }, + q: { type: "object", description: "Ransack query (e.g., {name_cont: 'john'})" }, + limit: { type: "integer", description: "Max records (default: 25)" }, }, + required: ["resource"], }, - { - name: "update", - description: "Update an existing record. Only fields the resource's ActiveAdmin " \ - "form permits are written, and the update respects ActiveAdmin authorization.", - inputSchema: { - type: "object", - properties: { - resource: { type: "string", description: "Resource name (e.g., 'User', 'Post')" }, - id: { type: ["integer", "string"], description: "Primary key of the record to update" }, - attributes: { type: "object", description: "Attributes to update (e.g., {name: 'New name'})" }, - }, - required: %w[resource id attributes], + }, + { + name: "update", + description: "Update an existing record. Only fields the resource's ActiveAdmin " \ + "form permits are written, and the update respects ActiveAdmin authorization.", + inputSchema: { + type: "object", + properties: { + resource: { type: "string", description: "Resource name (e.g., 'User', 'Post')" }, + id: { type: ["integer", "string"], description: "Primary key of the record to update" }, + attributes: { type: "object", description: "Attributes to update (e.g., {name: 'New name'})" }, }, + required: %w[resource id attributes], }, - ], - } + }, + ] end def call_tool(params) @@ -87,12 +139,19 @@ def call_tool(params) when "list_resources" then tool_list_resources when "query" then tool_query(args) when "update" then tool_update(args) - else { error: "Unknown tool: #{name}" } + else tool_action(name, args) end { content: [{ type: "text", text: JSON.pretty_generate(result) }] } end + def tool_action(name, args) + definition = ActionCatalog.find(name) + return { error: "Unknown tool: #{name}" } unless definition + + ActionRunner.new(definition: definition, current_user: @current_user).call(args) + end + def tool_list_resources entries = ResourceRegistry.resources.select { |entry| authorized_to_read?(entry) } { resources: entries.map { |entry| ResourceRegistry.resource_info(entry) } } diff --git a/spec/activeadmin_mcp/action_catalog_spec.rb b/spec/activeadmin_mcp/action_catalog_spec.rb new file mode 100644 index 0000000..d8511ff --- /dev/null +++ b/spec/activeadmin_mcp/action_catalog_spec.rb @@ -0,0 +1,71 @@ +require "spec_helper" + +RSpec.describe ActiveadminMcp::ActionCatalog do + def action(name, mcp:, verb: :post) + double("controller_action", name: name, http_verb: verb, mcp_options: mcp) + end + + def batch(name, mcp:, form: nil) + double("batch_action", sym: name, mcp_options: mcp, inputs: form) + end + + def config(member: [], collection: [], batch_actions: [], batch_enabled: true, name: "Volunteer") + double( + "config", + resource_class: double("model", name: name), + member_actions: member, + collection_actions: collection, + batch_actions: batch_actions, + batch_actions_enabled?: batch_enabled + ) + end + + def stub_resources(*configs) + entries = configs.map { |c| { name: c.resource_class.name, model: c.resource_class, config: c } } + allow(ActiveadminMcp::ResourceRegistry).to receive(:resources).and_return(entries) + end + + it "collects opted-in actions and ignores the rest" do + stub_resources(config(member: [ + action(:create_warning, mcp: { description: "Record a warning" }), + action(:undocumented, mcp: nil) + ])) + + expect(described_class.all.map(&:tool_name)).to eq(["volunteer_create_warning"]) + end + + # Verified against ActiveAdmin 3.5.2: batch_actions_enabled? can be false + # while batch actions are still registered. Exposing them would surface an + # action the admin UI itself hides. + it "skips batch actions when the namespace has them disabled" do + stub_resources(config(batch_actions: [batch(:suspend, mcp: { description: "Suspend" })], + batch_enabled: false)) + + expect(described_class.all).to be_empty + end + + it "includes batch actions when the namespace has them enabled" do + stub_resources(config(batch_actions: [batch(:suspend, mcp: { description: "Suspend" })])) + + expect(described_class.all.map(&:tool_name)).to eq(["volunteer_suspend"]) + end + + it "refuses a tool name that collides with a built-in tool" do + stub_resources(config(collection: [action(:resources, mcp: { description: "Clash" })], name: "List")) + + expect(described_class.all).to be_empty + end + + it "drops invalid declarations rather than exposing them" do + stub_resources(config(member: [action(:create_warning, mcp: { params: {} })])) + + expect(described_class.all).to be_empty + end + + it "finds a definition by tool name" do + stub_resources(config(member: [action(:create_warning, mcp: { description: "Record a warning" })])) + + expect(described_class.find("volunteer_create_warning").action_name).to eq(:create_warning) + expect(described_class.find("nope")).to be_nil + end +end diff --git a/spec/activeadmin_mcp/action_definition_spec.rb b/spec/activeadmin_mcp/action_definition_spec.rb new file mode 100644 index 0000000..1c9ddef --- /dev/null +++ b/spec/activeadmin_mcp/action_definition_spec.rb @@ -0,0 +1,224 @@ +require "spec_helper" + +RSpec.describe ActiveadminMcp::ActionDefinition do + def build_config(name: "Volunteer") + double("config", resource_class: double("model", name: name)) + end + + def member_action(name, mcp:, verb: :post) + double("controller_action", name: name, http_verb: verb, mcp_options: mcp) + end + + def batch_action(name, mcp:, form: nil) + double("batch_action", sym: name, mcp_options: mcp, inputs: form) + end + + it "returns nil when the action did not opt in" do + action = member_action(:undocumented, mcp: nil) + + expect(described_class.build(config: build_config, action: action, kind: :member)).to be_nil + end + + it "exposes the declared metadata and a namespaced tool name" do + action = member_action(:create_warning, mcp: { + description: "Record a warning", + params: { reason: { type: :string, required: true } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :member) + + expect(definition).to have_attributes( + kind: :member, + action_name: :create_warning, + resource_name: "Volunteer", + tool_name: "volunteer_create_warning", + description: "Record a warning", + http_verb: :post + ) + expect(definition.params).to eq(reason: { type: :string, required: true }) + expect(definition).to be_valid + end + + it "inherits batch action param types from the ActiveAdmin form hash" do + action = batch_action(:suspend, mcp: { description: "Suspend" }, form: { reason: :text, notify: :checkbox }) + + definition = described_class.build(config: build_config, action: action, kind: :batch) + + expect(definition.params).to eq( + reason: { type: :string }, + notify: { type: :boolean } + ) + end + + it "lets the mcp declaration layer hints over an inherited batch form type" do + action = batch_action(:suspend, form: { reason: :text }, mcp: { + description: "Suspend", + params: { reason: { hint: "Shown to the volunteer" } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :batch) + + expect(definition.params).to eq(reason: { type: :string, hint: "Shown to the volunteer" }) + end + + it "properly handles acronyms and namespaced resource names in tool names" do + action_acronym = member_action(:create_warning, mcp: { + description: "Record a warning", + params: {} + }) + action_namespaced = member_action(:create_warning, mcp: { + description: "Record a warning", + params: {} + }) + + definition_acronym = described_class.build( + config: build_config(name: "APIKey"), + action: action_acronym, + kind: :member + ) + definition_namespaced = described_class.build( + config: build_config(name: "Admin::Volunteer"), + action: action_namespaced, + kind: :member + ) + + expect(definition_acronym.tool_name).to eq("api_key_create_warning") + expect(definition_namespaced.tool_name).to eq("admin_volunteer_create_warning") + end + + it "is invalid when a param declares an unrecognised type" do + action = member_action(:create_warning, mcp: { + description: "Record a warning", + params: { reason: { type: :wibble } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :member) + + expect(definition).not_to be_valid + expect(definition.errors.first).to include("wibble") + end + + it "is invalid without a description" do + action = member_action(:create_warning, mcp: { params: {} }) + + definition = described_class.build(config: build_config, action: action, kind: :member) + + expect(definition).not_to be_valid + expect(definition.errors.first).to include("description") + end + + it "is invalid when a member action declares a param named id" do + action = member_action(:create_warning, mcp: { + description: "Record a warning", + params: { id: { type: :integer } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :member) + + expect(definition).not_to be_valid + expect(definition.errors).to include(match(/param id is reserved/)) + end + + it "is invalid when a batch action declares a param named ids" do + action = batch_action(:suspend, mcp: { + description: "Suspend", + params: { ids: { type: :array } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :batch) + + expect(definition).not_to be_valid + expect(definition.errors).to include(match(/param ids is reserved/)) + end + + # ActiveAdmin's batch_action controller method slices submitted inputs down + # to the declared form: keys, so a param declared only under mcp: would be + # advertised, required, validated, encoded — and then dropped before the + # block ever saw it. + it "is invalid when a batch param is missing from the ActiveAdmin form hash" do + action = batch_action(:suspend, form: { reason: :text }, mcp: { + description: "Suspend", + params: { reason: { hint: "Why" }, notify_manager: { type: :boolean, required: true } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :batch) + + expect(definition).not_to be_valid + expect(definition.errors).to include(match(/param notify_manager is not among the batch action's declared form/)) + end + + it "is valid when every batch param is in the form hash" do + action = batch_action(:suspend, form: { reason: :text, notify: :checkbox }, mcp: { + description: "Suspend", + params: { reason: { hint: "Why" } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :batch) + + expect(definition).to be_valid + end + + # With no form: hash, ActiveAdmin's batch_action controller calls + # `inputs.slice(*nil.try(:keys))`, i.e. `slice()`, which drops every + # submitted input. So a form-less batch action has an empty permitted set, + # and declaring any param is a declaration error, not a free pass. + it "is invalid when a form-less batch action declares any param" do + action = batch_action(:suspend, form: nil, mcp: { + description: "Suspend", + params: { reason: { type: :string } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :batch) + + expect(definition).not_to be_valid + expect(definition.errors).to include(match(/param reason/)) + end + + # A Proc form is evaluated by ActiveAdmin in controller context via + # MethodOrProcHelper.render_in_context, so we cannot know its keys at + # declaration time. We skip the check rather than guess, so this must not + # be falsely rejected. + it "does not reject a batch action whose form is a Proc" do + action = batch_action(:suspend, form: -> { { reason: :text } }, mcp: { + description: "Suspend", + params: { reason: { type: :string } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :batch) + + expect(definition).to be_valid + end + + it "does not apply the batch form rule to member actions" do + action = member_action(:create_warning, mcp: { + description: "Record a warning", + params: { reason: { type: :string } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :member) + + expect(definition).to be_valid + end + + it "is valid when a collection action declares a param named id" do + action = double("controller_action", name: :process, http_verb: :post, mcp_options: { + description: "Process something", + params: { id: { type: :string } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :collection) + + expect(definition).to be_valid + end + + it "is valid when a member action declares a param named ids" do + action = member_action(:create_warning, mcp: { + description: "Record a warning", + params: { ids: { type: :array } } + }) + + definition = described_class.build(config: build_config, action: action, kind: :member) + + expect(definition).to be_valid + end +end diff --git a/spec/activeadmin_mcp/action_params_spec.rb b/spec/activeadmin_mcp/action_params_spec.rb new file mode 100644 index 0000000..1d828ba --- /dev/null +++ b/spec/activeadmin_mcp/action_params_spec.rb @@ -0,0 +1,114 @@ +require "spec_helper" + +RSpec.describe ActiveadminMcp::ActionParams do + def definition(kind: :member, params: {}) + double("definition", kind: kind, params: params) + end + + def call(definition, arguments) + described_class.new(definition).call(arguments) + end + + it "extracts the record id for member actions" do + result = call(definition(kind: :member), { "id" => "42" }) + + expect(result[:record_id]).to eq("42") + expect(result).not_to have_key(:error) + end + + it "refuses a member action with no id" do + expect(call(definition(kind: :member), {})).to eq(error: "id is required") + end + + it "refuses a batch action with an empty ids list" do + expect(call(definition(kind: :batch), { "ids" => [] })).to eq(error: "ids is required") + end + + it "refuses a missing required param" do + result = call(definition(kind: :collection, params: { reason: { type: :string, required: true } }), {}) + + expect(result).to eq(error: "reason is required") + end + + it "refuses a value outside a binding enum" do + definition = definition(kind: :collection, params: { severity: { type: :string, enum: %w[low high] } }) + + result = call(definition, { "severity" => "urgent" }) + + expect(result[:error]).to eq("severity must be one of: low, high") + end + + it "does not enforce suggestions" do + definition = definition(kind: :collection, params: { category: { type: :string, suggestions: -> { %w[conduct] } } }) + + result = call(definition, { "category" => "something else" }) + + expect(result[:params]).to eq(category: "something else") + end + + it "coerces declared types" do + definition = definition(kind: :collection, params: { + count: { type: :integer }, notify: { type: :boolean } + }) + + result = call(definition, { "count" => "3", "notify" => "true" }) + + expect(result[:params]).to eq(count: 3, notify: true) + end + + it "drops arguments that were never declared" do + result = call(definition(kind: :collection, params: { reason: { type: :string } }), + { "reason" => "Late", "admin_override" => "true" }) + + expect(result[:params]).to eq(reason: "Late") + end + + it "coerces a value before checking enum (Finding 1: string '1' should match integer enum [1, 2, 3])" do + definition = definition(kind: :collection, params: { count: { type: :integer, enum: [1, 2, 3] } }) + + result = call(definition, { "count" => "1" }) + + expect(result[:params]).to eq(count: 1) + end + + it "refuses a coerced value outside enum" do + definition = definition(kind: :collection, params: { count: { type: :integer, enum: [1, 2, 3] } }) + + result = call(definition, { "count" => "9" }) + + expect(result[:error]).to eq("count must be one of: 1, 2, 3") + end + + it "refuses a value that cannot be coerced to integer (Finding 2: 'abc' for :integer)" do + definition = definition(kind: :collection, params: { count: { type: :integer } }) + + result = call(definition, { "count" => "abc" }) + + expect(result[:error]).to eq("count must be a integer") + end + + it "coerces the string 'false' to boolean false" do + definition = definition(kind: :collection, params: { active: { type: :boolean } }) + + result = call(definition, { "active" => "false" }) + + expect(result[:params]).to eq(active: false) + end + + it "refuses a value that cannot be coerced to boolean" do + definition = definition(kind: :collection, params: { active: { type: :boolean } }) + + result = call(definition, { "active" => "wibble" }) + + expect(result[:error]).to eq("active must be a boolean") + end + + it "accepts a required boolean param sent as false (not treated as missing)" do + definition = definition(kind: :collection, params: { active: { type: :boolean, required: true } }) + + result = call(definition, { "active" => false }) + + expect(result[:params]).to eq(active: false) + expect(result).not_to have_key(:error) + end +end diff --git a/spec/activeadmin_mcp/action_runner_spec.rb b/spec/activeadmin_mcp/action_runner_spec.rb new file mode 100644 index 0000000..a7e0b87 --- /dev/null +++ b/spec/activeadmin_mcp/action_runner_spec.rb @@ -0,0 +1,197 @@ +require "spec_helper" +require "support/active_admin" + +# Authorizes every action at the class level but scopes collections down to +# active records, the shape a real CanCanCan or Pundit adapter has: gate 1 +# passes for a batch action (its subject is the resource class), and only +# scope_collection knows which records are actually reachable. +class ActiveOnlyAuthorizationAdapter < ActiveAdmin::AuthorizationAdapter + def authorized?(_action, _subject = nil) + true + end + + def scope_collection(collection, _action = :read) + collection.where(active: true) + end +end + +RSpec.describe ActiveadminMcp::ActionRunner do + let(:config) { McpSpec::ActiveAdminHarness.volunteer_config } + let(:admin) { AdminUser.create!(email: "admin@example.com") } + let!(:volunteer) { Volunteer.create!(name: "Ann") } + + after do + Volunteer.delete_all + AdminUser.delete_all + end + + # Memoised deliberately: ActionCatalog.all rebuilds definitions on every + # call, so re-deriving one per example would stub a different object than the + # one under test. + def find_definition(name, kind) + ActiveadminMcp::ActionCatalog.all.find do |d| + d.action_name == name && d.kind == kind + end + end + + let(:definition) { find_definition(:create_warning, :member) } + let(:batch_definition) { find_definition(:suspend, :batch) } + + def run(definition, arguments, current_user: admin) + described_class.new(definition: definition, current_user: current_user).call(arguments) + end + + it "runs an authorized member action" do + result = run(definition, { "id" => volunteer.id.to_s, "reason" => "Late again" }) + + expect(result[:status]).to eq(302) + expect(volunteer.reload.name).to eq("Late again") + end + + it "refuses before dispatching when ActiveAdmin authorization says no" do + deny = double("authorization") + allow(deny).to receive(:authorized?).and_return(false) + allow(ActiveadminMcp::Authorization).to receive(:for).and_return(deny) + + result = run(definition, { "id" => volunteer.id.to_s, "reason" => "Late again" }) + + expect(result[:error]).to include("Not authorized") + expect(volunteer.reload.name).to eq("Ann") + end + + it "refuses when the record does not exist" do + result = run(definition, { "id" => "999999", "reason" => "Late" }) + + expect(result[:error]).to include("not found") + end + + it "surfaces a validation error from ActionParams without dispatching" do + result = run(definition, { "id" => volunteer.id.to_s }) + + expect(result).to eq(error: "reason is required") + expect(volunteer.reload.name).to eq("Ann") + end + + it "refuses when the permission proc returns false" do + allow(definition).to receive(:permission).and_return(->(_record) { false }) + + result = run(definition, { "id" => volunteer.id.to_s, "reason" => "Late" }) + + expect(result[:error]).to include("Not permitted") + expect(volunteer.reload.name).to eq("Ann") + end + + it "uses a string returned by the permission proc as the refusal reason" do + allow(definition).to receive(:permission).and_return(->(_record) { "Volunteer is suspended" }) + + result = run(definition, { "id" => volunteer.id.to_s, "reason" => "Late" }) + + expect(result[:error]).to eq("Volunteer is suspended") + end + + it "keeps a raising permission proc's message out of the refusal" do + allow(definition).to receive(:permission).and_return( + ->(_record) { raise "SQLite3::SQLException: no such table: nope_secret" } + ) + messages = [] + allow_any_instance_of(described_class).to receive(:warn) { |_, message| messages << message } + + result = run(definition, { "id" => volunteer.id.to_s, "reason" => "Late" }) + + expect(result[:error]).to eq("Permission check failed for volunteer_create_warning") + expect(messages.join).to include("nope_secret") + end + + it "runs the permission proc in controller context" do + seen = nil + allow(definition).to receive(:permission).and_return(proc { |record| seen = [record, current_active_admin_user]; true }) + + run(definition, { "id" => volunteer.id.to_s, "reason" => "Late" }) + + expect(seen).to eq([volunteer, admin]) + end + + it "runs a batch action over the selected records" do + other = Volunteer.create!(name: "Bea") + unselected = Volunteer.create!(name: "Cee") + + result = run(batch_definition, + { "ids" => [volunteer.id.to_s, other.id.to_s], "reason" => "No shows" }) + + expect(result[:status]).to eq(302) + expect(result[:flash]["notice"]).to eq("2 suspended: No shows") + expect(volunteer.reload.name).to eq("Suspended: No shows") + expect(other.reload.name).to eq("Suspended: No shows") + expect(unselected.reload.name).to eq("Cee") + end + + describe "collection-kind permission proc" do + let(:collection_definition) { find_definition(:export, :collection) } + + it "dispatches when a zero-arity permission proc returns true" do + allow(collection_definition).to receive(:permission).and_return(-> { true }) + + result = run(collection_definition, {}) + + expect(result[:status]).to eq(302) + expect(result[:flash]["notice"]).to eq("Exported") + end + + it "refuses generically when a zero-arity permission proc returns false" do + allow(collection_definition).to receive(:permission).and_return(-> { false }) + + result = run(collection_definition, {}) + + expect(result[:error]).to include("Not permitted") + expect(result[:flash]).to be_nil + end + + it "uses a string returned by the permission proc as the refusal reason" do + allow(collection_definition).to receive(:permission).and_return(-> { "Exports are disabled" }) + + result = run(collection_definition, {}) + + expect(result[:error]).to eq("Exports are disabled") + end + end + + # Gate 1 can only authorize the resource class for a batch action, so without + # an explicit id-scope check a client could name records the adapter excludes + # and ActiveAdmin would happily mutate them. + describe "batch ids outside the authorized scope" do + around do |example| + namespace = ActiveAdmin.application.namespaces[:admin] + previous = namespace.authorization_adapter + namespace.authorization_adapter = ActiveOnlyAuthorizationAdapter + example.run + namespace.authorization_adapter = previous + end + + it "refuses the whole call and mutates nothing when one id is out of scope" do + hidden = Volunteer.create!(name: "Hidden", active: false) + + result = run(batch_definition, + { "ids" => [volunteer.id.to_s, hidden.id.to_s], "reason" => "No shows" }) + + expect(result[:error]).to include("Not authorized", hidden.id.to_s) + expect(volunteer.reload.name).to eq("Ann") + expect(hidden.reload.name).to eq("Hidden") + end + + it "refuses ids that do not exist at all" do + result = run(batch_definition, { "ids" => ["999999"], "reason" => "No shows" }) + + expect(result[:error]).to include("999999") + end + + it "still runs when every id is inside the scope" do + other = Volunteer.create!(name: "Bea") + + result = run(batch_definition, + { "ids" => [volunteer.id.to_s, other.id.to_s], "reason" => "No shows" }) + + expect(result[:status]).to eq(302) + expect(volunteer.reload.name).to eq("Suspended: No shows") + end + end +end diff --git a/spec/activeadmin_mcp/action_schema_spec.rb b/spec/activeadmin_mcp/action_schema_spec.rb new file mode 100644 index 0000000..c33a5b2 --- /dev/null +++ b/spec/activeadmin_mcp/action_schema_spec.rb @@ -0,0 +1,75 @@ +require "spec_helper" + +RSpec.describe ActiveadminMcp::ActionSchema do + def definition(kind: :member, params: {}) + double("definition", kind: kind, params: params) + end + + it "requires an id for member actions" do + schema = described_class.new(definition(kind: :member)).to_h + + expect(schema[:properties]).to include(:id) + expect(schema[:required]).to include("id") + end + + it "requires an ids array for batch actions" do + schema = described_class.new(definition(kind: :batch)).to_h + + expect(schema[:properties][:ids]).to include(type: "array") + expect(schema[:required]).to include("ids") + end + + it "adds no record key for collection actions" do + schema = described_class.new(definition(kind: :collection)).to_h + + expect(schema[:properties]).to be_empty + expect(schema[:required]).to be_empty + end + + it "maps declared params, hints and required-ness" do + schema = described_class.new( + definition(kind: :collection, params: { + reason: { type: :string, required: true, hint: "Shown to the volunteer" } + }) + ).to_h + + expect(schema[:properties][:reason]).to eq(type: "string", description: "Shown to the volunteer") + expect(schema[:required]).to eq(["reason"]) + end + + it "emits a static enum as a binding enum" do + schema = described_class.new( + definition(kind: :collection, params: { severity: { type: :string, enum: %w[low high] } }) + ).to_h + + expect(schema[:properties][:severity][:enum]).to eq(%w[low high]) + end + + it "emits a suggestions proc as advisory examples" do + schema = described_class.new( + definition(kind: :collection, params: { category: { type: :string, suggestions: -> { %w[lateness conduct] } } }) + ).to_h + + expect(schema[:properties][:category][:examples]).to eq(%w[lateness conduct]) + expect(schema[:properties][:category]).not_to have_key(:enum) + expect(schema[:properties][:category][:description]).to include("lateness") + end + + it "keeps the tool usable when a suggestions proc raises" do + schema = described_class.new( + definition(kind: :collection, params: { category: { type: :string, suggestions: -> { raise "boom" } } }) + ).to_h + + expect(schema[:properties][:category]).to eq(type: "string") + end + + it "ensures no duplicate required entries even if a colliding param somehow reaches it" do + # ActionDefinition rejects this upstream, but to_h should never emit duplicates regardless + schema = described_class.new( + definition(kind: :member, params: { id: { type: :integer, required: true } }) + ).to_h + + expect(schema[:required]).to eq(["id"]) + expect(schema[:required].uniq).to eq(schema[:required]) + end +end diff --git a/spec/activeadmin_mcp/active_admin_ext_spec.rb b/spec/activeadmin_mcp/active_admin_ext_spec.rb new file mode 100644 index 0000000..5ec10bd --- /dev/null +++ b/spec/activeadmin_mcp/active_admin_ext_spec.rb @@ -0,0 +1,52 @@ +require "spec_helper" +require "support/active_admin" + +RSpec.describe ActiveadminMcp::ActiveAdminExt do + let(:config) { McpSpec::ActiveAdminHarness.volunteer_config } + + def member_action(name) + config.member_actions.find { |action| action.name.to_sym == name } + end + + before { described_class.apply! } + + it "reads the mcp option off a member action" do + expect(member_action(:create_warning).mcp_options).to eq( + description: "Record a warning against a volunteer", + params: { reason: { type: :string, required: true } } + ) + end + + it "returns nil for an action that did not opt in" do + expect(member_action(:undocumented).mcp_options).to be_nil + end + + it "reads the mcp option off a batch action" do + suspend = config.batch_actions.find { |action| action.sym == :suspend } + + expect(suspend.mcp_options).to eq(description: "Suspend the selected volunteers") + end + + # Guard spec. If a future ActiveAdmin starts validating or stripping unknown + # option keys, this fails loudly here rather than silently in a user's admin. + it "confirms ActiveAdmin carries unknown action option keys through untouched" do + options = member_action(:create_warning).instance_variable_get(:@options) + + expect(options).to include(method: :post) + expect(options).to have_key(:mcp) + end + + # If ActiveAdmin ever renames these classes, ActionCatalog's respond_to? + # guard makes every opted-in tool vanish from tools/list with nothing said + # anywhere. A warning is the only signal an operator would get. + describe "when ActiveAdmin does not provide the expected classes" do + it "warns and returns false rather than failing silently" do + allow(described_class).to receive(:applicable?).and_return(false) + messages = [] + allow(described_class).to receive(:warn) { |message| messages << message } + + expect(described_class.apply!).to be(false) + expect(messages.join).to include("no opted-in actions will be exposed") + end + end +end diff --git a/spec/activeadmin_mcp/controller_dispatcher_spec.rb b/spec/activeadmin_mcp/controller_dispatcher_spec.rb new file mode 100644 index 0000000..13ad460 --- /dev/null +++ b/spec/activeadmin_mcp/controller_dispatcher_spec.rb @@ -0,0 +1,159 @@ +require "spec_helper" +require "support/active_admin" + +# An authorization adapter that denies everything, used to prove that +# neutralising the namespace's authentication_method (see controller_with_mcp_user) +# does not also neutralise authorization, which must keep running in full. +class DenyingAuthorizationAdapter < ActiveAdmin::AuthorizationAdapter + def authorized?(_action, _subject = nil) + false + end +end + +RSpec.describe ActiveadminMcp::ControllerDispatcher do + let(:config) { McpSpec::ActiveAdminHarness.volunteer_config } + let(:admin) { AdminUser.create!(email: "admin@example.com") } + let(:volunteer) { Volunteer.create!(name: "Ann") } + + after do + Volunteer.delete_all + AdminUser.delete_all + end + + def dispatch(action:, params: {}, path: nil, path_params: {}) + described_class.new(config: config, current_user: admin).call( + action: action, + path: path || config.route_member_action_path(action, volunteer), + verb: :post, + params: params, + path_params: { id: volunteer.id.to_s }.merge(path_params) + ) + end + + it "runs the action and captures the redirect and flash" do + result = dispatch(action: :create_warning, params: { reason: "Late again" }) + + expect(result[:status]).to eq(302) + expect(result[:redirect_to]).to include("/admin/volunteers/#{volunteer.id}") + expect(result[:flash]).to eq("notice" => "Warning recorded") + expect(result).not_to have_key(:error) + end + + it "actually performs the action's side effect" do + dispatch(action: :create_warning, params: { reason: "Late again" }) + + expect(volunteer.reload.name).to eq("Late again") + end + + it "exposes the MCP user to the controller as the current admin user" do + dispatcher = described_class.new(config: config, current_user: admin) + controller = dispatcher.controller_with_mcp_user + + expect(controller.send(ActiveadminMcp.config.current_user_method)).to eq(admin) + expect(controller.send(:current_active_admin_user)).to eq(admin) + end + + it "returns an error hash rather than raising when the action blows up" do + allow_any_instance_of(config.controller).to receive(:create_warning).and_raise("kaboom") + allow_any_instance_of(described_class).to receive(:warn) + + result = dispatch(action: :create_warning, params: { reason: "Late" }) + + expect(result[:error]).to eq("Volunteer#create_warning failed") + end + + # An exception message can carry SQL, table names and file paths. The client + # gets a generic failure; the detail goes to the log. + it "keeps the exception message out of the client's result and logs it instead" do + allow_any_instance_of(config.controller).to receive(:create_warning) + .and_raise("SQLite3::SQLException: no such table: nope_secret: SELECT * FROM nope_secret") + messages = [] + allow_any_instance_of(described_class).to receive(:warn) { |_, message| messages << message } + + result = dispatch(action: :create_warning, params: { reason: "Late" }) + + expect(result[:error]).not_to include("nope_secret") + expect(messages.join).to include("nope_secret") + end + + # ActiveadminMcp.config.current_user_method and the ActiveAdmin namespace's + # own current_user_method can disagree; an action body calling either one must + # get the MCP user, not nil. + context "when the namespace names a different current user method" do + around do |example| + namespace = ActiveAdmin.application.namespaces[:admin] + previous = namespace.current_user_method + namespace.current_user_method = :current_namespace_admin + example.run + namespace.current_user_method = previous + end + + it "stubs the namespace's method as well as the configured one" do + controller = described_class.new(config: config, current_user: admin).controller_with_mcp_user + + expect(controller.send(:current_namespace_admin)).to eq(admin) + expect(controller.send(ActiveadminMcp.config.current_user_method)).to eq(admin) + expect(controller.send(:current_active_admin_user)).to eq(admin) + end + end + + it "defines each current user method once when the two settings agree" do + namespace = ActiveAdmin.application.namespaces[:admin] + allow(namespace).to receive(:current_user_method).and_return(ActiveadminMcp.config.current_user_method) + + controller = described_class.new(config: config, current_user: admin).controller_with_mcp_user + + defined_names = controller.singleton_methods.map(&:to_s) + expect(defined_names.count(ActiveadminMcp.config.current_user_method.to_s)).to eq(1) + end + + it "reports a rendered response without returning the body" do + result = dispatch(action: :undocumented) + + expect(result[:status]).to eq(200) + expect(result[:rendered]).to be(true) + expect(result).not_to have_key(:body) + end + + context "when the namespace has an authentication method" do + around do |example| + namespace = ActiveAdmin.application.namespaces[:admin] + previous = namespace.authentication_method + namespace.authentication_method = :authenticate_admin_user! + example.run + namespace.authentication_method = previous + end + + it "neutralises it rather than redirecting to a login page" do + result = dispatch(action: :create_warning, params: { reason: "Late again" }) + + expect(result[:redirect_to]).to include("/admin/volunteers/#{volunteer.id}") + expect(volunteer.reload.name).to eq("Late again") + end + end + + context "when the authorization adapter denies the action" do + around do |example| + namespace = ActiveAdmin.application.namespaces[:admin] + previous = namespace.authorization_adapter + namespace.authorization_adapter = DenyingAuthorizationAdapter + example.run + namespace.authorization_adapter = previous + end + + it "blocks the action instead of running it" do + result = dispatch(action: :create_warning, params: { reason: "Late again" }) + + # ActiveAdmin's default on_unauthorized_access handler rescues + # ActiveAdmin::AccessDenied internally and turns it into a redirect + # with a flash message, rather than letting the exception reach + # ControllerDispatcher's own rescue — so the denial surfaces as a + # flash entry, not a top-level :error key. What actually matters is + # proven below: the write never happened. + expect(result).not_to have_key(:error) + expect(result[:flash]&.values&.join).to match(/not authorized/i) + + expect(volunteer.reload.name).to eq("Ann") + end + end +end diff --git a/spec/activeadmin_mcp/request_handler_spec.rb b/spec/activeadmin_mcp/request_handler_spec.rb index 116e439..a18a1cd 100644 --- a/spec/activeadmin_mcp/request_handler_spec.rb +++ b/spec/activeadmin_mcp/request_handler_spec.rb @@ -1,8 +1,24 @@ require "spec_helper" +require "support/active_admin" RSpec.describe ActiveadminMcp::RequestHandler do subject(:handler) { described_class.new } + # A stand-in ActiveAdmin authorization adapter. `scope_collection` mirrors + # the real adapters by returning the collection it is handed, so tests can + # assert on the relation the handler builds. `calls` records every + # authorized? call, so a test can assert on the subject that was checked. + def adapter_class(authorized:, calls: []) + Class.new do + define_method(:initialize) { |*| } + define_method(:authorized?) do |action, subject = nil| + calls << [action, subject] + authorized + end + def scope_collection(collection, *) = collection + end + end + def handle(method, params = nil, id: 1) request = { "id" => id, "method" => method } request["params"] = params if params @@ -40,6 +56,8 @@ def handle(method, params = nil, id: 1) end describe "tools/list" do + before { allow(ActiveadminMcp::ActionCatalog).to receive(:all).and_return([]) } + it "advertises the list_resources, query and update tools" do tools = handle("tools/list")[:result][:tools] @@ -78,17 +96,6 @@ def call_tool(name, arguments = {}) JSON.parse(text) end - # A stand-in ActiveAdmin authorization adapter. `scope_collection` mirrors - # the real adapters by returning the collection it is handed, so tests can - # assert on the relation the handler builds. - def adapter_class(authorized:) - Class.new do - define_method(:initialize) { |*| } - define_method(:authorized?) { |*| authorized } - def scope_collection(collection, *) = collection - end - end - def resource_config(authorized: true) namespace = double("namespace", authorization_adapter: adapter_class(authorized: authorized)) double("config", namespace: namespace) @@ -235,4 +242,189 @@ def stub_resource(authorized: true) end end end + + describe "action tools" do + let(:resource_class) { Class.new } + + def definition(tool_name: "volunteer_create_warning", permission: nil, + params: { reason: { type: :string, required: true } }, + adapter: adapter_class(authorized: true)) + namespace = double("namespace", authorization_adapter: adapter) + double( + "definition", + tool_name: tool_name, + description: "Record a warning", + kind: :member, + params: params, + permission: permission, + action_name: :create_warning, + resource_name: "Volunteer", + config: double("config", namespace: namespace, resource_class: resource_class) + ) + end + + def handle(request, current_user: :admin) + ActiveadminMcp::RequestHandler.new(current_user: current_user).handle(request) + end + + def tool_names(definitions, current_user: :admin) + allow(ActiveadminMcp::ActionCatalog).to receive(:all).and_return(Array(definitions)) + allow(ActiveadminMcp::ResourceRegistry).to receive(:resources).and_return([]) + + handle({ "id" => 1, "method" => "tools/list" }, current_user: current_user)[:result][:tools] + .map { |tool| tool[:name] } + end + + it "lists opted-in actions alongside the built-in tools" do + allow(ActiveadminMcp::ActionCatalog).to receive(:all).and_return([definition]) + allow(ActiveadminMcp::ResourceRegistry).to receive(:resources).and_return([]) + + response = handle({ "id" => 1, "method" => "tools/list" }) + names = response[:result][:tools].map { |tool| tool[:name] } + + expect(names).to include("volunteer_create_warning") + tool = response[:result][:tools].find { |t| t[:name] == "volunteer_create_warning" } + expect(tool[:description]).to eq("Record a warning") + expect(tool[:inputSchema][:required]).to include("id", "reason") + end + + it "routes a call to the action runner" do + target = definition + allow(ActiveadminMcp::ActionCatalog).to receive(:find).with("volunteer_create_warning").and_return(target) + + runner = instance_double(ActiveadminMcp::ActionRunner, call: { status: 302 }) + allow(ActiveadminMcp::ActionRunner).to receive(:new) + .with(definition: target, current_user: :admin).and_return(runner) + + response = handle({ + "id" => 2, "method" => "tools/call", + "params" => { "name" => "volunteer_create_warning", + "arguments" => { "id" => "1", "reason" => "Late" } } + }) + + expect(runner).to have_received(:call).with({ "id" => "1", "reason" => "Late" }) + expect(response[:result][:content].first[:text]).to include("302") + end + + it "reports an unknown tool" do + allow(ActiveadminMcp::ActionCatalog).to receive(:find).and_return(nil) + + response = handle({ + "id" => 3, "method" => "tools/call", + "params" => { "name" => "nope", "arguments" => {} } + }) + + expect(response[:result][:content].first[:text]).to include("Unknown tool") + end + + describe "authorization at listing time" do + it "hides an action tool the authorization adapter refuses" do + denied = definition(adapter: adapter_class(authorized: false)) + + expect(tool_names(denied)).not_to include("volunteer_create_warning") + end + + it "checks the resource class, since there is no record at listing time" do + calls = [] + listed = definition(adapter: adapter_class(authorized: true, calls: calls)) + + tool_names(listed) + + expect(calls).to eq([[:create_warning, resource_class]]) + end + + it "hides only the offending tool when an adapter raises" do + exploding = Class.new do + define_method(:initialize) { |*| } + define_method(:authorized?) { |*| raise "adapter exploded" } + end + boom = definition(tool_name: "volunteer_boom", adapter: exploding) + allow_any_instance_of(described_class).to receive(:warn) + + names = tool_names([boom, definition]) + + expect(names).not_to include("volunteer_boom") + expect(names).to include("volunteer_create_warning") + end + + # The schema is what runs the application's `suggestions:` procs, so it + # must never be built for a tool the user is not authorized for. Filtering + # an assembled list would be too late: the proc would already have read + # the database and handed its rows over. + it "never runs a suggestions proc for a user the adapter refuses" do + ran = false + suggestions = -> { ran = true; %w[secret-category] } + denied = definition( + adapter: adapter_class(authorized: false), + params: { category: { type: :string, suggestions: suggestions } } + ) + + names = tool_names(denied) + + # Asserted first, deliberately: filtering the assembled list would hide + # the tool and still leak, so the proc not running is the real property. + expect(ran).to be(false) + expect(names).not_to include("volunteer_create_warning") + end + + it "does run a suggestions proc for an authorized user" do + ran = false + suggestions = -> { ran = true; %w[visible-category] } + allowed = definition(params: { category: { type: :string, suggestions: suggestions } }) + + expect(tool_names(allowed)).to include("volunteer_create_warning") + expect(ran).to be(true) + end + end + + # These need the real ActiveAdmin harness: the point of the fix is that the + # proc is instance_exec'd against a real controller, which no double can + # stand in for. + describe "a permission proc at listing time" do + let(:admin) { AdminUser.create!(email: "admin@example.com") } + + after { AdminUser.delete_all } + + def catalog_definition(action_name, kind) + ActiveadminMcp::ActionCatalog.all.find do |d| + d.action_name == action_name && d.kind == kind + end + end + + let(:export) { catalog_definition(:export, :collection) } + + it "evaluates a zero-arity proc in controller context" do + seen = nil + allow(export).to receive(:permission).and_return(-> { seen = current_active_admin_user; true }) + + names = tool_names(export, current_user: admin) + + expect(seen).to eq(admin) + expect(names).to include("volunteer_export") + end + + it "hides the tool when a controller-context proc refuses" do + allow(export).to receive(:permission).and_return(-> { current_active_admin_user.nil? }) + + expect(tool_names(export, current_user: admin)).not_to include("volunteer_export") + end + + it "hides only the raising tool and keeps the rest of the listing" do + allow(export).to receive(:permission).and_return(-> { raise "proc exploded" }) + allow_any_instance_of(described_class).to receive(:warn) + + names = tool_names([export, definition], current_user: admin) + + expect(names).not_to include("volunteer_export") + expect(names).to include("volunteer_create_warning", "query") + end + + it "leaves a member action's proc for call time" do + warning = catalog_definition(:create_warning, :member) + allow(warning).to receive(:permission).and_return(->(_record) { false }) + + expect(tool_names(warning, current_user: admin)).to include("volunteer_create_warning") + end + end + end end diff --git a/spec/e2e/fixture_app/README.md b/spec/e2e/fixture_app/README.md index 6e9f87b..8816f93 100644 --- a/spec/e2e/fixture_app/README.md +++ b/spec/e2e/fixture_app/README.md @@ -12,19 +12,47 @@ path there. Each one exists to give a claim in the README something to bite on: - `app/admin/posts.rb` permits `title` and `body` but **not** `slug`, so the - suite can prove an unpermitted attribute is dropped rather than written. + suite can prove an unpermitted attribute is dropped rather than written. It + also carries the MCP action fixtures: + - `member_action :publish` is opted in via `mcp:` with a required + `visibility` param bound to a static `enum:`, so the suite can prove a + value outside the enum is refused before dispatch and a permitted value + runs against the real controller. + - `member_action :archive` has no `mcp:` key at all, so the suite can prove + the opt-in guarantee: an action that exists in the admin UI is not + automatically exposed as a tool. + - `batch_action :set_status` is opted in via `mcp:` with only a + description — its param type is inherited from `form:` — so the suite can + prove a batch action applies to exactly the selected records and leaves + the rest untouched. + - `member_action :explode` raises from its body, so the suite can prove a + failing action comes back as a generic error naming the resource and + action, with the exception's own message — which can carry SQL, table + names and file paths — kept away from the MCP client. + - `member_action :feature` carries a `permission:` proc that takes the + record and returns a refusal *string* for a draft post, so the suite can + prove a record-aware proc keeps its tool advertised (it cannot be resolved + at listing time), refuses at call time with the proc's own wording, and + allows the call once the record satisfies it. + - `collection_action :purge_drafts` is opted in via `mcp:` with a + zero-argument `permission:` proc that calls `current_admin_user`, so the + suite can prove the proc is evaluated in controller context at + `tools/list` time rather than raising `NameError` and silently hiding the + tool. - `app/admin/authors.rb` registers `actions :index, :show`, so the suite can prove `update` refuses a resource the admin UI would not let you edit. - `app/models/*.rb` allowlist `ransackable_attributes`, which Ransack 4 requires before it will filter on an attribute at all. -- `db/migrate/*.rb` create the `authors` and `posts` tables. They are checked +- `db/migrate/*.rb` create the `authors` and `posts` tables, and add the + `status` column `posts` needs for the MCP action fixtures. They are checked in with fixed version numbers rather than produced by `rails generate model`, so the schema under test is visible and does not change from run to run. - `db/seeds.rb` is restorative: it resets existing rows rather than only - creating missing ones, because the suite's `update` examples mutate a post - and the generated application is cached between runs. It reads the admin - credentials from the environment so that they are defined in exactly one - place, `AppBuilder`. + creating missing ones, because the suite's `update` examples mutate a post, + the MCP action examples mutate a post's `status` (directly and via a batch + action), and the generated application is cached between runs. It reads the + admin credentials from the environment so that they are defined in exactly + one place, `AppBuilder`. Two files here are not copied into the application: diff --git a/spec/e2e/fixture_app/app/admin/posts.rb b/spec/e2e/fixture_app/app/admin/posts.rb index cd6c28b..ae47914 100644 --- a/spec/e2e/fixture_app/app/admin/posts.rb +++ b/spec/e2e/fixture_app/app/admin/posts.rb @@ -2,4 +2,82 @@ # the MCP `update` tool drops attributes the admin form does not accept. ActiveAdmin.register Post do permit_params :title, :body + + # Opted in via `mcp:`, with a required `visibility` param bound to a static + # enum, so the e2e suite can prove an out-of-enum value is refused before + # dispatch, and that a permitted value actually runs against the real + # controller. + member_action :publish, method: :post, mcp: { + description: "Publish a post with the given visibility", + params: { + visibility: { + type: :string, + required: true, + enum: %w[public unlisted], + hint: "Who can see the post once published", + }, + }, + } do + resource.update!(status: params[:visibility]) + redirect_to resource_path(resource), notice: "Published" + end + + # No `mcp:` key at all, so the e2e suite can prove the opt-in guarantee: an + # action that exists in the admin UI is not automatically exposed as a tool. + member_action :archive, method: :post do + resource.update!(status: "archived") + redirect_to resource_path(resource), notice: "Archived" + end + + # Opted in via `mcp:` with only a description: its param type is inherited + # from `form:`, so the e2e suite can prove a batch action applies to exactly + # the selected records and leaves the rest untouched. + batch_action :set_status, form: { status: :text }, mcp: { + description: "Set the status on the selected posts", + } do |ids, inputs| + Post.where(id: ids).update_all(status: inputs["status"]) + redirect_to collection_path, notice: "Status updated" + end + + # The body raises deliberately, so the e2e suite can prove an action that + # blows up comes back as a generic error naming the resource and action, + # with the exception's own message kept away from the MCP client — it can + # carry SQL, table names and file paths. + member_action :explode, method: :post, mcp: { + description: "Always raises, so the error path has something to bite on", + } do + raise ActiveRecord::StatementInvalid, "SQLite3::SQLException: no such table: classified_dossier" + end + + # Opted in with a `permission:` proc that takes the record. A proc needing a + # record cannot be resolved at tools/list time, so the tool stays advertised + # and the proc runs at call time instead. It returns a String for a draft + # post, which the client should see as the refusal reason, and true once the + # post has been published — so the suite can prove the proc is consulted + # per record rather than simply always refusing. + member_action :feature, method: :post, mcp: { + description: "Feature a published post on the front page", + permission: lambda { |post| + post.status == "draft" ? "Only a published post can be featured" : true + }, + } do + resource.update!(status: "featured") + redirect_to resource_path(resource), notice: "Featured" + end + + # Opted in via `mcp:` with a ZERO-ARGUMENT `permission:` proc that calls + # `current_admin_user`, so the e2e suite can prove the proc is evaluated in + # controller context at tools/list time. `current_admin_user` is only + # defined on the controller: if listing-time evaluation ever regressed to a + # bare `proc.call`, this would raise NameError, the tool would be hidden by + # the rescue, and the example asserting it IS listed would fail. + # E2E_ADMIN_EMAIL is only set for the process that seeds the database, not + # for the running server, so the seeded admin's email is fixed here rather + # than read from the environment: it has to match AppBuilder::ADMIN_EMAIL. + collection_action :purge_drafts, method: :post, mcp: { + description: "Purge draft posts, restricted to the seeded admin", + permission: -> { current_admin_user&.email == "admin@example.com" }, + } do + redirect_to collection_path, notice: "Purged" + end end diff --git a/spec/e2e/fixture_app/db/migrate/20260101000003_add_status_to_posts.rb b/spec/e2e/fixture_app/db/migrate/20260101000003_add_status_to_posts.rb new file mode 100644 index 0000000..cb5235d --- /dev/null +++ b/spec/e2e/fixture_app/db/migrate/20260101000003_add_status_to_posts.rb @@ -0,0 +1,5 @@ +class AddStatusToPosts < ActiveRecord::Migration[7.2] + def change + add_column :posts, :status, :string, default: "draft", null: false + end +end diff --git a/spec/e2e/fixture_app/db/seeds.rb b/spec/e2e/fixture_app/db/seeds.rb index 29bf6e9..b5620f1 100644 --- a/spec/e2e/fixture_app/db/seeds.rb +++ b/spec/e2e/fixture_app/db/seeds.rb @@ -1,7 +1,9 @@ # Restorative by design: the e2e suite's `update` examples rewrite a post's -# title, and the generated application is cached between runs, so seeding has -# to reset existing rows rather than only create missing ones. Records are -# keyed on stable natural keys (email, slug) that no example mutates. +# title, and the MCP action examples rewrite a post's status (directly and via +# a batch action), and the generated application is cached between runs, so +# seeding has to reset existing rows rather than only create missing ones. +# Records are keyed on stable natural keys (email, slug) that no example +# mutates. admin_email = ENV.fetch("E2E_ADMIN_EMAIL") admin_password = ENV.fetch("E2E_ADMIN_PASSWORD") @@ -30,6 +32,7 @@ Post.find_or_initialize_by(slug: slug).tap do |post| post.title = title post.body = body + post.status = "draft" post.save! end end diff --git a/spec/e2e/mcp_actions_spec.rb b/spec/e2e/mcp_actions_spec.rb new file mode 100644 index 0000000..7c57389 --- /dev/null +++ b/spec/e2e/mcp_actions_spec.rb @@ -0,0 +1,136 @@ +RSpec.describe "MCP actions declared with an mcp: option" do + let(:client) { E2E::McpClient.new(url: E2E::AppServer.instance.mcp_url, token: E2E.token) } + + def tool_names + client.tools_list["tools"].map { |tool| tool["name"] } + end + + def tool(name) + client.tools_list["tools"].find { |candidate| candidate["name"] == name } + end + + def post_status(slug) + client.call_tool("query", resource: "Post", q: { slug_eq: slug })["records"].first["status"] + end + + def post_id(slug) + client.call_tool("query", resource: "Post", q: { slug_eq: slug })["records"].first["id"] + end + + describe "a member_action" do + describe "in tools/list" do + it "is advertised as its own tool, with its declared params in the input schema" do + publish = tool("post_publish") + + expect(publish).not_to be_nil + expect(publish["inputSchema"]["properties"]).to include("visibility") + expect(publish["inputSchema"]["properties"]["visibility"]["enum"]).to eq(%w[public unlisted]) + expect(publish["inputSchema"]["required"]).to include("visibility") + end + + it "is not advertised when it carries no mcp: key, proving MCP exposure is opt-in" do + expect(tool_names).not_to include("post_archive") + end + end + + describe "when called" do + it "runs against the real ActiveAdmin controller, so the change is visible on a subsequent query" do + id = post_id("small-gods") + + result = client.call_tool("post_publish", id: id, visibility: "public") + + expect(result["error"]).to be_nil + expect(post_status("small-gods")).to eq("public") + end + + it "refuses a value outside a param's static enum: before dispatch, and leaves the record's status unchanged" do + id = post_id("small-gods") + + result = client.call_tool("post_publish", id: id, visibility: "top-secret") + + expect(result["error"]).to match(/visibility/) + expect(post_status("small-gods")).to eq("draft") + end + + it "refuses a call omitting a required param before dispatch, and leaves the record's status unchanged" do + id = post_id("small-gods") + + result = client.call_tool("post_publish", id: id) + + expect(result["error"]).to match(/visibility/) + expect(post_status("small-gods")).to eq("draft") + end + + it "is refused as an unknown tool when it carries no mcp: key, and leaves the record untouched, since being absent from tools/list would not on its own stop it being dispatched by name" do + result = client.call_tool("post_archive", id: post_id("small-gods")) + + expect(result["error"]).to eq("Unknown tool: post_archive") + expect(post_status("small-gods")).to eq("draft") + end + + it "reports a generic failure naming the resource and action when its body raises, keeping the exception's own message away from the client where it could disclose SQL, table names or file paths" do + result = client.call_tool("post_explode", id: post_id("small-gods")) + + expect(result["error"]).to eq("Post#explode failed") + expect(result["error"]).not_to include("classified_dossier") + expect(result["error"]).not_to match(/SQLite3|no such table|StatementInvalid/) + end + end + + describe "guarded by a permission: proc that takes the record" do + it "is still advertised, because a proc needing a record cannot be resolved at tools/list time and must refuse at call time instead" do + expect(tool_names).to include("post_feature") + end + + it "refuses the call with the string the proc returned as the reason, and leaves the record unchanged" do + result = client.call_tool("post_feature", id: post_id("small-gods")) + + expect(result["error"]).to eq("Only a published post can be featured") + expect(post_status("small-gods")).to eq("draft") + end + + it "allows the call once the record satisfies the proc, proving the proc is consulted per record rather than refusing unconditionally as a proc hardcoded to refuse would also do" do + id = post_id("small-gods") + client.call_tool("post_publish", id: id, visibility: "public") + + result = client.call_tool("post_feature", id: id) + + expect(result["error"]).to be_nil + expect(post_status("small-gods")).to eq("featured") + end + end + end + + describe "a collection_action" do + describe "in tools/list" do + it "is advertised when its zero-argument permission: proc calls current_admin_user, proving the proc is evaluated in controller context at listing time rather than raising NameError and silently hiding the tool" do + expect(tool_names).to include("post_purge_drafts") + end + end + end + + describe "a batch_action" do + describe "in tools/list" do + it "is advertised with an ids array, and with its param types taken from the action's ActiveAdmin form: hash rather than from the mcp: declaration" do + set_status = tool("post_set_status") + + expect(set_status).not_to be_nil + expect(set_status["inputSchema"]["properties"]["ids"]["type"]).to eq("array") + expect(set_status["inputSchema"]["required"]).to include("ids") + expect(set_status["inputSchema"]["properties"]["status"]["type"]).to eq("string") + end + end + + describe "when called" do + it "applies to exactly the selected records, leaving an unselected record untouched" do + selected_id = post_id("a-wizard-of-earthsea") + + result = client.call_tool("post_set_status", ids: [selected_id], status: "archived") + + expect(result["error"]).to be_nil + expect(post_status("a-wizard-of-earthsea")).to eq("archived") + expect(post_status("the-tombs-of-atuan")).to eq("draft") + end + end + end +end diff --git a/spec/e2e/mcp_server_spec.rb b/spec/e2e/mcp_server_spec.rb index 18377ec..9798e70 100644 --- a/spec/e2e/mcp_server_spec.rb +++ b/spec/e2e/mcp_server_spec.rb @@ -12,10 +12,10 @@ expect(result["capabilities"]).to have_key("tools") end - it "advertises the three tools" do + it "advertises the three built-in tools, alongside whatever the fixture app has opted in to MCP" do names = client.tools_list["tools"].map { |tool| tool["name"] } - expect(names).to contain_exactly("list_resources", "query", "update") + expect(names).to include("list_resources", "query", "update") end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 195e9f6..7c1b285 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -2,6 +2,19 @@ require "active_record" require "action_controller" require "activeadmin_mcp" +require "sqlite3" + +# spec/support/active_admin.rb and spec/support/active_record.rb each define +# their own tables against a shared in-memory SQLite database (see the +# comments in those files for why it must be shared rather than two separate +# ":memory:" databases). A SQLite shared-cache database is destroyed the +# moment its last connection closes, and ActiveRecord::Base.establish_connection +# closes whatever pool it replaces — so without a connection held open +# independently of ActiveRecord for the life of the process, the database +# (and every table in it) would vanish the first time either support file's +# pool got replaced. This constant just keeps one connection open so that +# never happens. +KEEPALIVE_SQLITE_CONNECTION = SQLite3::Database.new("file:activeadmin_mcp_test?mode=memory&cache=shared") RSpec.configure do |config| config.expect_with :rspec do |expectations| diff --git a/spec/support/active_admin.rb b/spec/support/active_admin.rb new file mode 100644 index 0000000..e6fb49e --- /dev/null +++ b/spec/support/active_admin.rb @@ -0,0 +1,97 @@ +# Boots a minimal Rails + ActiveAdmin application so specs can exercise real +# ActiveAdmin objects (resource configs, controllers, dispatch) rather than +# doubles. Required only by specs that genuinely need it; the rest of the +# suite stays double-based and never loads ActiveAdmin. +require "rails" +require "active_record" +require "action_controller/railtie" +require "active_model/railtie" + +# Shared-cache URI, not a bare ":memory:" database — see the comment in +# spec/support/active_record.rb for why: two anonymous in-memory databases +# would fight over ActiveRecord::Base's single connection pool depending on +# spec load order, stranding whichever support file's tables loaded first. +ActiveRecord::Base.establish_connection( + adapter: "sqlite3", database: "file:activeadmin_mcp_test?mode=memory&cache=shared" +) +ActiveRecord::Schema.verbose = false +ActiveRecord::Schema.define do + create_table :volunteers, force: true do |t| + t.string :name + t.boolean :active, default: true + t.timestamps + end + + create_table :admin_users, force: true do |t| + t.string :email + t.timestamps + end +end + +class Volunteer < ActiveRecord::Base; end +class AdminUser < ActiveRecord::Base; end + +require "active_admin" + +module McpSpec + class HarnessApp < Rails::Application + config.eager_load = false + config.root = File.expand_path("../tmp/harness", __dir__) + config.secret_key_base = "a" * 64 + config.logger = Logger.new(IO::NULL) + config.active_support.to_time_preserves_timezone = :zone + end + + module ActiveAdminHarness + def self.volunteer_config + ActiveAdmin.application.namespaces[:admin].resources.find do |resource| + resource.resource_class == Volunteer + end + end + end +end + +FileUtils.mkdir_p(McpSpec::HarnessApp.config.root) +Rails.application.initialize! + +# InheritedResources, which ActiveAdmin's resource controllers inherit from, +# references ::ApplicationController at load time. +class ApplicationController < ActionController::Base; end + +# Runs ActiveAdmin's before_load hooks, which is what defines +# ActiveAdmin::BatchAction and mixes batch action support into Resource. +ActiveAdmin.application.load! + +# Batch actions default to disabled in a bare boot like this one; a real app +# enables them in its ActiveAdmin initializer. +ActiveAdmin.application.namespaces[:admin].batch_actions = true + +ActiveAdmin.register Volunteer do + actions :index, :show, :edit, :update + + member_action :create_warning, method: :post, mcp: { + description: "Record a warning against a volunteer", + params: { reason: { type: :string, required: true } } + } do + resource.update(name: params[:reason]) + redirect_to resource_path(resource), notice: "Warning recorded" + end + + member_action :undocumented, method: :post do + head :ok + end + + collection_action :export, method: :get, mcp: { description: "Export volunteers" } do + redirect_to collection_path, notice: "Exported" + end + + batch_action :suspend, form: { reason: :text }, + mcp: { description: "Suspend the selected volunteers" } do |ids, inputs| + Volunteer.where(id: ids).update_all(name: "Suspended: #{inputs[:reason]}") + redirect_to collection_path, notice: "#{ids.size} suspended: #{inputs[:reason]}" + end +end + +Rails.application.routes.draw do + ActiveAdmin.routes(self) +end diff --git a/spec/support/active_record.rb b/spec/support/active_record.rb index bcd52b2..0a9a3e6 100644 --- a/spec/support/active_record.rb +++ b/spec/support/active_record.rb @@ -2,7 +2,17 @@ # Spin up an in-memory SQLite database with just the tables the ApiToken # model needs. Loaded only by specs that exercise the ActiveRecord model. -ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:") +# +# Uses a shared-cache URI rather than a bare ":memory:" database. Both this +# file and spec/support/active_admin.rb call establish_connection, which +# replaces ActiveRecord::Base's connection pool wholesale; with two distinct +# anonymous ":memory:" databases, whichever support file loads last would +# silently strand the other's tables for the rest of the suite. A shared +# in-memory database lets both support files add their tables to the same +# underlying store regardless of load order. +ActiveRecord::Base.establish_connection( + adapter: "sqlite3", database: "file:activeadmin_mcp_test?mode=memory&cache=shared" +) ActiveRecord::Schema.verbose = false ActiveRecord::Schema.define do